bpo-41718: regrtest saved_test_environment avoids imports#24934
bpo-41718: regrtest saved_test_environment avoids imports#24934vstinner merged 1 commit intopython:masterfrom vstinner:regrtest_save_env2
Conversation
saved_test_environment no longer imports modules at startup, but try to get them from sys.modules. If an module is missing, skip the test. It also sets directly support.environment_altered. runtest() now now two saved_test_environment instances: one before importing the test module, one after importing it. Remove imports from test.libregrtest.save_env: * asyncio * logging * multiprocessing * shutil * sysconfig * urllib.request * warnings This change may miss some bugs in tests, but it reduces the number of modules imported by libregrtest.
|
Number of modules imported while running test_sys_modules of https://bugs.python.org/issue41718#msg376374
More imports can be removed later (in refleak.py), but I prefer to keep this PR as short as possible. |
|
I ran two manual tests, to check that the altered environment is still detected:
|
|
Overall I think this looks good. Change description wise, "This change may miss some bugs in tests," could use some more explanation as that is a very vague statement that could lead the reader to think it'll miss test failures (which I do not believe you meant). What scenarios and what might be missed? |
| def get_multiprocessing_process__dangling(self): | ||
| if not multiprocessing: | ||
| return None | ||
| multiprocessing_process = self.try_get_module('multiprocessing.process') |
There was a problem hiding this comment.
Hm. The previous code checked the multiprocessing.process imported successful or not. Do we need keep it?
There was a problem hiding this comment.
The current code ignores ImportError. What do you mean?
try:
import _multiprocessing, multiprocessing.process
except ImportError:
multiprocessing = None
There was a problem hiding this comment.
Oh, sorry. Wrong comment in here. You raised the SkipTestEnviroment in here and catch it in __enter__(). So it is same as the old codes.
|
@gpshead: "Change description wise, "This change may miss some bugs in tests," could use some more explanation as that is a very vague statement that could lead the reader to think it'll miss test failures (which I do not believe you meant). What scenarios and what might be missed?" I can rephase this part of the commit message as: "If a test method imports a module (ex: warnings) and the test has a side effect (ex: add a warnings filter), the side effect is not detected, because the module was not imported when Python enters the saved_test_environment context manager." Does it make sense? IMO it's an acceptable trade-off. Side effects are rare. Imports only done in test methods and not at the module level are rare. I don't think that currently saved_test_environment is perfect and can detect every single side effect. saved_test_environment is more a best-effort approach trying to detect the most common and most annoying issues. |
|
Hum, the overall rationale is a little bit far, it can be found at: https://bugs.python.org/issue40275#msg366337 |
|
That makes senses @vstinner. Thanks! I applied it as an edit to the change description above. |
5 participants
saved_test_environmentno longer imports modules at startup, but triesto get them from
sys.modules. If a module is missing, it skips the test.It also directly sets
support.environment_altered.runtest()now creates twosaved_test_environmentinstances: one beforeimporting the test module, one after importing it.
Removed imports from
test.libregrtest.save_env:This change may miss some bugs when a test method imports a module (ex: warnings) and the test has a side effect (ex: add a warnings filter), the side effect is not detected, because the module was not imported when Python enters the saved_test_environment context manager. But it reduces the number of modules imported by
libregrtest.https://bugs.python.org/issue41718