From c83c1c4bda7e92bcda00dad29798b8fc73028997 Mon Sep 17 00:00:00 2001 From: Ran Benita Date: Tue, 27 Feb 2024 10:51:31 +0200 Subject: [PATCH 1/2] fixtures: add `_iter_chain` helper method Will be reused in the next commit. --- src/_pytest/fixtures.py | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-) diff --git a/src/_pytest/fixtures.py b/src/_pytest/fixtures.py index 99de03fe849..86a8eef0452 100644 --- a/src/_pytest/fixtures.py +++ b/src/_pytest/fixtures.py @@ -540,6 +540,16 @@ def getfixturevalue(self, argname: str) -> Any: ) return fixturedef.cached_result[0] + def _iter_chain(self) -> Iterator["SubRequest"]: + """Yield all SubRequests in the chain, from self up. + + Note: does *not* yield the TopRequest. + """ + current = self + while isinstance(current, SubRequest): + yield current + current = current._parent_request + def _get_active_fixturedef( self, argname: str ) -> Union["FixtureDef[object]", PseudoFixtureDef[object]]: @@ -557,11 +567,7 @@ def _get_active_fixturedef( return fixturedef def _get_fixturestack(self) -> List["FixtureDef[Any]"]: - current = self - values: List[FixtureDef[Any]] = [] - while isinstance(current, SubRequest): - values.append(current._fixturedef) # type: ignore[has-type] - current = current._parent_request + values = [request._fixturedef for request in self._iter_chain()] values.reverse() return values @@ -705,7 +711,7 @@ def __init__( ) self._parent_request: Final[FixtureRequest] = request self._scope_field: Final = scope - self._fixturedef: Final = fixturedef + self._fixturedef: Final[FixtureDef[object]] = fixturedef if param is not NOTSET: self.param = param self.param_index: Final = param_index From bd45ccd2ca399121fa91baa3fa0d25c2c36b6671 Mon Sep 17 00:00:00 2001 From: Ran Benita Date: Tue, 27 Feb 2024 10:37:40 +0200 Subject: [PATCH 2/2] fixtures: avoid mutable `arg2index` state in favor of looking up the request chain pytest allows a fixture to request its own name (directly or indirectly), in which case the fixture with the same name but one level up is used. To know which fixture should be used next, pytest keeps a mutable item-global dict `_arg2index` which maintains this state. This is not great: - Mutable state like this is hard to understand and reason about. - It is conceptually buggy; the indexing is global (e.g. if requesting `fix1` and `fix2`, the indexing is shared between them), but actually different branches of the subrequest tree should not affect each other. This is not an issue in practice because pytest keeps a cache of the fixturedefs it resolved anyway (`_fixture_defs`), but if the cache is removed it becomes evident. Instead of the `_arg2index` state, count how many `argname`s deep we are in the subrequest tree ("the fixture stack") and use that for the index. This way, no global mutable state and the logic is very localized and easier to understand. This is slower, however fixture stacks should not be so deep that this matters much, I hope. --- src/_pytest/fixtures.py | 32 ++++++++++++++++---------------- 1 file changed, 16 insertions(+), 16 deletions(-) diff --git a/src/_pytest/fixtures.py b/src/_pytest/fixtures.py index 86a8eef0452..fff95521865 100644 --- a/src/_pytest/fixtures.py +++ b/src/_pytest/fixtures.py @@ -343,7 +343,6 @@ def __init__( pyfuncitem: "Function", fixturename: Optional[str], arg2fixturedefs: Dict[str, Sequence["FixtureDef[Any]"]], - arg2index: Dict[str, int], fixture_defs: Dict[str, "FixtureDef[Any]"], *, _ispytest: bool = False, @@ -357,16 +356,6 @@ def __init__( # collection. Dynamically requested fixtures (using # `request.getfixturevalue("foo")`) are added dynamically. self._arg2fixturedefs: Final = arg2fixturedefs - # 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; in this case it gets - # the value of the fixture it overrides, one level up. - # The _arg2index state keeps the current depth in the overriding chain. - # The fixturedefs list in _arg2fixturedefs for a given name is ordered from - # furthest to closest, so we use negative indexing -1, -2, ... to go from - # last to first. - self._arg2index: Final = arg2index # The evaluated argnames so far, mapping to the FixtureDef they resolved # to. self._fixture_defs: Final = fixture_defs @@ -424,11 +413,24 @@ def _getnextfixturedef(self, argname: str) -> "FixtureDef[Any]": # The are no fixtures with this name applicable for the function. if not fixturedefs: raise FixtureLookupError(argname, self) - index = self._arg2index.get(argname, 0) - 1 - # The fixture requested its own name, but no remaining to override. + + # 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) - self._arg2index[argname] = index + return fixturedefs[index] @property @@ -660,7 +662,6 @@ def __init__(self, pyfuncitem: "Function", *, _ispytest: bool = False) -> None: fixturename=None, pyfuncitem=pyfuncitem, arg2fixturedefs=pyfuncitem._fixtureinfo.name2fixturedefs.copy(), - arg2index={}, fixture_defs={}, _ispytest=_ispytest, ) @@ -706,7 +707,6 @@ def __init__( fixturename=fixturedef.argname, fixture_defs=request._fixture_defs, arg2fixturedefs=request._arg2fixturedefs, - arg2index=request._arg2index, _ispytest=_ispytest, ) self._parent_request: Final[FixtureRequest] = request