From 163b03cf25d7822a16cc95fb637b88186ad4767b Mon Sep 17 00:00:00 2001 From: Chris NeJame Date: Wed, 15 Jan 2020 11:00:42 -0500 Subject: [PATCH 1/6] fixtures register finalizers with all fixtures before them in the stack --- AUTHORS | 1 + changelog/6436.bugfix.rst | 3 + src/_pytest/fixtures.py | 59 ++++++++++++++++- testing/python/fixtures.py | 132 +++++++++++++++++++++++++++++++++++++ 4 files changed, 192 insertions(+), 3 deletions(-) create mode 100644 changelog/6436.bugfix.rst diff --git a/AUTHORS b/AUTHORS index af0dc62c4d8..a0922f54202 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..9afa252d57d --- /dev/null +++ b/changelog/6436.bugfix.rst @@ -0,0 +1,3 @@ +:class:`FixtureDef <_pytest.fixtures.FixtureDef>` objects 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 b39503f0e20..3a0ff1e3e01 100644 --- a/src/_pytest/fixtures.py +++ b/src/_pytest/fixtures.py @@ -886,9 +886,7 @@ def finish(self, request): self._finalizers = [] def execute(self, request): - # get required arguments and register our own finish() - # with their finalization - for argname in self.argnames: + for argname in self._dependee_fixture_argnames(request): fixturedef = request._get_active_fixturedef(argname) if argname != "request": fixturedef.addfinalizer(functools.partial(self.finish, request=request)) @@ -912,6 +910,61 @@ def execute(self, request): hook = self._fixturemanager.session.gethookproxy(request.node.fspath) return hook.pytest_fixture_setup(fixturedef=self, request=request) + def _dependee_fixture_argnames(self, request): + """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 hasattr(parent_request, "_parent_request"): + if hasattr(parent_request, "_fixturedef"): + 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 {*active_fixture_argnames, *self.argnames} + def cache_key(self, request): 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 bfbe359515c..e8240da206c 100644 --- a/testing/python/fixtures.py +++ b/testing/python/fixtures.py @@ -1748,6 +1748,138 @@ def test_world(self): reprec.assertoutcome(passed=3) +class TestMultiLevelAutouseAndParameterization: + def test_setup_and_teardown_order(self, testdir): + """Tests that parameterized fixtures effect subsequent fixtures. (#6436) + + If a fixture uses a parameterized fixture, or, for any other reason, is executed + after the parameterized fixture in the fixture stack, then it should be affected + by the parameterization, and as a result, should be torn down before the + parameterized fixture, every time the parameterized 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)" + ) + result.stdout.fnmatch_lines( + """ + SETUP S s_fix + SETUP P p_fix[p1] + SETUP M m_fix[m1] + SETUP C another_c_fix (fixtures used: m_fix) + SETUP C c_fix + SETUP F f_fix[f1] + test_auto.py::TestFixtures::test_a[p1-m1-f1] {0} + TEARDOWN F f_fix[f1] + SETUP F f_fix[f2] + test_auto.py::TestFixtures::test_a[p1-m1-f2] {0} + TEARDOWN F f_fix[f2] + SETUP F f_fix[f1] + test_auto.py::TestFixtures::test_b[p1-m1-f1] {0} + TEARDOWN F f_fix[f1] + SETUP F f_fix[f2] + test_auto.py::TestFixtures::test_b[p1-m1-f2] {0} + TEARDOWN F f_fix[f2] + TEARDOWN C c_fix + TEARDOWN C another_c_fix + TEARDOWN M m_fix[m1] + SETUP M m_fix[m2] + SETUP C another_c_fix (fixtures used: m_fix) + SETUP C c_fix + SETUP F f_fix[f1] + test_auto.py::TestFixtures::test_a[p1-m2-f1] {0} + TEARDOWN F f_fix[f1] + SETUP F f_fix[f2] + test_auto.py::TestFixtures::test_a[p1-m2-f2] {0} + TEARDOWN F f_fix[f2] + SETUP F f_fix[f1] + test_auto.py::TestFixtures::test_b[p1-m2-f1] {0} + TEARDOWN F f_fix[f1] + SETUP F f_fix[f2] + test_auto.py::TestFixtures::test_b[p1-m2-f2] {0} + TEARDOWN F f_fix[f2] + TEARDOWN C c_fix + TEARDOWN C another_c_fix + TEARDOWN M m_fix[m2] + TEARDOWN P p_fix[p1] + SETUP P p_fix[p2] + SETUP M m_fix[m1] + SETUP C another_c_fix (fixtures used: m_fix) + SETUP C c_fix + SETUP F f_fix[f1] + test_auto.py::TestFixtures::test_a[p2-m1-f1] {0} + TEARDOWN F f_fix[f1] + SETUP F f_fix[f2] + test_auto.py::TestFixtures::test_a[p2-m1-f2] {0} + TEARDOWN F f_fix[f2] + SETUP F f_fix[f1] + test_auto.py::TestFixtures::test_b[p2-m1-f1] {0} + TEARDOWN F f_fix[f1] + SETUP F f_fix[f2] + test_auto.py::TestFixtures::test_b[p2-m1-f2] {0} + TEARDOWN F f_fix[f2] + TEARDOWN C c_fix + TEARDOWN C another_c_fix + TEARDOWN M m_fix[m1] + SETUP M m_fix[m2] + SETUP C another_c_fix (fixtures used: m_fix) + SETUP C c_fix + SETUP F f_fix[f1] + test_auto.py::TestFixtures::test_a[p2-m2-f1] {0} + TEARDOWN F f_fix[f1] + SETUP F f_fix[f2] + test_auto.py::TestFixtures::test_a[p2-m2-f2] {0} + TEARDOWN F f_fix[f2] + SETUP F f_fix[f1] + test_auto.py::TestFixtures::test_b[p2-m2-f1] {0} + TEARDOWN F f_fix[f1] + SETUP F f_fix[f2] + test_auto.py::TestFixtures::test_b[p2-m2-f2] {0} + TEARDOWN F f_fix[f2] + TEARDOWN C c_fix + TEARDOWN C another_c_fix + TEARDOWN M m_fix[m2] + TEARDOWN P p_fix[p2] + TEARDOWN S s_fix + """.format( + test_fixtures_used + ) + ) + + class TestAutouseManagement: def test_autouse_conftest_mid_directory(self, testdir): pkgdir = testdir.mkpydir("xyz123") From b84d70b0f1e440b352c3236ffc0c48f04bd31814 Mon Sep 17 00:00:00 2001 From: Daniel Hahler Date: Thu, 23 Jan 2020 19:53:22 +0100 Subject: [PATCH 2/6] _dependee_fixture_argnames: return tuple Note: black cannot parse `return *active_fixture_argnames, *self.argnames` yet (fixed in master, https://github.com/psf/black/pull/1121). Tested manually using: ```python @pytest.fixture(scope="session") def xdist_suffix(request): print("\nxdist_suffix") suffixes.append("xdist") @pytest.fixture(scope="session") def parallel_suffix(tox_suffix, xdist_suffix): pass def test_suffix(parallel_suffix): assert suffixes == ["tox", "xdist"] ``` When using a set there the order is not deterministic, i.e. the test is flaky. --- 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 3a0ff1e3e01..d2e4e6d3313 100644 --- a/src/_pytest/fixtures.py +++ b/src/_pytest/fixtures.py @@ -963,7 +963,7 @@ def _dependee_fixture_argnames(self, request): stack_slice_index = min([current_fix_index, *parent_fixture_indexes]) active_fixture_argnames = all_fix_names[:stack_slice_index] - return {*active_fixture_argnames, *self.argnames} + return tuple(active_fixture_argnames) + self.argnames def cache_key(self, request): return request.param_index if not hasattr(request, "param") else request.param From cfee6e7094d1f2fc512974a18768599d1d0291b1 Mon Sep 17 00:00:00 2001 From: Chris NeJame Date: Thu, 23 Jan 2020 14:50:21 -0500 Subject: [PATCH 3/6] avoid exponential recursion by checking if already added --- src/_pytest/fixtures.py | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/src/_pytest/fixtures.py b/src/_pytest/fixtures.py index d2e4e6d3313..cc93ca8634f 100644 --- a/src/_pytest/fixtures.py +++ b/src/_pytest/fixtures.py @@ -889,7 +889,14 @@ def execute(self, request): for argname in self._dependee_fixture_argnames(request): fixturedef = request._get_active_fixturedef(argname) if argname != "request": - fixturedef.addfinalizer(functools.partial(self.finish, request=request)) + for fin in fixturedef._finalizers: + if "request" in getattr(fin, "keywords", {}): + if self == fin.keywords["request"]._fixturedef: + break + else: + fixturedef.addfinalizer( + functools.partial(self.finish, request=request) + ) my_cache_key = self.cache_key(request) if self.cached_result is not None: From a950653d58eea205088cdee32b4d0ffef19ba952 Mon Sep 17 00:00:00 2001 From: Chris NeJame Date: Fri, 24 Jan 2020 16:03:41 -0500 Subject: [PATCH 4/6] cleaned up, DAMPified, and added tests to ensure fixtures only add ther finalizer to a dependee fixture once --- src/_pytest/fixtures.py | 34 +++++++++++++++++++++++++--------- testing/python/fixtures.py | 20 ++++++++++++++++++++ 2 files changed, 45 insertions(+), 9 deletions(-) diff --git a/src/_pytest/fixtures.py b/src/_pytest/fixtures.py index cc93ca8634f..1a74925af9d 100644 --- a/src/_pytest/fixtures.py +++ b/src/_pytest/fixtures.py @@ -887,16 +887,13 @@ def finish(self, request): def execute(self, request): for argname in self._dependee_fixture_argnames(request): + if argname == "request": + continue fixturedef = request._get_active_fixturedef(argname) - if argname != "request": - for fin in fixturedef._finalizers: - if "request" in getattr(fin, "keywords", {}): - if self == fin.keywords["request"]._fixturedef: - break - else: - fixturedef.addfinalizer( - functools.partial(self.finish, request=request) - ) + if not self._will_be_finalized_by_fixture(fixturedef): + fixturedef.addfinalizer( + functools.partial(self.finish, request=request) + ) my_cache_key = self.cache_key(request) if self.cached_result is not None: @@ -917,6 +914,25 @@ def execute(self, request): hook = self._fixturemanager.session.gethookproxy(request.node.fspath) return hook.pytest_fixture_setup(fixturedef=self, request=request) + def _will_be_finalized_by_fixture(self, fixturedef): + """Whether or not this fixture be finalized by the passed fixture. + + Every ``:class:FixtureDef`` keeps a list of all the finishers (tear downs) of + other ``:class:FixtureDef`` instances that it should run before running its own. + Finishers 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 a ``:class:FixtureDef`` to check if it has already told + another ``:class:FixtureDef`` that the latter ``:class:FixtureDef`` is + responsible for tearing down this ``:class:FixtureDef``. + """ + for finalizer in fixturedef._finalizers: + if "request" in getattr(finalizer, "keywords", {}): + if self == finalizer.keywords["request"]._fixturedef: + return True + return False + def _dependee_fixture_argnames(self, request): """A list of argnames for fixtures that this fixture depends on. diff --git a/testing/python/fixtures.py b/testing/python/fixtures.py index e8240da206c..da5bdcfb823 100644 --- a/testing/python/fixtures.py +++ b/testing/python/fixtures.py @@ -4423,3 +4423,23 @@ def test_suffix(fix_combined): ) result = testdir.runpytest("-vv", str(p1)) 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_one(self, request): + a = request._get_active_fixturedef("a") + assert len(a._finalizers) From b1e1b73001b7a3db06a470689116b8d2b699557a Mon Sep 17 00:00:00 2001 From: Chris NeJame Date: Tue, 28 Jan 2020 13:49:15 -0500 Subject: [PATCH 5/6] add type annotations --- src/_pytest/fixtures.py | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/src/_pytest/fixtures.py b/src/_pytest/fixtures.py index 1a74925af9d..0a23c25149e 100644 --- a/src/_pytest/fixtures.py +++ b/src/_pytest/fixtures.py @@ -5,8 +5,10 @@ from collections import defaultdict from collections import deque from collections import OrderedDict +from typing import Any from typing import Dict from typing import List +from typing import Optional from typing import Tuple import attr @@ -885,15 +887,13 @@ def finish(self, request): self.cached_result = None self._finalizers = [] - def execute(self, request): + def execute(self, request: SubRequest) -> Optional[Any]: for argname in self._dependee_fixture_argnames(request): if argname == "request": continue fixturedef = request._get_active_fixturedef(argname) if not self._will_be_finalized_by_fixture(fixturedef): - fixturedef.addfinalizer( - functools.partial(self.finish, request=request) - ) + fixturedef.addfinalizer(functools.partial(self.finish, request=request)) my_cache_key = self.cache_key(request) if self.cached_result is not None: @@ -914,7 +914,7 @@ def execute(self, request): hook = self._fixturemanager.session.gethookproxy(request.node.fspath) return hook.pytest_fixture_setup(fixturedef=self, request=request) - def _will_be_finalized_by_fixture(self, fixturedef): + def _will_be_finalized_by_fixture(self, fixturedef: "FixtureDef") -> bool: """Whether or not this fixture be finalized by the passed fixture. Every ``:class:FixtureDef`` keeps a list of all the finishers (tear downs) of @@ -933,7 +933,7 @@ def _will_be_finalized_by_fixture(self, fixturedef): return True return False - def _dependee_fixture_argnames(self, request): + 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 @@ -976,7 +976,7 @@ def _dependee_fixture_argnames(self, request): current_fix_index = len(request.fixturenames) parent_fixture_indexes = set() - parent_request = request._parent_request + parent_request = getattr(request, "_parent_request") while hasattr(parent_request, "_parent_request"): if hasattr(parent_request, "_fixturedef"): parent_fix_name = parent_request._fixturedef.argname @@ -986,7 +986,7 @@ def _dependee_fixture_argnames(self, 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 + return tuple(tuple(active_fixture_argnames) + self.argnames) def cache_key(self, request): return request.param_index if not hasattr(request, "param") else request.param From 4e0778386e3c3ec5e34ff92b00919a08511063d0 Mon Sep 17 00:00:00 2001 From: Daniel Hahler Date: Mon, 9 Mar 2020 13:01:35 +0100 Subject: [PATCH 6/6] fixup! cleaned up, DAMPified, and added tests to ensure fixtures only add ther finalizer to a dependee fixture once --- testing/python/fixtures.py | 1 - 1 file changed, 1 deletion(-) diff --git a/testing/python/fixtures.py b/testing/python/fixtures.py index da5bdcfb823..c3a3d3bb19b 100644 --- a/testing/python/fixtures.py +++ b/testing/python/fixtures.py @@ -4426,7 +4426,6 @@ def test_suffix(fix_combined): class TestFinalizerOnlyAddedOnce: - @pytest.fixture(scope="class", autouse=True) def a(self): pass