Warn when a collected test module was imported too early, and fix three cases where it is

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.

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.

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.

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``.

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.

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 <assertion-rewriting>`
 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 <http://pybites.blogspot.com/2011/07/behind-scenes-of-pytests-new-assertion.html>`_.
 
 Assertion rewriting caches files on disk

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

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(".")

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

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)
 

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
 

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
 

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: