-
-
Notifications
You must be signed in to change notification settings - Fork 750
test_nanny_worker_port_range hangs on Windows #5956
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
test_nanny_worker_port_range hangs on Windows #5956
Conversation
Unit Test Results 12 files + 4 12 suites +4 7h 16m 40s ⏱️ + 4h 7m 35s For more details on these failures, see this check. Results for commit 1b3e6c3. ± Comparison against base commit 2d3fddc. ♻️ This comment has been updated with latest results. |
8ad80e8 to
6a9abbe
Compare
6a9abbe to
68fbbcc
Compare
|
To clarify: this PR does not make the tests with |
| print("\n\nPrint from stdout\n=================\n") | ||
| print(out.decode()) | ||
| if dump_stdout: | ||
| print("\n" + "-" * 27 + " Subprocess stdout/stderr" + "-" * 27) |
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.
| print("\n" + "-" * 27 + " Subprocess stdout/stderr" + "-" * 27) | |
| print("\n" + "-" * 27 + " Subprocess stdout" + "-" * 27) |
Are you not printing err because kwargs["stderr"] = subprocess.STDOUT means it's already been printed?
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.
Yes, they are merged together because I was afraid of the subprocess getting stuck on stdout while a test is reading from stderr and vice versa.
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.
Oh I see yes, stderr is redirected into stdout, which is then captured.
| "--no-dashboard", | ||
| ] | ||
| ], | ||
| flush_output=False, |
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.
Why not flush_output=True here and remove the await to_thread(worker.communicate, timeout=5)? Maybe there's a subtle difference in timing I'm missing.
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 and another test were abusing Proc.communicate to just wait for the subprocess to finish. I changed them now.
|
Following up here @crusaderky . Thank you for doing this PR. I'm curious, a lot of the tests that I've run into that fail unexpectedly use popen. Do you have thoughts on any additional cleanup here that might make sense? I don't have any ideas in particular, this comment is just a shot in the dark. |
|
(I found this PR through git blame) |
Uh oh!
There was an error while loading. Please reload this page.