gh-108927: Fix removing testing modules from sys.modules by serhiy-storchaka · Pull Request #108952 · python/cpython · GitHub

The code that does this properly never worked since Python 3.0.

And removing only the tested module breaks import machinery if it has submodules used in other tests.

Issue: test_unittest failure triggered by test_import + test_importlib #108927

It breaks import machinery if the test module has submodules used in other tests.

Presumably the reload (the reason for the unload) was intentional. Do we know why that was added? Do those reasons still apply?

Hum. I think that the problem is that only a single module is unload from sys.modules.

If we want to save/restore modules, IMO the whole sys.modules dictionary must be saved/restored. Not a single entry.

Would it make sense to save/restore the whole sys.modules dict? Would it fix #108927 ?

Tests crash with this change, and it means that there is other huge issue hidden until now.

If we want to save/restore modules, IMO the whole sys.modules dictionary must be saved/restored. Not a single entry.

It is already done in run_test() in main.py, so I do not understand why it did not have effect and why removing the module was needed here.

Unloading the test module is done for 23 years (commit 5796d26) and so far, so I'm surprised that today it became a problem. Maybe it's not the right place to fix the issue.

commit 5796d26794eee634a4a06637d99d8d5c58da2bdb
Author: Guido van Rossum <guido@python.org>
Date:   Fri Apr 21 21:35:06 2000 +0000

    Patch by Vladimir Marangozov to unload additionally imported modules
    after each test has been run.  This avoids excessive memory growth
    during the tests.

diff --git a/Lib/test/regrtest.py b/Lib/test/regrtest.py
index 6364d7179c..924890ab76 100755
--- a/Lib/test/regrtest.py
+++ b/Lib/test/regrtest.py
@@ -105,6 +105,7 @@ def main(tests=None, testdir=None):
     if single:
         tests = tests[:1]
     test_support.verbose = verbose      # Tell tests to be moderately quiet
+    save_modules = sys.modules.keys()
     for test in tests:
         if not quiet:
             print test
@@ -118,6 +119,10 @@ def main(tests=None, testdir=None):
                 print "test", test,
                 print "skipped -- an optional feature could not be imported"
             skipped.append(test)
+        # Unload the newly imported modules (best effort finalization)
+        for module in sys.modules.keys():
+            if module not in save_modules:
+                test_support.unload(module)
     if good and not quiet:
         if not bad and not skipped and len(good) > 1:
             print "All",

It is already done in run_test() in main.py, so I do not understand why it did not have effect and why removing the module was needed here.

I would prefer to dig deeper before changing this code.

The crash is not related to this change. See #108976. If exclude test_pdb, all tests are passed successfully.

Unloading the test module is done for 23 years (commit 5796d26) and so far, so I'm surprised that today it became a problem.

Ah, that code does not work since 3.0.

It saves sys.modules.keys() which is a view in Python 3, and reflects the current content of sys.modules, so the code never finds any "newly imported" modules.

Suggested change

      
                        for module in list(sys.modules):
          
                        for module in sys.modules:

No it does not work, because the dict is modified during iteration.

…Wav.rst Co-authored-by: Brett Cannon <brett@python.org>

I don't understand this code. Why do we have to unload test modules?

If it's important to unload test modules, I would suggest to move this code inside single.py, to make a more consistent behavior for the different ways to run tests.

Do you ask why the code of libregrtest is so complicated? 🤷‍♂️

I think that it only makes sense in the single-process run. As well as removing the testing module before reloading in _load_run_test(). In the multi-process run it is only a waste of time. As well as significant part of save_env (except cleaning up external resources like filesystem).

It saves sys.modules.keys() which is a view in Python 3, and reflects the current content of sys.modules, so the code never finds any "newly imported" modules.

Oh... I see :-) So if the code is dead for 20+ years, maybe remove it? I don't know what's the best option here.

I would expect that running a test would restore Python (sys.modules) in the exact same state.

LGTM. Thanks for updated tests. I confirm that tests fail I revert Lib/test/libregrtest/ changes.

Co-authored-by: Victor Stinner <vstinner@python.org>

Thanks @serhiy-storchaka for the PR 🌮🎉.. I'm working now to backport this PR to: 3.11, 3.12.
🐍🍒⛏🤖

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

…nGH-108952) It breaks import machinery if the test module has submodules used in other tests. (cherry picked from commit e08b70f) Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>

