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-102251: Disable non-rerunnable test in test_import by erlend-aasland · Pull Request #106013 · python/cpython

Conversation

@erlend-aasland

@erlend-aasland erlend-aasland commented Jun 23, 2023 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Contributor

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

  • Issue: Fix Refleak in test_import #102251

Sorry, something went wrong.

Uh oh!

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

All reactions
@erlend-aasland
pythongh-102251: Fix test_import ref leak
e099505
test_basic_multiple_interpreters_main_no_reset() does not support rerunning
@erlend-aasland
erlend-aasland requested review from brettcannon, ericsnowcurrently, ncoghlan and warsaw as code owners June 23, 2023 08:33
@erlend-aasland
erlend-aasland requested a review from vstinner June 23, 2023 08:33
@bedevere-bot bedevere-bot added the awaiting core review label Jun 23, 2023
@bedevere-bot bedevere-bot mentioned this pull request Jun 23, 2023
Fix Refleak in test_import #102251
Closed
@erlend-aasland erlend-aasland added the skip news label Jun 23, 2023
@sunmy2019

sunmy2019 commented Jun 23, 2023

Copy link
Copy Markdown
Member

I like this solution more. 👍

erlend-aasland reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

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

@erlend-aasland

erlend-aasland commented Jun 23, 2023

Copy link
Copy Markdown
Contributor Author

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 the no_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.

All reactions

Sorry, something went wrong.

Uh oh!

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

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

This comment was marked as outdated.

Sign in to view
@bedevere-bot bedevere-bot removed the 🔨 test-with-refleak-buildbots Test PR w/ refleak buildbots; report in status section label Jun 23, 2023
@erlend-aasland
Doh
e5baa02
@erlend-aasland erlend-aasland added the 🔨 test-with-refleak-buildbots Test PR w/ refleak buildbots; report in status section label Jun 23, 2023
@bedevere-bot

This comment was marked as outdated.

Sign in to view
@bedevere-bot bedevere-bot removed the 🔨 test-with-refleak-buildbots Test PR w/ refleak buildbots; report in status section label Jun 23, 2023
@erlend-aasland
erlend-aasland requested a review from sunmy2019 June 23, 2023 11:05
@erlend-aasland
Also skip reruns for test_basic_multiple_interpreters_deleted_no_reset()
0784bc1
@erlend-aasland erlend-aasland added the 🔨 test-with-refleak-buildbots Test PR w/ refleak buildbots; report in status section label Jun 23, 2023
@bedevere-bot

This comment was marked as outdated.

Sign in to view
@bedevere-bot bedevere-bot removed the 🔨 test-with-refleak-buildbots Test PR w/ refleak buildbots; report in status section label Jun 23, 2023
Yhg1s
Yhg1s approved these changes Jun 23, 2023
View reviewed changes
@bedevere-bot bedevere-bot added awaiting merge and removed awaiting core review labels Jun 23, 2023
@erlend-aasland erlend-aasland mentioned this pull request Jun 23, 2023
gh-102251: add missing cleanups for test_import #104796
Closed
@erlend-aasland

erlend-aasland commented Jun 23, 2023 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Contributor Author

Comment on ref leak run:

For all buildbots, only test_peg_generator is leaking, except:

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

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

All reactions

Sorry, something went wrong.

Uh oh!

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

sunmy2019
sunmy2019 reviewed Jun 23, 2023
View reviewed changes

@sunmy2019 sunmy2019 left a comment

Copy link
Copy Markdown
Member

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.

Other parts LGTM

Sorry, something went wrong.

Uh oh!

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

All reactions
Comment thread Lib/test/test_import/__init__.py Resolved

Uh oh!

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

@erlend-aasland erlend-aasland mentioned this pull request Jun 23, 2023
gh-105063: Disable test_peg_generator.TestCParser bco. ref leaks #106024
Merged
erlend-aasland added 2 commits June 23, 2023 19:50
@erlend-aasland
Address review: test_basic_multiple_interpreters_main_no_reset() is n…
7083645
…ot affected
@erlend-aasland
Pull in main
0c7839c
@erlend-aasland erlend-aasland added the 🔨 test-with-refleak-buildbots Test PR w/ refleak buildbots; report in status section label Jun 23, 2023
@bedevere-bot

This comment was marked as outdated.

