Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Allow using a \Traversable as data provider, not only \Iterater #2666

Closed
wants to merge 3 commits into from
Closed

Allow using a \Traversable as data provider, not only \Iterater #2666

wants to merge 3 commits into from

Conversation

issei-m
Copy link
Contributor

@issei-m issei-m commented May 2, 2017

I'm not sure whether it's a bug or not, so I'm targeting 6.1 the current stable branch.

@codecov-io
Copy link

codecov-io commented May 2, 2017 •
edited
Loading

Codecov Report

Merging #2666 into 6.1 will increase coverage by 0.02%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff              @@
##                6.1    #2666      +/-   ##
============================================
+ Coverage     53.83%   53.86%   +0.02%     
  Complexity     2740     2740              
============================================
  Files           102      102              
  Lines          7179     7179              
============================================
+ Hits           3865     3867       +2     
+ Misses         3314     3312       -2
Impacted Files Coverage Δ Complexity Δ
src/Util/Test.php 88.25% <100%> (ø) 192 <0> (ø) ⬇️
src/Util/PHP/DefaultPhpProcess.php 42.04% <0%> (+2.27%) 31% <0%> (ø) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d9ce668...04c6f69. Read the comment docs.

@keradus
Copy link
Contributor

keradus commented May 3, 2017

please add a test for that

@bkrukowski
Copy link

+1

It is a bug, because function iterator_to_array expects instance of \Traversable, please take a look on result of command php --rf iterator_to_array:

Function [ <internal:SPL> function iterator_to_array ] {

  - Parameters [2] {
    Parameter #0 [ <required> Traversable $iterator ]
    Parameter #1 [ <optional> $use_keys ]
  }
}

So IMO it should be applied also for older versions.

keradus
keradus reviewed May 4, 2017
View reviewed changes
src/Util/Test.php
@@ -517,7 +518,7 @@ private static function getDataFromDataProviderAnnotation($docComment, $classNam
$data = $dataProviderMethod->invoke($object, $methodName);
}

if ($data instanceof Iterator) {
if ($data instanceof Traversable) {
$data = \iterator_to_array($data);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not go even forward and use iterable instead ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't do that because iterator_to_array cannot be passed an array.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@keradus Iterable is not not class or interface, it is pseudo-type, is_iterable function was introduced in PHP 7.1 and phpunit supports also older versions

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that's why I lowercased it, didn't I?
is_iterable is polyfilled even in 5.x

https://github.com/symfony/polyfill/tree/master/src/Php71

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@keradus You're right, but as @issei-m has mentioned - iterator_to_array expects \Traversable.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I referred to main change of this PR.
And my proposal is deeper - to allow dataProvider not only work properly with Traversable, but all iterable.

whith this change, it will probably be like that, good to prove by utests, which are already there

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And my proposal is deeper - to allow dataProvider not only work properly with Traversable, but all iterable.

I think I don't understand. Traversable and array are all iterable possibilities, so IMO this change is good enough. Is there something what is skipped?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nah, I asked for tests proving that, which @issei-m done later ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@keradus I also don't figure it out. Could you tell me your perspective in more detail, and what should I specifically do here?

@issei-m
Copy link
Contributor Author

issei-m commented May 4, 2017

@keradus Test has been added!

keradus
keradus reviewed May 4, 2017
View reviewed changes
tests/_files/MultipleDataProviderTest.php Outdated
yield [null, 'ok', null];
yield [null, 'ok', null];
yield [null, 'ok', null];
return new WrapperIteratorAggregate([
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please don't modify existing tests. add new one instead.

@issei-m
Copy link
Contributor Author

issei-m commented May 8, 2017

@keradus Sorry for the late response. I've fixed test you mentioned.

@sebastianbergmann sebastianbergmann added the type/enhancement A new idea that should be implemented label May 8, 2017
@sebastianbergmann sebastianbergmann self-assigned this May 8, 2017
@sebastianbergmann sebastianbergmann added this to the PHPUnit 6.2 milestone May 8, 2017
@sebastianbergmann
Copy link
Owner

Merged manually into master. Thanks!

@issei-m issei-m deleted the allow-using-traversable-as-data-provider branch May 8, 2017 10:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bkrukowski bkrukowski bkrukowski left review comments

@keradus keradus keradus approved these changes

Assignees

@sebastianbergmann sebastianbergmann

Labels
type/enhancement A new idea that should be implemented
Projects
None yet
Milestone
PHPUnit 6.2
Development

Successfully merging this pull request may close these issues.
5 participants
@issei-m @codecov-io @keradus @bkrukowski @sebastianbergmann