From b77d0deaf53697630ba534f62dc871f4eb275f14 Mon Sep 17 00:00:00 2001 From: Bruno Oliveira Date: Sat, 1 Jul 2023 12:37:46 -0300 Subject: [PATCH] Fix duplicated imports with importlib mode and doctest-modules (#11148) The initial implementation (in #7246) introduced the `importlib` mode, which never added the imported module to `sys.modules`, so it included a test to ensure calling `import_path` twice would yield different modules. Not adding modules to `sys.modules` proved problematic, so we began to add the imported module to `sys.modules` in #7870, but failed to realize that given we are now changing `sys.modules`, we might as well avoid importing it more than once. Then #10088 came along, passing `importlib` also when importing application modules (as opposed to only test modules before), which caused problems due to imports having side-effects and the expectation being that they are imported only once. With this PR, `import_path` returns the module immediately if already in `sys.modules`. Fix #10811 Fix #10341 --- changelog/10811.bugfix.rst | 2 ++ src/_pytest/pathlib.py | 2 ++ testing/acceptance_test.py | 35 +++++++++++++++++++++++++++++++++++ testing/test_pathlib.py | 27 +++++++++++++++++++-------- 4 files changed, 58 insertions(+), 8 deletions(-) create mode 100644 changelog/10811.bugfix.rst diff --git a/changelog/10811.bugfix.rst b/changelog/10811.bugfix.rst new file mode 100644 index 000000000..aa26414e4 --- /dev/null +++ b/changelog/10811.bugfix.rst @@ -0,0 +1,2 @@ +Fixed issue when using ``--import-mode=importlib`` together with ``--doctest-modules`` that caused modules +to be imported more than once, causing problems with modules that have import side effects. diff --git a/src/_pytest/pathlib.py b/src/_pytest/pathlib.py index e43310ef0..14fb2e3ae 100644 --- a/src/_pytest/pathlib.py +++ b/src/_pytest/pathlib.py @@ -523,6 +523,8 @@ def import_path( if mode is ImportMode.importlib: module_name = module_name_from_path(path, root) + with contextlib.suppress(KeyError): + return sys.modules[module_name] for meta_importer in sys.meta_path: spec = meta_importer.find_spec(module_name, [str(path.parent)]) diff --git a/testing/acceptance_test.py b/testing/acceptance_test.py index 453e26972..429fb4e43 100644 --- a/testing/acceptance_test.py +++ b/testing/acceptance_test.py @@ -1315,3 +1315,38 @@ def test_function_return_non_none_warning(pytester: Pytester) -> None: ) res = pytester.runpytest() res.stdout.fnmatch_lines(["*Did you mean to use `assert` instead of `return`?*"]) + + +def test_doctest_and_normal_imports_with_importlib(pytester: Pytester) -> None: + """ + Regression test for #10811: previously import_path with ImportMode.importlib would + not return a module if already in sys.modules, resulting in modules being imported + multiple times, which causes problems with modules that have import side effects. + """ + # Uses the exact reproducer form #10811, given it is very minimal + # and illustrates the problem well. + pytester.makepyfile( + **{ + "pmxbot/commands.py": "from . import logging", + "pmxbot/logging.py": "", + "tests/__init__.py": "", + "tests/test_commands.py": """ + import importlib + from pmxbot import logging + + class TestCommands: + def test_boo(self): + assert importlib.import_module('pmxbot.logging') is logging + """, + } + ) + pytester.makeini( + """ + [pytest] + addopts= + --doctest-modules + --import-mode importlib + """ + ) + result = pytester.runpytest_subprocess() + result.stdout.fnmatch_lines("*1 passed*") diff --git a/testing/test_pathlib.py b/testing/test_pathlib.py index c15a81ea1..3d574e856 100644 --- a/testing/test_pathlib.py +++ b/testing/test_pathlib.py @@ -7,6 +7,7 @@ from textwrap import dedent from types import ModuleType from typing import Any from typing import Generator +from typing import Iterator import pytest from _pytest.monkeypatch import MonkeyPatch @@ -282,29 +283,36 @@ class TestImportPath: import_path(tmp_path / "invalid.py", root=tmp_path) @pytest.fixture - def simple_module(self, tmp_path: Path) -> Path: - fn = tmp_path / "_src/tests/mymod.py" + def simple_module( + self, tmp_path: Path, request: pytest.FixtureRequest + ) -> Iterator[Path]: + name = f"mymod_{request.node.name}" + fn = tmp_path / f"_src/tests/{name}.py" fn.parent.mkdir(parents=True) fn.write_text("def foo(x): return 40 + x", encoding="utf-8") - return fn + module_name = module_name_from_path(fn, root=tmp_path) + yield fn + sys.modules.pop(module_name, None) - def test_importmode_importlib(self, simple_module: Path, tmp_path: Path) -> None: + def test_importmode_importlib( + self, simple_module: Path, tmp_path: Path, request: pytest.FixtureRequest + ) -> None: """`importlib` mode does not change sys.path.""" module = import_path(simple_module, mode="importlib", root=tmp_path) assert module.foo(2) == 42 # type: ignore[attr-defined] assert str(simple_module.parent) not in sys.path assert module.__name__ in sys.modules - assert module.__name__ == "_src.tests.mymod" + assert module.__name__ == f"_src.tests.mymod_{request.node.name}" assert "_src" in sys.modules assert "_src.tests" in sys.modules - def test_importmode_twice_is_different_module( + def test_remembers_previous_imports( self, simple_module: Path, tmp_path: Path ) -> None: - """`importlib` mode always returns a new module.""" + """`importlib` mode called remembers previous module (#10341, #10811).""" module1 = import_path(simple_module, mode="importlib", root=tmp_path) module2 = import_path(simple_module, mode="importlib", root=tmp_path) - assert module1 is not module2 + assert module1 is module2 def test_no_meta_path_found( self, simple_module: Path, monkeypatch: MonkeyPatch, tmp_path: Path @@ -317,6 +325,9 @@ class TestImportPath: # mode='importlib' fails if no spec is found to load the module import importlib.util + # Force module to be re-imported. + del sys.modules[module.__name__] + monkeypatch.setattr( importlib.util, "spec_from_file_location", lambda *args: None )