fixtures: replace fixture representation with a class (#12473)

During the sprint we discussed fixing the above issue and thought maybe it's a better idea to add a better representation to fixtures. To do this without patching the object more, this PR refactors fixtures to have a class with attributes.

The main rational behind that is:

- Now we have a type for fixtures makes it easier to check for fixture types and no need to perform cast on objects. Removed monkey patching.

- They are no longer functions that carry a class within them.

- When you decorate a function with a fixture it's not possible to call it anymore. So it makes more sense for it to not be a function or method.

Fixes #11525

---------

Co-authored-by: Sviatoslav Sydorenko (Святослав Сидоренко) <[email protected]>
Co-authored-by: Bruno Oliveira <[email protected]>
Co-authored-by: Ran Benita <[email protected]>

Author: 4 people

AUTHORS

@@ -393,6 +393,7 @@ Serhii Mozghovyi
 Seth Junot
 Shantanu Jain
 Sharad Nair
+Shaygan Hooshyari
 Shubham Adep
 Simon Blanchard
 Simon Gomizelj

changelog/11525.improvement.rst

@@ -0,0 +1,3 @@
+Fixtures are now clearly represented in the output as a "fixture object", not as a normal function as before, making it easy for beginners to catch mistakes such as referencing a fixture declared in the same module but not requested in the test function.
+
+-- by :user:`the-compiler` and :user:`glyphack`

doc/en/conf.py

@@ -75,6 +75,7 @@
     ("py:class", "_pytest._code.code.TerminalRepr"),
     ("py:class", "TerminalRepr"),
     ("py:class", "_pytest.fixtures.FixtureFunctionMarker"),
+    ("py:class", "_pytest.fixtures.FixtureFunctionDefinition"),
     ("py:class", "_pytest.logging.LogCaptureHandler"),
     ("py:class", "_pytest.mark.structures.ParameterSet"),
     # Intentionally undocumented/private

src/_pytest/assertion/rewrite.py

@@ -31,6 +31,7 @@
 from _pytest._version import version
 from _pytest.assertion import util
 from _pytest.config import Config
+from _pytest.fixtures import FixtureFunctionDefinition
 from _pytest.main import Session
 from _pytest.pathlib import absolutepath
 from _pytest.pathlib import fnmatch_ex
@@ -472,7 +473,8 @@ def _format_assertmsg(obj: object) -> str:
 
 def _should_repr_global_name(obj: object) -> bool:
     if callable(obj):
-        return False
+        # For pytest fixtures the __repr__ method provides more information than the function name.
+        return isinstance(obj, FixtureFunctionDefinition)
 
     try:
         return not hasattr(obj, "__name__")

src/_pytest/compat.py

@@ -4,7 +4,6 @@
 from __future__ import annotations
 
 from collections.abc import Callable
-import dataclasses
 import enum
 import functools
 import inspect
@@ -205,59 +204,16 @@ def ascii_escaped(val: bytes | str) -> str:
     return ret.translate(_non_printable_ascii_translate_table)
 
 
-@dataclasses.dataclass
-class _PytestWrapper:
-    """Dummy wrapper around a function object for internal use only.
-
-    Used to correctly unwrap the underlying function object when we are
-    creating fixtures, because we wrap the function object ourselves with a
-    decorator to issue warnings when the fixture function is called directly.
-    """
-
-    obj: Any
-
-
 def get_real_func(obj):
     """Get the real function object of the (possibly) wrapped object by
-    functools.wraps or functools.partial."""
-    start_obj = obj
-    for i in range(100):
-        # __pytest_wrapped__ is set by @pytest.fixture when wrapping the fixture function
-        # to trigger a warning if it gets called directly instead of by pytest: we don't
-        # want to unwrap further than this otherwise we lose useful wrappings like @mock.patch (#3774)
-        new_obj = getattr(obj, "__pytest_wrapped__", None)
-        if isinstance(new_obj, _PytestWrapper):
-            obj = new_obj.obj
-            break
-        new_obj = getattr(obj, "__wrapped__", None)
-        if new_obj is None:
-            break
-        obj = new_obj
-    else:
-        from _pytest._io.saferepr import saferepr
+    :func:`functools.wraps`, or :func:`functools.partial`."""
+    obj = inspect.unwrap(obj)
 
-        raise ValueError(
-            f"could not find real function of {saferepr(start_obj)}\nstopped at {saferepr(obj)}"
-        )
     if isinstance(obj, functools.partial):
         obj = obj.func
     return obj
 
 
-def get_real_method(obj, holder):
-    """Attempt to obtain the real function object that might be wrapping
-    ``obj``, while at the same time returning a bound method to ``holder`` if
-    the original object was a bound method."""
-    try:
-        is_method = hasattr(obj, "__func__")
-        obj = get_real_func(obj)
-    except Exception:  # pragma: no cover
-        return obj
-    if is_method and hasattr(obj, "__get__") and callable(obj.__get__):
-        obj = obj.__get__(holder)
-    return obj
-
-
 def getimfunc(func):
     try:
         return func.__func__

src/_pytest/fixtures.py

@@ -40,10 +40,8 @@
 from _pytest._code.code import FormattedExcinfo
 from _pytest._code.code import TerminalRepr
 from _pytest._io import TerminalWriter
-from _pytest.compat import _PytestWrapper
 from _pytest.compat import assert_never
 from _pytest.compat import get_real_func
-from _pytest.compat import get_real_method
 from _pytest.compat import getfuncargnames
 from _pytest.compat import getimfunc
 from _pytest.compat import getlocation
@@ -152,13 +150,12 @@ def get_scope_node(node: nodes.Node, scope: Scope) -> nodes.Node | None:
         assert_never(scope)
 
 
+# TODO: Try to use FixtureFunctionDefinition instead of the marker
 def getfixturemarker(obj: object) -> FixtureFunctionMarker | None:
-    """Return fixturemarker or None if it doesn't exist or raised
-    exceptions."""
-    return cast(
-        Optional[FixtureFunctionMarker],
-        safe_getattr(obj, "_pytestfixturefunction", None),
-    )
+    """Return fixturemarker or None if it doesn't exist"""
+    if isinstance(obj, FixtureFunctionDefinition):
+        return obj._fixture_function_marker
+    return None
 
 
 # Algorithm for sorting on a per-parametrized resource setup basis.
@@ -1181,31 +1178,6 @@ def pytest_fixture_setup(
     return result
 
 
-def wrap_function_to_error_out_if_called_directly(
-    function: FixtureFunction,
-    fixture_marker: FixtureFunctionMarker,
-) -> FixtureFunction:
-    """Wrap the given fixture function so we can raise an error about it being called directly,
-    instead of used as an argument in a test function."""
-    name = fixture_marker.name or function.__name__
-    message = (
-        f'Fixture "{name}" called directly. Fixtures are not meant to be called directly,\n'
-        "but are created automatically when test functions request them as parameters.\n"
-        "See https://docs.pytest.org/en/stable/explanation/fixtures.html for more information about fixtures, and\n"
-        "https://docs.pytest.org/en/stable/deprecations.html#calling-fixtures-directly about how to update your code."
-    )
-
-    @functools.wraps(function)
-    def result(*args, **kwargs):
-        fail(message, pytrace=False)
-
-    # Keep reference to the original function in our own custom attribute so we don't unwrap
-    # further than this point and lose useful wrappings like @mock.patch (#3774).
-    result.__pytest_wrapped__ = _PytestWrapper(function)  # type: ignore[attr-defined]
-
-    return cast(FixtureFunction, result)
-
-
 @final
 @dataclasses.dataclass(frozen=True)
 class FixtureFunctionMarker:
@@ -1220,19 +1192,21 @@ class FixtureFunctionMarker:
     def __post_init__(self, _ispytest: bool) -> None:
         check_ispytest(_ispytest)
 
-    def __call__(self, function: FixtureFunction) -> FixtureFunction:
+    def __call__(self, function: FixtureFunction) -> FixtureFunctionDefinition:
         if inspect.isclass(function):
             raise ValueError("class fixtures not supported (maybe in the future)")
 
-        if getattr(function, "_pytestfixturefunction", False):
+        if isinstance(function, FixtureFunctionDefinition):
             raise ValueError(
                 f"@pytest.fixture is being applied more than once to the same function {function.__name__!r}"
             )
 
         if hasattr(function, "pytestmark"):
             warnings.warn(MARKED_FIXTURE, stacklevel=2)
 
-        function = wrap_function_to_error_out_if_called_directly(function, self)
+        fixture_definition = FixtureFunctionDefinition(
+            function=function, fixture_function_marker=self, _ispytest=True
+        )
 
         name = self.name or function.__name__
         if name == "request":
@@ -1242,21 +1216,68 @@ def __call__(self, function: FixtureFunction) -> FixtureFunction:
                 pytrace=False,
             )
 
-        # Type ignored because https://github.com/python/mypy/issues/2087.
-        function._pytestfixturefunction = self  # type: ignore[attr-defined]
-        return function
+        return fixture_definition
+
+
+# TODO: paramspec/return type annotation tracking and storing
+class FixtureFunctionDefinition:
+    def __init__(
+        self,
+        *,
+        function: Callable[..., Any],
+        fixture_function_marker: FixtureFunctionMarker,
+        instance: object | None = None,
+        _ispytest: bool = False,
+    ) -> None:
+        check_ispytest(_ispytest)
+        self.name = fixture_function_marker.name or function.__name__
+        # In order to show the function that this fixture contains in messages.
+        # Set the __name__ to be same as the function __name__ or the given fixture name.
+        self.__name__ = self.name
+        self._fixture_function_marker = fixture_function_marker
+        if instance is not None:
+            self._fixture_function = cast(
+                Callable[..., Any], function.__get__(instance)
+            )
+        else:
+            self._fixture_function = function
+        functools.update_wrapper(self, function)
+
+    def __repr__(self) -> str:
+        return f"<pytest_fixture({self._fixture_function})>"
+
+    def __get__(self, instance, owner=None):
+        """Behave like a method if the function it was applied to was a method."""
+        return FixtureFunctionDefinition(
+            function=self._fixture_function,
+            fixture_function_marker=self._fixture_function_marker,
+            instance=instance,
+            _ispytest=True,
+        )
+
+    def __call__(self, *args: Any, **kwds: Any) -> Any:
+        message = (
+            f'Fixture "{self.name}" called directly. Fixtures are not meant to be called directly,\n'
+            "but are created automatically when test functions request them as parameters.\n"
+            "See https://docs.pytest.org/en/stable/explanation/fixtures.html for more information about fixtures, and\n"
+            "https://docs.pytest.org/en/stable/deprecations.html#calling-fixtures-directly"
+        )
+        fail(message, pytrace=False)
+
+    def _get_wrapped_function(self) -> Callable[..., Any]:
+        return self._fixture_function
 
 
 @overload
 def fixture(
-    fixture_function: FixtureFunction,
+    fixture_function: Callable[..., object],
     *,
     scope: _ScopeName | Callable[[str, Config], _ScopeName] = ...,
     params: Iterable[object] | None = ...,
     autouse: bool = ...,
     ids: Sequence[object | None] | Callable[[Any], object | None] | None = ...,
     name: str | None = ...,
-) -> FixtureFunction: ...
+) -> FixtureFunctionDefinition: ...
 
 
 @overload