GH-112712 is a backport of this pull request to the 3.11 branch.

…nGH-108952) It breaks import machinery if the test module has submodules used in other tests. (cherry picked from commit e08b70f) Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>

…08952) (ПР-112712) It breaks import machinery if the test module has submodules used in other tests. (cherry picked from commit e08b70f) Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>

…08952) (ПР-112711) It breaks import machinery if the test module has submodules used in other tests. (cherry picked from commit e08b70f) Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>

It looks like the new test fails when running the tests from an installed Python installation (see https://buildbot.python.org/all/#/builders/350/builds/4974 and https://buildbot.python.org/all/#/builders/1154/builds/418), could someone take a look?

Thank you @Yhg1s. Please look at #112765.

It looks like the new test fails when running the tests from an installed Python installation

FTR we are seeing the failures with the recently released 3.11.7.

@hroncok:

FTR we are seeing the failures with the recently released 3.11.7.

I suppose that #112784 already fixed the issue in the 3.11 branch. Sadly, this fix is not part of 3.11.7 release.

Buildbots were too slow to report the failure on the 3.11 branch before the release? It seems like we have "Installed" buildbot builders on the 3.11 branch.

Yes. What's done is done, but may I suggest a release not be made with commits before the buildbots have time to recognize a failure?

3.11 commit 8dbda1c: Mon Dec 4 17:16:03 2023 +0100
Git tag v3.11.7: Mon Dec 4 17:56:31 2023 +0000

Ah right, there were less than 1 hour between the commit and the tag.

@hroncok:

Yes. What's done is done, but may I suggest a release not be made with commits before the buildbots have time to recognize a failure?

Usually, the branch is not locked around bugfix 3.x.y releases.

cc @pablogsal 3.11 release manager.

The release script blocks if there are failing build bots so this was releaseable because all buildbots were green at the time (or the API reported green).

Is very difficult to time this correctly because even if we block a branch we do this hours before the release so we don't know when all builsbots have finished.

We can discuss blocking a day in advance to prevent this from happening or some other mechanism.

I will discuss this with other RMs to see what can be done.

In my experience, it's rare to discover bugs so soon just after a release. I'm not sure that it's worth it to deeply change the release process.

Maybe the release process should run tests on installed Python, rather than in the source code directory.

…nGH-108952) It breaks import machinery if the test module has submodules used in other tests.

serhiy-storchaka added type-bug An unexpected behavior, bug, or error tests Tests in the Lib/test dir labels Sep 5, 2023

serhiy-storchaka requested a review from vstinner September 5, 2023 17:47

bedevere-bot added the awaiting core review label Sep 5, 2023

bedevere-bot mentioned this pull request Sep 5, 2023

test_unittest failure triggered by test_import + test_importlib #108927

Closed

pythongh-108927 : Do not remove tested modules from sys.modules

339f260

It breaks import machinery if the test module has submodules used in other tests.

serhiy-storchaka force-pushed the regretest-no-remove-test-module branch from 4f7617c to 339f260 Compare September 5, 2023 17:47

Yhg1s mentioned this pull request Sep 5, 2023

gh-108927: Fix test_import + test_importlib + test_unittest problem #108929

Merged

vstinner reviewed Sep 5, 2023

View reviewed changes

serhiy-storchaka added 2 commits September 6, 2023 11:00

Update NEWS entry.

e52be9b

Fix unloading the newly imported modules after testing.

68c014e

serhiy-storchaka changed the title gh-108927: Do not remove tested modules from sys.modules gh-108927: Fix removing testing modules from sys.modules Sep 6, 2023

serhiy-storchaka added 2 commits September 12, 2023 10:41

Merge branch 'main' into regretest-no-remove-test-module

c1ad70c

Add tests.

3f25ab4

serhiy-storchaka requested review from brettcannon, ericsnowcurrently, ncoghlan and warsaw as code owners September 12, 2023 08:52

brettcannon approved these changes Sep 12, 2023

View reviewed changes

Update Misc/NEWS.d/next/Tests/2023-09-05-20-46-35.gh-issue-108927.Tpw…

7673248

…Wav.rst Co-authored-by: Brett Cannon <brett@python.org>

vstinner reviewed Sep 13, 2023

View reviewed changes

serhiy-storchaka added 2 commits September 13, 2023 10:55

Move save/unload modules down in single.py.

5df4c8b

