From a9d1f55a0f9b6f536967caf0f317cdbc8034a073 Mon Sep 17 00:00:00 2001 From: Ran Benita Date: Fri, 8 Mar 2024 22:39:44 +0200 Subject: [PATCH 1/4] fixtures: simplify scope checking There are two non-optimal things in the current way scope checking is done: - It runs on `SubRequest`, but doesn't use the `SubRequest's scope, which is confusing. Instead it takes `invoking_scope` and `requested_scope`. - Because `_check_scope` is only defined on `SubRequest` and not `TopRequest`, `_compute_fixture_value` first creates the `SubRequest` only then checks the scope (hence the need for the previous point). Instead, also define `_check_scope` on `TopRequest` (always valid), and remove `invoking_scope`, using `self._scope` instead. --- src/_pytest/fixtures.py | 64 ++++++++++++++++++++++++----------------- 1 file changed, 38 insertions(+), 26 deletions(-) diff --git a/src/_pytest/fixtures.py b/src/_pytest/fixtures.py index 45412159e..e322c3f18 100644 --- a/src/_pytest/fixtures.py +++ b/src/_pytest/fixtures.py @@ -388,6 +388,14 @@ class FixtureRequest(abc.ABC): """Scope string, one of "function", "class", "module", "package", "session".""" return self._scope.value + @abc.abstractmethod + def _check_scope( + self, + requested_fixturedef: Union["FixtureDef[object]", PseudoFixtureDef[object]], + requested_scope: Scope, + ) -> None: + raise NotImplementedError() + @property def fixturenames(self) -> List[str]: """Names of all active fixtures in this request.""" @@ -632,12 +640,12 @@ class FixtureRequest(abc.ABC): ) fail(msg, pytrace=False) + # Check if a higher-level scoped fixture accesses a lower level one. + self._check_scope(fixturedef, scope) + subrequest = SubRequest( self, scope, param, param_index, fixturedef, _ispytest=True ) - - # Check if a higher-level scoped fixture accesses a lower level one. - subrequest._check_scope(argname, self._scope, scope) try: # Call the fixture function. fixturedef.execute(request=subrequest) @@ -669,6 +677,14 @@ class TopRequest(FixtureRequest): def _scope(self) -> Scope: return Scope.Function + def _check_scope( + self, + requested_fixturedef: Union["FixtureDef[object]", PseudoFixtureDef[object]], + requested_scope: Scope, + ) -> None: + # TopRequest always has function scope so always valid. + pass + @property def node(self): return self._pyfuncitem @@ -740,37 +756,33 @@ class SubRequest(FixtureRequest): def _check_scope( self, - argname: str, - invoking_scope: Scope, + requested_fixturedef: Union["FixtureDef[object]", PseudoFixtureDef[object]], requested_scope: Scope, ) -> None: - if argname == "request": + if isinstance(requested_fixturedef, PseudoFixtureDef): return - if invoking_scope > requested_scope: + if self._scope > requested_scope: # Try to report something helpful. - text = "\n".join(self._factorytraceback()) + argname = requested_fixturedef.argname + fixture_stack = "\n".join( + self._format_fixturedef_line(fixturedef) + for fixturedef in self._get_fixturestack() + ) + requested_fixture = self._format_fixturedef_line(requested_fixturedef) fail( f"ScopeMismatch: You tried to access the {requested_scope.value} scoped " - f"fixture {argname} with a {invoking_scope.value} scoped request object, " - f"involved factories:\n{text}", + f"fixture {argname} with a {self._scope.value} scoped request object, " + f"involved factories:\n{fixture_stack}\n{requested_fixture}", pytrace=False, ) - def _factorytraceback(self) -> List[str]: - lines = [] - for fixturedef in self._get_fixturestack(): - factory = fixturedef.func - fs, lineno = getfslineno(factory) - if isinstance(fs, Path): - session: Session = self._pyfuncitem.session - p = bestrelpath(session.path, fs) - else: - p = fs - lines.append( - "%s:%d: def %s%s" - % (p, lineno + 1, factory.__name__, inspect.signature(factory)) - ) - return lines + def _format_fixturedef_line(self, fixturedef: "FixtureDef[object]") -> str: + factory = fixturedef.func + path, lineno = getfslineno(factory) + if isinstance(path, Path): + path = bestrelpath(self._pyfuncitem.session.path, path) + signature = inspect.signature(factory) + return f"{path}:{lineno + 1}: def {factory.__name__}{signature}" def addfinalizer(self, finalizer: Callable[[], object]) -> None: self._fixturedef.addfinalizer(finalizer) @@ -1111,7 +1123,7 @@ def pytest_fixture_setup( fixdef = request._get_active_fixturedef(argname) assert fixdef.cached_result is not None result, arg_cache_key, exc = fixdef.cached_result - request._check_scope(argname, request._scope, fixdef._scope) + request._check_scope(fixdef, fixdef._scope) kwargs[argname] = result fixturefunc = resolve_fixture_function(fixturedef, request) From 71671f60b570e631d0b663ec4285319ade3c1ce5 Mon Sep 17 00:00:00 2001 From: Ran Benita Date: Fri, 8 Mar 2024 23:08:28 +0200 Subject: [PATCH 2/4] fixtures: improve fixture scope mismatch message - Separate the requesting from the requested. - Avoid the term "factory", I think most people don't distinguish between "fixture" and "fixture function" (i.e. "factory") and would find the term "factory" unfamiliar. --- src/_pytest/fixtures.py | 5 +++-- testing/python/fixtures.py | 15 ++++++++++++--- 2 files changed, 15 insertions(+), 5 deletions(-) diff --git a/src/_pytest/fixtures.py b/src/_pytest/fixtures.py index e322c3f18..40568a4a4 100644 --- a/src/_pytest/fixtures.py +++ b/src/_pytest/fixtures.py @@ -771,8 +771,9 @@ class SubRequest(FixtureRequest): requested_fixture = self._format_fixturedef_line(requested_fixturedef) fail( f"ScopeMismatch: You tried to access the {requested_scope.value} scoped " - f"fixture {argname} with a {self._scope.value} scoped request object, " - f"involved factories:\n{fixture_stack}\n{requested_fixture}", + f"fixture {argname} with a {self._scope.value} scoped request object. " + f"Requesting fixture stack:\n{fixture_stack}\n" + f"Requested fixture:\n{requested_fixture}", pytrace=False, ) diff --git a/testing/python/fixtures.py b/testing/python/fixtures.py index 6edff6ecd..8d59b36d3 100644 --- a/testing/python/fixtures.py +++ b/testing/python/fixtures.py @@ -1247,8 +1247,9 @@ class TestFixtureUsages: result = pytester.runpytest() result.stdout.fnmatch_lines( [ - "*ScopeMismatch*involved factories*", + "*ScopeMismatch*Requesting fixture stack*", "test_receives_funcargs_scope_mismatch.py:6: def arg2(arg1)", + "Requested fixture:", "test_receives_funcargs_scope_mismatch.py:2: def arg1()", "*1 error*", ] @@ -1274,7 +1275,13 @@ class TestFixtureUsages: ) result = pytester.runpytest() result.stdout.fnmatch_lines( - ["*ScopeMismatch*involved factories*", "* def arg2*", "*1 error*"] + [ + "*ScopeMismatch*Requesting fixture stack*", + "* def arg2(arg1)", + "Requested fixture:", + "* def arg1()", + "*1 error*", + ], ) def test_invalid_scope(self, pytester: Pytester) -> None: @@ -2488,8 +2495,10 @@ class TestFixtureMarker: assert result.ret == ExitCode.TESTS_FAILED result.stdout.fnmatch_lines( [ - "*ScopeMismatch*involved factories*", + "*ScopeMismatch*Requesting fixture stack*", "test_it.py:6: def fixmod(fixfunc)", + "Requested fixture:", + "test_it.py:3: def fixfunc()", ] ) From f5de111357d26780760df78b503fdc8f4731f611 Mon Sep 17 00:00:00 2001 From: Ran Benita Date: Fri, 8 Mar 2024 23:18:43 +0200 Subject: [PATCH 3/4] fixtures: check scope mismatch in `getfixturevalue` already-cached case This makes sure the scope is always compatible, and also allows using `getfixturevalue` in `pytest_fixture_setup` so less internal magic. --- src/_pytest/fixtures.py | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/src/_pytest/fixtures.py b/src/_pytest/fixtures.py index 40568a4a4..2d3593df0 100644 --- a/src/_pytest/fixtures.py +++ b/src/_pytest/fixtures.py @@ -573,6 +573,8 @@ class FixtureRequest(abc.ABC): raise self._compute_fixture_value(fixturedef) self._fixture_defs[argname] = fixturedef + else: + self._check_scope(fixturedef, fixturedef._scope) return fixturedef def _get_fixturestack(self) -> List["FixtureDef[Any]"]: @@ -1121,11 +1123,7 @@ def pytest_fixture_setup( """Execution of fixture setup.""" kwargs = {} for argname in fixturedef.argnames: - fixdef = request._get_active_fixturedef(argname) - assert fixdef.cached_result is not None - result, arg_cache_key, exc = fixdef.cached_result - request._check_scope(fixdef, fixdef._scope) - kwargs[argname] = result + kwargs[argname] = request.getfixturevalue(argname) fixturefunc = resolve_fixture_function(fixturedef, request) my_cache_key = fixturedef.cache_key(request) From ff551b768520b1f510dbd8413d810191a79d1471 Mon Sep 17 00:00:00 2001 From: Ran Benita Date: Fri, 8 Mar 2024 23:24:38 +0200 Subject: [PATCH 4/4] fixtures: simplify a bit of code --- src/_pytest/fixtures.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/_pytest/fixtures.py b/src/_pytest/fixtures.py index 2d3593df0..4b7c10752 100644 --- a/src/_pytest/fixtures.py +++ b/src/_pytest/fixtures.py @@ -1061,9 +1061,7 @@ class FixtureDef(Generic[FixtureValue]): # with their finalization. for argname in self.argnames: fixturedef = request._get_active_fixturedef(argname) - if argname != "request": - # PseudoFixtureDef is only for "request". - assert isinstance(fixturedef, FixtureDef) + if not isinstance(fixturedef, PseudoFixtureDef): fixturedef.addfinalizer(functools.partial(self.finish, request=request)) my_cache_key = self.cache_key(request)