Skip to content

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

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.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
serhiy-storchaka merged 20 commits into from
Dec 4, 2023
Merged

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
339f260
gh-108927: Do not remove tested modules from sys.modules
serhiy-storchaka Sep 5, 2023
e52be9b
Update NEWS entry.
serhiy-storchaka Sep 5, 2023
68c014e
Fix unloading the newly imported modules after testing.
serhiy-storchaka Sep 6, 2023
c1ad70c
Merge branch 'main' into regretest-no-remove-test-module
serhiy-storchaka Sep 12, 2023
3f25ab4
Add tests.
serhiy-storchaka Sep 12, 2023
7673248
Update Misc/NEWS.d/next/Tests/2023-09-05-20-46-35.gh-issue-108927.Tpw…
serhiy-storchaka Sep 12, 2023
5df4c8b
Move save/unload modules down in single.py.
serhiy-storchaka Sep 13, 2023
eb7766b
Merge branch 'main' into regretest-no-remove-test-module
serhiy-storchaka Sep 13, 2023
0eae535
Refactoring.
serhiy-storchaka Sep 13, 2023
66437b5
Remove also attributes from parent modules.
serhiy-storchaka Sep 13, 2023
8470b33
Merge branch 'main' into regretest-no-remove-test-module
serhiy-storchaka Oct 14, 2023
544c068
Merge branch 'main' into regretest-no-remove-test-module
serhiy-storchaka Nov 27, 2023
1e7ea34
Merge branch 'main' into regretest-no-remove-test-module
serhiy-storchaka Nov 28, 2023
56621ab
Only unload modules if run tests sequentially.
serhiy-storchaka Nov 28, 2023
ba5597a
Apply suggestions from code review
serhiy-storchaka Dec 1, 2023
d025e22
Merge branch 'main' into regretest-no-remove-test-module
serhiy-storchaka Dec 4, 2023
1aae341
Add a comment.
serhiy-storchaka Dec 4, 2023
017f01d
Test that test modules are really unloaded.
serhiy-storchaka Dec 4, 2023
5086405
Merge remote-tracking branch 'refs/remotes/origin/regretest-no-remove…
serhiy-storchaka Dec 4, 2023
b3ea8fa
Update Lib/test/test_regrtest.py
serhiy-storchaka Dec 4, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Only unload modules if run tests sequentially.
  • Loading branch information
@serhiy-storchaka
serhiy-storchaka committed Nov 28, 2023
commit 56621abe6c06ea57bc2bdca360eea2d6fb4f70b1
15 changes: 15 additions & 0 deletions Lib/test/libregrtest/main.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -309,6 +309,8 @@ def run_tests_sequentially(self, runtests) -> None:
else:
tracer = None

save_modules = set(sys.modules)

@vstinner vstinner Nov 28, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I would prefer to move this logic inside libregrtest/single.py, in the run_single_test() function.

@serhiy-storchaka serhiy-storchaka Dec 1, 2023

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I did it, but it is only needed in single-process run. In multi-process run it only adds an overhead. So I moved it back.


jobs = runtests.get_jobs()
if jobs is not None:
tests = count(jobs, 'test')
Expand All @@ -331,6 +333,19 @@ def run_tests_sequentially(self, runtests) -> None:

result = self.run_test(test_name, runtests, tracer)

# Unload the newly imported modules (best effort finalization)
Comment thread
serhiy-storchaka marked this conversation as resolved.
new_modules = [module for module in sys.modules
if module not in save_modules and
module.startswith("test.")]
for module in new_modules:
sys.modules.pop(module, None)
# Remove the attribute of the parent module.
parent, _, name = module.rpartition('.')

@vstinner vstinner Nov 28, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Maybe raise an error if the parent itself has a parent other than test.. Currently, it should not happen. But it may happen in the future, if tests are reorganized.

@serhiy-storchaka serhiy-storchaka Dec 1, 2023

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

What do you mean? The parent should be "test" or start with "test.", because the module starts with "test.". If the parent starts with "test." it will be removed as well. If it was already removed before children, the code ignores KeyError.

@vstinner vstinner Dec 1, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

If a test imports test.test_a.test_b.test_c, does this code unloads test.test_a, test.test_a.test_b and test.test_a.test_b.test_c?

@serhiy-storchaka serhiy-storchaka Dec 1, 2023

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yes, if they were not imported before running tests.

try:
delattr(sys.modules[parent], name)
except (KeyError, AttributeError):
pass

if result.must_stop(self.fail_fast, self.fail_env_changed):
break

Expand Down
20 changes: 0 additions & 20 deletions Lib/test/libregrtest/save_env.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -329,23 +329,3 @@ def __exit__(self, exc_type, exc_val, exc_tb):
f" Before: {original}\n"
f" After: {current} ")
return False


class saved_test_modules:

def __enter__(self):
self.save_modules = set(sys.modules)

def __exit__(self, exc_type, exc_val, exc_tb):
# Unload the newly imported modules (best effort finalization)
new_modules = [module for module in sys.modules
if module not in self.save_modules and
module.startswith("test.")]
for module in new_modules:
sys.modules.pop(module, None)
# Remove the attribute of the parent module.
parent, _, name = module.rpartition('.')
try:
delattr(sys.modules[parent], name)
except (KeyError, AttributeError):
pass
7 changes: 3 additions & 4 deletions Lib/test/libregrtest/single.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@
from .filter import match_test
from .result import State, TestResult, TestStats
from .runtests import RunTests
from .save_env import saved_test_environment, saved_test_modules
from .save_env import saved_test_environment
from .setup import setup_tests
from .testresult import get_test_runner
from .utils import (
Expand Down Expand Up @@ -173,9 +173,8 @@ def _runtest_env_changed_exc(result: TestResult, runtests: RunTests,
clear_caches()
support.gc_collect()

with (saved_test_modules(),
saved_test_environment(test_name,
runtests.verbose, quiet, pgo=pgo)):
with saved_test_environment(test_name,
runtests.verbose, quiet, pgo=pgo):
_load_run_test(result, runtests)
except support.ResourceDenied as exc:
if not quiet and not pgo:
Expand Down