Fix duplicated imports with importlib mode and doctest-modules

The initial implementation (in pytest-dev#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 pytest-dev#7870, but failed to realize that given we are now changing `sys.modules`, we might
as well avoid importing it more than once.

Then pytest-dev#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 pytest-dev#10811, pytest-dev#10341

Author: nicoddemus

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.

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)])

testing/acceptance_test.py

@@ -1315,3 +1315,38 @@ def test_stuff():
     )
     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*")

testing/test_pathlib.py

@@ -7,6 +7,7 @@
 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 @@ def test_invalid_path(self, tmp_path: Path) -> None:
             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 @@ def test_no_meta_path_found(
         # 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
         )