gh-102251: Disable non-rerunnable test in test_import by erlend-aasland · Pull Request #106013 · python/cpython

erlend-aasland

Conversation

test_basic_multiple_interpreters_main_no_reset() does not support rerunning

I like this solution more. 👍

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.

This comment was marked as outdated.

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.

Choose a reason for hiding this comment

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

Other parts LGTM

This comment was marked as outdated.

erlend-aasland changed the title ~~gh-102251: Fix test_import ref leak~~ gh-102251: Disable non-rerunnable test in test_import

Jun 23, 2023

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

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

The usual fix is to run such test in subprocesses.

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

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

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 request

Sep 18, 2023

…-106013)

(cherry picked from commit 4849a80)

Co-authored-by: Erlend E. Aasland <erlend.aasland@protonmail.com>

Yhg1s pushed a commit that referenced this pull request

Sep 18, 2023

…) (#109540)

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

Co-authored-by: Erlend E. Aasland <erlend.aasland@protonmail.com>

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.

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)')

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