From 7dceabfcb2a8d178761bd862d37e01cbcc23f6d1 Mon Sep 17 00:00:00 2001 From: Bruno Oliveira Date: Wed, 27 Feb 2019 21:10:37 -0300 Subject: [PATCH 1/5] Ensure fixtures obtained with getfixturevalue() are finalized in the correct order Fix #1895 --- src/_pytest/fixtures.py | 23 ++++++++++++++++++----- src/_pytest/runner.py | 1 + testing/python/fixture.py | 2 +- 3 files changed, 20 insertions(+), 6 deletions(-) diff --git a/src/_pytest/fixtures.py b/src/_pytest/fixtures.py index b2ad9aae3..fa45dffc1 100644 --- a/src/_pytest/fixtures.py +++ b/src/_pytest/fixtures.py @@ -585,11 +585,13 @@ class FixtureRequest(FuncargnamesCompatAttr): # call the fixture function fixturedef.execute(request=subrequest) finally: - # if fixture function failed it might have registered finalizers - self.session._setupstate.addfinalizer( - functools.partial(fixturedef.finish, request=subrequest), - subrequest.node, - ) + self._schedule_finalizers(fixturedef, subrequest) + + def _schedule_finalizers(self, fixturedef, subrequest): + # if fixture function failed it might have registered finalizers + self.session._setupstate.addfinalizer( + functools.partial(fixturedef.finish, request=subrequest), subrequest.node + ) def _check_scope(self, argname, invoking_scope, requested_scope): if argname == "request": @@ -659,6 +661,16 @@ class SubRequest(FixtureRequest): def addfinalizer(self, finalizer): self._fixturedef.addfinalizer(finalizer) + def _schedule_finalizers(self, fixturedef, subrequest): + # 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.funcargnames: + fixturedef.addfinalizer( + functools.partial(self._fixturedef.finish, request=self) + ) + super(SubRequest, self)._schedule_finalizers(fixturedef, subrequest) + scopes = "session package module class function".split() scopenum_function = scopes.index("function") @@ -858,6 +870,7 @@ class FixtureDef(object): def execute(self, request): # get required arguments and register our own finish() # with their finalization + # TODO CHECK HOW TO AVOID EXPLICITLY FINALIZING AGAINST ARGNAMES for argname in self.argnames: fixturedef = request._get_active_fixturedef(argname) if argname != "request": diff --git a/src/_pytest/runner.py b/src/_pytest/runner.py index 8357991fe..55dcd8054 100644 --- a/src/_pytest/runner.py +++ b/src/_pytest/runner.py @@ -327,6 +327,7 @@ class SetupState(object): assert callable(finalizer) # assert colitem in self.stack # some unit tests don't setup stack :/ self._finalizers.setdefault(colitem, []).append(finalizer) + pass def _pop_and_teardown(self): colitem = self.stack.pop() diff --git a/testing/python/fixture.py b/testing/python/fixture.py index 3d557cec8..d9a512d70 100644 --- a/testing/python/fixture.py +++ b/testing/python/fixture.py @@ -1866,7 +1866,7 @@ class TestAutouseManagement(object): "setup-2", "step1-2", "step2-2", "teardown-2",] """ ) - reprec = testdir.inline_run("-s") + reprec = testdir.inline_run("-v") reprec.assertoutcome(passed=5) def test_ordering_autouse_before_explicit(self, testdir): From 525639eaa00aed6d545244b63b10f9f8facce737 Mon Sep 17 00:00:00 2001 From: Bruno Oliveira Date: Thu, 28 Feb 2019 20:28:54 -0300 Subject: [PATCH 2/5] Rename fixtures testing file to be consistent with the module name --- testing/python/{fixture.py => fixtures.py} | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename testing/python/{fixture.py => fixtures.py} (100%) diff --git a/testing/python/fixture.py b/testing/python/fixtures.py similarity index 100% rename from testing/python/fixture.py rename to testing/python/fixtures.py From d97473e551ef70831c601f9c46f55ef030bb646f Mon Sep 17 00:00:00 2001 From: Bruno Oliveira Date: Thu, 28 Feb 2019 20:59:37 -0300 Subject: [PATCH 3/5] Add test and CHANGELOG for #1895 --- changelog/1895.bugfix.rst | 2 ++ testing/python/fixtures.py | 38 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 40 insertions(+) create mode 100644 changelog/1895.bugfix.rst diff --git a/changelog/1895.bugfix.rst b/changelog/1895.bugfix.rst new file mode 100644 index 000000000..44b921ad9 --- /dev/null +++ b/changelog/1895.bugfix.rst @@ -0,0 +1,2 @@ +Fix bug where fixtures requested dynamically via ``request.getfixturevalue()`` might be teardown +before the requesting fixture. diff --git a/testing/python/fixtures.py b/testing/python/fixtures.py index d9a512d70..45c7a17ae 100644 --- a/testing/python/fixtures.py +++ b/testing/python/fixtures.py @@ -562,6 +562,44 @@ class TestRequestBasic(object): reprec = testdir.inline_run() reprec.assertoutcome(passed=1) + def test_getfixturevalue_teardown(self, testdir): + """ + Issue #1895 + + `test_inner` requests `inner` fixture, which in turns requests `resource` + using getfixturevalue. `test_func` then requests `resource`. + + `resource` is teardown before `inner` because the fixture mechanism won't consider + `inner` dependent on `resource` when it is get via `getfixturevalue`: `test_func` + will then cause the `resource`'s finalizer to be called first because of this. + """ + testdir.makepyfile( + """ + import pytest + + @pytest.fixture(scope='session') + def resource(): + r = ['value'] + yield r + r.pop() + + @pytest.fixture(scope='session') + def inner(request): + resource = request.getfixturevalue('resource') + assert resource == ['value'] + yield + assert resource == ['value'] + + def test_inner(inner): + pass + + def test_func(resource): + pass + """ + ) + result = testdir.runpytest() + result.stdout.fnmatch_lines("* 2 passed in *") + @pytest.mark.parametrize("getfixmethod", ("getfixturevalue", "getfuncargvalue")) def test_getfixturevalue(self, testdir, getfixmethod): item = testdir.getitem( From 6a2d122a5069ca339f4f5d5faf8759162e2fbabc Mon Sep 17 00:00:00 2001 From: Bruno Oliveira Date: Sat, 2 Mar 2019 09:56:15 -0300 Subject: [PATCH 4/5] Remove code debugging leftovers --- src/_pytest/fixtures.py | 1 - src/_pytest/runner.py | 1 - testing/python/fixtures.py | 2 +- 3 files changed, 1 insertion(+), 3 deletions(-) diff --git a/src/_pytest/fixtures.py b/src/_pytest/fixtures.py index fa45dffc1..2635d095e 100644 --- a/src/_pytest/fixtures.py +++ b/src/_pytest/fixtures.py @@ -870,7 +870,6 @@ class FixtureDef(object): def execute(self, request): # get required arguments and register our own finish() # with their finalization - # TODO CHECK HOW TO AVOID EXPLICITLY FINALIZING AGAINST ARGNAMES for argname in self.argnames: fixturedef = request._get_active_fixturedef(argname) if argname != "request": diff --git a/src/_pytest/runner.py b/src/_pytest/runner.py index 55dcd8054..8357991fe 100644 --- a/src/_pytest/runner.py +++ b/src/_pytest/runner.py @@ -327,7 +327,6 @@ class SetupState(object): assert callable(finalizer) # assert colitem in self.stack # some unit tests don't setup stack :/ self._finalizers.setdefault(colitem, []).append(finalizer) - pass def _pop_and_teardown(self): colitem = self.stack.pop() diff --git a/testing/python/fixtures.py b/testing/python/fixtures.py index 45c7a17ae..7b328c5b2 100644 --- a/testing/python/fixtures.py +++ b/testing/python/fixtures.py @@ -1904,7 +1904,7 @@ class TestAutouseManagement(object): "setup-2", "step1-2", "step2-2", "teardown-2",] """ ) - reprec = testdir.inline_run("-v") + reprec = testdir.inline_run("-s") reprec.assertoutcome(passed=5) def test_ordering_autouse_before_explicit(self, testdir): From c334adc78ff54a7fef992b62e2fc425d924bc8a1 Mon Sep 17 00:00:00 2001 From: Daniel Hahler Date: Sun, 3 Mar 2019 11:20:00 -0300 Subject: [PATCH 5/5] Apply suggestions from code review Co-Authored-By: nicoddemus --- testing/python/fixtures.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/testing/python/fixtures.py b/testing/python/fixtures.py index 7b328c5b2..1ea37f85c 100644 --- a/testing/python/fixtures.py +++ b/testing/python/fixtures.py @@ -566,11 +566,11 @@ class TestRequestBasic(object): """ Issue #1895 - `test_inner` requests `inner` fixture, which in turns requests `resource` - using getfixturevalue. `test_func` then requests `resource`. + `test_inner` requests `inner` fixture, which in turn requests `resource` + using `getfixturevalue`. `test_func` then requests `resource`. `resource` is teardown before `inner` because the fixture mechanism won't consider - `inner` dependent on `resource` when it is get via `getfixturevalue`: `test_func` + `inner` dependent on `resource` when it is used via `getfixturevalue`: `test_func` will then cause the `resource`'s finalizer to be called first because of this. """ testdir.makepyfile(