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

bpo-37531: Fix regrtest timeout for subprocesses #14679

Closed
wants to merge 4 commits into from

Conversation

@nanjekyejoannah
Copy link
Contributor

commented Jul 9, 2019

Fix regrtest timeout for subprocesses.

Naming things is the hardest thing in computer science. Interested in any reviews in a better name If any for the new function I introduced. cc @vstinner

https://bugs.python.org/issue37531

Joannah nanjekye added 2 commits Jul 9, 2019
@ammaraskar

This comment has been minimized.

Copy link
Member

commented Jul 10, 2019

Hey @nanjekyejoannah, did you accidentally attach the wrong bpo ticket for this? 3753 seems really old and unrelated.

@ZackerySpytz

This comment has been minimized.

Copy link
Contributor

commented Jul 10, 2019

It seems that you have changed the file mode of several files under Lib/test. Why? I don't think those changes should be made.

@nanjekyejoannah nanjekyejoannah changed the title bpo-3753: Fix regrtest timeout for subprocesses bpo-37531: Fix regrtest timeout for subprocesses Jul 10, 2019

@vstinner
Copy link
Member

left a comment

It seems like you modified Lib/test/test_os.py permissions somehow, please revert this change.

Lib/test/libregrtest/runtest.py Outdated Show resolved Hide resolved
Lib/test/libregrtest/runtest.py Outdated Show resolved Hide resolved
nanjekyejoannah
@nanjekyejoannah

This comment has been minimized.

Copy link
Contributor Author

commented Jul 18, 2019

I have made the requested changes; please review again.

@bedevere-bot

This comment has been minimized.

Copy link

commented Jul 18, 2019

Thanks for making the requested changes!

: please review the changes made to this pull request.

@nanjekyejoannah

This comment has been minimized.

Copy link
Contributor Author

commented Jul 18, 2019

Hey @nanjekyejoannah, did you accidentally attach the wrong bpo ticket for this? 3753 seems really old and unrelated.

Yes, thanks for catching this. I made the necessary changes.

@nanjekyejoannah

This comment has been minimized.

Copy link
Contributor Author

commented Jul 18, 2019

It seems that you have changed the file mode of several files under Lib/test. Why? I don't think those changes should be made.

oh yes, I have reverted this.

@vstinner

This comment has been minimized.

Copy link
Member

commented Aug 1, 2019

Overall, the change is great and seems to work as expected, but I tested it and it doesn't work as I expected:

  • it doesn't report stdout/stderr on timeout
  • regression: it ignores stdout/stderr on "CHILD_ERROR" whereas it was properly passed to the main process previously
  • faulthandler and Popen.communicate() use the same timeout, so it's unclear which one which trigger first: I tuned timeouts to give more time to faulthandler. I prefer faulthandler since it dumps the traceback where Python was stuck, whereas Popen.communicate() timeout only returns "truncated" output with no traceback
  • the timeout is reported as an exception, not as as "result"

I tried to add a commit to this PR but I wasn't allowed, so I forked this PR and created PR #15072 to add my changes on top of it.

@nanjekyejoannah

This comment has been minimized.

Copy link
Contributor Author

commented Aug 1, 2019

Closing this then in favor of PR #15072

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.