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.

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

Fix for assertCount expected size for Generator #3304

Closed
wants to merge 2 commits into from
Closed

Fix for assertCount expected size for Generator #3304

wants to merge 2 commits into from

Conversation

kubawerlos
Copy link
Contributor

@kubawerlos kubawerlos changed the base branch from master to 7.3 September 13, 2018 18:22
@kubawerlos kubawerlos changed the title Fix for assertCounts expected size for Generator Fix for assertCount expected size for Generator Sep 13, 2018
@sebastianbergmann
Copy link
Owner

This does not solve the underlying problem that assertCount() modifies the generator.

@kubawerlos
Copy link
Contributor Author

I don't know if it is possible in PHP.

the only sensible thing to do here would be to disallow assertCount() on generators.

Should we follow this path? If so, target it to master as BC break or to 7.3 as bugfix?

@JeroenDeDauw
Copy link
Contributor

@sebastianbergmann I don't see why that would be a problem. Going through a generator always does this in PHP, so it is expected behavior. Disallow it in assertCount would just force people to create such assertions themselves, which seems like loss-loss to me.

@epdenouden
Copy link
Contributor

From a developer perspective: whatever the underlying language feature does internally, I would expect an assertCount to work as the name suggests and not give me "size 0" errors.

Copy link
Contributor

@guilliamxavier guilliamxavier left a comment

Choose a reason for hiding this comment

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

Please consider:

$gen2 = function () { yield 1; yield 2; };
$gen3 = function () { yield 1; yield 2; yield 3; };

$count2 = new Count(2);
Assert::assertTrue($count2->evaluate($gen2(), '', true));
Assert::assertFalse($count2->evaluate($gen3(), '', true)); // this should still work

$count3 = new Count(3);
Assert::assertFalse($count3->evaluate($gen2(), '', true));
Assert::assertTrue($count3->evaluate($gen3(), '', true)); // this should still work

Maybe need to reset $this->countOfGenerator = null; at the beginning of function matches?

Also please consider #3302 (comment) (i.e. there are non-Countable, Traversable things that are not instanceof Generator but suffer the same kind of problem)...

@JeroenDeDauw
Copy link
Contributor

JeroenDeDauw commented Sep 29, 2018

I can confirm that this PR is breaking that behavior and does not clean up the already existing confusion between Generators and Iterables that cannot be rewinded. I'll submit a modified solution in a bit.

Edit: here it is: #3316

@sebastianbergmann
Copy link
Owner

Superseded by #3316.

@kubawerlos kubawerlos deleted the generator-failing-assert-count branch September 30, 2018 07:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants