diff --git a/changelog/13564.improvement.rst b/changelog/13564.improvement.rst new file mode 100644 index 00000000000..af8346eef17 --- /dev/null +++ b/changelog/13564.improvement.rst @@ -0,0 +1,4 @@ +Issue a warning when fixtures are wrapped with a decorator that prevents +correct discovery, including ``@classmethod`` above ``@pytest.fixture`` and +``@staticmethod`` above ``@pytest.fixture``. Put ``@pytest.fixture`` above +those decorators on test classes. diff --git a/src/_pytest/fixtures.py b/src/_pytest/fixtures.py index f4ca2eac455..6e1c63a4b53 100644 --- a/src/_pytest/fixtures.py +++ b/src/_pytest/fixtures.py @@ -76,6 +76,7 @@ from _pytest.scope import Scope from _pytest.scope import ScopeName from _pytest.warning_types import PytestWarning +from _pytest.warning_types import warn_explicit_for if sys.version_info < (3, 11): @@ -1409,7 +1410,9 @@ def __init__( def __repr__(self) -> str: return f"" - def __get__(self, instance, owner=None): + def __get__( + self, instance: object, owner: type | None = None + ) -> FixtureFunctionDefinition: """Behave like a method if the function it was applied to was a method.""" return FixtureFunctionDefinition( function=self._fixture_function, @@ -2024,6 +2027,99 @@ def _register_fixture( # Global plugin autouse fixtures go under Session. self._node_autousenames.setdefault(self.session, []).append(name) + def _lookup_in_type_dict( + self, owner: type | types.ModuleType, name: str + ) -> object | None: + """Return ``name`` from ``owner``'s ``__dict__`` without invoking descriptors. + + For classes, walks the MRO so inherited ``staticmethod`` / + ``classmethod`` wrappers are visible. ``getattr`` / descriptor + ``__get__`` would hide those wrappers (bound methods, or the bare + :class:`FixtureFunctionDefinition` under a ``staticmethod``). + """ + if isinstance(owner, types.ModuleType): + return owner.__dict__.get(name) + for base in owner.__mro__: + try: + return cast(object, base.__dict__[name]) + except KeyError: + continue + return None + + def _find_wrapped_fixture_def( + self, obj: object + ) -> FixtureFunctionDefinition | None: + """Find a FixtureFunctionDefinition in ``obj``'s ``__wrapped__`` chain. + + Uses :func:`inspect.unwrap` with a ``stop`` callback so unwrapping does + not continue past :class:`FixtureFunctionDefinition` (which itself sets + ``__wrapped__`` via :func:`functools.update_wrapper`). + + Returns the definition if found, else ``None``. Loops, excessive depth, + and objects that raise on attribute access are treated as not found. + """ + # Skip mock objects to avoid false positives when traversing + # their wrapper chains (they have a _mock_name attribute). + if safe_getattr(obj, "_mock_name", None) is not None: + return None + + try: + # unwrap is typed for callables, but we also pass descriptors + # (classmethod/staticmethod) that expose __wrapped__/__func__. + # Use type(...) is, not isinstance: some proxies raise on + # isinstance via a broken __class__ (see pytest#4266). + unwrapped = inspect.unwrap( + cast(Callable[..., Any], obj), + stop=lambda o: type(o) is FixtureFunctionDefinition, + ) + except Exception: + # ValueError for wrapper loops / depth; also "evil objects" + # that raise on attribute access (see pytest#214). + return None + + if type(unwrapped) is FixtureFunctionDefinition: + return unwrapped + return None + + def _check_for_wrapped_fixture(self, name: str, obj: object) -> None: + """Warn if ``obj`` is a fixture hidden behind wrappers. + + ``obj`` must be the raw class/module ``__dict__`` entry (see + :meth:`_lookup_in_type_dict`), not the result of ``getattr``. + + ``@staticmethod`` above ``@pytest.fixture`` is always warned: discovery + may still succeed, but the decorator order is wrong. A leading + ``self``/``cls`` parameter already fails as a missing fixture and needs + no extra special-casing here. + """ + fixture_def = self._find_wrapped_fixture_def(obj) + # None: not a wrapped fixture. is obj: bare FixtureFunctionDefinition. + if fixture_def is None or fixture_def is obj: + return + + # warn_explicit_for needs a real function (__code__/__globals__). + # @pytest.fixture above @classmethod/@staticmethod stores the descriptor + # as _fixture_function; peel once to reach the underlying def. + fixture_func: object = fixture_def._get_wrapped_function() + if type(fixture_func) is classmethod or type(fixture_func) is staticmethod: + fixture_func = fixture_func.__func__ + if not isinstance(fixture_func, types.FunctionType): # pragma: no cover + return + + if type(obj) is classmethod: + msg = ( + f"cannot discover fixture {name!r} because it is wrapped by " + f"@classmethod; place @pytest.fixture above @classmethod" + ) + elif type(obj) is staticmethod: + msg = ( + f"fixture {name!r} is wrapped by @staticmethod above " + f"@pytest.fixture; place @pytest.fixture above @staticmethod" + ) + else: + msg = f"cannot discover fixture {name!r} due to being wrapped in decorators" + warn_explicit_for(fixture_func, PytestWarning(msg)) + @overload def parsefactories( self, @@ -2096,16 +2192,30 @@ def parsefactories( effective_node = node_or_obj # Avoid accessing `@property` (and other descriptors) when iterating fixtures. + holderobj_tp: type | types.ModuleType if not safe_isclass(holderobj) and not isinstance(holderobj, types.ModuleType): - holderobj_tp: object = type(holderobj) + holderobj_tp = type(holderobj) else: - holderobj_tp = holderobj + holderobj_tp = cast("type | types.ModuleType", holderobj) for name in dir(holderobj): + # Read the raw __dict__ entry first so staticmethod/classmethod + # wrappers are not hidden by descriptor binding. + raw_obj = self._lookup_in_type_dict(holderobj_tp, name) + if raw_obj is not None: + self._check_for_wrapped_fixture(name, raw_obj) + # The attribute can be an arbitrary descriptor, so the attribute # access below can raise. safe_getattr() ignores such exceptions. obj_ub = safe_getattr(holderobj_tp, name, None) if type(obj_ub) is FixtureFunctionDefinition: + # On Python 3.9-3.12, classmethod chains through descriptors, so + # getattr may return a FixtureFunctionDefinition even when the + # raw __dict__ entry is classmethod(fixture). Do not register + # that case; users must put @pytest.fixture above @classmethod. + if type(raw_obj) is classmethod: + continue + marker = obj_ub._fixture_function_marker if marker.name: fixture_name = marker.name diff --git a/src/_pytest/legacypath.py b/src/_pytest/legacypath.py index 59e8ef6e742..e1fc38d19ed 100644 --- a/src/_pytest/legacypath.py +++ b/src/_pytest/legacypath.py @@ -251,8 +251,8 @@ def __str__(self) -> str: class LegacyTestdirPlugin: - @staticmethod @fixture + @staticmethod def testdir(pytester: Pytester) -> Testdir: """ Identical to :fixture:`pytester`, and provides an instance whose methods return @@ -294,15 +294,15 @@ def getbasetemp(self) -> LEGACY_PATH: class LegacyTmpdirPlugin: - @staticmethod @fixture(scope="session") + @staticmethod def tmpdir_factory(request: FixtureRequest) -> TempdirFactory: """Return a :class:`pytest.TempdirFactory` instance for the test session.""" # Set dynamically by pytest_configure(). return request.config._tmpdirhandler # type: ignore - @staticmethod @fixture + @staticmethod def tmpdir(tmp_path: Path) -> LEGACY_PATH: """Return a temporary directory (as `legacy_path`_ object) which is unique to each test function invocation. @@ -456,7 +456,10 @@ def pytest_configure(config: Config) -> None: _tmpdirhandler = TempdirFactory(tmp_path_factory, _ispytest=True) mp.setattr(config, "_tmpdirhandler", _tmpdirhandler, raising=False) - config.pluginmanager.register(LegacyTmpdirPlugin, "legacypath-tmpdir") + # Register an instance so @fixture above @staticmethod unwraps to a bare + # function. Class registration would keep the staticmethod descriptor, + # which resolve_fixture_function then binds onto test instances (#13564). + config.pluginmanager.register(LegacyTmpdirPlugin(), "legacypath-tmpdir") @hookimpl @@ -464,5 +467,5 @@ def pytest_plugin_registered(plugin: object, manager: PytestPluginManager) -> No # pytester is not loaded by default and is commonly loaded from a conftest, # so checking for it in `pytest_configure` is not enough. is_pytester = plugin is manager.get_plugin("pytester") - if is_pytester and not manager.is_registered(LegacyTestdirPlugin): - manager.register(LegacyTestdirPlugin, "legacypath-pytester") + if is_pytester and not manager.has_plugin("legacypath-pytester"): + manager.register(LegacyTestdirPlugin(), "legacypath-pytester") diff --git a/testing/python/fixtures.py b/testing/python/fixtures.py index 7000eeccbbd..69044226b6d 100644 --- a/testing/python/fixtures.py +++ b/testing/python/fixtures.py @@ -5794,3 +5794,230 @@ def test_foobar(self, fixture_bar): ) result = pytester.runpytest("-v") result.assert_outcomes(passed=1) + + +@pytest.mark.filterwarnings("default:cannot discover fixture *:pytest.PytestWarning") +def test_custom_decorated_fixture_warning(pytester: Pytester) -> None: + """Fixtures wrapped by custom decorators using functools.wraps warn.""" + pytester.makepyfile( + """ + import pytest + import functools + + def custom_deco(func): + @functools.wraps(func) + def wrapper(*args, **kwargs): + return func(*args, **kwargs) + return wrapper + + class TestClass: + @custom_deco + @pytest.fixture + def my_fixture(self): + return "fixture_value" + + def test_fixture_usage(self, my_fixture): + assert my_fixture == "fixture_value" + """ + ) + result = pytester.runpytest_inprocess( + "-v", "-rw", "-W", "default::pytest.PytestWarning" + ) + + result.stdout.fnmatch_lines( + [ + "*test_custom_decorated_fixture_warning.py:*: " + "PytestWarning: cannot discover fixture 'my_fixture' " + "due to being wrapped in decorators*" + ] + ) + + result.stdout.fnmatch_lines(["*fixture 'my_fixture' not found*"]) + result.assert_outcomes(errors=1) + + +@pytest.mark.filterwarnings("default:cannot discover fixture *:pytest.PytestWarning") +def test_custom_decorated_fixture_above_classmethod_warning( + pytester: Pytester, +) -> None: + """Warn when wraps hides a fixture that itself wraps @classmethod. + + The fixture definition stores the classmethod descriptor; warning emission + peels it to reach the underlying function for warn_explicit_for. + """ + pytester.makepyfile( + """ + import pytest + import functools + + def custom_deco(func): + @functools.wraps(func) + def wrapper(*args, **kwargs): + return func(*args, **kwargs) + return wrapper + + class TestClass: + @custom_deco + @pytest.fixture(scope="class") + @classmethod + def my_fixture(cls): + return "fixture_value" + + def test_fixture_usage(self, my_fixture): + assert my_fixture == "fixture_value" + """ + ) + result = pytester.runpytest_inprocess( + "-v", "-rw", "-W", "default::pytest.PytestWarning" + ) + + result.stdout.fnmatch_lines( + [ + "*PytestWarning: cannot discover fixture 'my_fixture' " + "due to being wrapped in decorators*" + ] + ) + result.stdout.fnmatch_lines(["*fixture 'my_fixture' not found*"]) + result.assert_outcomes(errors=1) + + +@pytest.mark.filterwarnings("default:cannot discover fixture *:pytest.PytestWarning") +def test_classmethod_above_fixture_warning(pytester: Pytester) -> None: + """@classmethod above @pytest.fixture hides the fixture (#13507).""" + pytester.makepyfile( + """ + import pytest + + class TestFixture: + @classmethod + @pytest.fixture(scope="class") + def fixt(cls): + return 1 + + def test_fixt(self, fixt): + assert fixt == 1 + """ + ) + result = pytester.runpytest_inprocess( + "-v", "-rw", "-W", "default::pytest.PytestWarning" + ) + + result.stdout.fnmatch_lines( + [ + "*test_classmethod_above_fixture_warning.py:*: " + "PytestWarning: cannot discover fixture 'fixt' because it is " + "wrapped by @classmethod; place @pytest.fixture above @classmethod*" + ] + ) + result.stdout.fnmatch_lines(["*fixture 'fixt' not found*"]) + result.assert_outcomes(errors=1) + + +@pytest.mark.filterwarnings( + "default:fixture * is wrapped by @staticmethod*:pytest.PytestWarning" +) +def test_staticmethod_above_fixture_warning(pytester: Pytester) -> None: + """@staticmethod above @pytest.fixture always warns. + + Unlike ``classmethod``, discovery still finds the fixture via + ``staticmethod.__get__``, so the test can pass; a leading ``self``/``cls`` + already fails as a missing fixture without special-casing here. + """ + pytester.makepyfile( + """ + import pytest + + class TestFixture: + @staticmethod + @pytest.fixture + def fixt(): + return 1 + + def test_fixt(self, fixt): + assert fixt == 1 + """ + ) + result = pytester.runpytest_inprocess( + "-v", "-rw", "-W", "default::pytest.PytestWarning" + ) + + result.stdout.fnmatch_lines( + [ + "*test_staticmethod_above_fixture_warning.py:*: " + "PytestWarning: fixture 'fixt' is wrapped by @staticmethod above " + "@pytest.fixture; place @pytest.fixture above @staticmethod*" + ] + ) + result.assert_outcomes(passed=1) + + +def test_fixture_above_classmethod_still_works(pytester: Pytester) -> None: + """Documented order @pytest.fixture above @classmethod remains discoverable.""" + pytester.makepyfile( + """ + import pytest + + class TestFixture: + @pytest.fixture(scope="class") + @classmethod + def fixt(cls): + return 1 + + def test_fixt(self, fixt): + assert fixt == 1 + """ + ) + result = pytester.runpytest("-v") + result.assert_outcomes(passed=1) + + +def test_fixture_above_staticmethod_still_works(pytester: Pytester) -> None: + """@pytest.fixture above @staticmethod remains discoverable without warning.""" + pytester.makepyfile( + """ + import pytest + + class TestFixture: + @pytest.fixture + @staticmethod + def fixt(): + return 1 + + def test_fixt(self, fixt): + assert fixt == 1 + """ + ) + result = pytester.runpytest("-W", "error::pytest.PytestWarning", "-v") + result.assert_outcomes(passed=1) + + +@pytest.mark.filterwarnings("default:cannot discover fixture *:pytest.PytestWarning") +def test_classmethod_above_fixture_warning_inherited(pytester: Pytester) -> None: + """MRO ``__dict__`` lookup finds @classmethod wrappers on a base class.""" + pytester.makepyfile( + """ + import pytest + + class Base: + @classmethod + @pytest.fixture(scope="class") + def fixt(cls): + return 1 + + class TestFixture(Base): + def test_fixt(self, fixt): + assert fixt == 1 + """ + ) + result = pytester.runpytest_inprocess( + "-v", "-rw", "-W", "default::pytest.PytestWarning" + ) + + result.stdout.fnmatch_lines( + [ + "*PytestWarning: cannot discover fixture 'fixt' because it is " + "wrapped by @classmethod; place @pytest.fixture above @classmethod*" + ] + ) + result.stdout.fnmatch_lines(["*fixture 'fixt' not found*"]) + result.assert_outcomes(errors=1)