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: assertCount will no longer cause a rewind error when given a generator (as per #2149) #2523

Closed
wants to merge 1 commit into from

Conversation

Nessworthy
Copy link
Contributor

@Nessworthy Nessworthy commented Feb 20, 2017

Hi, first attempt at a pull request, attempting to tackle #2149!

The issue was found to be in two areas in Framework\Constraint\Count::getCountOf:

  1. Firstly, where a generator had already been iterated at least once, iterator_count() attempted to rewind the generator object which fired off the error.
  2. Secondly, where a generator had not been iterated through at all, after the iterator_count() call exhausted it, the method would then attempt to rewind the generator object to set it back to the original position, also causing the error.

The fix attempts to get around this by looping through the generator itself and counting up the times it was iterated. Non-generators are not affected by this change.

A new test was added to cover both a fresh generator and a generator which had been partially exhausted already.

The caveats from this are that the generator will always be fully exhausted as a result of the assertion, and that counting a partially exhausted generator will only be able to return the count from its current position to the end.

Feedback is appreciated!

@sebastianbergmann sebastianbergmann added 5.7 type/bug Something is broken labels Feb 24, 2017
@sebastianbergmann sebastianbergmann self-assigned this Feb 24, 2017
@sebastianbergmann
Copy link
Owner

Merged manually into 5.7, 6.0, and master.

@Nessworthy Nessworthy deleted the counting-generators branch March 15, 2017 15:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type/bug Something is broken
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants