-
-
Notifications
You must be signed in to change notification settings - Fork 30k
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
gh-108927: Fix removing testing modules from sys.modules #108952
Changes from 5 commits
339f260
e52be9b
68c014e
c1ad70c
3f25ab4
7673248
5df4c8b
eb7766b
0eae535
66437b5
8470b33
544c068
1e7ea34
56621ab
ba5597a
d025e22
1aae341
017f01d
5086405
b3ea8fa
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -293,7 +293,7 @@ def run_tests_sequentially(self, runtests): | |||||
else: | ||||||
tracer = None | ||||||
|
||||||
save_modules = sys.modules.keys() | ||||||
save_modules = set(sys.modules) | ||||||
|
||||||
jobs = runtests.get_jobs() | ||||||
if jobs is not None: | ||||||
|
@@ -318,9 +318,9 @@ def run_tests_sequentially(self, runtests): | |||||
result = self.run_test(test_name, runtests, tracer) | ||||||
|
||||||
# Unload the newly imported modules (best effort finalization) | ||||||
serhiy-storchaka marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
for module in sys.modules.keys(): | ||||||
for module in list(sys.modules): | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No it does not work, because the dict is modified during iteration. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||||||
if module not in save_modules and module.startswith("test."): | ||||||
support.unload(module) | ||||||
sys.modules.pop(module, None) | ||||||
|
||||||
if result.must_stop(self.fail_fast, self.fail_env_changed): | ||||||
break | ||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,6 @@ | ||
import unittest | ||
import test_regrtest_b.util | ||
|
||
class Test(unittest.TestCase): | ||
def test(self): | ||
test_regrtest_b.util |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
import unittest | ||
|
||
class Test(unittest.TestCase): | ||
def test(self): | ||
pass |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,6 @@ | ||
import unittest | ||
import test_regrtest_b.util | ||
|
||
class Test(unittest.TestCase): | ||
def test(self): | ||
test_regrtest_b.util |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1866,6 +1866,14 @@ def test_random_seed(self): | |
def test_random_seed_workers(self): | ||
self._check_random_seed(run_workers=True) | ||
|
||
def test_import_from_tests(self): | ||
serhiy-storchaka marked this conversation as resolved.
Show resolved
Hide resolved
|
||
testdir = os.path.join(os.path.dirname(__file__), | ||
'regrtestdata', 'import_from_tests') | ||
tests = [f'test_regrtest_{name}' for name in ('a', 'b', 'c')] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would you mind to add a comment to explain the purpose of these test? By reading the code, it's unclear to me. Explain that the test execution order matters for this test. Does the test fails if a regression is introduced? Maybe add a test_regrtest_d test checking if test_regrtest_a, test_regrtest_b and test_regrtest_c are not loaded? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done. |
||
args = ['-Wd', '-E', '-bb', '-m', 'test', '--testdir=%s' % testdir, *tests] | ||
output = self.run_python(args) | ||
self.check_executed_tests(output, tests, stats=3) | ||
|
||
|
||
class TestUtils(unittest.TestCase): | ||
def test_format_duration(self): | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,4 @@ | ||
Fixed order dendence in running tests in the same process | ||
serhiy-storchaka marked this conversation as resolved.
Show resolved
Hide resolved
|
||
when a test that has submodules (e.g. test_importlib) follows a test that | ||
imports its submodule (e.g. test_importlib.util) and precedes a test | ||
(e.g. test_unittest or test_compileall) that uses that submodule. |
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.
I would prefer to move this logic inside
libregrtest/single.py
, in therun_single_test()
function.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.
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.