Always pick an open port when running tests#6591
Conversation
012ba28 to
5b4e280
Compare
Unit Test ResultsSee test report for an extended history of previous test failures. This is useful for diagnosing flaky tests. 15 files + 6 15 suites +6 6h 56m 15s ⏱️ + 3h 26m 4s For more details on these failures, see this check. Results for commit 9620479. ± Comparison against base commit 4239aed. ♻️ This comment has been updated with latest results. |
distributed/utils_test.py
Outdated
| def free_port(global_port_lock, name_of_test): | ||
| with _get_open_port(global_port_lock) as port: | ||
| print(f"Using free port {port} for test {name_of_test}") | ||
| yield str(port) |
There was a problem hiding this comment.
why do you need to change it to str?
There was a problem hiding this comment.
|
+1 I like the idea behind this |
distributed/utils_test.py
Outdated
| def free_port(global_port_lock, name_of_test): | ||
| with _get_open_port(global_port_lock) as port: | ||
| print(f"Using free port {port} for test {name_of_test}") | ||
| yield str(port) |
There was a problem hiding this comment.
fjetter
left a comment
There was a problem hiding this comment.
I removed the file locking nonsense again since it didn't work reliably, at least not with tmpfiles.
If we choose to go for pytest-xdist we might just want to skip the tests that require default ports or ensure they are all scheduled on the same xdist worker
distributed/utils.py
Outdated
|
|
||
| def open_port(host=""): | ||
| """Return a probably-open port | ||
| def open_port(host="", port=0): |
There was a problem hiding this comment.
the version of ephemeral-port-reserve seems to be pretty close to what we had but much more sophisticated and likely better tested. Since open_port is something that might've been used elsewhere I decided to vendor the code.
There was a problem hiding this comment.
That version didn't work on windows so I reverted to the old one.
771342c to
0d9bb03
Compare
|
I replaced most occurrences of hard coded ports (there are still plenty in test_core.py) |
| "--port", | ||
| str(port1), |
There was a problem hiding this comment.
nit: I find f-strings marginally neater here
| "--port", | |
| str(port1), | |
| f"--port={port1}", |
There was a problem hiding this comment.
agreed. I was not entirely sure if our popen would handle this properly but it does 🎉
I changed most occurrences with some regex replacements. might have missed a few but will not go back for this
42f1a57 to
afe006d
Compare
| @pytest.mark.skipif(WINDOWS, reason="POSIX only") | ||
| @pytest.mark.parametrize("sig", [signal.SIGINT, signal.SIGTERM]) | ||
| def test_signal_handling(loop, sig): | ||
| port = open_port() | ||
| with subprocess.Popen( | ||
| ["python", "-m", "distributed.cli.dask_scheduler"], | ||
| [ | ||
| "python", | ||
| "-m", | ||
| "distributed.cli.dask_scheduler", | ||
| f"--port={port}", | ||
| "--dashboard-address=:0", | ||
| ], | ||
| stdout=subprocess.PIPE, | ||
| stderr=subprocess.STDOUT, | ||
| ) as scheduler: | ||
| # Wait for scheduler to start | ||
| with Client(f"127.0.0.1:{Scheduler.default_port}", loop=loop) as c: | ||
| with Client(f"127.0.0.1:{port}", loop=loop) as c: | ||
| pass | ||
| scheduler.send_signal(sig) | ||
| stdout, stderr = scheduler.communicate() |
There was a problem hiding this comment.
I believe this test is causing a lot of cascading failures by blocking ports after not shutting down properly. I notice we're not using the custom popen (which I'm OK with) but I'm wondering why this is not shutting down properly. Anyhow, with these port changes this should be more robust
|
On OSX 3.10 I still get a connection refused error but this is likely caused by a conflict in the dashboard port, see also #6612 |
This PR adds pytest fixtures that ensure a port being used in a test is free (no 100% guarantee)
I added logic to enable this even for concurrent runs using pytest-xdist, i.e. this is a step towards