Skip to content

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

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

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 An unexpected behavior, bug, or error tests Tests in the Lib/test dir needs backport to 3.9 only security fixes needs backport to 3.10 only security fixes labels Apr 16, 2022
@gpshead gpshead self-assigned this Apr 16, 2022
@gpshead
Copy link
Member Author

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 April 16, 2022 08:57
@gpshead gpshead added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Apr 16, 2022
@bedevere-bot
Copy link

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

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

@bedevere-bot bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Apr 16, 2022
@gpshead gpshead added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Apr 16, 2022
@bedevere-bot
Copy link

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

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

@bedevere-bot bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section 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
@miss-islington
Copy link
Contributor

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

@miss-islington
Copy link
Contributor

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 April 16, 2022 18:46
@miss-islington
Copy link
Contributor

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 pull request 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 gpshead removed the needs backport to 3.9 only security fixes label Apr 16, 2022
@gpshead gpshead removed the needs backport to 3.10 only security fixes label Apr 16, 2022
gpshead added a commit that referenced this pull request 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 pull request 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 pull request 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)
hello-adam pushed a commit to hello-adam/cpython that referenced this pull request Jun 2, 2022
…s to actually test what they claim (pythonGH-91600) (pythonGH-91612) (python#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 Tests in the Lib/test dir type-bug An unexpected behavior, bug, or error
Projects
None yet
Development

Successfully merging this pull request may close these issues.

test_concurrent_futures does not exercise each mp_context method reliably due to code smell
3 participants