-
-
Notifications
You must be signed in to change notification settings - Fork 7.5k
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
TST: always set a (long) timeout for subprocess and always use our wrapper #27726
Conversation
If you are using this and really need unbounded timeouts, explicitly pass `timeout=None`
The second commit finally cleans up all of the |
1bfdf4d
to
ec90d87
Compare
3727a38
to
2852ff7
Compare
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.
Unfortunately, while subprocess_run_helper
defaults to check=True
, subprocess_run_for_testing
defaults to check=False
.
2852ff7
to
0515861
Compare
@@ -50,7 +50,7 @@ def setup(): | |||
set_reproducibility_for_testing() | |||
|
|||
|
|||
def subprocess_run_for_testing(command, env=None, timeout=None, stdout=None, | |||
def subprocess_run_for_testing(command, env=None, timeout=60, stdout=None, | |||
stderr=None, check=False, text=True, |
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.
stderr=None, check=False, text=True, | |
stderr=None, check=True, text=True, |
Since this is used for testing it seems like we should set check=True
by default. It looks like you currently set this in many of the calls. Are there even more tests without setting check?
If you are using this and really need unbounded timeouts, explicitly pass
timeout=None
This is an attempt to avoid azure from hanging and then timing out.