diff --git a/AUTHORS b/AUTHORS index 5aa9045208a..3419accfa6b 100644 --- a/AUTHORS +++ b/AUTHORS @@ -393,6 +393,7 @@ Serhii Mozghovyi Seth Junot Shantanu Jain Sharad Nair +Shaygan Hooshyari Shubham Adep Simon Blanchard Simon Gomizelj diff --git a/changelog/11525.improvement.rst b/changelog/11525.improvement.rst new file mode 100644 index 00000000000..1935ce59343 --- /dev/null +++ b/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` diff --git a/doc/en/conf.py b/doc/en/conf.py index 9558a75f927..47fc70dce85 100644 --- a/doc/en/conf.py +++ b/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 diff --git a/src/_pytest/assertion/rewrite.py b/src/_pytest/assertion/rewrite.py index c414b30a4a8..41e3d271396 100644 --- a/src/_pytest/assertion/rewrite.py +++ b/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__") diff --git a/src/_pytest/compat.py b/src/_pytest/compat.py index 1aa7495bddb..053ef1fd1fd 100644 --- a/src/_pytest/compat.py +++ b/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__ diff --git a/src/_pytest/fixtures.py b/src/_pytest/fixtures.py index fc6541c1404..8b79dbcb932 100644 --- a/src/_pytest/fixtures.py +++ b/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,11 +1192,11 @@ 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}" ) @@ -1232,7 +1204,9 @@ def __call__(self, function: FixtureFunction) -> FixtureFunction: 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"" + + 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 diff --git a/testing/code/test_source.py b/testing/code/test_source.py index d78d9e7025a..843233fe21e 100644 --- a/testing/code/test_source.py +++ b/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 ) diff --git a/testing/python/fixtures.py b/testing/python/fixtures.py index cac00ab5401..dc69781095b 100644 --- a/testing/python/fixtures.py +++ b/testing/python/fixtures.py @@ -1606,6 +1606,63 @@ def teardown_module(): result = pytester.runpytest() result.stdout.no_fnmatch_line("* ERROR at teardown *") + def test_unwrapping_pytest_fixture(self, pytester: Pytester) -> None: + """Ensure the unwrap method on `FixtureFunctionDefinition` correctly wraps and unwraps methods and functions""" + pytester.makepyfile( + """ + import pytest + import inspect + + class FixtureFunctionDefTestClass: + def __init__(self) -> None: + self.i = 10 + + @pytest.fixture + def fixture_function_def_test_method(self): + return self.i + + + @pytest.fixture + def fixture_function_def_test_func(): + return 9 + + + def test_get_wrapped_func_returns_method(): + obj = FixtureFunctionDefTestClass() + wrapped_function_result = ( + obj.fixture_function_def_test_method._get_wrapped_function() + ) + assert inspect.ismethod(wrapped_function_result) + assert wrapped_function_result() == 10 + + + def test_get_wrapped_func_returns_function(): + assert fixture_function_def_test_func._get_wrapped_function()() == 9 + """ + ) + result = pytester.runpytest() + result.assert_outcomes(passed=2) + + def test_fixture_wrapped_looks_liked_wrapped_function( + self, pytester: Pytester + ) -> None: + """Ensure that `FixtureFunctionDefinition` behaves like the function it wrapped.""" + pytester.makepyfile( + """ + import pytest + + @pytest.fixture + def fixture_function_def_test_func(): + return 9 + fixture_function_def_test_func.__doc__ = "documentation" + + def test_fixture_has_same_doc(): + assert fixture_function_def_test_func.__doc__ == "documentation" + """ + ) + result = pytester.runpytest() + result.assert_outcomes(passed=1) + class TestFixtureManagerParseFactories: @pytest.fixture @@ -4526,6 +4583,21 @@ def fixt(): ) +def test_fixture_class(pytester: Pytester) -> None: + """Check if an error is raised when using @pytest.fixture on a class.""" + pytester.makepyfile( + """ + import pytest + + @pytest.fixture + class A: + pass + """ + ) + result = pytester.runpytest() + result.assert_outcomes(errors=1) + + def test_fixture_param_shadowing(pytester: Pytester) -> None: """Parametrized arguments would be shadowed if a fixture with the same name also exists (#5036)""" pytester.makepyfile( diff --git a/testing/test_assertrewrite.py b/testing/test_assertrewrite.py index eed59a2dce7..02d1c3e52ff 100644 --- a/testing/test_assertrewrite.py +++ b/testing/test_assertrewrite.py @@ -975,6 +975,23 @@ def __repr__(self): assert "UnicodeDecodeError" not in msg assert "UnicodeEncodeError" not in msg + def test_assert_fixture(self, pytester: Pytester) -> None: + pytester.makepyfile( + """\ + import pytest + @pytest.fixture + def fixt(): + return 42 + + def test_something(): # missing "fixt" argument + assert fixt == 42 + """ + ) + result = pytester.runpytest() + result.stdout.fnmatch_lines( + ["*assert )> == 42*"] + ) + class TestRewriteOnImport: def test_pycache_is_a_file(self, pytester: Pytester) -> None: diff --git a/testing/test_collection.py b/testing/test_collection.py index 7d28610e015..ccd57eeef43 100644 --- a/testing/test_collection.py +++ b/testing/test_collection.py @@ -1284,7 +1284,7 @@ def test_1(): """ ) result = pytester.runpytest() - result.stdout.fnmatch_lines(["*1 passed in*"]) + result.assert_outcomes(passed=1) assert result.ret == 0 @@ -1348,7 +1348,7 @@ def test_collect_pyargs_with_testpaths( with monkeypatch.context() as mp: mp.chdir(root) result = pytester.runpytest_subprocess() - result.stdout.fnmatch_lines(["*1 passed in*"]) + result.assert_outcomes(passed=1) def test_initial_conftests_with_testpaths(pytester: Pytester) -> None: diff --git a/testing/test_compat.py b/testing/test_compat.py index 86868858956..3722bfcfb40 100644 --- a/testing/test_compat.py +++ b/testing/test_compat.py @@ -7,7 +7,6 @@ from functools import wraps from typing import TYPE_CHECKING -from _pytest.compat import _PytestWrapper from _pytest.compat import assert_never from _pytest.compat import get_real_func from _pytest.compat import safe_getattr @@ -38,10 +37,7 @@ def __getattr__(self, attr): with pytest.raises( ValueError, - match=( - "could not find real function of \n" - "stopped at " - ), + match=("wrapper loop when unwrapping "), ): get_real_func(evil) @@ -65,10 +61,13 @@ def func(): wrapped_func2 = decorator(decorator(wrapped_func)) assert get_real_func(wrapped_func2) is func - # special case for __pytest_wrapped__ attribute: used to obtain the function up until the point - # a function was wrapped by pytest itself - wrapped_func2.__pytest_wrapped__ = _PytestWrapper(wrapped_func) - assert get_real_func(wrapped_func2) is wrapped_func + # obtain the function up until the point a function was wrapped by pytest itself + @pytest.fixture + def wrapped_func3(): + pass # pragma: no cover + + wrapped_func4 = decorator(wrapped_func3) + assert get_real_func(wrapped_func4) is wrapped_func3._get_wrapped_function() def test_get_real_func_partial() -> None: