Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 33.9k
gh-108388: Convert test_concurrent_futures to package#108401
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
Uh oh!
There was an error while loading. Please reload this page.
Conversation
vstinner commented Aug 24, 2023 • edited by bedevere-bot
Loading Uh oh!
There was an error while loading. Please reload this page.
edited by bedevere-bot
Uh oh!
There was an error while loading. Please reload this page.
Convert test_concurrent_futures to a package of 7 sub-tests. Add remote_globals to create_executor_tests()
vstinner commented Aug 24, 2023
Timing of running tests:
With the PR: |
sobolevn left a comment
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.
To fix the CI
Uh oh!
There was an error while loading. Please reload this page.
vstinner commented Aug 24, 2023
GHA job timings:
Details. Windows x86: Windows x64: macOS: Ubuntu: Address Sanitizer: |
gpshead left a comment
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.
I believe you unintentionally got rid of most of the Threading tests.
(do not merge this even after fixing up this review round's comments, this requires further very close side by side review that Github's UI is incapable of providing as it lacks the ability to track file splits and moves as a diff of what's changed -- I'll need to spend time on the command line in a client looking things over)
Otherwise: Thank you for taking this on! This test suite has needed refactoring into something manageable forever.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
bedevere-bot commented Aug 24, 2023
When you're done making the requested changes, leave the comment: |
gpshead commented Aug 24, 2023
I moved this PR to Draft state to signal that I don't want anyone to think it is ready to merge even if someone approves it until after we've taken a close look to verify that we haven't lost anything in the refactoring. |
vstinner commented Aug 24, 2023
I compared |
vstinner commented Aug 24, 2023
For your comments like "ThreadPoolExecutorTest is defined above but missing here" on test_process_pool.py, I suggest you looking at which tests are executed rather than only looking at the code. For example: You can compare between the main branch and my PR: I get the same 15 tests, but with my PR, test names also get an additional |
vstinner commented Aug 24, 2023
I splitted test_process_pool in two testrs:
Before, test_process_poll was the sum (75 tests, ~42 sec) 🙂. |
gpshead commented Aug 24, 2023
Thanks. confirmed locally as well, all tests are present and accounted for! |
vstinner commented Aug 24, 2023
Thanks for the review @gpshead! For sure, there is room for improvement for these tests. I hope that this split will ease future maintenance. |
miss-islington commented Aug 24, 2023
Thanks @vstinner for the PR 🌮🎉.. I'm working now to backport this PR to: 3.12. |
miss-islington commented Aug 24, 2023
Sorry, @vstinner, I could not cleanly backport this to |
bedevere-bot commented Aug 24, 2023
GH-108443 is a backport of this pull request to the 3.12 branch. |
gpshead commented Aug 24, 2023
the merge conflict on this is probably just the list in regrtest runtests.py. retry that automation after the test_multiprocessing one is in 3.12. |
vstinner commented Aug 24, 2023
Right. I backported the change to Python 3.12. There is a minor conflict in libregrtest on: The backport is in conflict with PR #108401 (split multiprocessing tests) which may be merged first. |
GH-109704 is a backport of this pull request to the 3.11 branch. |
vstinner commented Sep 22, 2023 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
Since I was asked to backport all test changes (including refactoring) to stable branches, I backport this change to Python 3.11: GH-109704. |
#109704) * gh-108388: Convert test_concurrent_futures to package (#108401) Convert test_concurrent_futures to a package of sub-tests. (cherry picked from commit aa6f787) Notes on backport to 3.11: * AsCompletedTests: Revert test_future_times_out() => test_zero_timeout() * Restore TODO comment * ThreadPoolExecutorTest.test_hang_global_shutdown_lock(): add @support.requires_resource('cpu').
Convert test_concurrent_futures to a package of 7 sub-tests.
Add remote_globals to create_executor_tests()