From 1eebb78ee7a9fa53f0b855ea98cfa2b20dc3afd9 Mon Sep 17 00:00:00 2001 From: jakkdl Date: Tue, 26 Mar 2024 15:44:13 +0100 Subject: [PATCH] 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 d77dbace9..734733b10 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 f5f3ab398..546fadaf8 100644 --- a/src/_pytest/fixtures.py +++ b/src/_pytest/fixtures.py @@ -1035,19 +1035,25 @@ class FixtureDef(Generic[FixtureValue]): 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 @@ class FixtureDef(Generic[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 2dc8460d5..12ca6e926 100644 --- a/testing/python/fixtures.py +++ b/testing/python/fixtures.py @@ -4753,39 +4753,6 @@ def test_scoped_fixture_teardown_order(pytester: Pytester) -> 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