Sign in to view
@bedevere-bot bedevere-bot removed the 🔨 test-with-refleak-buildbots Test PR w/ refleak buildbots; report in status section label Jun 23, 2023
@erlend-aasland erlend-aasland changed the title gh-102251: Fix test_import ref leak gh-102251: Disable non-rerunnable test in test_import Jun 23, 2023
brettcannon
brettcannon approved these changes Jun 23, 2023
View reviewed changes
@erlend-aasland
erlend-aasland merged commit 4849a80 into python:main Jun 23, 2023
@bedevere-bot bedevere-bot removed the awaiting merge label Jun 23, 2023
@erlend-aasland
erlend-aasland deleted the fix-test-import-leaks-alt branch June 23, 2023 23:34
@vstinner

vstinner commented Jun 28, 2023

Copy link
Copy Markdown
Member

@no_rerun(reason="rerun not possible; module state is never cleared (see gh-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 to run such test in subprocesses.

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

erlend-aasland, sunmy2019, and ericsnowcurrently reacted with thumbs up emoji
All reactions
  • 👍 3 reactions

Sorry, something went wrong.

Uh oh!

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

@erlend-aasland

erlend-aasland commented Jun 28, 2023

Copy link
Copy Markdown
Contributor Author

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

All reactions

Sorry, something went wrong.

Uh oh!

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

@erlend-aasland

erlend-aasland commented Jun 28, 2023

Copy link
Copy Markdown
Contributor Author

The usual fix is to run such test in subprocesses.

ISTM we should rework parts of test_import to follow this pattern.

sunmy2019 and ericsnowcurrently reacted with thumbs up emoji
All reactions
  • 👍 2 reactions

Sorry, something went wrong.

Uh oh!

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

@Yhg1s

Yhg1s commented Sep 18, 2023

Copy link
Copy Markdown
Member

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

All reactions

Sorry, something went wrong.

Uh oh!

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

@Yhg1s Yhg1s added the needs backport to 3.12 only security fixes label Sep 18, 2023
@miss-islington

miss-islington commented Sep 18, 2023

Copy link
Copy Markdown
Contributor

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

All reactions

Sorry, something went wrong.

Uh oh!

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

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Sep 18, 2023
@erlend-aasland @miss-islington
pythongh-102251: Disable non-rerunnable test in test_import (pythonGH…
5dd1046
…-106013)

(cherry picked from commit 4849a80)

Co-authored-by: Erlend E. Aasland <[email protected]>
@bedevere-app

bedevere-app Bot commented Sep 18, 2023

Copy link
Copy Markdown

GH-109540 is a backport of this pull request to the 3.12 branch.

All reactions

Sorry, something went wrong.

Uh oh!

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

@bedevere-app bedevere-app Bot removed the needs backport to 3.12 only security fixes label Sep 18, 2023
Yhg1s pushed a commit that referenced this pull request Sep 18, 2023
@miss-islington @erlend-aasland
[3.12] gh-102251: Disable non-rerunnable test in test_import (GH-106013…
f6fc831
…) (#109540)

gh-102251: Disable non-rerunnable test in test_import (GH-106013)
(cherry picked from commit 4849a80)

Co-authored-by: Erlend E. Aasland <[email protected]>
@vstinner

vstinner commented Sep 18, 2023

Copy link
Copy Markdown
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
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

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

@vstinner

vstinner commented Sep 18, 2023

Copy link
Copy Markdown
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
All reactions
  • ❤️ 1 reaction

Sorry, something went wrong.

Uh oh!

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

@erlend-aasland

erlend-aasland commented Sep 18, 2023

Copy link
Copy Markdown
Contributor Author

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

Footnotes

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

vstinner reacted with heart emoji
All reactions
  • ❤️ 1 reaction

Sorry, something went wrong.

Uh oh!

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

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

Reviewers

@sunmy2019 sunmy2019 sunmy2019 left review comments
@brettcannon brettcannon brettcannon approved these changes
@Yhg1s Yhg1s Yhg1s approved these changes
@ericsnowcurrently ericsnowcurrently Awaiting requested review from ericsnowcurrently ericsnowcurrently is a code owner
@ncoghlan ncoghlan Awaiting requested review from ncoghlan ncoghlan is a code owner
@warsaw warsaw Awaiting requested review from warsaw warsaw is a code owner
@vstinner vstinner Awaiting requested review from vstinner

Uh oh!

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

Assignees

No one assigned

Labels

skip news

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

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

7 participants

@erlend-aasland @sunmy2019 @bedevere-bot @vstinner @Yhg1s @miss-islington @brettcannon
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.