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 issue #142 #194

Closed
wants to merge 7 commits into from
Closed

Conversation

cjerdonek
Copy link
Collaborator

@cjerdonek cjerdonek commented Jul 3, 2017

[Fix for issue #142] This issue was quite tricky for me to develop a test case for. The hardest part is that you need to find a way to call cancel on close() when it is at this particular line of code. I was led to do that in a roundabout way.

I'm open to suggestions on ways to simplify the test or on ways to eliminate the need for sleeps (which could potentially lead to test flakiness, though the sleeps are quite large currently). Or if it's good enough for the time being, that's fine too!

@cjerdonek
Copy link
Collaborator Author

cjerdonek commented Jul 4, 2017

FYI, I updated the test case in the PR to have only one call to asyncio.sleep() that isn't "minimal." (In contrast, the asyncio.sleep(1) call is minimal in the sense that it doesn't actually wait for 1 second but instead waits the minimum amount of time.) The non-minimal call is needed to give close() enough time to advance far enough into its execution so that it is actually waiting on the worker.

@cjerdonek cjerdonek changed the title Fix issue 142 Fix issue #142 Jul 5, 2017
@cjerdonek
Copy link
Collaborator Author

Ping, @aaugustin. Just wanted to make sure you had seen this. No rush, just hadn't seen an acknowledgment of the diagnosis, etc.

@cjerdonek
Copy link
Collaborator Author

Rebased, fixed flake8, and added changelog entry.

@cjerdonek
Copy link
Collaborator Author

Rebased.

@aaugustin
Copy link
Member

Since the bug and the fix are in protocol.py, the test should be in test_protocol.py. Writing a test at a lower level is "easier" (not "easy") because you can control exactly which inputs are fed into the event loop and what happens it what order. See 0ae899c.

@aaugustin aaugustin closed this Sep 3, 2017
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.

2 participants