From 530be285751143febe54b8974b234eed5eb8b079 Mon Sep 17 00:00:00 2001 From: Ran Benita Date: Thu, 14 Mar 2024 20:24:25 +0200 Subject: [PATCH 1/6] fixtures: use early return in `_get_active_fixturedef` --- src/_pytest/fixtures.py | 25 ++++++++++++++----------- 1 file changed, 14 insertions(+), 11 deletions(-) diff --git a/src/_pytest/fixtures.py b/src/_pytest/fixtures.py index 656160ff7dc..17d6ddc7fe5 100644 --- a/src/_pytest/fixtures.py +++ b/src/_pytest/fixtures.py @@ -570,18 +570,21 @@ def _get_active_fixturedef( self, argname: str ) -> Union["FixtureDef[object]", PseudoFixtureDef[object]]: fixturedef = self._fixture_defs.get(argname) - if fixturedef is None: - try: - fixturedef = self._getnextfixturedef(argname) - except FixtureLookupError: - if argname == "request": - cached_result = (self, [0], None) - return PseudoFixtureDef(cached_result, Scope.Function) - raise - self._compute_fixture_value(fixturedef) - self._fixture_defs[argname] = fixturedef - else: + if fixturedef is not None: self._check_scope(fixturedef, fixturedef._scope) + return fixturedef + + try: + fixturedef = self._getnextfixturedef(argname) + except FixtureLookupError: + if argname == "request": + cached_result = (self, [0], None) + return PseudoFixtureDef(cached_result, Scope.Function) + raise + + self._compute_fixture_value(fixturedef) + + self._fixture_defs[argname] = fixturedef return fixturedef def _get_fixturestack(self) -> List["FixtureDef[Any]"]: From d217d68cde0c34d619862f15c773ecc02ecdaabe Mon Sep 17 00:00:00 2001 From: Ran Benita Date: Thu, 14 Mar 2024 20:29:33 +0200 Subject: [PATCH 2/6] fixtures: inline `_compute_fixture_value` --- src/_pytest/fixtures.py | 31 ++++++++++--------------------- 1 file changed, 10 insertions(+), 21 deletions(-) diff --git a/src/_pytest/fixtures.py b/src/_pytest/fixtures.py index 17d6ddc7fe5..81fbf504f20 100644 --- a/src/_pytest/fixtures.py +++ b/src/_pytest/fixtures.py @@ -582,26 +582,7 @@ def _get_active_fixturedef( return PseudoFixtureDef(cached_result, Scope.Function) raise - self._compute_fixture_value(fixturedef) - - self._fixture_defs[argname] = fixturedef - return fixturedef - - def _get_fixturestack(self) -> List["FixtureDef[Any]"]: - values = [request._fixturedef for request in self._iter_chain()] - values.reverse() - return values - - def _compute_fixture_value(self, fixturedef: "FixtureDef[object]") -> None: - """Create a SubRequest based on "self" and call the execute method - of the given FixtureDef object. - - If the FixtureDef has cached the result it will do nothing, otherwise it will - setup and run the fixture, cache the value, and schedule a finalizer for it. - """ - # prepare a subrequest object before calling fixture function - # (latter managed by fixturedef) - argname = fixturedef.argname + # Prepare a SubRequest object for calling the fixture. funcitem = self._pyfuncitem try: callspec = funcitem.callspec @@ -627,7 +608,7 @@ def _compute_fixture_value(self, fixturedef: "FixtureDef[object]") -> None: ) fail(msg, pytrace=False) if has_params: - frame = inspect.stack()[3] + frame = inspect.stack()[2] frameinfo = inspect.getframeinfo(frame[0]) source_path = absolutepath(frameinfo.filename) source_lineno = frameinfo.lineno @@ -658,6 +639,14 @@ def _compute_fixture_value(self, fixturedef: "FixtureDef[object]") -> None: # Make sure the fixture value is cached, running it if it isn't fixturedef.execute(request=subrequest) + self._fixture_defs[argname] = fixturedef + return fixturedef + + def _get_fixturestack(self) -> List["FixtureDef[Any]"]: + values = [request._fixturedef for request in self._iter_chain()] + values.reverse() + return values + @final class TopRequest(FixtureRequest): From 3c77aec1dac0894ec4ca774b71ec91c85cf91dd1 Mon Sep 17 00:00:00 2001 From: Ran Benita Date: Thu, 14 Mar 2024 20:33:13 +0200 Subject: [PATCH 3/6] fixtures: move "request" check early --- src/_pytest/fixtures.py | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/src/_pytest/fixtures.py b/src/_pytest/fixtures.py index 81fbf504f20..1ff4e173b0e 100644 --- a/src/_pytest/fixtures.py +++ b/src/_pytest/fixtures.py @@ -569,18 +569,18 @@ def _iter_chain(self) -> Iterator["SubRequest"]: def _get_active_fixturedef( self, argname: str ) -> Union["FixtureDef[object]", PseudoFixtureDef[object]]: + if argname == "request": + cached_result = (self, [0], None) + return PseudoFixtureDef(cached_result, Scope.Function) + + # If we already finished computing a fixture by this name in this item, + # return it. fixturedef = self._fixture_defs.get(argname) if fixturedef is not None: self._check_scope(fixturedef, fixturedef._scope) return fixturedef - try: - fixturedef = self._getnextfixturedef(argname) - except FixtureLookupError: - if argname == "request": - cached_result = (self, [0], None) - return PseudoFixtureDef(cached_result, Scope.Function) - raise + fixturedef = self._getnextfixturedef(argname) # Prepare a SubRequest object for calling the fixture. funcitem = self._pyfuncitem From acf2971f46a9518b3552d48ea9541a1951c2b207 Mon Sep 17 00:00:00 2001 From: Ran Benita Date: Sat, 16 Mar 2024 23:56:11 +0200 Subject: [PATCH 4/6] fixtures: inline `_getnextfixturedef` into `_get_active_fixturedef` --- src/_pytest/fixtures.py | 70 ++++++++++++++++++++--------------------- 1 file changed, 34 insertions(+), 36 deletions(-) diff --git a/src/_pytest/fixtures.py b/src/_pytest/fixtures.py index 1ff4e173b0e..fbce950ae44 100644 --- a/src/_pytest/fixtures.py +++ b/src/_pytest/fixtures.py @@ -410,41 +410,6 @@ def node(self): """Underlying collection node (depends on current request scope).""" raise NotImplementedError() - def _getnextfixturedef(self, argname: str) -> "FixtureDef[Any]": - fixturedefs = self._arg2fixturedefs.get(argname, None) - if fixturedefs is None: - # We arrive here because of a dynamic call to - # getfixturevalue(argname) usage which was naturally - # not known at parsing/collection time. - fixturedefs = self._fixturemanager.getfixturedefs(argname, self._pyfuncitem) - if fixturedefs is not None: - self._arg2fixturedefs[argname] = fixturedefs - # No fixtures defined with this name. - if fixturedefs is None: - raise FixtureLookupError(argname, self) - # The are no fixtures with this name applicable for the function. - if not fixturedefs: - raise FixtureLookupError(argname, self) - - # A fixture may override another fixture with the same name, e.g. a - # fixture in a module can override a fixture in a conftest, a fixture in - # a class can override a fixture in the module, and so on. - # An overriding fixture can request its own name (possibly indirectly); - # in this case it gets the value of the fixture it overrides, one level - # up. - # Check how many `argname`s deep we are, and take the next one. - # `fixturedefs` is sorted from furthest to closest, so use negative - # indexing to go in reverse. - index = -1 - for request in self._iter_chain(): - if request.fixturename == argname: - index -= 1 - # If already consumed all of the available levels, fail. - if -index > len(fixturedefs): - raise FixtureLookupError(argname, self) - - return fixturedefs[index] - @property def config(self) -> Config: """The pytest config object associated with this request.""" @@ -580,7 +545,40 @@ def _get_active_fixturedef( self._check_scope(fixturedef, fixturedef._scope) return fixturedef - fixturedef = self._getnextfixturedef(argname) + # Find the appropriate fixturedef. + fixturedefs = self._arg2fixturedefs.get(argname, None) + if fixturedefs is None: + # We arrive here because of a dynamic call to + # getfixturevalue(argname) which was naturally + # not known at parsing/collection time. + fixturedefs = self._fixturemanager.getfixturedefs(argname, self._pyfuncitem) + if fixturedefs is not None: + self._arg2fixturedefs[argname] = fixturedefs + # No fixtures defined with this name. + if fixturedefs is None: + raise FixtureLookupError(argname, self) + # The are no fixtures with this name applicable for the function. + if not fixturedefs: + raise FixtureLookupError(argname, self) + + # A fixture may override another fixture with the same name, e.g. a + # fixture in a module can override a fixture in a conftest, a fixture in + # a class can override a fixture in the module, and so on. + # An overriding fixture can request its own name (possibly indirectly); + # in this case it gets the value of the fixture it overrides, one level + # up. + # Check how many `argname`s deep we are, and take the next one. + # `fixturedefs` is sorted from furthest to closest, so use negative + # indexing to go in reverse. + index = -1 + for request in self._iter_chain(): + if request.fixturename == argname: + index -= 1 + # If already consumed all of the available levels, fail. + if -index > len(fixturedefs): + raise FixtureLookupError(argname, self) + + fixturedef = fixturedefs[index] # Prepare a SubRequest object for calling the fixture. funcitem = self._pyfuncitem From 2e8fb9f1401d727e20f004326752fd1922f9c601 Mon Sep 17 00:00:00 2001 From: Ran Benita Date: Sun, 17 Mar 2024 00:36:00 +0200 Subject: [PATCH 5/6] fixtures: extract a `_check_fixturedef` method This stuff is less interesting when reading `_get_active_fixturedef`. --- src/_pytest/fixtures.py | 73 ++++++++++++++++++++--------------------- 1 file changed, 35 insertions(+), 38 deletions(-) diff --git a/src/_pytest/fixtures.py b/src/_pytest/fixtures.py index fbce950ae44..87ce5099e8f 100644 --- a/src/_pytest/fixtures.py +++ b/src/_pytest/fixtures.py @@ -560,7 +560,6 @@ def _get_active_fixturedef( # The are no fixtures with this name applicable for the function. if not fixturedefs: raise FixtureLookupError(argname, self) - # A fixture may override another fixture with the same name, e.g. a # fixture in a module can override a fixture in a conftest, a fixture in # a class can override a fixture in the module, and so on. @@ -577,13 +576,11 @@ def _get_active_fixturedef( # If already consumed all of the available levels, fail. if -index > len(fixturedefs): raise FixtureLookupError(argname, self) - fixturedef = fixturedefs[index] # Prepare a SubRequest object for calling the fixture. - funcitem = self._pyfuncitem try: - callspec = funcitem.callspec + callspec = self._pyfuncitem.callspec except AttributeError: callspec = None if callspec is not None and argname in callspec.params: @@ -595,41 +592,8 @@ def _get_active_fixturedef( param = NOTSET param_index = 0 scope = fixturedef._scope - - has_params = fixturedef.params is not None - fixtures_not_supported = getattr(funcitem, "nofuncargs", False) - if has_params and fixtures_not_supported: - msg = ( - f"{funcitem.name} does not support fixtures, maybe unittest.TestCase subclass?\n" - f"Node id: {funcitem.nodeid}\n" - f"Function type: {type(funcitem).__name__}" - ) - fail(msg, pytrace=False) - if has_params: - frame = inspect.stack()[2] - frameinfo = inspect.getframeinfo(frame[0]) - source_path = absolutepath(frameinfo.filename) - source_lineno = frameinfo.lineno - try: - source_path_str = str( - source_path.relative_to(funcitem.config.rootpath) - ) - except ValueError: - source_path_str = str(source_path) - location = getlocation(fixturedef.func, funcitem.config.rootpath) - msg = ( - "The requested fixture has no parameter defined for test:\n" - f" {funcitem.nodeid}\n\n" - f"Requested fixture '{fixturedef.argname}' defined in:\n" - f"{location}\n\n" - f"Requested here:\n" - f"{source_path_str}:{source_lineno}" - ) - fail(msg, pytrace=False) - - # Check if a higher-level scoped fixture accesses a lower level one. + self._check_fixturedef_without_param(fixturedef) self._check_scope(fixturedef, scope) - subrequest = SubRequest( self, scope, param, param_index, fixturedef, _ispytest=True ) @@ -640,6 +604,39 @@ def _get_active_fixturedef( self._fixture_defs[argname] = fixturedef return fixturedef + def _check_fixturedef_without_param(self, fixturedef: "FixtureDef[object]") -> None: + """Check that this request is allowed to execute this fixturedef without + a param.""" + funcitem = self._pyfuncitem + has_params = fixturedef.params is not None + fixtures_not_supported = getattr(funcitem, "nofuncargs", False) + if has_params and fixtures_not_supported: + msg = ( + f"{funcitem.name} does not support fixtures, maybe unittest.TestCase subclass?\n" + f"Node id: {funcitem.nodeid}\n" + f"Function type: {type(funcitem).__name__}" + ) + fail(msg, pytrace=False) + if has_params: + frame = inspect.stack()[3] + frameinfo = inspect.getframeinfo(frame[0]) + source_path = absolutepath(frameinfo.filename) + source_lineno = frameinfo.lineno + try: + source_path_str = str(source_path.relative_to(funcitem.config.rootpath)) + except ValueError: + source_path_str = str(source_path) + location = getlocation(fixturedef.func, funcitem.config.rootpath) + msg = ( + "The requested fixture has no parameter defined for test:\n" + f" {funcitem.nodeid}\n\n" + f"Requested fixture '{fixturedef.argname}' defined in:\n" + f"{location}\n\n" + f"Requested here:\n" + f"{source_path_str}:{source_lineno}" + ) + fail(msg, pytrace=False) + def _get_fixturestack(self) -> List["FixtureDef[Any]"]: values = [request._fixturedef for request in self._iter_chain()] values.reverse() From 882c4da2f37702b00bdbd3b6c74e9821d33e0204 Mon Sep 17 00:00:00 2001 From: Ran Benita Date: Sun, 17 Mar 2024 20:06:29 +0200 Subject: [PATCH 6/6] fixtures: inline `fail_fixturefunc` Doesn't add much. --- src/_pytest/fixtures.py | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/src/_pytest/fixtures.py b/src/_pytest/fixtures.py index 87ce5099e8f..09fd07422fc 100644 --- a/src/_pytest/fixtures.py +++ b/src/_pytest/fixtures.py @@ -35,6 +35,7 @@ import _pytest from _pytest import nodes from _pytest._code import getfslineno +from _pytest._code import Source from _pytest._code.code import FormattedExcinfo from _pytest._code.code import TerminalRepr from _pytest._io import TerminalWriter @@ -864,13 +865,6 @@ def toterminal(self, tw: TerminalWriter) -> None: tw.line("%s:%d" % (os.fspath(self.filename), self.firstlineno + 1)) -def fail_fixturefunc(fixturefunc, msg: str) -> NoReturn: - fs, lineno = getfslineno(fixturefunc) - location = f"{fs}:{lineno + 1}" - source = _pytest._code.Source(fixturefunc) - fail(msg + ":\n\n" + str(source.indent()) + "\n" + location, pytrace=False) - - def call_fixture_func( fixturefunc: "_FixtureFunc[FixtureValue]", request: FixtureRequest, kwargs ) -> FixtureValue: @@ -900,7 +894,13 @@ def _teardown_yield_fixture(fixturefunc, it) -> None: except StopIteration: pass else: - fail_fixturefunc(fixturefunc, "fixture function has more than one 'yield'") + fs, lineno = getfslineno(fixturefunc) + fail( + f"fixture function has more than one 'yield':\n\n" + f"{Source(fixturefunc).indent()}\n" + f"{fs}:{lineno + 1}", + pytrace=False, + ) def _eval_scope_callable(