From 61d06a9fb08c4ddbf000f5c3b0295f24e4e0eade Mon Sep 17 00:00:00 2001 From: John Litborn <11260241+jakkdl@users.noreply.github.com> Date: Sat, 16 Mar 2024 22:45:56 +0100 Subject: [PATCH] Don't add fixture finalizer if the value is cached (#11833) Fixes #1489 --- changelog/1489.bugfix.rst | 1 + src/_pytest/fixtures.py | 50 +++++++++++++--------------------- testing/python/fixtures.py | 55 ++++++++++++++++++++++++++++++++++++++ 3 files changed, 75 insertions(+), 31 deletions(-) create mode 100644 changelog/1489.bugfix.rst diff --git a/changelog/1489.bugfix.rst b/changelog/1489.bugfix.rst new file mode 100644 index 00000000000..70c5dd1252e --- /dev/null +++ b/changelog/1489.bugfix.rst @@ -0,0 +1 @@ +Fix some instances where teardown of higher-scoped fixtures was not happening in the reverse order they were initialized in. diff --git a/src/_pytest/fixtures.py b/src/_pytest/fixtures.py index 8b25d743ca4..48429a023ac 100644 --- a/src/_pytest/fixtures.py +++ b/src/_pytest/fixtures.py @@ -543,6 +543,11 @@ def getfixturevalue(self, argname: str) -> Any: :raises pytest.FixtureLookupError: If the given fixture could not be found. """ + # Note that in addition to the use case described in the docstring, + # getfixturevalue() is also called by pytest itself during item and fixture + # setup to evaluate the fixtures that are requested statically + # (using function parameters, autouse, etc). + fixturedef = self._get_active_fixturedef(argname) assert fixturedef.cached_result is not None, ( f'The fixture value for "{argname}" is not available. ' @@ -587,9 +592,8 @@ 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. - This will force the FixtureDef object to throw away any previous - results and compute a new fixture value, which will be stored into - the FixtureDef object itself. + 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) @@ -646,18 +650,9 @@ def _compute_fixture_value(self, fixturedef: "FixtureDef[object]") -> None: subrequest = SubRequest( self, scope, param, param_index, fixturedef, _ispytest=True ) - try: - # Call the fixture function. - fixturedef.execute(request=subrequest) - finally: - self._schedule_finalizers(fixturedef, subrequest) - def _schedule_finalizers( - self, fixturedef: "FixtureDef[object]", subrequest: "SubRequest" - ) -> None: - # If fixture function failed it might have registered finalizers. - finalizer = functools.partial(fixturedef.finish, request=subrequest) - subrequest.node.addfinalizer(finalizer) + # Make sure the fixture value is cached, running it if it isn't + fixturedef.execute(request=subrequest) @final @@ -788,21 +783,6 @@ def _format_fixturedef_line(self, fixturedef: "FixtureDef[object]") -> str: def addfinalizer(self, finalizer: Callable[[], object]) -> None: self._fixturedef.addfinalizer(finalizer) - def _schedule_finalizers( - self, fixturedef: "FixtureDef[object]", subrequest: "SubRequest" - ) -> None: - # If the executing fixturedef was not explicitly requested in the argument list (via - # getfixturevalue inside the fixture call) then ensure this fixture def will be finished - # first. - if ( - fixturedef.argname not in self._fixture_defs - and fixturedef.argname not in self._pyfuncitem.fixturenames - ): - fixturedef.addfinalizer( - functools.partial(self._fixturedef.finish, request=self) - ) - super()._schedule_finalizers(fixturedef, subrequest) - @final class FixtureLookupError(LookupError): @@ -1055,12 +1035,13 @@ def finish(self, request: SubRequest) -> None: raise BaseExceptionGroup(msg, exceptions[::-1]) def execute(self, request: SubRequest) -> FixtureValue: + finalizer = functools.partial(self.finish, request=request) # Get required arguments and register our own finish() # with their finalization. for argname in self.argnames: fixturedef = request._get_active_fixturedef(argname) if not isinstance(fixturedef, PseudoFixtureDef): - fixturedef.addfinalizer(functools.partial(self.finish, request=request)) + fixturedef.addfinalizer(finalizer) my_cache_key = self.cache_key(request) if self.cached_result is not None: @@ -1080,7 +1061,14 @@ def execute(self, request: SubRequest) -> FixtureValue: assert self.cached_result is None ihook = request.node.ihook - result = ihook.pytest_fixture_setup(fixturedef=self, request=request) + try: + # Setup the fixture, run the code in it, and cache the value + # in self.cached_result + result = ihook.pytest_fixture_setup(fixturedef=self, request=request) + finally: + # schedule our finalizer, even if the setup failed + request.node.addfinalizer(finalizer) + return result def cache_key(self, request: SubRequest) -> object: diff --git a/testing/python/fixtures.py b/testing/python/fixtures.py index 6051ab18fd8..1e22270e51b 100644 --- a/testing/python/fixtures.py +++ b/testing/python/fixtures.py @@ -4696,3 +4696,58 @@ def test_crash_expected_setup_and_teardown() -> None: ) result = pytester.runpytest() assert result.ret == 0 + + +def test_scoped_fixture_teardown_order(pytester: Pytester) -> None: + """ + Make sure teardowns happen in reverse order of setup with scoped fixtures, when + a later test only depends on a subset of scoped fixtures. + + Regression test for https://github.com/pytest-dev/pytest/issues/1489 + """ + pytester.makepyfile( + """ + from typing import Generator + + import pytest + + + last_executed = "" + + + @pytest.fixture(scope="module") + def fixture_1() -> Generator[None, None, None]: + global last_executed + assert last_executed == "" + last_executed = "fixture_1_setup" + yield + assert last_executed == "fixture_2_teardown" + last_executed = "fixture_1_teardown" + + + @pytest.fixture(scope="module") + def fixture_2() -> Generator[None, None, None]: + global last_executed + assert last_executed == "fixture_1_setup" + last_executed = "fixture_2_setup" + yield + assert last_executed == "run_test" + last_executed = "fixture_2_teardown" + + + def test_fixture_teardown_order(fixture_1: None, fixture_2: None) -> None: + global last_executed + assert last_executed == "fixture_2_setup" + last_executed = "run_test" + + + def test_2(fixture_1: None) -> None: + # This would previously queue an additional teardown of fixture_1, + # despite fixture_1's value being cached, which caused fixture_1 to be + # torn down before fixture_2 - violating the rule that teardowns should + # happen in reverse order of setup. + pass + """ + ) + result = pytester.runpytest() + assert result.ret == 0