Use session.config.hook instead of ihook. Fixes #2124 #2127

malinoff · 2016-12-10T10:48:17Z

Make sure to include one or more tests for your change;
Add yourself to AUTHORS;
Add a new entry to CHANGELOG.rst
- Choose any open position to avoid merge conflicts with other PRs.
- Add a link to the issue you are fixing (if any) using RST syntax.
- The pytest team likes to have people to acknowledged in the CHANGELOG, so please add a thank note to yourself ("Thanks @user for the PR") and a link to your GitHub profile. It may sound weird thanking yourself, but otherwise a maintainer would have to do it manually before or after merging instead of just using GitHub's merge button. This makes it easier on the maintainers to merge PRs.

coveralls · 2016-12-10T10:56:17Z

Coverage remained the same at 92.832% when pulling 522d59e on malinoff:fix-2124 into da40bcf on pytest-dev:master.

RonnyPfannschmidt · 2016-12-10T13:52:07Z

_pytest/fixtures.py

@@ -783,8 +783,8 @@ def execute(self, request):
            self.finish()
            assert not hasattr(self, "cached_result")

-        ihook = self._fixturemanager.session.ihook
-        return ihook.pytest_fixture_setup(fixturedef=self, request=request)
+        hook = self._fixturemanager.session.config.hook


as far as i understand, at this place, config.hook is an incorrect choice, as it does not apply fs path filtering

@hpk42 please cross-check my concern

Shouldn't then session.gethookproxy(fspath) be used here instead?

@nicoddemus i think so

@nicoddemus @RonnyPfannschmidt I disagree - session.gethookproxy(fspath) is specifically what I'm changing in this PR, because session.ihook does precisely session.gethookproxy(self.fspath) (https://github.com/pytest-dev/pytest/blob/master/_pytest/main.py#L267-L270) and it doesn't work.

Session.fspath is indeeed broken, you need the item fspath, which has a different value

malinoff · 2016-12-14T03:02:16Z

@hpk42 please, review my PR. It blocks me.

malinoff · 2016-12-20T04:46:18Z

@RonnyPfannschmidt @nicoddemus ok, I've changed pytest_fixture_setup hook call to use an item fspath (please, verify its correctness), but I'm not sure about pytest_fixture_post_finalizer - there is no request in there.

coveralls · 2016-12-20T04:51:55Z

Coverage remained the same at 92.832% when pulling a63b34c on malinoff:fix-2124 into da40bcf on pytest-dev:master.

RonnyPfannschmidt · 2016-12-20T07:53:33Z

@malinoff the scope of this one is unfortunately growing

a pr like this where one detail after another creeps up tends to be very frustrating for the contributor and i am very thankful that you patiently stick to it

i believe the request needs to be turned into a argument of the finish() method so that it can use it as needed

nicoddemus · 2017-04-13T22:21:59Z

@malinoff @RonnyPfannschmidt gentle ping. 😁

RonnyPfannschmidt · 2017-06-29T19:34:45Z

@nicoddemus i reviewed correct passing of the data - and it seems currently impossible, the complete fixture definition work-flow is completely broken as far as i can tell and it only works because it does the wrong thing which is mostly not wrong enough to be a problem

@hpk42 im deferring the complete issue to you

…tems with empty "parent.nodeid" if run from a different root directory

…info remove unused _pytest.runner.NodeInfo class

nicoddemus · 2017-11-10T21:11:19Z

@RonnyPfannschmidt should we close this? The PR is now stale anyway...

malinoff · 2017-11-11T02:55:12Z

This PR is not stale. I'm waiting for the guidance of how should I proceed.

nicoddemus · 2017-11-11T02:59:21Z

This PR is not stale. I'm waiting for the guidance of how should I proceed.

Oh OK thanks for the quick response.

Sorry if my comment implied that you were somehow to blame; it as more of an observation that we have not seen activity on it over some months and I was under the impression that it would not get anymore attention.

nicoddemus · 2017-11-11T05:02:20Z

Took the opportunity to take a further look into this, and I managed to pass request objects to finalize which I think is reasonable:

diff --git a/_pytest/fixtures.py b/_pytest/fixtures.py
index f71f357..5ac210c 100644
--- a/_pytest/fixtures.py
+++ b/_pytest/fixtures.py
@@ -4,6 +4,7 @@ import inspect
 import sys
 import warnings

