-
Notifications
You must be signed in to change notification settings - Fork 266
PHPLIB-322: Add resume logic to ChangeStream::rewind() #479
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
Conversation
src/ChangeStream.php
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Extra space for indentation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume this was removed because rewind() now throws its own exception and would prevent us from testing next()
I'd suggest replacing with:
/* Note: we intentionally do not start iteration with rewind() to ensure
* that we test extraction functionality within next(). */
Borrowed from the socket exception test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Side thought: does this actually throw or does it only introduce a delay with its own getMore? Since we've yet to insert a document, I'd expect rewind() to not find any result to attempt extraction.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right, the rewind() doesn't throw because it doesn't attempt extraction.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This now makes for very amusing diff output.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
testRewindExtractsResumeTokenAndNextResumes()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
testRewindResumesAfterCursorNotFound()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can simply omit this line. Asserting that rewind() doesn't throw an exception is enough to test that it can resume after not finding a cursor.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment was resolved. No longer applicable to whatever line this points to.
https://jira.mongodb.org/browse/PHPLIB-322