Skip to content

Commit

Permalink
Fix duplicated imports with importlib mode and doctest-modules
Browse files Browse the repository at this point in the history
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
  • Loading branch information
nicoddemus committed Jul 1, 2023
1 parent 2f7415c commit d2ae7d3
Show file tree
Hide file tree
Showing 4 changed files with 58 additions and 8 deletions.
2 changes: 2 additions & 0 deletions changelog/10811.bugfix.rst
Original file line number Diff line number Diff line change
@@ -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.
2 changes: 2 additions & 0 deletions src/_pytest/pathlib.py
Original file line number Diff line number Diff line change
Expand Up @@ -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)])
Expand Down
35 changes: 35 additions & 0 deletions testing/acceptance_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -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*")
27 changes: 19 additions & 8 deletions testing/test_pathlib.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand All @@ -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
)
Expand Down

0 comments on commit d2ae7d3

Please sign in to comment.