+import functools
 import py
 from py._code.code import FormattedExcinfo

@@ -521,7 +522,7 @@ class FixtureRequest(FuncargnamesCompatAttr):
             val = fixturedef.execute(request=subrequest)
         finally:
             # if fixture function failed it might have registered finalizers
-            self.session._setupstate.addfinalizer(fixturedef.finish,
+            self.session._setupstate.addfinalizer(functools.partial(fixturedef.finish, request=subrequest),
                                                   subrequest.node)
         return val

@@ -742,7 +743,7 @@ class FixtureDef:
     def addfinalizer(self, finalizer):
         self._finalizer.append(finalizer)

-    def finish(self):
+    def finish(self, request):
         exceptions = []
         try:
             while self._finalizer:
@@ -757,7 +758,7 @@ class FixtureDef:
                 py.builtin._reraise(*e)

         finally:
-            ihook = self._fixturemanager.session.ihook
+            ihook = self._fixturemanager.session.gethookproxy(request.node.fspath)
             ihook.pytest_fixture_post_finalizer(fixturedef=self)
             # even if finalization fails, we invalidate
             # the cached fixture value
@@ -770,7 +771,7 @@ class FixtureDef:
         for argname in self.argnames:
             fixturedef = request._get_active_fixturedef(argname)
             if argname != "request":
-                fixturedef.addfinalizer(self.finish)
+                fixturedef.addfinalizer(functools.partial(self.finish, request))

         my_cache_key = request.param_index
         cached_result = getattr(self, "cached_result", None)
@@ -783,10 +784,10 @@ class FixtureDef:
                     return result
             # we have a previous but differently parametrized fixture instance
             # so we need to tear it down before creating a new one
-            self.finish()
+            self.finish(request)
             assert not hasattr(self, "cached_result")

-        ihook = self._fixturemanager.session.ihook
+        ihook = self._fixturemanager.session.gethookproxy(request.node.fspath)
         return ihook.pytest_fixture_setup(fixturedef=self, request=request)

     def __repr__(self):

This passes all tests for me on Python 3.6 and produces correct results on my short test below:

# conftest.py
import pytest

def pytest_fixture_post_finalizer():
    print('ROOT conftest: pytest_fixture_post_finalizer')

def pytest_fixture_setup():
    print('ROOT conftest: pytest_fixture_setup')

@pytest.fixture(scope='module')
def my_fixture():
    yield 'my_fixture value'

# tests/conftest.py
def pytest_fixture_post_finalizer():
    print('SUB conftest: pytest_fixture_post_finalizer')

def pytest_fixture_setup():
    print('SUB conftest: pytest_fixture_setup')

# tests/test_usage.py
def test_usage(my_fixture):
    print('TEST USAGE')

def test_foobar(my_fixture):
    print('TEST FOOBAR')

The hooks are called appropriately in the correct order:

SUB conftest: pytest_fixture_setup
ROOT conftest: pytest_fixture_setup
.TEST USAGE
.TEST FOOBAR
SUB conftest: pytest_fixture_post_finalizer
ROOT conftest: pytest_fixture_post_finalizer
SUB conftest: pytest_fixture_post_finalizer
ROOT conftest: pytest_fixture_post_finalizer

Before the patch:

test_usage.py ROOT conftest: pytest_fixture_setup
.TEST USAGE
.TEST FOOBAR
ROOT conftest: pytest_fixture_post_finalizer
ROOT conftest: pytest_fixture_post_finalizer

Finally, if we are fixing this, we should take the opportunity to pass request to pytest_fixture_post_finalizer as well.

This was not easy, took me a couple of hours of debugging to understand all the moving parts involved. The parts I think were hard for me to make sense while debugging was the relationship between FixtureDef, Request, SubRequest and SetupState: at first glance it seems finalization is centered and handled by the first three, while it is actually deferred by them to SetupState. I believe this is an artifact before there were proper fixtures and the user was required to manage "setup state" by hand (pre 2.0?).

Also surprising why finalize() is called so many times (once for each item which uses it).

I previously thought the pytest_runtest_protocol(item, nextitem) exists because it is responsible for destroying fixtures: knowing next item, the function can know which scopes should be torn down. This is not the case, and the destruction happens somewhat implicitly by stacks of callbacks. It works, but perhaps a more explicit approach would be easier to understand.

This implicit definition really tripped me while debugging pytest-dev#2127, unfortunately hidden as it was in the middle of all the variable declarations. I think the explicit definition is much easier for the eyes and IDEs to find.

Make SubRequest.addfinalizer an explicit method

RonnyPfannschmidt · 2017-11-11T08:48:59Z

@nicoddemus looks good, the failure is unrelated, can you also resolve the conflicts by a merge ?`

…d-with-pyargs Fix pytest-dev#2775 - running pytest with "--pyargs" will result in Items with …

nicoddemus · 2017-11-12T13:41:20Z

_pytest/fixtures.py

            if hasattr(self, "cached_result"):
                del self.cached_result
+            self._finalizers = []


@RonnyPfannschmidt I realized we should also do this at this point to avoid keeping instances of bound methods alive in case they are registered as finalizers and an exception occurred during teardown, do you agree?

nicoddemus · 2017-11-12T13:45:36Z

_pytest/main.py

@@ -499,8 +499,16 @@ def __init__(self, fspath, parent=None, config=None, session=None):
        super(FSCollector, self).__init__(name, parent, config, session)
        self.fspath = fspath

+    def _check_initialpaths_for_relpath(self):


Unfortunately I first merged with upstream/master while working here so this PR is bringing #2776 into features as well. Because it is a small patch I don't think this will get into the way of reviewing things so if you guys are OK with it I won't bother trying to fix history in this case.

nicoddemus · 2017-11-12T13:46:51Z

Updated the CHANGELOG and changed the target branch to features because pytest_fixture_post_finalizer hook can now receive a request argument.

coveralls · 2017-11-12T14:05:04Z

Coverage increased (+0.2%) to 92.832% when pulling a6f2d2d on malinoff:fix-2124 into d1af369 on pytest-dev:features.

nicoddemus · 2017-11-12T23:58:48Z

Thanks @malinoff for all the work and patience on this. 👍

The leak was caused by the (unused) `FixtureRequest._fixture_values` cache. This was introduced because the `partial` object (created to call FixtureDef.finish() bound with the Request) is kept alive through the entire session when a function-scoped fixture depends on a session-scoped (or higher) fixture because of the nested `addfinalizer` calls. FixtureDef.finish() started receiving a request object in order to obtain the proper hook proxy object (pytest-dev#2127), but this does not seem useful at all in practice because `pytest_fixture_post_finalizer` will be called with the `request` object of the moment the fixture value was *created*, not the request object active when the fixture value is being destroyed. We should probably deprecate/remove the request parameter from `pytest_fixture_post_finalizer`. Fix pytest-dev#2981

Use session.config.hook instead of ihook. Fixes pytest-dev#2124

522d59e

RonnyPfannschmidt reviewed Dec 10, 2016

View reviewed changes

Switch to item fspath

a63b34c

Christian Boelsen and others added 4 commits September 13, 2017 17:15

Fix pytest-dev#2775 - running pytest with "--pyargs" will result in I…

14b6380

…tems with empty "parent.nodeid" if run from a different root directory

Remove unnecessary complexity in _check_initialpaths_for_relpath().

794d458

remove unused _pytest.runner.NodeInfo class

76f3be4

Merge pull request pytest-dev#2911 from RonnyPfannschmidt/remove-node…

742f9cb

…info remove unused _pytest.runner.NodeInfo class

Make SubRequest.addfinalizer an explicit method

f320686

This implicit definition really tripped me while debugging pytest-dev#2127, unfortunately hidden as it was in the middle of all the variable declarations. I think the explicit definition is much easier for the eyes and IDEs to find.

nicoddemus mentioned this pull request Nov 11, 2017

Make SubRequest.addfinalizer an explicit method #2914

Merged

Merge pull request pytest-dev#2914 from nicoddemus/addfinalizer-refactor

b671c5a

Make SubRequest.addfinalizer an explicit method

RonnyPfannschmidt and others added 8 commits November 11, 2017 14:33

Merge branch 'master' into fix-missing-nodeid-with-pyargs

f0f2d2b

Merge pull request pytest-dev#2776 from cryporchild/fix-missing-nodei…

259b86b

…d-with-pyargs Fix pytest-dev#2775 - running pytest with "--pyargs" will result in Items with …

Merge remote-tracking branch 'upstream/master' into malinoff/fix-2124

258031a

pytest_fixture_post_finalizer now receives a request argument

6550b99

Merge remote-tracking branch 'upstream/features' into malinoff/fix-2124

f074fd9

Add changelog entries for pytest-dev#2124

063335a

Fix passing request to finish() in FixtureDef

bdad345

Explicitly clear finalizers list in finalize to ensure cleanup

6d3fe0b

Rename FixtureDef.finalizer to FixtureDef.finalizers

a6f2d2d

nicoddemus changed the base branch from master to features November 12, 2017 13:36

nicoddemus reviewed Nov 12, 2017

View reviewed changes

RonnyPfannschmidt approved these changes Nov 12, 2017

View reviewed changes

nicoddemus merged commit 685387a into pytest-dev:features Nov 12, 2017

nicoddemus mentioned this pull request Nov 12, 2017

fixture hooks use session.ihook instead of a fitting hookproxy #2124

Closed

nicoddemus mentioned this pull request Nov 29, 2017

pytest 3.3: fixture-related breakage #2979

Closed

nicoddemus mentioned this pull request Dec 13, 2017

Fix memory leak caused by fixture values never been garbage collected #3030

Merged

bluetech mentioned this pull request Jan 12, 2022

fixture hooks not running in packages #2361

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use session.config.hook instead of ihook. Fixes #2124 #2127

Use session.config.hook instead of ihook. Fixes #2124 #2127

malinoff commented Dec 10, 2016 •

edited

Loading

coveralls commented Dec 10, 2016

RonnyPfannschmidt Dec 10, 2016

nicoddemus Dec 14, 2016

RonnyPfannschmidt Dec 14, 2016

malinoff Dec 16, 2016

RonnyPfannschmidt Dec 16, 2016

malinoff commented Dec 14, 2016

malinoff commented Dec 20, 2016 •

edited

Loading

coveralls commented Dec 20, 2016

RonnyPfannschmidt commented Dec 20, 2016

nicoddemus commented Apr 13, 2017

RonnyPfannschmidt commented Jun 29, 2017

nicoddemus commented Nov 10, 2017

malinoff commented Nov 11, 2017

nicoddemus commented Nov 11, 2017

nicoddemus commented Nov 11, 2017

RonnyPfannschmidt commented Nov 11, 2017

nicoddemus Nov 12, 2017

nicoddemus Nov 12, 2017

nicoddemus commented Nov 12, 2017 •

edited

Loading

coveralls commented Nov 12, 2017 •

edited

Loading

nicoddemus commented Nov 12, 2017

Use session.config.hook instead of ihook. Fixes #2124 #2127

Use session.config.hook instead of ihook. Fixes #2124 #2127

Conversation

malinoff commented Dec 10, 2016 • edited Loading

coveralls commented Dec 10, 2016

RonnyPfannschmidt Dec 10, 2016

Choose a reason for hiding this comment

nicoddemus Dec 14, 2016

Choose a reason for hiding this comment

RonnyPfannschmidt Dec 14, 2016

Choose a reason for hiding this comment

malinoff Dec 16, 2016

Choose a reason for hiding this comment

RonnyPfannschmidt Dec 16, 2016

Choose a reason for hiding this comment

malinoff commented Dec 14, 2016

malinoff commented Dec 20, 2016 • edited Loading

coveralls commented Dec 20, 2016

RonnyPfannschmidt commented Dec 20, 2016

nicoddemus commented Apr 13, 2017

RonnyPfannschmidt commented Jun 29, 2017

nicoddemus commented Nov 10, 2017

malinoff commented Nov 11, 2017

nicoddemus commented Nov 11, 2017

nicoddemus commented Nov 11, 2017

RonnyPfannschmidt commented Nov 11, 2017

nicoddemus Nov 12, 2017

Choose a reason for hiding this comment

nicoddemus Nov 12, 2017

Choose a reason for hiding this comment

nicoddemus commented Nov 12, 2017 • edited Loading

coveralls commented Nov 12, 2017 • edited Loading

nicoddemus commented Nov 12, 2017

malinoff commented Dec 10, 2016 •

edited

Loading

malinoff commented Dec 20, 2016 •

edited

Loading

nicoddemus commented Nov 12, 2017 •

edited

Loading

coveralls commented Nov 12, 2017 •

edited

Loading