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

gh-91607: Fix several test_concurrent_futures tests to actually test what they claim #91600

Merged
merged 3 commits into from Apr 16, 2022

Conversation

gpshead
Copy link
Member

@gpshead gpshead commented Apr 16, 2022

Many ProcessPoolExecutor based tests were ignoring the mp_context
and using the default instead. This meant we lacked proper test
coverage of all of them.

Also removes the old _prime_executor() worker delay seeding code
as it appears to have no point and causes 20-30 seconds extra
latency on this already long test. It also interfered with some
of the refactoring to fix the above to not needlessly create their
own executor when setUp has already created an appropriate one.

Many ProcessPoolExecutor based tests were ignoring the mp_context
and using the default instead.  This meant we lacked proper test
coverage of all of them.

Also removes the old _prime_executor() worker delay seeding code
as it appears to have no point and causes 20-30 seconds extra
latency on this already long test.  It also interfered with some
of the refactoring to fix the above to not needlessly create their
own executor when setUp has already created an appropriate one.
@gpshead gpshead added type-bug tests needs backport to 3.9 needs backport to 3.10 labels Apr 16, 2022
@gpshead gpshead self-assigned this Apr 16, 2022
@gpshead
Copy link
Member Author

@gpshead gpshead commented Apr 16, 2022

I'll file an issue to track and add news after broader testing. This came about while attempting to test my fixes for #90622 as tests that should've been failing were not and code clearly looked suspicious.

This is quite old test suite code, it could do with a refactoring. But one reason is remains as is with its own reinvention of a way to parameterize tests because neither pytest or absltest.parameterized is available for stdlib tests hurts.

@gpshead gpshead requested review from brianquinlan and aeros Apr 16, 2022
@gpshead gpshead added the 🔨 test-with-buildbots label Apr 16, 2022
@bedevere-bot
Copy link

@bedevere-bot bedevere-bot commented Apr 16, 2022

🤖 New build scheduled with the buildbot fleet by @gpshead for commit 2b25f54 🤖

If you want to schedule another build, you need to add the "🔨 test-with-buildbots" label again.

@bedevere-bot bedevere-bot removed the 🔨 test-with-buildbots label Apr 16, 2022
@gpshead gpshead added the 🔨 test-with-buildbots label Apr 16, 2022
@bedevere-bot
Copy link

@bedevere-bot bedevere-bot commented Apr 16, 2022

🤖 New build scheduled with the buildbot fleet by @gpshead for commit 9f0604c 🤖

If you want to schedule another build, you need to add the "🔨 test-with-buildbots" label again.

@bedevere-bot bedevere-bot removed the 🔨 test-with-buildbots label Apr 16, 2022
@gpshead gpshead changed the title Fix several test_concurrent_futures tests to actually test what they claim gh-91607: Fix several test_concurrent_futures tests to actually test what they claim Apr 16, 2022
@gpshead gpshead merged commit 7fa3a5a into python:main Apr 16, 2022
13 checks passed
@miss-islington
Copy link
Contributor

@miss-islington miss-islington commented Apr 16, 2022

Thanks @gpshead for the PR 🌮🎉.. I'm working now to backport this PR to: 3.9, 3.10.
🐍🍒🤖

@miss-islington
Copy link
Contributor

@miss-islington miss-islington commented Apr 16, 2022

Sorry, @gpshead, I could not cleanly backport this to 3.10 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker 7fa3a5a2197896066e3fe53ee325ac6ab54c3414 3.10

@gpshead gpshead deleted the unfrell_process_pool_tests branch Apr 16, 2022
@miss-islington
Copy link
Contributor

@miss-islington miss-islington commented Apr 16, 2022

Sorry @gpshead, I had trouble checking out the 3.9 backport branch.
Please backport using cherry_picker on command line.
cherry_picker 7fa3a5a2197896066e3fe53ee325ac6ab54c3414 3.9

gpshead added a commit to gpshead/cpython that referenced this issue Apr 16, 2022
…ctually test what they claim (pythonGH-91600)

* Fix test_concurrent_futures to actually test what it says.

Many ProcessPoolExecutor based tests were ignoring the mp_context
and using the default instead.  This meant we lacked proper test
coverage of all of them.

Also removes the old _prime_executor() worker delay seeding code
as it appears to have no point and causes 20-30 seconds extra
latency on this already long test.  It also interfered with some
of the refactoring to fix the above to not needlessly create their
own executor when setUp has already created an appropriate one.

* Don't import the name from multiprocessing directly to avoid confusion.

* 📜🤖 Added by blurb_it.

Co-authored-by: blurb-it[bot] <43283697+blurb-it[bot]@users.noreply.github.com>.
(cherry picked from commit 7fa3a5a)

Co-authored-by: Gregory P. Smith <greg@krypto.org>
gpshead added a commit that referenced this issue Apr 16, 2022
…y test what they claim (GH-91600) (#91612)

* Fix test_concurrent_futures to actually test what it says.

Many ProcessPoolExecutor based tests were ignoring the mp_context
and using the default instead.  This meant we lacked proper test
coverage of all of them.

Also removes the old _prime_executor() worker delay seeding code
as it appears to have no point and causes 20-30 seconds extra
latency on this already long test.  It also interfered with some
of the refactoring to fix the above to not needlessly create their
own executor when setUp has already created an appropriate one.

* Don't import the name from multiprocessing directly to avoid confusion.

(cherry picked from commit 7fa3a5a)

Co-authored-by: Gregory P. Smith <greg@krypto.org>
gpshead added a commit to gpshead/cpython that referenced this issue Apr 16, 2022
…s to actually test what they claim (pythonGH-91600) (pythonGH-91612)

* Fix test_concurrent_futures to actually test what it says.

Many ProcessPoolExecutor based tests were ignoring the mp_context
and using the default instead.  This meant we lacked proper test
coverage of all of them.

Also removes the old _prime_executor() worker delay seeding code
as it appears to have no point and causes 20-30 seconds extra
latency on this already long test.  It also interfered with some
of the refactoring to fix the above to not needlessly create their
own executor when setUp has already created an appropriate one.

* Don't import the name from multiprocessing directly to avoid confusion.

(cherry picked from commit 7fa3a5a)

Co-authored-by: Gregory P. Smith <greg@krypto.org>.
(cherry picked from commit 9a45893)

Co-authored-by: Gregory P. Smith <greg@krypto.org>
gpshead added a commit that referenced this issue Apr 17, 2022
…ctually test what they claim (GH-91600) (GH-91612) (#91617)

* Fix test_concurrent_futures to actually test what it says.

Many ProcessPoolExecutor based tests were ignoring the mp_context
and using the default instead.  This meant we lacked proper test
coverage of all of them.

Also removes the old _prime_executor() worker delay seeding code
as it appears to have no point and causes 20-30 seconds extra
latency on this already long test.  It also interfered with some
of the refactoring to fix the above to not needlessly create their
own executor when setUp has already created an appropriate one.

* Don't import the name from multiprocessing directly to avoid confusion.

(cherry picked from commit 7fa3a5a)
(cherry picked from commit 9a45893)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tests type-bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants