diff --git a/changelog/1930.bugfix.1.rst b/changelog/1930.bugfix.1.rst new file mode 100644 index 00000000000..2bc428e5d0c --- /dev/null +++ b/changelog/1930.bugfix.1.rst @@ -0,0 +1,6 @@ +Assertions in a package's ``__init__.py`` are now rewritten when that file is named on the command line. + +The early-bailout optimisation derived the module basenames to consider from the +initial paths, which for ``pkg/__init__.py`` yielded ``__init__`` rather than the +importable name ``pkg``, so the rewrite hook declined the module before it was +recognised as an initial path. diff --git a/changelog/1930.bugfix.2.rst b/changelog/1930.bugfix.2.rst new file mode 100644 index 00000000000..20ef9f32f9e --- /dev/null +++ b/changelog/1930.bugfix.2.rst @@ -0,0 +1,5 @@ +Assertions in a package's ``__init__.py`` are now rewritten under ``--import-mode=importlib``. + +The meta path finders were consulted for the package using the package's own +directory as the search path rather than the directory containing it, so the +assertion-rewrite hook never matched and the file was loaded unrewritten. diff --git a/changelog/1930.bugfix.3.rst b/changelog/1930.bugfix.3.rst new file mode 100644 index 00000000000..1f681fb26b5 --- /dev/null +++ b/changelog/1930.bugfix.3.rst @@ -0,0 +1,6 @@ +``--pyargs`` arguments that name a file rather than a module no longer import that file while resolving it. + +Resolving ``--pyargs t.py`` asked importlib for ``t.py``, which imports ``t`` +as its parent package. The test module ended up in ``sys.modules`` before +collection, and a module which is already imported can no longer have its +assertions rewritten. diff --git a/changelog/1930.doc.rst b/changelog/1930.doc.rst new file mode 100644 index 00000000000..8361395b50d --- /dev/null +++ b/changelog/1930.doc.rst @@ -0,0 +1 @@ +Documented that a test module which has already been imported when collection reaches it cannot have its assertions rewritten, and how to avoid importing one too early from a ``conftest.py``. diff --git a/changelog/1930.improvement.rst b/changelog/1930.improvement.rst new file mode 100644 index 00000000000..a55181a8a6a --- /dev/null +++ b/changelog/1930.improvement.rst @@ -0,0 +1,5 @@ +pytest now warns with :class:`~pytest.PytestAssertRewriteWarning` when it collects a test module that was already imported, and whose assertions therefore could not be rewritten. + +This most commonly happens when a ``conftest.py`` imports a module at the top +level which is only recognised as a test module because it was named on the +command line. Until now the loss of assertion introspection was silent. diff --git a/doc/en/how-to/assert.rst b/doc/en/how-to/assert.rst index 5564aa9ce83..637e8852d7b 100644 --- a/doc/en/how-to/assert.rst +++ b/doc/en/how-to/assert.rst @@ -499,6 +499,16 @@ the conftest file: FAILED test_foocompare.py::test_compare - assert Comparing Foo instances: 1 failed in 0.12s +.. note:: + + The ``conftest.py`` above imports the test module at the top level. That is + safe here only because ``test_foocompare.py`` matches the ``python_files`` + patterns, so pytest rewrites it whoever imports it first. Importing a module + that pytest would only recognise as a test module for another reason -- because + it was named on the command line, for instance -- costs that module its + assertion introspection; see :ref:`assert-details` below. Moving the import + inside the hook body avoids the question entirely. + .. _`return-not-none`: Returning non-None value in test functions @@ -560,6 +570,14 @@ You can manually enable assertion rewriting for an imported module by calling :ref:`register_assert_rewrite ` before you import it (a good place to do that is in your root ``conftest.py``). +Rewriting happens on import, so a module which has *already* been imported by the +time collection reaches it cannot be rewritten any more, even if it is a test +module. The usual cause is a top-level ``import`` in a ``conftest.py`` or a +plugin. pytest emits a :class:`pytest.PytestAssertRewriteWarning` when it +collects such a module; either delay the import until after collection -- moving +it into the hook or fixture that needs it -- or call +:func:`pytest.register_assert_rewrite` before it. + For further information, Benjamin Peterson wrote up `Behind the scenes of pytest's new assertion rewriting `_. Assertion rewriting caches files on disk diff --git a/src/_pytest/assertion/__init__.py b/src/_pytest/assertion/__init__.py index a171633f320..f40994aa049 100644 --- a/src/_pytest/assertion/__init__.py +++ b/src/_pytest/assertion/__init__.py @@ -4,10 +4,12 @@ from __future__ import annotations from collections.abc import Generator +import os import sys from typing import Any from typing import Protocol from typing import TYPE_CHECKING +import warnings from _pytest.assertion import rewrite from _pytest.assertion import truncate @@ -23,6 +25,8 @@ if TYPE_CHECKING: + from types import ModuleType + from _pytest.main import Session @@ -147,6 +151,41 @@ def undo() -> None: return hook +def warn_if_not_rewritten( + config: Config, mod: ModuleType, path: os.PathLike[str] +) -> None: + """Warn if *mod* should have been assertion-rewritten but was imported too early. + + Rewriting only happens on import, so a module which something else -- a + conftest, a plugin, another test module -- has already imported by the time + collection reaches it silently loses assertion introspection (#1930). + """ + from _pytest.warning_types import PytestAssertRewriteWarning + + state = config.stash.get(assertstate_key, None) + if state is None or state.hook is None: + # Rewriting is disabled (``--assert=plain``) or the plugin is blocked. + return + hook = state.hook + loader = mod.__spec__.loader if mod.__spec__ is not None else None + if isinstance(loader, type(hook)): + return + if rewrite.AssertionRewriter.is_rewrite_disabled(mod.__doc__ or ""): + return + if not hook._should_rewrite(mod.__name__, os.fspath(path), state): + return + warnings.warn( + PytestAssertRewriteWarning( + f"Module {mod.__name__!r} ({os.fspath(path)}) was already imported " + f"when pytest collected it, so its assertions were not rewritten " + f"and will not be introspected.\n" + f"It was most likely imported by a conftest file or a plugin. " + f"Delay that import until after collection, or call " + f"pytest.register_assert_rewrite({mod.__name__!r}) before it." + ) + ) + + def pytest_collection(session: Session) -> None: # This hook is only called when test modules are collected # so for example not in the managing process of pytest-xdist diff --git a/src/_pytest/assertion/rewrite.py b/src/_pytest/assertion/rewrite.py index 27953336c5c..29d76b69af7 100644 --- a/src/_pytest/assertion/rewrite.py +++ b/src/_pytest/assertion/rewrite.py @@ -194,6 +194,11 @@ def _early_rewrite_bailout(self, name: str, state: AssertionState) -> bool: parts = str(initial_path).split(os.sep) # add 'path' to basenames to be checked. self._basenames_to_check_rewrite.add(os.path.splitext(parts[-1])[0]) + if parts[-1] == "__init__.py" and len(parts) > 1: + # A package's ``__init__.py`` is imported under the name of + # the directory containing it, so that is the basename which + # has to survive the bailout below. + self._basenames_to_check_rewrite.add(parts[-2]) # Note: conftest already by default in _basenames_to_check_rewrite. parts = name.split(".") diff --git a/src/_pytest/main.py b/src/_pytest/main.py index d43a68b4679..9a212d601dc 100644 --- a/src/_pytest/main.py +++ b/src/_pytest/main.py @@ -1069,6 +1069,12 @@ def search_pypath( ) -> str | None: """Search sys.path for the given a dotted module name, and return its file system path if found.""" + if module_name.endswith(".py"): + # Looks like a package module, but is actually a filename. Asking + # importlib about it would import everything up to the last dot as a + # package -- for `t.py` that imports `t`, and a test module which is + # already in sys.modules can no longer be assertion-rewritten (#1930). + return None try: spec = importlib.util.find_spec(module_name) # AttributeError: looks like package module, but actually filename diff --git a/src/_pytest/pathlib.py b/src/_pytest/pathlib.py index 8eb593d194d..975007fa211 100644 --- a/src/_pytest/pathlib.py +++ b/src/_pytest/pathlib.py @@ -734,7 +734,11 @@ def _import_module_using_spec( # Checking with sys.meta_path first in case one of its hooks can import this module, # such as our own assertion-rewrite hook. - find_spec_path = [str(module_path.parent)] + if module_path.name == "__init__.py": + # A package is found in the directory *containing* the package directory. + find_spec_path = [str(module_path.parent.parent)] + else: + find_spec_path = [str(module_path.parent)] for meta_importer in sys.meta_path: spec = meta_importer.find_spec(module_name, find_spec_path) diff --git a/src/_pytest/python.py b/src/_pytest/python.py index bc3d55243f7..a87416e5b79 100644 --- a/src/_pytest/python.py +++ b/src/_pytest/python.py @@ -42,6 +42,7 @@ from _pytest._code.code import TerminalRepr from _pytest._code.code import Traceback from _pytest._io.saferepr import saferepr +from _pytest.assertion import warn_if_not_rewritten from _pytest.compat import ascii_escaped from _pytest.compat import get_default_arg_names from _pytest.compat import get_real_func @@ -570,6 +571,7 @@ def importtestmodule( "If you want to skip a specific test or an entire class, " "use the @pytest.mark.skip or @pytest.mark.skipif decorators." ) from e + warn_if_not_rewritten(config, mod, path) config.pluginmanager.consider_module(mod) return mod diff --git a/testing/acceptance_test.py b/testing/acceptance_test.py index 3983bd0007d..7310b4e0846 100644 --- a/testing/acceptance_test.py +++ b/testing/acceptance_test.py @@ -692,6 +692,20 @@ def test_pyargs_filename_looks_like_module(self, pytester: Pytester) -> None: result = pytester.runpytest("--pyargs", "t.py") assert result.ret == ExitCode.OK + def test_pyargs_filename_looks_like_module_is_rewritten( + self, pytester: Pytester + ) -> None: + """The argument must not be imported while resolving it (#1930). + + Asking importlib about `t.py` imports `t` as its parent package, and a + module already in sys.modules cannot be assertion-rewritten any more. + """ + pytester.path.joinpath("t.py").write_text( + "def test():\n x = 1\n assert x == 2\n", encoding="utf-8" + ) + result = pytester.runpytest("--pyargs", "t.py") + result.stdout.fnmatch_lines(["E*assert 1 == 2"]) + def test_cmdline_python_package(self, pytester: Pytester, monkeypatch) -> None: import warnings diff --git a/testing/test_assertrewrite.py b/testing/test_assertrewrite.py index c9736f8fa48..d79d05d5ee4 100644 --- a/testing/test_assertrewrite.py +++ b/testing/test_assertrewrite.py @@ -1253,6 +1253,49 @@ def test_rewritten(): ) assert pytester.runpytest_subprocess().ret == 0 + def test_warn_collected_module_imported_too_early(self, pytester: Pytester) -> None: + """Collecting an already-imported test module warns (#1930). + + The module is only a test module because it was named on the + command line, so the conftest import wins the race and assertion + introspection is silently lost. + """ + pytester.makeconftest("import foo") + pytester.makepyfile( + foo=""" + def test_compare(): + x = 1 + assert x == 2 + """ + ) + # needs to be a subprocess because pytester explicitly disables this warning + result = pytester.runpytest_subprocess("foo.py") + result.stdout.fnmatch_lines( + [ + "*PytestAssertRewriteWarning: Module 'foo'*was already imported*", + "*pytest.register_assert_rewrite('foo')*", + ] + ) + + def test_no_warning_when_import_delayed(self, pytester: Pytester) -> None: + """Importing the module from inside a hook leaves rewriting intact.""" + pytester.makeconftest( + """ + def pytest_assertrepr_compare(op, left, right): + import foo # noqa: F401 + """ + ) + pytester.makepyfile( + foo=""" + def test_compare(): + x = 1 + assert x == 2 + """ + ) + result = pytester.runpytest_subprocess("foo.py") + result.stdout.fnmatch_lines(["E*assert 1 == 2"]) + result.stdout.no_fnmatch_line("*PytestAssertRewriteWarning*") + def test_remember_rewritten_modules( self, pytestconfig, pytester: Pytester, monkeypatch ) -> None: @@ -1873,6 +1916,33 @@ def spy_write_pyc(*args, **kwargs): assert len(write_pyc_called) == 1 +def test_rewrite_package_init_with_importlib_mode(pytester: Pytester) -> None: + """A package's ``__init__.py`` is rewritten under ``--import-mode=importlib``. + + The meta path finder has to be asked for the package in the directory + *containing* it, not in the package directory itself (#1930). + """ + pytester.makeini( + """ + [pytest] + python_files = *.py + pythonpath = . + """ + ) + pytester.makepyfile( + **{ + "pkg/__init__.py": "", + "pkg/sub/__init__.py": """\ + def test_init(): + x = 1 + assert x == 2 + """, + } + ) + result = pytester.runpytest("--import-mode=importlib") + result.stdout.fnmatch_lines(["E*assert 1 == 2"]) + + class TestEarlyRewriteBailout: @pytest.fixture def hook( @@ -1937,6 +2007,25 @@ def fix(): return 1 assert hook.find_spec("foobar") is not None assert self.find_spec_calls == ["conftest", "test_foo", "foobar"] + def test_package_init_given_as_initial_path(self, pytester: Pytester) -> None: + """A package ``__init__.py`` named on the command-line is rewritten. + + The bailout derives the basenames to check from the initial paths, which + for an ``__init__.py`` is the name of the package directory, not + ``__init__`` (#1930). + """ + pytester.makepyfile( + **{ + "sub/__init__.py": """\ + def test_init(): + x = 1 + assert x == 2 + """ + } + ) + result = pytester.runpytest("sub/__init__.py") + result.stdout.fnmatch_lines(["E*assert 1 == 2"]) + def test_pattern_contains_subdirectories( self, pytester: Pytester, hook: AssertionRewritingHook ) -> None: