GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — gh-91607: Fix several test_concurrent_futures tests to actually test what they claim by gpshead · Pull Request #91600 · python/cpython

Conversation

@gpshead

@gpshead gpshead commented Apr 16, 2022

Copy link
Copy Markdown
Member

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@gpshead
Fix test_concurrent_futures to actually test what it says.
2b25f54
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 labels Apr 16, 2022
@gpshead gpshead self-assigned this Apr 16, 2022
@bedevere-bot bedevere-bot added the awaiting core review label Apr 16, 2022
@gpshead

gpshead commented Apr 16, 2022

Copy link
Copy Markdown
Member Author

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@gpshead
gpshead requested review from aeros and brianquinlan April 16, 2022 08:57
@gpshead gpshead mentioned this pull request Apr 16, 2022
gh-90622: Do not spawn ProcessPool workers on demand via fork method. #91598
Merged
@gpshead gpshead added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Apr 16, 2022
@bedevere-bot

bedevere-bot commented Apr 16, 2022

Copy link
Copy Markdown

🤖 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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@bedevere-bot bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Apr 16, 2022
@gpshead
Don't import the name from multiprocessing directly to avoid confusion.
9f0604c
@gpshead gpshead added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Apr 16, 2022
@bedevere-bot

bedevere-bot commented Apr 16, 2022

Copy link
Copy Markdown

🤖 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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@bedevere-bot bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Apr 16, 2022
@gpshead gpshead mentioned this pull request Apr 16, 2022
test_concurrent_futures does not exercise each mp_context method reliably due to code smell #91607
Closed
@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
@blurb-it
📜🤖 Added by blurb_it.
53d2c9a
@gpshead
gpshead merged commit 7fa3a5a into python:main Apr 16, 2022
@miss-islington

miss-islington commented Apr 16, 2022

Copy link
Copy Markdown
Contributor

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@bedevere-bot bedevere-bot removed the awaiting core review label Apr 16, 2022
@miss-islington

miss-islington commented Apr 16, 2022

Copy link
Copy Markdown
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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@gpshead
gpshead deleted the unfrell_process_pool_tests branch April 16, 2022 18:46
@miss-islington

miss-islington commented Apr 16, 2022

Copy link
Copy Markdown
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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

gpshead added a commit to gpshead/cpython that referenced this pull request Apr 16, 2022
@gpshead
[3.10] pythongh-91607: Fix several test_concurrent_futures tests to a…
631fa3c
…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 <[email protected]>
@gpshead gpshead mentioned this pull request Apr 16, 2022
[3.10] gh-91607: Fix several test_concurrent_futures tests to test what they claim #91612
Merged
@gpshead gpshead removed the needs backport to 3.9 label Apr 16, 2022
@gpshead gpshead removed the needs backport to 3.10 label Apr 16, 2022
gpshead added a commit that referenced this pull request Apr 16, 2022
@gpshead
[3.10] gh-91607: Fix several test_concurrent_futures tests to actuall…
9a45893
…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 <[email protected]>
gpshead added a commit to gpshead/cpython that referenced this pull request Apr 16, 2022
@gpshead
[3.9] [3.10] pythongh-91607: Fix several test_concurrent_futures test…
d701602
…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 <[email protected]>.
(cherry picked from commit 9a45893)

Co-authored-by: Gregory P. Smith <[email protected]>
gpshead added a commit that referenced this pull request Apr 17, 2022
@gpshead
[3.9] [3.10] gh-91607: Fix several test_concurrent_futures tests to a…
2a43afd
…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)
@gpshead gpshead linked an issue Apr 17, 2022 that may be closed by this pull request
test_concurrent_futures does not exercise each mp_context method reliably due to code smell #91607
Closed
@AlexWaygood AlexWaygood mentioned this pull request May 3, 2022
test_concurrent_futures sometimes hangs indefinitely on Windows #92222
Closed
hello-adam pushed a commit to hello-adam/cpython that referenced this pull request Jun 2, 2022
@gpshead @hello-adam
[3.9] [3.10] pythongh-91607: Fix several test_concurrent_futures test…
bb9db97
…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

Reviewers

@brianquinlan brianquinlan Awaiting requested review from brianquinlan
@aeros aeros Awaiting requested review from aeros

Uh oh!

There was an error while loading. Please reload this page.

Assignees

@gpshead gpshead

Labels

tests Tests in the Lib/test dir type-bug An unexpected behavior, bug, or error

Projects

None yet

Milestone

No milestone

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

Uh oh!

There was an error while loading. Please reload this page.

3 participants

@gpshead @bedevere-bot @miss-islington
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.