Fix tests to support parallel execution with pytest-xdist#593
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #593 +/- ##
==========================================
+ Coverage 97.75% 97.86% +0.10%
==========================================
Files 23 23
Lines 5709 5748 +39
Branches 351 353 +2
==========================================
+ Hits 5581 5625 +44
+ Misses 79 73 -6
- Partials 49 50 +1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
| host: str = "localhost" | ||
| port: int = 8025 | ||
| host: str = "127.0.0.1" | ||
| # port 0 means "pick a random unused port" |
There was a problem hiding this comment.
We may want to mention the keyword “ephemeral port”, which is what the standards are calling it.
There was a problem hiding this comment.
@cuu508 missed this?
| # port 0 means "pick a random unused port" | |
| # port 0 means "pick a random unused port" | |
| # it is also known as an ephemeral port |
|
|
||
|
|
||
| def watch_for_tls(ready_flag: MP_Event, retq: MP.Queue): | ||
| # Rather than using a hardcoded port, pick a random port, |
There was a problem hiding this comment.
Is there any way to make use of an ephemeral port here too? What are the challenges?
There was a problem hiding this comment.
The test case runs aiosmtpd.main.main() in a separate process (using multiprocessing). We can pass port=0 to it, and it will run on an ephemeral port, but there is no convenient way to communicate the chosen port back to the test case.
There was a problem hiding this comment.
Alright, let's postpone this then.
|
@cuu508 looks like this also needs a rebase with conflict resolution. |
What do these changes do?
This is a follow-up to #524 where @webknjaz noted:
This PR updates tests (and has some supporting changes in Controller too) to allow tests to complete with
pytest-xdist. Hopefully it fixes the flaky tests in Windows too.The test suite uses a hard-coded port for listening sockets: 8025. When running tests in parallel this is a problem, as tests will race to bind to the same port. One solution would be to hardcode a different port in each test case. This would require test writers to keep track which ports have already been "used", and could create annoying issues when copy-pasting code and forgetting to change the port number. An IMHO nicer alternative is to use a special port "0" which tells
asyncio.create_serverto pick a random unused port. This is what this PR does.There were a few complications.
If
asyncio.create_serverpicks the port, there needs some mechanism for the testcase to figure out what port was chosen, so it can connect to it as a client. I changed the InetMixin class to:hostnameandportconstructor arguments in therequested_hostnameandrequested_portinstance attributeshostnameandportinstance attributes to properties that read the hostname and port fromself.server.sockets.This way, clients and test cases can pass
port=0to the controller constructor, and then read the chosen port fromcontroller.portproperty and its corresponding hostname from thecontroller.hostnameproperty. The testing code accessescontroller.portin several places, and these parts don't need to change.The other complication was with tests in
test_main.py. Here, if we pass-l :0arguments tomain()then it will pick a random port, but there's no convenient way to communicate the port back to the testing code. What I chose to do instead is to generate a random port in the test code and pass it tomainusing the-largument. There's a small chance of multiple test cases randomly choosing the same port number, but there are only 3 test cases that need to do this, and each picks from ~16K ports so the probability of collision is tiny.There's one change that is related to making
pytest-xdistwork, but not related to hardcoded ports. It is intest_smtp.py,test_bad_encodingstest case. The test was blowing up because pytest-xdist was trying to serialize logs which contain badly encoded strings. The least intrusive fix I could think of was to, temporarily for this test case, reduce the log level to WARNING, so the bad strings don't end up in logs.Are there changes in behavior for the user?
Yes: the behavior of
Controller.portandController.hostnamehas changed:Controller.portwill contain the randomly chosen port, not value 0controller.end(), bothController.portandController.hostnamewill return None (because the listening sockets are closed and we cannot consult them any more)Controller.requested_portandController.requested_hostnameinstance attributes, containing the values that were passed to the constructorRelated issue number
Checklist
NEWS.rstfile