@@ -1279,7 +1300,7 @@ def fixture(
     autouse: bool = False,
     ids: Sequence[object | None] | Callable[[Any], object | None] | None = None,
     name: str | None = None,
-) -> FixtureFunctionMarker | FixtureFunction:
+) -> FixtureFunctionMarker | FixtureFunctionDefinition:
     """Decorator to mark a fixture factory function.
 
     This decorator can be used, with or without parameters, to define a
@@ -1774,33 +1795,31 @@ def parsefactories(
             # 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)
-            marker = getfixturemarker(obj_ub)
-            if not isinstance(marker, FixtureFunctionMarker):
-                # Magic globals  with __getattr__ might have got us a wrong
-                # fixture attribute.
-                continue
-
-            # OK we know it is a fixture -- now safe to look up on the _instance_.
-            obj = getattr(holderobj, name)
-
-            if marker.name:
-                name = marker.name
-
-            # During fixture definition we wrap the original fixture function
-            # to issue a warning if called directly, so here we unwrap it in
-            # order to not emit the warning when pytest itself calls the
-            # fixture function.
-            func = get_real_method(obj, holderobj)
-
-            self._register_fixture(
-                name=name,
-                nodeid=nodeid,
-                func=func,
-                scope=marker.scope,
-                params=marker.params,
-                ids=marker.ids,
-                autouse=marker.autouse,
-            )
+            if type(obj_ub) is FixtureFunctionDefinition:
+                marker = obj_ub._fixture_function_marker
+                if marker.name:
+                    fixture_name = marker.name
+                else:
+                    fixture_name = name
+
+                # OK we know it is a fixture -- now safe to look up on the _instance_.
+                try:
+                    obj = getattr(holderobj, name)
+                # if the fixture is named in the decorator we cannot find it in the module
+                except AttributeError:
+                    obj = obj_ub
+
+                func = obj._get_wrapped_function()
+
+                self._register_fixture(
+                    name=fixture_name,
+                    nodeid=nodeid,
+                    func=func,
+                    scope=marker.scope,
+                    params=marker.params,
+                    ids=marker.ids,
+                    autouse=marker.autouse,
+                )
 
     def getfixturedefs(
         self, argname: str, node: nodes.Node

testing/code/test_source.py

@@ -478,14 +478,14 @@ def deco_mark():
     def deco_fixture():
         assert False
 
-    src = inspect.getsource(deco_fixture)
+    src = inspect.getsource(deco_fixture._get_wrapped_function())
     assert src == "    @pytest.fixture\n    def deco_fixture():\n        assert False\n"
-    # currently Source does not unwrap decorators, testing the
-    # existing behavior here for explicitness, but perhaps we should revisit/change this
-    # in the future
-    assert str(Source(deco_fixture)).startswith("@functools.wraps(function)")
+    # Make sure the decorator is not a wrapped function
+    assert not str(Source(deco_fixture)).startswith("@functools.wraps(function)")
     assert (
-        textwrap.indent(str(Source(get_real_func(deco_fixture))), "    ") + "\n" == src
+        textwrap.indent(str(Source(deco_fixture._get_wrapped_function())), "    ")
+        + "\n"
+        == src
     )