Merge branch 'main' into regretest-no-remove-test-module

eb7766b

vstinner approved these changes Dec 4, 2023

View reviewed changes

Comment thread Lib/test/test_regrtest.py Outdated

bedevere-app Bot added awaiting merge and removed awaiting core review labels Dec 4, 2023

Update Lib/test/test_regrtest.py

b3ea8fa

Co-authored-by: Victor Stinner <vstinner@python.org>

serhiy-storchaka added needs backport to 3.11 only security fixes needs backport to 3.12 only security fixes labels Dec 4, 2023

serhiy-storchaka enabled auto-merge (squash) December 4, 2023 15:11

serhiy-storchaka merged commit e08b70f into python:main Dec 4, 2023

bedevere-app Bot removed the awaiting merge label Dec 4, 2023

bedevere-app Bot removed the needs backport to 3.12 only security fixes label Dec 4, 2023

bedevere-app Bot removed the needs backport to 3.11 only security fixes label Dec 4, 2023

serhiy-storchaka deleted the regretest-no-remove-test-module branch December 5, 2023 17:12

pablogsal mentioned this pull request Jan 9, 2024

test_unload_tests fails in Python 3.11 when executed from an installed directory #113862

Closed

aisk pushed a commit to aisk/cpython that referenced this pull request Feb 11, 2024

pythongh-108927 : Fix removing testing modules from sys.modules (pytho…

a096f48

…nGH-108952) It breaks import machinery if the test module has submodules used in other tests.

Glyphack pushed a commit to Glyphack/cpython that referenced this pull request Sep 2, 2024

pythongh-108927 : Fix removing testing modules from sys.modules (pytho…

6db4be2

…nGH-108952) It breaks import machinery if the test module has submodules used in other tests.

                   module_name = abs_module_name(result.test_name, ns.testdir)
-                  # Remove the module from sys.module to reload it if it was already imported
-                  sys.modules.pop(module_name, None)

gh-108927: Fix removing testing modules from sys.modules by serhiy-storchaka · Pull Request #108952 · python/cpython · GitHub

Uh oh!

Uh oh!

gh-108927: Fix removing testing modules from sys.modules - #108952

gh-108927: Fix removing testing modules from sys.modules#108952serhiy-storchaka merged 20 commits intopython:mainpython/cpython:mainfrom serhiy-storchaka:regretest-no-remove-test-moduleserhiy-storchaka/cpython:regretest-no-remove-test-moduleCopy head branch name to clipboard

Conversation

serhiy-storchaka commented Sep 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Yhg1s commented Sep 5, 2023

Uh oh!

vstinner Sep 5, 2023

Choose a reason for hiding this comment

Uh oh!

serhiy-storchaka commented Sep 5, 2023

Uh oh!

vstinner commented Sep 6, 2023

Uh oh!

vstinner commented Sep 6, 2023

Uh oh!

serhiy-storchaka commented Sep 6, 2023

Uh oh!

serhiy-storchaka commented Sep 6, 2023

Uh oh!

brettcannon Sep 12, 2023

Choose a reason for hiding this comment

Uh oh!

serhiy-storchaka Sep 12, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

vstinner Sep 13, 2023

Choose a reason for hiding this comment

Uh oh!

serhiy-storchaka Sep 13, 2023

Choose a reason for hiding this comment

Uh oh!

vstinner commented Sep 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vstinner left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

miss-islington-app Bot commented Dec 4, 2023

Uh oh!

bedevere-app Bot commented Dec 4, 2023

Uh oh!

bedevere-app Bot commented Dec 4, 2023

Uh oh!

Yhg1s commented Dec 5, 2023

Uh oh!

serhiy-storchaka commented Dec 5, 2023

Uh oh!

hroncok commented Dec 6, 2023

Uh oh!

vstinner commented Dec 6, 2023

Uh oh!

hroncok commented Dec 6, 2023

Uh oh!

vstinner commented Dec 6, 2023

Uh oh!

pablogsal commented Dec 6, 2023

Uh oh!

vstinner commented Dec 6, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

gh-108927: Fix removing testing modules from sys.modules#108952
serhiy-storchaka merged 20 commits into
python:mainfrom
serhiy-storchaka:regretest-no-remove-test-module

serhiy-storchaka commented Sep 5, 2023 •
edited
Loading

vstinner commented Sep 13, 2023 •
edited
Loading