Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

gh-102251: Disable non-rerunnable test in test_import#106013

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged

Conversation

erlend-aasland
Copy link
Contributor

@erlend-aaslanderlend-aasland commentedJun 23, 2023
edited
Loading

Alternative togh-104796; proposed as this PR has a much smaller diff and is less intrusive.

test_basic_multiple_interpreters_main_no_reset() does not support rerunning
@sunmy2019
Copy link
Member

I like this solution more. 👍

erlend-aasland reacted with thumbs up emoji

@erlend-aasland
Copy link
ContributorAuthor

I like this solution more. 👍

Yes; my rationale is:

Keep the diff down; keep the solution as simple as possible. We want to be able to easily remove theno_rerun() hack when the original problem has been addressed. With this solution we can simply remove the decorator from the affected test function. Also, it limits the decorator to this file only, and adds a relatively big warning for the programmer.

@erlend-aaslanderlend-aasland added the 🔨 test-with-refleak-buildbotsTest PR w/ refleak buildbots; report in status section labelJun 23, 2023
@bedevere-bot

This comment was marked as outdated.

@bedevere-botbedevere-bot removed the 🔨 test-with-refleak-buildbotsTest PR w/ refleak buildbots; report in status section labelJun 23, 2023
@erlend-aaslanderlend-aasland added the 🔨 test-with-refleak-buildbotsTest PR w/ refleak buildbots; report in status section labelJun 23, 2023
@bedevere-bot

This comment was marked as outdated.

@bedevere-botbedevere-bot removed the 🔨 test-with-refleak-buildbotsTest PR w/ refleak buildbots; report in status section labelJun 23, 2023
@erlend-aaslanderlend-aasland added the 🔨 test-with-refleak-buildbotsTest PR w/ refleak buildbots; report in status section labelJun 23, 2023
@bedevere-bot

This comment was marked as outdated.

@bedevere-botbedevere-bot removed the 🔨 test-with-refleak-buildbotsTest PR w/ refleak buildbots; report in status section labelJun 23, 2023
@erlend-aasland
Copy link
ContributorAuthor

erlend-aasland commentedJun 23, 2023
edited
Loading

Comment on ref leak run:

For all buildbots, onlytest_peg_generator is leaking, except:

  • buildbot/s390x RHEL8 Refleaks PR: test_import leaked [1, 1, 4] memory blocks, sum=6Link

Suggesting to land this PR as a start, then focus on the RHEL8/s390x issue.

Copy link
Member

@sunmy2019sunmy2019 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Other parts LGTM

@erlend-aaslanderlend-aasland added the 🔨 test-with-refleak-buildbotsTest PR w/ refleak buildbots; report in status section labelJun 23, 2023
@bedevere-bot

This comment was marked as outdated.

@bedevere-botbedevere-bot removed the 🔨 test-with-refleak-buildbotsTest PR w/ refleak buildbots; report in status section labelJun 23, 2023
@erlend-aaslanderlend-aasland changed the titlegh-102251: Fix test_import ref leakgh-102251: Disable non-rerunnable test in test_importJun 23, 2023
@erlend-aaslanderlend-aasland merged commit4849a80 intopython:mainJun 23, 2023
@erlend-aaslanderlend-aasland deleted the fix-test-import-leaks-alt branchJune 23, 2023 23:34
@vstinner
Copy link
Member

@no_rerun(reason="rerun not possible; module state is never cleared (seegh-102251)")

IMO @no_rerun() is bad pattern and should be avoided or removed. They are many tests with side effects which cannot be cancelled because there is no API for that. For example, in the past, there were tests registering codecs and error handlers.

The usual fix is torun such test in subprocesses.

For example, test_audit runs subprocesses on functions registering audit hooks, since there is no function to unregister a audit hookby design.

erlend-aasland, sunmy2019, and ericsnowcurrently reacted with thumbs up emoji

@erlend-aasland
Copy link
ContributorAuthor

@vstinner: I agree that it is a bad pattern. This is a temporary fix only. See my comment earlier in this PR:

We want to be able to easily remove the no_rerun() hack when the original problem has been addressed.

@erlend-aasland
Copy link
ContributorAuthor

The usual fix is to run such test in subprocesses.

ISTM we should rework parts oftest_import to follow this pattern.

sunmy2019 and ericsnowcurrently reacted with thumbs up emoji

@Yhg1s
Copy link
Member

This needs a backport to 3.12 to resolve the test_import leak that only shows up when you runpython -m test -R:: test_import instead ofpython -m test -R :: test_import (note the space between-R ::). Some of the buildbots run with-R::.

@Yhg1sYhg1s added the needs backport to 3.12only security fixes labelSep 18, 2023
@miss-islington
Copy link
Contributor

Thanks@erlend-aasland for the PR 🌮🎉.. I'm working now to backport this PR to: 3.12.
🐍🍒⛏🤖

miss-islington pushed a commit to miss-islington/cpython that referenced this pull requestSep 18, 2023
…-106013)(cherry picked from commit4849a80)Co-authored-by: Erlend E. Aasland <erlend.aasland@protonmail.com>
@bedevere-app
Copy link

GH-109540 is a backport of this pull request to the3.12 branch.

@bedevere-appbedevere-appbot removed the needs backport to 3.12only security fixes labelSep 18, 2023
Yhg1s pushed a commit that referenced this pull requestSep 18, 2023
…) (#109540)gh-102251: Disable non-rerunnable test in test_import (GH-106013)(cherry picked from commit4849a80)Co-authored-by: Erlend E. Aasland <erlend.aasland@protonmail.com>
@vstinner
Copy link
Member

If a test makes a change which cannot be undone, one option is to run it in a subprocess.

Another option is to add an API to undo the change. For example, codecs.unregister() was added to fix tests adding codecs.

See issue#108963 which is a similar issue.

erlend-aasland reacted with thumbs up emoji

@vstinner
Copy link
Member

Anyway, thanks for removing this ugly workaround :-(

    def setUpClass(cls):        if '-R' in sys.argv or '--huntrleaks' in sys.argv:            # https://github.com/python/cpython/issues/102251            raise unittest.SkipTest('unresolved refleaks (see gh-102251)')
erlend-aasland reacted with heart emoji

@erlend-aasland
Copy link
ContributorAuthor

Oh, I totally forgot about this. I still think Victor's proposal1 should be applied; this PR is aworkaround, not a fix :)

Footnotes

  1. "The usual fix is to run such test in subprocesses." — V. Stinner.

vstinner reacted with heart emoji

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@sunmy2019sunmy2019sunmy2019 left review comments

@brettcannonbrettcannonbrettcannon approved these changes

@Yhg1sYhg1sYhg1s approved these changes

@ericsnowcurrentlyericsnowcurrentlyAwaiting requested review from ericsnowcurrentlyericsnowcurrently is a code owner

@ncoghlanncoghlanAwaiting requested review from ncoghlanncoghlan is a code owner

@warsawwarsawAwaiting requested review from warsawwarsaw is a code owner

@vstinnervstinnerAwaiting requested review from vstinner

Assignees
No one assigned
Labels
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

7 participants
@erlend-aasland@sunmy2019@bedevere-bot@vstinner@Yhg1s@miss-islington@brettcannon

[8]ページ先頭

©2009-2025 Movatter.jp