From 358513b8a076f00f25dc5740404f7e4c8c855cce Mon Sep 17 00:00:00 2001 From: jakkdl Date: Mon, 18 Mar 2024 15:11:51 +0100 Subject: [PATCH 1/3] Don't reregister subfixture finalizer in parent fixture if value is cached. --- changelog/12135.bugfix.rst | 1 + src/_pytest/fixtures.py | 18 ++++++-- testing/python/fixtures.py | 85 ++++++++++++++++++++++++++++++++++++++ 3 files changed, 100 insertions(+), 4 deletions(-) create mode 100644 changelog/12135.bugfix.rst diff --git a/changelog/12135.bugfix.rst b/changelog/12135.bugfix.rst new file mode 100644 index 00000000000..d77dbace976 --- /dev/null +++ b/changelog/12135.bugfix.rst @@ -0,0 +1 @@ +Fix subfixtures adding their finalizer multiple times to parent fixtures, causing incorrect teardown ordering in some instances. diff --git a/src/_pytest/fixtures.py b/src/_pytest/fixtures.py index 48429a023ac..f5f3ab398d3 100644 --- a/src/_pytest/fixtures.py +++ b/src/_pytest/fixtures.py @@ -1035,13 +1035,18 @@ 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. + # Ensure arguments (parent fixtures) are loaded. + # If their cache has been invalidated it will finish itself and subfixtures, + # which will set our self.cached_result = None. + # If/when parent fixture parametrization is included in our cache key this + # can be moved after checking our cache key and not require saving in a list. + parent_fixtures_to_add_finalizer = [] for argname in self.argnames: fixturedef = request._get_active_fixturedef(argname) if not isinstance(fixturedef, PseudoFixtureDef): - fixturedef.addfinalizer(finalizer) + # save fixture as one to add our finalizer to, if we're not cached + # resolves #12135 + parent_fixtures_to_add_finalizer.append(fixturedef) my_cache_key = self.cache_key(request) if self.cached_result is not None: @@ -1060,6 +1065,11 @@ def execute(self, request: SubRequest) -> FixtureValue: self.finish(request) assert self.cached_result is None + finalizer = functools.partial(self.finish, request=request) + # add finalizer to parent fixtures + for parent_fixture in parent_fixtures_to_add_finalizer: + parent_fixture.addfinalizer(finalizer) + ihook = request.node.ihook try: # Setup the fixture, run the code in it, and cache the value diff --git a/testing/python/fixtures.py b/testing/python/fixtures.py index 1e22270e51b..2dc8460d5cc 100644 --- a/testing/python/fixtures.py +++ b/testing/python/fixtures.py @@ -4751,3 +4751,88 @@ def test_2(fixture_1: None) -> None: ) result = pytester.runpytest() assert result.ret == 0 + + +def test_subfixture_not_queue_multiple_finalizers(pytester: Pytester) -> None: + """Make sure subfixtures don't needlessly requeue their finalizer multiple times in + parent fixture. + + Regression test for #12135 + """ + pytester.makepyfile( + """ + import pytest + + def _assert_len(request): + assert len(request._get_active_fixturedef('fixture_1')._finalizers) == 1 + + + @pytest.fixture(scope="module") + def fixture_1(): + ... + + @pytest.fixture(scope="module") + def fixture_2(request, fixture_1): + yield + + def test_1(request, fixture_2): + _assert_len(request) + + def test_2(request, fixture_2): + _assert_len(request) + """ + ) + result = pytester.runpytest() + assert result.ret == 0 + + +def test_subfixture_teardown_order(pytester: Pytester) -> None: + """ + Make sure fixtures don't re-register their finalization in parent fixtures multiple + times, causing ordering failure in their teardowns. + + Regression test for #12135 + """ + pytester.makepyfile( + """ + import pytest + + execution_order = [] + + @pytest.fixture(scope="class") + def fixture_1(): + ... + + @pytest.fixture(scope="class") + def fixture_2(fixture_1): + execution_order.append("setup 2") + yield + execution_order.append("teardown 2") + + @pytest.fixture(scope="class") + def fixture_3(fixture_1): + execution_order.append("setup 3") + yield + execution_order.append("teardown 3") + + class TestFoo: + def test_initialize_fixtures(self, fixture_2, fixture_3): + ... + + # This would previously reschedule fixture_2's finalizer in the parent fixture, + # causing it to be torn down before fixture 3. + def test_reschedule_fixture_2(self, fixture_2): + ... + + # Force finalization directly on fixture_1 + # Otherwise the cleanup would sequence 3&2 before 1 as normal. + @pytest.mark.parametrize("fixture_1", [None], indirect=["fixture_1"]) + def test_finalize_fixture_1(self, fixture_1): + ... + + def test_result(): + assert execution_order == ["setup 2", "setup 3", "teardown 3", "teardown 2"] + """ + ) + result = pytester.runpytest() + assert result.ret == 0 From 1eebb78ee7a9fa53f0b855ea98cfa2b20dc3afd9 Mon Sep 17 00:00:00 2001 From: jakkdl Date: Tue, 26 Mar 2024 15:44:13 +0100 Subject: [PATCH 2/3] update comments, and remove test relying on internal functionality --- changelog/12135.bugfix.rst | 2 +- src/_pytest/fixtures.py | 30 +++++++++++++++++++----------- testing/python/fixtures.py | 33 --------------------------------- 3 files changed, 20 insertions(+), 45 deletions(-) diff --git a/changelog/12135.bugfix.rst b/changelog/12135.bugfix.rst index d77dbace976..734733b100d 100644 --- a/changelog/12135.bugfix.rst +++ b/changelog/12135.bugfix.rst @@ -1 +1 @@ -Fix subfixtures adding their finalizer multiple times to parent fixtures, causing incorrect teardown ordering in some instances. +Fix fixtures adding their finalizer multiple times to fixtures they request, causing unreliable and non-intuitive teardown ordering in some instances. diff --git a/src/_pytest/fixtures.py b/src/_pytest/fixtures.py index f5f3ab398d3..546fadaf840 100644 --- a/src/_pytest/fixtures.py +++ b/src/_pytest/fixtures.py @@ -1035,19 +1035,25 @@ def finish(self, request: SubRequest) -> None: raise BaseExceptionGroup(msg, exceptions[::-1]) def execute(self, request: SubRequest) -> FixtureValue: - # Ensure arguments (parent fixtures) are loaded. - # If their cache has been invalidated it will finish itself and subfixtures, - # which will set our self.cached_result = None. - # If/when parent fixture parametrization is included in our cache key this - # can be moved after checking our cache key and not require saving in a list. - parent_fixtures_to_add_finalizer = [] + """Return the value of this fixture, executing it if not cached.""" + # Ensure dependent fixtures that this fixture requests are loaded. + # This needs to be done before checking if we have a cached value, since + # if a dependent fixture has their cache invalidated, e.g. due to + # parametrization, they finalize themselves and fixtures depending on it + # (which will likely include this fixture) setting `self.cached_result = None`. + # See #4871 + requested_fixtures_that_should_finalize_us = [] for argname in self.argnames: fixturedef = request._get_active_fixturedef(argname) + # Saves requested fixtures in a list so we later can add our finalizer + # to them, ensuring that if a requested fixture gets torn down we get torn + # down first. This is generally handled by SetupState, but still currently + # needed when this fixture is not parametrized but depends on a parametrized + # fixture. if not isinstance(fixturedef, PseudoFixtureDef): - # save fixture as one to add our finalizer to, if we're not cached - # resolves #12135 - parent_fixtures_to_add_finalizer.append(fixturedef) + requested_fixtures_that_should_finalize_us.append(fixturedef) + # Check for (and return) cached value/exception. my_cache_key = self.cache_key(request) if self.cached_result is not None: cache_key = self.cached_result[1] @@ -1065,9 +1071,11 @@ def execute(self, request: SubRequest) -> FixtureValue: self.finish(request) assert self.cached_result is None + # Add finalizer to requested fixtures we saved previously. + # We make sure to do this after checking for cached value to avoid + # adding our finalizer multiple times. (#12135) finalizer = functools.partial(self.finish, request=request) - # add finalizer to parent fixtures - for parent_fixture in parent_fixtures_to_add_finalizer: + for parent_fixture in requested_fixtures_that_should_finalize_us: parent_fixture.addfinalizer(finalizer) ihook = request.node.ihook diff --git a/testing/python/fixtures.py b/testing/python/fixtures.py index 2dc8460d5cc..12ca6e92630 100644 --- a/testing/python/fixtures.py +++ b/testing/python/fixtures.py @@ -4753,39 +4753,6 @@ def test_2(fixture_1: None) -> None: assert result.ret == 0 -def test_subfixture_not_queue_multiple_finalizers(pytester: Pytester) -> None: - """Make sure subfixtures don't needlessly requeue their finalizer multiple times in - parent fixture. - - Regression test for #12135 - """ - pytester.makepyfile( - """ - import pytest - - def _assert_len(request): - assert len(request._get_active_fixturedef('fixture_1')._finalizers) == 1 - - - @pytest.fixture(scope="module") - def fixture_1(): - ... - - @pytest.fixture(scope="module") - def fixture_2(request, fixture_1): - yield - - def test_1(request, fixture_2): - _assert_len(request) - - def test_2(request, fixture_2): - _assert_len(request) - """ - ) - result = pytester.runpytest() - assert result.ret == 0 - - def test_subfixture_teardown_order(pytester: Pytester) -> None: """ Make sure fixtures don't re-register their finalization in parent fixtures multiple From 6bbd2b21842e28675e7b18396e9359411f22da1c Mon Sep 17 00:00:00 2001 From: John Litborn <11260241+jakkdl@users.noreply.github.com> Date: Sun, 31 Mar 2024 12:14:49 +0200 Subject: [PATCH 3/3] Update fixtures.py fix ambigious phrasing in comment --- src/_pytest/fixtures.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/_pytest/fixtures.py b/src/_pytest/fixtures.py index 21d53da4ec3..265ed601d9f 100644 --- a/src/_pytest/fixtures.py +++ b/src/_pytest/fixtures.py @@ -1036,7 +1036,7 @@ def finish(self, request: SubRequest) -> None: def execute(self, request: SubRequest) -> FixtureValue: """Return the value of this fixture, executing it if not cached.""" - # Ensure dependent fixtures that this fixture requests are loaded. + # Ensure that the dependent fixtures requested by this fixture are loaded. # This needs to be done before checking if we have a cached value, since # if a dependent fixture has their cache invalidated, e.g. due to # parametrization, they finalize themselves and fixtures depending on it