diff --git a/AUTHORS b/AUTHORS index b28e5613389..c1f0e323f8a 100644 --- a/AUTHORS +++ b/AUTHORS @@ -55,6 +55,7 @@ Charles Cloud Charles Machalow Charnjit SiNGH (CCSJ) Chris Lamb +Chris NeJame Christian Boelsen Christian Fetzer Christian Neumüller diff --git a/changelog/6436.bugfix.rst b/changelog/6436.bugfix.rst new file mode 100644 index 00000000000..656fb29d861 --- /dev/null +++ b/changelog/6436.bugfix.rst @@ -0,0 +1,3 @@ +Fixtures now properly register their finalizers with autouse and +parameterized fixtures that execute before them in the fixture stack so they are torn +down at the right times, and in the right order. diff --git a/src/_pytest/fixtures.py b/src/_pytest/fixtures.py index bf77d09f119..94ada346626 100644 --- a/src/_pytest/fixtures.py +++ b/src/_pytest/fixtures.py @@ -1024,9 +1024,12 @@ def finish(self, request: SubRequest) -> None: def execute(self, request: SubRequest) -> _FixtureValue: # Get required arguments and register our own finish() # with their finalization. - for argname in self.argnames: + for argname in self._dependee_fixture_argnames(request): + if argname == "request": + continue fixturedef = request._get_active_fixturedef(argname) - if argname != "request": + assert not isinstance(fixturedef, PseudoFixtureDef) + if not self._will_be_finalized_by_fixture(fixturedef): # PseudoFixtureDef is only for "request". assert isinstance(fixturedef, FixtureDef) fixturedef.addfinalizer(functools.partial(self.finish, request=request)) @@ -1052,6 +1055,79 @@ def execute(self, request: SubRequest) -> _FixtureValue: result = hook.pytest_fixture_setup(fixturedef=self, request=request) return result + def _will_be_finalized_by_fixture(self, fixturedef: "FixtureDef[object]") -> bool: + """Whether or not this fixture will be finalized by the passed fixture. + + Every :class:`FixtureDef` keeps a list of all the finalizers (tear downs) of + other :class:`FixtureDef instances that it should run before running its own. + Finalizers are added to this list not by this :class:`FixtureDef`, but by the + other :class:`FixtureDef` instances. They tell this instance that it's + responsible for tearing them down before it tears itself down. + + This method allows ``self`` to check if it has already told ``fixturedef`` that + it is responsible for tearing down ``self``. + """ + for finalizer in fixturedef._finalizers: + if "request" in getattr(finalizer, "keywords", {}): + request = getattr(finalizer, "keywords")["request"] + if self == request._fixturedef: + return True + return False + + def _dependee_fixture_argnames(self, request: SubRequest) -> Tuple[str, ...]: + """A list of argnames for fixtures that this fixture depends on. + + Given a request, this looks at the currently known list of fixture argnames, and + attempts to determine what slice of the list contains fixtures that it can know + should execute before it. This information is necessary so that this fixture can + know what fixtures to register its finalizer with to make sure that if they + would be torn down, they would tear down this fixture before themselves. It's + crucial for fixtures to be torn down in the inverse order that they were set up + in so that they don't try to clean up something that another fixture is still + depending on. + + When autouse fixtures are involved, it can be tricky to figure out when fixtures + should be torn down. To solve this, this method leverages the ``fixturenames`` + list provided by the ``request`` object, as this list is at least somewhat + sorted (in terms of the order fixtures are set up in) by the time this method is + reached. It's sorted enough that the starting point of fixtures that depend on + this one can be found using the ``self._parent_request`` stack. + + If a request in the ``self._parent_request`` stack has a ``:class:FixtureDef`` + associated with it, then that fixture is dependent on this one, so any fixture + names that appear in the list of fixture argnames that come after it can also be + ruled out. The argnames of all fixtures associated with a request in the + ``self._parent_request`` stack are found, and the lowest index argname is + considered the earliest point in the list of fixture argnames where everything + from that point onward can be considered to execute after this fixture. + Everything before this point can be considered fixtures that this fixture + depends on, and so this fixture should register its finalizer with all of them + to ensure that if any of them are to be torn down, they will tear this fixture + down first. + + This is the first part of the list of fixture argnames that is returned. The last + part of the list is everything in ``self.argnames`` as those are explicit + dependees of this fixture, so this fixture should definitely register its + finalizer with them. + """ + all_fix_names = request.fixturenames + try: + current_fix_index = all_fix_names.index(self.argname) + except ValueError: + current_fix_index = len(request.fixturenames) + parent_fixture_indexes = set() + + parent_request = request._parent_request + while isinstance(parent_request, SubRequest): + parent_fix_name = parent_request._fixturedef.argname + if parent_fix_name in all_fix_names: + parent_fixture_indexes.add(all_fix_names.index(parent_fix_name)) + parent_request = parent_request._parent_request + + stack_slice_index = min([current_fix_index, *parent_fixture_indexes]) + active_fixture_argnames = all_fix_names[:stack_slice_index] + return tuple(active_fixture_argnames) + self.argnames + def cache_key(self, request: SubRequest) -> object: return request.param_index if not hasattr(request, "param") else request.param diff --git a/testing/python/fixtures.py b/testing/python/fixtures.py index d54583858ca..8a3c1de43a3 100644 --- a/testing/python/fixtures.py +++ b/testing/python/fixtures.py @@ -1725,6 +1725,144 @@ def test_world(self): reprec.assertoutcome(passed=3) +class TestMultiLevelAutouseAndParametrization: + def test_setup_and_teardown_order(self, testdir): + """Tests that parametrized fixtures affect subsequent fixtures. (#6436) + + If a fixture uses a parametrized fixture, or, for any other reason, is executed + after the parametrized fixture in the fixture stack, then it should be affected + by the parametrization, and as a result, should be torn down before the + parametrized fixture, every time the parametrized fixture is torn down. This + should be the case even if autouse is involved and/or the linear order of + fixture execution isn't deterministic. In other words, before any fixture can be + torn down, every fixture that was executed after it must also be torn down. + """ + testdir.makepyfile( + test_auto=""" + import pytest + + def f(param): + return param + + @pytest.fixture(scope="session", autouse=True) + def s_fix(request): + yield + + @pytest.fixture(scope="package", params=["p1", "p2"], ids=f, autouse=True) + def p_fix(request): + yield + + @pytest.fixture(scope="module", params=["m1", "m2"], ids=f, autouse=True) + def m_fix(request): + yield + + @pytest.fixture(scope="class", autouse=True) + def another_c_fix(m_fix): + yield + + @pytest.fixture(scope="class") + def c_fix(): + yield + + @pytest.fixture(scope="function", params=["f1", "f2"], ids=f, autouse=True) + def f_fix(request): + yield + + class TestFixtures: + def test_a(self, c_fix): + pass + + def test_b(self, c_fix): + pass + """ + ) + result = testdir.runpytest("--setup-plan") + test_fixtures_used = ( + "(fixtures used: another_c_fix, c_fix, f_fix, m_fix, p_fix, request, s_fix)" + ) + expected = ( + "SETUP S s_fix\n" + " SETUP P p_fix['p1']\n" + " SETUP M m_fix['m1']\n" + " SETUP C another_c_fix (fixtures used: m_fix)\n" + " SETUP C c_fix\n" + " SETUP F f_fix['f1']\n" + " test_auto.py::TestFixtures::test_a[p1-m1-f1] {0}\n" + " TEARDOWN F f_fix['f1']\n" + " SETUP F f_fix['f2']\n" + " test_auto.py::TestFixtures::test_a[p1-m1-f2] {0}\n" + " TEARDOWN F f_fix['f2']\n" + " SETUP F f_fix['f1']\n" + " test_auto.py::TestFixtures::test_b[p1-m1-f1] {0}\n" + " TEARDOWN F f_fix['f1']\n" + " SETUP F f_fix['f2']\n" + " test_auto.py::TestFixtures::test_b[p1-m1-f2] {0}\n" + " TEARDOWN F f_fix['f2']\n" + " TEARDOWN C c_fix\n" + " TEARDOWN C another_c_fix\n" + " TEARDOWN M m_fix['m1']\n" + " SETUP M m_fix['m2']\n" + " SETUP C another_c_fix (fixtures used: m_fix)\n" + " SETUP C c_fix\n" + " SETUP F f_fix['f1']\n" + " test_auto.py::TestFixtures::test_a[p1-m2-f1] {0}\n" + " TEARDOWN F f_fix['f1']\n" + " SETUP F f_fix['f2']\n" + " test_auto.py::TestFixtures::test_a[p1-m2-f2] {0}\n" + " TEARDOWN F f_fix['f2']\n" + " SETUP F f_fix['f1']\n" + " test_auto.py::TestFixtures::test_b[p1-m2-f1] {0}\n" + " TEARDOWN F f_fix['f1']\n" + " SETUP F f_fix['f2']\n" + " test_auto.py::TestFixtures::test_b[p1-m2-f2] {0}\n" + " TEARDOWN F f_fix['f2']\n" + " TEARDOWN C c_fix\n" + " TEARDOWN C another_c_fix\n" + " TEARDOWN M m_fix['m2']\n" + " TEARDOWN P p_fix['p1']\n" + " SETUP P p_fix['p2']\n" + " SETUP M m_fix['m1']\n" + " SETUP C another_c_fix (fixtures used: m_fix)\n" + " SETUP C c_fix\n" + " SETUP F f_fix['f1']\n" + " test_auto.py::TestFixtures::test_a[p2-m1-f1] {0}\n" + " TEARDOWN F f_fix['f1']\n" + " SETUP F f_fix['f2']\n" + " test_auto.py::TestFixtures::test_a[p2-m1-f2] {0}\n" + " TEARDOWN F f_fix['f2']\n" + " SETUP F f_fix['f1']\n" + " test_auto.py::TestFixtures::test_b[p2-m1-f1] {0}\n" + " TEARDOWN F f_fix['f1']\n" + " SETUP F f_fix['f2']\n" + " test_auto.py::TestFixtures::test_b[p2-m1-f2] {0}\n" + " TEARDOWN F f_fix['f2']\n" + " TEARDOWN C c_fix\n" + " TEARDOWN C another_c_fix\n" + " TEARDOWN M m_fix['m1']\n" + " SETUP M m_fix['m2']\n" + " SETUP C another_c_fix (fixtures used: m_fix)\n" + " SETUP C c_fix\n" + " SETUP F f_fix['f1']\n" + " test_auto.py::TestFixtures::test_a[p2-m2-f1] {0}\n" + " TEARDOWN F f_fix['f1']\n" + " SETUP F f_fix['f2']\n" + " test_auto.py::TestFixtures::test_a[p2-m2-f2] {0}\n" + " TEARDOWN F f_fix['f2']\n" + " SETUP F f_fix['f1']\n" + " test_auto.py::TestFixtures::test_b[p2-m2-f1] {0}\n" + " TEARDOWN F f_fix['f1']\n" + " SETUP F f_fix['f2']\n" + " test_auto.py::TestFixtures::test_b[p2-m2-f2] {0}\n" + " TEARDOWN F f_fix['f2']\n" + " TEARDOWN C c_fix\n" + " TEARDOWN C another_c_fix\n" + " TEARDOWN M m_fix['m2']\n" + " TEARDOWN P p_fix['p2']\n" + "TEARDOWN S s_fix".format(test_fixtures_used).split("\n") + ) + result.stdout.fnmatch_lines(expected, consecutive=True) + + class TestAutouseManagement: def test_autouse_conftest_mid_directory(self, testdir): pkgdir = testdir.mkpydir("xyz123") @@ -4236,6 +4374,25 @@ def test_suffix(fix_combined): assert result.ret == 0 +class TestFinalizerOnlyAddedOnce: + @pytest.fixture(scope="class", autouse=True) + def a(self): + pass + + @pytest.fixture(scope="class", autouse=True) + def b(self, a): + pass + + def test_a_will_finalize_b(self, request): + a = request._get_active_fixturedef("a") + b = request._get_active_fixturedef("b") + assert b._will_be_finalized_by_fixture(a) + + def test_a_only_finishes_once(self, request): + a = request._get_active_fixturedef("a") + assert len(a._finalizers) + + def test_yield_fixture_with_no_value(testdir): testdir.makepyfile( """