diff --git a/changelog/10151.improvement.rst b/changelog/10151.improvement.rst new file mode 100644 index 00000000000..5d6e4e6572f --- /dev/null +++ b/changelog/10151.improvement.rst @@ -0,0 +1,6 @@ +When a fixture is not found, the error now points at definitions of that name +which exist elsewhere in the collection tree but are not visible to the test. + +Moving a fixture into a conftest one directory over leaves the name registered +but out of scope, and the previous message only listed the fixtures that *were* +available, giving no sign that the fixture was there all along. diff --git a/changelog/1511.improvement.rst b/changelog/1511.improvement.rst new file mode 100644 index 00000000000..d26e9d4529e --- /dev/null +++ b/changelog/1511.improvement.rst @@ -0,0 +1,13 @@ +pytest now warns with :class:`~pytest.PytestImportedFixtureWarning` when two +modules register the same fixture function, which is what importing a fixture in +order to reuse it does. + +The second registration is a separate :class:`~pytest.FixtureDef` for the same +function, so a session-scoped fixture reached that way runs once per registering +module, and the duplicate is invisible in the source of either file. The warning +names both modules and points at the line that brought the fixture in. + +Importing a fixture from a module that is not itself a plugin or a collected +module registers it only once, and is not warned about. Neither is inheriting a +fixture from a base class in another module, nor building one with a factory +defined elsewhere. diff --git a/doc/en/reference/fixtures.rst b/doc/en/reference/fixtures.rst index c4a8d01ff0e..0d174106c2d 100644 --- a/doc/en/reference/fixtures.rst +++ b/doc/en/reference/fixtures.rst @@ -141,6 +141,19 @@ You can have multiple nested directories/packages containing your tests, and each directory can have its own ``conftest.py`` with its own fixtures, adding on to the ones provided by the ``conftest.py`` files in parent directories. +.. warning:: + + Do not ``import`` a fixture in order to reuse it. Importing binds the + fixture in the importing module as well, so if the module it came from is + itself a plugin or a collected module, the fixture ends up registered + *twice*: a session-scoped fixture registered twice runs twice, and neither + file shows why. pytest emits + :class:`~pytest.PytestImportedFixtureWarning` when two modules register the + same fixture function. + + Reach the fixture through a ``conftest.py``, or make its module a plugin by + listing it in :globalvar:`pytest_plugins`, and drop the import. + For example, given a test file structure like this: :: diff --git a/doc/en/reference/reference.rst b/doc/en/reference/reference.rst index 54074262401..2ac2e9bf591 100644 --- a/doc/en/reference/reference.rst +++ b/doc/en/reference/reference.rst @@ -1311,6 +1311,9 @@ Custom warnings generated in some situations such as improper usage or deprecate .. autoclass:: pytest.PytestExperimentalApiWarning :show-inheritance: +.. autoclass:: pytest.PytestImportedFixtureWarning + :show-inheritance: + .. autoclass:: pytest.PytestReturnNotNoneWarning :show-inheritance: diff --git a/src/_pytest/fixtures.py b/src/_pytest/fixtures.py index 30f44d44dfc..011a2dd4ddc 100644 --- a/src/_pytest/fixtures.py +++ b/src/_pytest/fixtures.py @@ -75,6 +75,7 @@ from _pytest.scope import HIGH_SCOPES from _pytest.scope import Scope from _pytest.scope import ScopeName +from _pytest.warning_types import PytestImportedFixtureWarning from _pytest.warning_types import PytestWarning from _pytest.warning_types import warn_explicit_for @@ -958,6 +959,10 @@ def addfinalizer(self, finalizer: Callable[[], object]) -> None: self._fixturedef.addfinalizer(finalizer) +#: Definitions listed in a fixture-not-found hint before it collapses to a count. +_MAX_HINTED_DEFINITIONS = 5 + + @final class FixtureLookupError(LookupError): """Could not return a requested fixture (missing or invalid).""" @@ -1008,11 +1013,52 @@ def formatrepr(self) -> FixtureLookupErrorRepr: ) else: msg = f"fixture '{self.argname}' not found" + hint = self._out_of_scope_definitions_hint() + if hint is not None: + msg += f"\n {hint}" msg += "\n available fixtures: {}".format(", ".join(sorted(available))) msg += "\n use 'pytest --fixtures [testpath]' for help on them." return FixtureLookupErrorRepr(fspath, lineno, tblines, msg, self.argname) + def _out_of_scope_definitions_hint(self) -> str | None: + """Describe definitions of the missing name that exist but are out of scope. + + A fixture that was moved to a sibling conftest is still registered, just + not visible from here, and a bare "not found" sends the reader looking + for a name that is in front of them. See #10151. + + Only called once the name is known to be invisible to the requesting + node, so every definition found here is by construction out of scope. + """ + assert self.argname is not None + fm = self.request._fixturemanager + invocation_dir = self.request._pyfuncitem.config.invocation_params.dir + locations = set() + for fixturedef in fm._get_all_fixture_defs_for_name(self.argname): + try: + locations.add(_pretty_fixture_path(invocation_dir, fixturedef.func)) + except (AttributeError, TypeError): + # inspect.getfile() and __code__ are what can fail here. A + # fixture with no locatable source is worse than no hint, so + # drop that one rather than the whole hint. + continue + if not locations: + return None + shown = sorted(locations)[:_MAX_HINTED_DEFINITIONS] + hidden = len(locations) - len(shown) + places = ", ".join(shown) + if hidden: + places += f", and {hidden} more" + if len(locations) == 1: + return ( + f"hint: '{self.argname}' is defined in {places}, but not visible here" + ) + return ( + f"hint: '{self.argname}' is defined in {len(locations)} places, " + f"none visible here: {places}" + ) + class FixtureLookupErrorRepr(TerminalRepr): def __init__( @@ -1752,6 +1798,40 @@ def deduplicate_names(*seqs: Iterable[str]) -> tuple[str, ...]: return tuple(dict.fromkeys(name for seq in seqs for name in seq)) +def _warn_duplicate_fixture( + holderobj: types.ModuleType, + fixture_name: str, + func: Callable[..., object], + first: types.ModuleType, +) -> None: + origin = getattr(func, "__module__", None) or "another module" + message = PytestImportedFixtureWarning( + f"fixture '{fixture_name}', defined in '{origin}', is registered twice: " + f"by '{first.__name__}' and by '{holderobj.__name__}'.\n" + f"Importing a fixture registers another copy of it, so it can run more " + f"than once and shadow other definitions of the same name.\n" + f"Drop the import and reach it through a conftest.py, or list " + f"'{origin}' in 'pytest_plugins'." + ) + # Nothing records which line bound the name -- that is lost once the module + # is imported -- and the message already names both modules, so the file is + # the anchor. A module synthesised at runtime has no __file__; name it + # rather than drop the anchor entirely. + filename = getattr(holderobj, "__file__", None) or f"<{holderobj.__name__}>" + try: + warnings.warn_explicit( + message, + PytestImportedFixtureWarning, + filename=filename, + module=holderobj.__name__, + registry=holderobj.__dict__.setdefault("__warningregistry__", {}), + lineno=1, + ) + except Warning as w: + # Under -W error the location is dropped, so carry it in the message. + raise type(w)(f"{w}\n at {filename}") from None + + class FixtureManager: """pytest fixture definitions and information is stored and managed from this class. @@ -1801,6 +1881,13 @@ def __init__(self, session: Session) -> None: self._arg2nodeid2fixturedefs: Final[ dict[str, dict[str, list[FixtureDef[Any]]]] ] = {} + # The first module holder each fixture function was registered from, + # keyed by id() and holding the function to keep that id valid. + # A second module registering the same function is importing it, which + # registers a duplicate -- see #1511. + self._fixturefunc2module: Final[ + dict[int, tuple[types.ModuleType, Callable[..., object]]] + ] = {} # A mapping from a node to a list of autouse fixture names it defines. # The Session entry holds global usefixtures from config. self._node_autousenames: Final[dict[nodes.Node, list[str]]] = { @@ -2329,6 +2416,16 @@ def parsefactories( func = obj._get_wrapped_function() + # Only module holders: a class legitimately inherits fixtures + # from a base in another module, registering the same function + # once per subclass, and that is not a duplicate. + if isinstance(holderobj, types.ModuleType): + first = self._fixturefunc2module.setdefault( + id(func), (holderobj, func) + )[0] + if first is not holderobj: + _warn_duplicate_fixture(holderobj, fixture_name, func, first) + self._register_fixture( name=fixture_name, func=func, @@ -2352,6 +2449,16 @@ def _get_all_fixture_defs(self) -> Iterable[FixtureDef[Any]]: for fixturedefs in nodeid2fixturedefs.values(): yield from fixturedefs + def _get_all_fixture_defs_for_name(self, argname: str) -> Iterable[FixtureDef[Any]]: + """Get all FixtureDefs registered under a name, whatever their visibility. + + The order is not guaranteed. + """ + for fixturedefs in self._arg2node2fixturedefs.get(argname, {}).values(): + yield from fixturedefs + for fixturedefs in self._arg2nodeid2fixturedefs.get(argname, {}).values(): + yield from fixturedefs + def _get_all_fixture_defs_for_node( self, node: nodes.Node ) -> Iterable[FixtureDef[Any]]: diff --git a/src/_pytest/warning_types.py b/src/_pytest/warning_types.py index c5b339dec4c..c0540249655 100644 --- a/src/_pytest/warning_types.py +++ b/src/_pytest/warning_types.py @@ -82,6 +82,17 @@ class PytestReturnNotNoneWarning(PytestWarning): __module__ = "pytest" +@final +class PytestImportedFixtureWarning(PytestWarning): + """Warning emitted when two modules register the same fixture function, + which happens when one of them imported it from the other. + + See :ref:`conftest` for details. + """ + + __module__ = "pytest" + + @final class PytestUnknownMarkWarning(PytestWarning): """Warning emitted on use of unknown markers. diff --git a/src/pytest/__init__.py b/src/pytest/__init__.py index f6ea24f95a1..0fe5798160d 100644 --- a/src/pytest/__init__.py +++ b/src/pytest/__init__.py @@ -87,6 +87,7 @@ from _pytest.warning_types import PytestDeprecationWarning from _pytest.warning_types import PytestExperimentalApiWarning from _pytest.warning_types import PytestFDWarning +from _pytest.warning_types import PytestImportedFixtureWarning from _pytest.warning_types import PytestRemovedIn10Warning from _pytest.warning_types import PytestReturnNotNoneWarning from _pytest.warning_types import PytestUnhandledThreadExceptionWarning @@ -139,6 +140,7 @@ "PytestDeprecationWarning", "PytestExperimentalApiWarning", "PytestFDWarning", + "PytestImportedFixtureWarning", "PytestPluginManager", "PytestRemovedIn10Warning", "PytestReturnNotNoneWarning", diff --git a/testing/python/fixtures.py b/testing/python/fixtures.py index bc7b5a40cc2..34af0089ba9 100644 --- a/testing/python/fixtures.py +++ b/testing/python/fixtures.py @@ -718,6 +718,132 @@ def test_lookup_error(unknown): ) result.stdout.no_fnmatch_line("*INTERNAL*") + def test_funcarg_lookup_error_hints_at_out_of_scope_definition( + self, pytester: Pytester + ) -> None: + """A fixture moved into a sibling conftest is registered, just not here. + + The bare "not found" sends the reader hunting for a name that is + already in the tree, one directory over. See #10151. + """ + pytester.makepyfile( + **{ + "pkg/tests/conftest.py": """ + import pytest + + @pytest.fixture + def myfixture(): pass + """, + "pkg/tests/test_near.py": "def test_near(myfixture): pass", + "pkg/utils/tests/test_far.py": "def test_far(myfixture): pass", + } + ) + result = pytester.runpytest() + result.stdout.fnmatch_lines( + [ + "E fixture 'myfixture' not found", + "> hint: 'myfixture' is defined in pkg?tests?conftest.py:*," + " but not visible here", + ] + ) + result.assert_outcomes(passed=1, errors=1) + + def test_funcarg_lookup_error_hint_collapses_many_definitions( + self, pytester: Pytester + ) -> None: + """Past a handful of locations a count beats a wall of paths.""" + files = {"test_top.py": "def test_top(shared): pass"} + for name in "abcdefg": + files[f"{name}/conftest.py"] = """ + import pytest + + @pytest.fixture + def shared(): pass + """ + # Unique basenames: rootdir has no __init__.py, so equal ones collide. + files[f"{name}/test_use_{name}.py"] = f"def test_use_{name}(shared): pass" + pytester.makepyfile(**files) + result = pytester.runpytest() + result.stdout.fnmatch_lines( + [ + "E fixture 'shared' not found", + "> hint: 'shared' is defined in 7 places, none visible here:" + " a?conftest.py:*, and 2 more", + ] + ) + result.assert_outcomes(passed=7, errors=1) + + def test_funcarg_lookup_error_hints_at_a_legacy_nodeid_definition( + self, pytester: Pytester + ) -> None: + """Fixtures registered by the deprecated nodeid API are hinted at too. + + They live in a separate mapping, so the hint has to read both. + This test can be deleted with FIXTURE_NODEID_DEPRECATED deprecation. + """ + pytester.makeconftest( + """ + import pytest + + def pytest_collection_finish(session): + session._fixturemanager._register_fixture( + name="legacy", + func=lambda: 0, + nodeid="somewhere/else", + ) + """ + ) + pytester.makepyfile("def test_it(legacy): pass") + result = pytester.runpytest("-Wignore::pytest.PytestRemovedIn10Warning") + result.stdout.fnmatch_lines( + [ + "E fixture 'legacy' not found", + "> hint: 'legacy' is defined in conftest.py:*," + " but not visible here", + ] + ) + + def test_funcarg_lookup_error_hint_skips_a_fixture_with_no_source( + self, pytester: Pytester + ) -> None: + """One unlocatable definition must not cost the whole hint. + + A plugin may register a builtin or a C callable, which has no file to + point at; the remaining definitions are still worth naming. + """ + pytester.makeconftest( + """ + import pytest + + def pytest_collection_finish(session): + fm = session._fixturemanager + fm._register_fixture(name="mixed", func=len, nodeid="no/source") + fm._register_fixture( + name="mixed", func=lambda: 0, nodeid="somewhere/else" + ) + """ + ) + pytester.makepyfile("def test_it(mixed): pass") + result = pytester.runpytest("-Wignore::pytest.PytestRemovedIn10Warning") + result.stdout.fnmatch_lines( + [ + "E fixture 'mixed' not found", + "> hint: 'mixed' is defined in conftest.py:*," + " but not visible here", + ] + ) + + def test_funcarg_lookup_error_has_no_hint_for_an_unknown_name( + self, pytester: Pytester + ) -> None: + """The hint speaks about definitions that exist; this name has none.""" + pytester.makepyfile("def test_it(never_defined_anywhere): pass") + result = pytester.runpytest() + result.stdout.fnmatch_lines( + ["E fixture 'never_defined_anywhere' not found"] + ) + result.stdout.no_fnmatch_line("*hint:*") + def test_fixture_excinfo_leak(self, pytester: Pytester) -> None: # on python2 sys.excinfo would leak into fixture executions pytester.makepyfile( @@ -743,6 +869,290 @@ def test_leak(leak): assert result.ret == 0 +#: pytest's own suite turns warnings into errors; the inner runs opt back out. +_SHOW_DUPLICATE_WARNING = "-Wdefault::pytest.PytestImportedFixtureWarning" + + +class TestImportedFixtureWarning: + """Importing a fixture can register it twice; say so. See #1511. + + The harm is the duplicate registration, not the import, so every case here + turns on whether two *module* holders end up registering the same function. + """ + + def test_two_conftests_importing_one_fixture_warn(self, pytester: Pytester) -> None: + """The reported bug: one definition, two registrations, no sign in either file.""" + pytester.makepyfile( + **{ + "helpers.py": """ + import pytest + + @pytest.fixture + def shared(): return 1 + """, + "a/conftest.py": "from helpers import shared", + "a/test_a.py": "def test_a(shared): assert shared == 1", + "b/conftest.py": "from helpers import shared", + "b/test_b.py": "def test_b(shared): assert shared == 1", + } + ) + # The conftests live in subdirectories; only their own dirs get onto + # sys.path, so make the shared module importable from both. + pytester.syspathinsert() + result = pytester.runpytest(_SHOW_DUPLICATE_WARNING) + result.stdout.fnmatch_lines( + [ + "*conftest.py:1: PytestImportedFixtureWarning: fixture 'shared'," + " defined in 'helpers', is registered twice: by '*conftest' and" + " by '*conftest'.", + ] + ) + result.assert_outcomes(passed=2, warnings=1) + + def test_importing_from_a_collected_module_warns(self, pytester: Pytester) -> None: + """The defining module is itself a holder, so the import is the second one.""" + pytester.makepyfile( + test_helpers=""" + import pytest + + @pytest.fixture + def shared(): return 1 + + def test_helpers(shared): assert shared == 1 + """, + test_it=""" + from test_helpers import shared + + def test_it(shared): assert shared == 1 + """, + ) + result = pytester.runpytest(_SHOW_DUPLICATE_WARNING) + result.stdout.fnmatch_lines( + [ + "*test_it.py:1: PytestImportedFixtureWarning: fixture 'shared'," + " defined in 'test_helpers', is registered twice*", + ] + ) + result.assert_outcomes(passed=2, warnings=1) + + def test_importing_from_a_module_that_is_never_a_holder_does_not_warn( + self, pytester: Pytester + ) -> None: + """One registration is not a duplicate, whatever route the name took. + + ``helpers`` is never collected and never a plugin, so the fixture is + registered exactly once and behaves exactly as if it were defined here. + """ + pytester.makepyfile( + helpers=""" + import pytest + + @pytest.fixture + def shared(): return 1 + """, + test_it=""" + from helpers import shared + + def test_it(shared): assert shared == 1 + """, + ) + result = pytester.runpytest() + result.assert_outcomes(passed=1, warnings=0) + + def test_plugin_split_across_its_own_modules_does_not_warn( + self, pytester: Pytester + ) -> None: + """A plugin keeping its fixtures in a sibling module registers them once. + + This is pytest-django's layout, and it tripped the first version of + this check. + """ + pytester.makepyfile( + **{ + "myplugin/__init__.py": "", + "myplugin/fixtures.py": """ + import pytest + + @pytest.fixture + def shared(): return 1 + """, + "myplugin/plugin.py": "from myplugin.fixtures import shared", + "conftest.py": "pytest_plugins = ['myplugin.plugin']", + "test_it.py": "def test_it(shared): assert shared == 1", + } + ) + result = pytester.runpytest() + result.assert_outcomes(passed=1, warnings=0) + + def test_defining_module_registered_last_is_still_reported( + self, pytester: Pytester + ) -> None: + """A conftest is parsed before the test module it imported the fixture from. + + The second holder is then the module that *defines* the fixture, so the + pair is reported the other way round. + """ + pytester.makepyfile( + **{ + "conftest.py": "from test_helpers import shared", + "test_helpers.py": """ + import pytest + + @pytest.fixture + def shared(): return 1 + """, + "test_it.py": "def test_it(shared): assert shared == 1", + } + ) + result = pytester.runpytest(_SHOW_DUPLICATE_WARNING) + result.stdout.fnmatch_lines( + [ + "*test_helpers.py:1: PytestImportedFixtureWarning: fixture 'shared'," + " defined in 'test_helpers', is registered twice: by 'conftest' and" + " by 'test_helpers'." + ] + ) + + def test_the_location_survives_being_turned_into_an_error( + self, pytester: Pytester + ) -> None: + """-W error drops the location, so the message has to carry it. + + This is how the warning surfaces in a suite that errors on warnings, + which is how it first showed up in pytest's own plugin CI. + """ + pytester.makepyfile( + test_helpers=""" + import pytest + + @pytest.fixture + def shared(): return 1 + """, + test_it=""" + from test_helpers import shared + + def test_it(shared): assert shared == 1 + """, + ) + result = pytester.runpytest("-Werror::pytest.PytestImportedFixtureWarning") + result.stdout.fnmatch_lines(["*at *test_it.py"]) + assert result.ret != 0 + + def test_a_module_with_no_file_is_named_instead_of_located( + self, pytester: Pytester + ) -> None: + """A plugin module built at runtime still gets an anchor, just not a path.""" + pytester.makepyfile( + helpers=""" + import pytest + + @pytest.fixture + def shared(): return 1 + """, + **{ + "conftest.py": """ + import sys + import types + + synthetic = types.ModuleType("synthetic_plugin") + exec("from helpers import shared", synthetic.__dict__) + sys.modules["synthetic_plugin"] = synthetic + + pytest_plugins = ["helpers", "synthetic_plugin"] + """, + "test_it.py": "def test_it(shared): assert shared == 1", + }, + ) + # Registering a plugin happens before the per-item warning capture, so + # turn it into an error to get it onto the inner run's stdout. + result = pytester.runpytest("-Werror::pytest.PytestImportedFixtureWarning") + result.stdout.fnmatch_lines( + [ + "*fixture 'shared', defined in 'helpers', is registered twice:" + " by 'helpers' and by 'synthetic_plugin'.", + "*at ", + ] + ) + assert result.ret != 0 + + def test_inheriting_a_fixture_from_another_module_does_not_warn( + self, pytester: Pytester + ) -> None: + """Two subclasses register the same function by design, once each.""" + pytester.makepyfile( + base=""" + import pytest + + class BaseTests: + @pytest.fixture + def shared(self): return 1 + """, + test_it=""" + from base import BaseTests + + class TestOne(BaseTests): + def test_one(self, shared): assert shared == 1 + + class TestTwo(BaseTests): + def test_two(self, shared): assert shared == 1 + """, + ) + result = pytester.runpytest() + result.assert_outcomes(passed=2, warnings=0) + + def test_fixture_built_by_a_factory_elsewhere_does_not_warn( + self, pytester: Pytester + ) -> None: + """A factory hands back a fresh function per call, so two calls are two fixtures.""" + pytester.makepyfile( + helpers=""" + import pytest + + def make_fixture(value): + @pytest.fixture + def produced(): return value + return produced + """, + **{ + "a/conftest.py": """ + from helpers import make_fixture + + produced = make_fixture(1) + """, + "a/test_a.py": "def test_a(produced): assert produced == 1", + "b/conftest.py": """ + from helpers import make_fixture + + produced = make_fixture(2) + """, + "b/test_b.py": "def test_b(produced): assert produced == 2", + }, + ) + pytester.syspathinsert() + result = pytester.runpytest() + result.assert_outcomes(passed=2, warnings=0) + + def test_a_plugin_module_defining_its_own_fixtures_does_not_warn( + self, pytester: Pytester + ) -> None: + """`pytest_plugins` is the recommended alternative; it must stay quiet.""" + pytester.makepyfile( + helpers=""" + import pytest + + @pytest.fixture + def shared(): return 1 + """, + test_it=""" + pytest_plugins = ["helpers"] + + def test_it(shared): assert shared == 1 + """, + ) + result = pytester.runpytest() + result.assert_outcomes(passed=1, warnings=0) + + class TestRequestBasic: def test_request_attributes(self, pytester: Pytester) -> None: item = pytester.getitem(