Account for changes in fixture dependencies properly

[Anton3]

Move fixture teardown to SetupState

A fixture's teardown should first perform teardown of the dependent fixtures - this is the only known way that maintains the correct teardown order in all cases. Before this PR, this code resided in FixtureDef. The disadvantage is that the responsibilities and implementation of SetupState (including exception handling) was spread across the codebase. This PR moves the execution of fixture teardown into SetupState, which facilitates code reuse and enforces Single-Responsibility Principle.

A more important issue was that fixture teardown for parametrized fixtures was performed during the setup of the next test. This lead to incorrect attribution of fixture teardown errors, time spent and logs to the next test. This PR makes sure to precompute param changes for parametrized fixtures in advance using nextitem and always runs fixture teardown at test teardown stage.

Relevant tests:

• test_teardown_stale_fixtures_single_exception - shows correct attribution of teardown errors
• test_teardown_stale_fixtures_multiple_exceptions
• test_parametrized_fixture_carries_over_unaware_item
• test_fixture_rebuilt_when_param_appears
• test_fixture_not_rebuilt_when_not_requested
• test_cache_mismatch_error_on_sudden_getfixturevalue - as was insisted in the review comments, if we discover at setup or call stage that a fixture needs to be recomputed, we prefer to error out instead
• test_cache_key_mismatch_error_on_unexpected_param_change
• test_fixture_teardown_when_not_requested_but_param_changes
• test_fixture_teardown_with_reordered_tests
• test_parametrized_fixtures_teardown_in_reverse_setup_order
• test_parametrized_fixture_teardown_order_with_mixed_scopes
• test_override_fixture_with_new_parametrized_fixture - this is a peculiar test that works by sheer luck; we should track fixture overrides more precisely in the future

Closes: #9287

Add FinalizerHandle to avoid accumulating stale finalizers

To reiterate, adding teardown of the dependent fixture as a "finalizer" to the dependency-fixture is the only known way of achieving the fully correct teardown order. But this meant that stale finalizers from function-scoped fixtures accumulated at higher-scoped fixtures and Nodes. This manifested as a "memory leak" in very large test suites.

Besides performance concerns, with getfixturevalue dependency tracking added, a stale finalizer could result in tearing down a fixture randomly.

To solve those issues, this PR introduces FinalizerHandle that allows to remove stale finalizers, and plucks all remaining self-finalizers at fixture's teardown.

Relevant tests:

• test_stale_finalizer_not_invoked

Closes: #4871

Execute pytest_fixture_post_finalizer once per fixture teardown

This is implemented by turning pytest_fixture_post_finalizer call into one of fixture finalizers in SetupState.

Also, exception handling is improved: if pytest_fixture_post_finalizer raises an exception, the fixture is still reset correctly.

Relevant tests:

• test_fixture_post_finalizer_called_once
• test_fixture_post_finalizer_hook_exception

Closes: #5848
Closes: #14114

Account for changes in getfixturevalue dependencies

Previously, for a given fixture, only dependencies from argnames were checked to see if they need to be recomputed, recomputing the current fixture. Meanwhile, if a fixture depends on a parametrized fixture via getfixturevalue, then the current fixture's value could erroneously be carried over, even though the dependency was recomputed due to its parameter change.

The fix involves tracking fixture dependencies (_own_fixture_defs) in FixtureRequest, then adding finalizers to all those fixtures in FixtureDef.execute.

This precise dependency information was also used in setuponly.py.

Relevant tests:

• test_getfixturevalue_parametrized_dependency_tracked
• test_getfixturevalue_parametrized_dependency_order
• test_request_stealing_then_getfixturevalue_on_parametrized

Closes: #14103

[Anton3]

@RonnyPfannschmidt please review 😃

[RonnyPfannschmidt]

I'm currently not able to do a deep review

This has a few details that give me headaches

• it sidesteps setup/teardown minimizing and im not sure if its feasible to integrate in a comprehensible manner
• its unclear to me what happens if the involved fixtures are indirect to a test
• Its unclear to me what happens if someone creates a cycle via the new dependency tracking

[Anton3]

@RonnyPfannschmidt

it sidesteps setup/teardown minimizing and im not sure if its feasible to integrate in a comprehensible manner

Are you wondering if fixing #14095 is an overall positive change? If so, I can remove that part from the PR for now, and it can be investigated later separately.

There is also a minor issue with my implementation. When I do

request._execute_fixturedef(fixturedef, only_maintain_cache=True)

it may finish that fixture, but when we setup the current fixture, we may discover that the control flow in the fixture body changed, and we no longer need the fixture this time.

I can change it so that the other fixtures are not finish-ed unnecessarily. We basically just check recursively whether params are OK for dependencies.

its unclear to me what happens if the involved fixtures are indirect to a test

What do you mean? Parametrized / non-parametrized case? In the parametrized case, we cannot get the parametrized fixture using getfixturevalue (pytest will not be able to resolve the test name), but I can add a test that requests the dependent fixture via getfixturevalue, it should be fine.

Its unclear to me what happens if someone creates a cycle via the new dependency tracking

The initial cache handling step uses only_maintain_cache=True to avoid bogus dependencies. I believe that _get_active_fixturedef (the part that got moved into _find_fixturedef) will detect the cycle before the dependency is cached anywhere.

There is test_detect_recursive_dependency_error for a recursive dependency between fixtures. I will write a simple test for recursive dependency using getfixturevalue.

[RonnyPfannschmidt]

trigger a rebuild is more correct than not
however there should be different test reordering as just correctly triggering teardown makes the testrun a bit more expensive

with indirect i mean what happens if a replaced fixture is not a dependency of the directly used fixtures, but rather down in the dependency tree

# untested quickly written example
        import pytest

        @pytest.fixture(scope="session")
        def foo():
            return "outer"

        @pytest.fixture(scope="session")
        def bar(foo):
            return f"dependent_{foo}"

        @fixture(scope="session):
        def baz(bar): 
            return bar


        def test_before_class(baz):
            assert bar == "dependent_outer"

        class TestOverride:
            @pytest.fixture(scope="session")
            def foo(self):
                return "inner"

            def test_in_class(self, baz):
                assert bar == "dependent_inner"

        def test_after_class(baz):
            assert bar == "dependent_outer"

with loops i mean what actually happens when one fixture goads another ones closure into calling getfixturevalue for itself

# untested quickly written example
@fixture
def trickster(request):
   return request.getfixturevalue
   
 @fixture
 def evil_recurse(trickster):
     return partial(trickster, "evil_recurse")
     
@fixture 
def trigger(evil_recurse):
    evil_recurse()

[Anton3]

I've found another issue in my implementation. Suppose we depend on some fixture dynamically. After a recomputation we stop depending on it. Everything is fine, but at some point that fixture gets finalized, and because we did addfinalizer(self.finish) on it, we get finalized out of blue, perhaps in the middle of a test.

One solution is to add "evaluation epoch" to the finalizers to avoid finalizing a more recent evaluation of the fixture than expected.

[RonnyPfannschmidt]

teardown should idieally only happen in teardown phases - thats why the teardown_towards heleprs got added

we should fail rather than teardown in setup/call phases

[Anton3]

I've moved parametrized fixture finalization to teardown phase. In FixtureDef.execute: find the next item that uses this fixture; if the fixture has a different param there, finalize now.

We need to be cautious with what to teardown when. Consider these tests:

@pytest.fixture(scope="session")
def foo(request):
    return request.param

@pytest.mark.parametrize("foo", [1], indirect=True)
def test_a(foo):
    pass

@pytest.mark.parametrize("foo", [1], indirect=True)
def test_b(foo):
    pass

def test_c():
    pass

@pytest.mark.parametrize("foo", [2], indirect=True)
def test_d(foo):
    pass

Finalizing foo[1] in test_c's teardown phase is not appropriate.

For each parametrized fixture in the current item we should find the next item that uses the fixture with that parameter and if parameters differ then finalize the fixture in the current item's teardown.

This requires some code restructuring, because we now require arbitrary lookahead instead of just nextitem. I've swept that under the rug with session._current_item_index for now. Tell me if you got better ideas.

Overall, setup-plan did get prettier. A hefty chunk of FixtureDef.execute evaporated.

I need to write more tests, for issues you pointed out and some more:

• Reproducer for this issue, explaining the need for _cache_epoch
• Demo for the new error in _check_cache_hit
• Check that request.param is correct in pytest_fixture_post_finalizer, explaining the need for _request_cache
• Throwing without setting cached_result in pytest_fixture_setup, possibly adding finalizers before that
• What happens to cached fixtures when a test fails with --maxfail. That's why we should not call addfinalizers on arbitrary test items ahead, they will get ignored with --maxfail
• Some edge cases in _should_finalize_in_current_item

[Anton3]

I've reworked _current_item_index into a slightly less hacky solution. This relies on session.items containing the tests, seems reasonable, pytest_collection doc seems to require it. Perhaps there may be a better place for injecting the next items info.

More importantly, this disallows pytest_runtestloop overrides that reorder tests and insert custom items on the fly. Here are the implementations I've seen:

1. Running items in parallel in subprocesses (chunked or strided split), while keeping order - OK
2. Selecting a subset of tests to run by config options (BTW hack: should be done in collection hooks) - OK
3. Rerunning flaky tests - QUESTIONABLE
   • Running tests until the end (nextitem=None), then rerunning flaky tests while keeping the order - OK
   • Rerunning flaky tests immediately - OK (might teardown-setup more fixtures than necessary)
   • Running tests in parallel in subprocesses (until nextitem=None), collecting flaky tests per-subprocess, then running flaky tests while keeping the order - OK
   • Running tests in parallel in subprocesses, collecting flaky tests in the order they failed, then running them together - NOT OK
4. pytest-xdist - looks OK

The implementations of pytest_runtestloop I've seen will work. But some plugin somewhere may be broken by those changes. We should probably document them.

---

Alternatively, we could avoid all of those troubles by dropping support for carrying fixtures along tests that do not depend on them:

@pytest.fixture(scope="session")
def foo(request):
    return request.param

@pytest.parametrize("foo", [1], indirect=True)
def test_a(foo):
    pass

def test_b():
    assert 2 + 2 == 4

@pytest.parametrize("foo", [1], indirect=True)
def test_c(foo):
    pass

In this case, pytest auto-reorders fixtures based on param values (if reordering is not turned off by the user), but in a more complex scenario additional teardown-setup work will surface:

@pytest.fixture(scope="session")
def foo():
    pass

@pytest.fixture(scope="session")
def bar():
    pass

@pytest.fixture(scope="session")
def baz():
    pass

@pytest.parametrize(("foo", "bar"), [1, 1], indirect=True)
def test_a(foo, bar):
    pass

@pytest.parametrize(("bar, "baz"), [1, 1], indirect=True)
def test_b(bar, baz):
    pass

@pytest.parametrize(("baz, "foo"), [1, 1], indirect=True)
def test_c(bar, baz):
    pass

Currently all those fixtures are carried over. They will not be carried over.

---

And so I come back to the "disgusting" idea of tearing down parametrized fixtures in the test before the parameter changes. So in this test setup, foo will be torn down in test_c, even if foo has nothing to do with test_c. I will go on with this route.

[Anton3]

Done!

I should add the following tests (besides the ones mentioned earlier in one, two):

• When exactly parametrized fixture teardown happens (the "ugly" situation described in this comment) - more of a golden test
• Reordering tests in pytest_runtestloop such that the index caching I suggested previously would lead to stale values of parametrized fixtures (and undetectable using nextitem)
• What happens if pytest_fixture_post_finalizer throws
• pytest_fixture_post_finalizer is no longer called multiple times for the same cache teardown
• Golden test for the order of fixture teardown (in the PR, non-function-scoped parametrized fixtures are finalized between function and class scope)

[Anton3]

By the way, this PR also relates to:

• Refactor fixture finalization #4871 - pytest_fixture_post_finalizer are no longer called extra times
• Pytest does not support indirect parameters for overriden fixtures #2043 - no longer trying to execute fixturedefs that are hidden in the current item
• run class scoped parametrized fixture's dependency fixture's teardown after each test class iteration #9427 - is a duplicate of Fixtures are not rebuilt when param changes for a fixture they depend on, if the dependency is via getfixturevalue #14103, and vice versa

[Anton3]

@RonnyPfannschmidt what do you think is the lesser evil in this case?

import pytest

@pytest.fixture(scope="session")
def foo(request):
    return getattr(request, "param", None)

@pytest.mark.parametrize("foo", [1], indirect=True)
def test_a(foo):
    assert foo == 1

def test_b(request):
    hmmm(request)

@pytest.mark.parametrize("foo", [1], indirect=True)
def test_c(foo):
    assert foo == 1

1. Teardown the fixture at the end of test_a, because nextitem does not request the fixture
2. Carry the fixture over, but if test_b requests foo via getfixturevalue, then we fail, because we refuse to perform teardown at call phase

* Assume that parametrized tests autoreordering is turned off, or that the user forced a custom order, or that there are multiple parameters, and autoreordering had to pick an argname to not be consecutive, and our fixture was picked.

UPD: I will go with option 1 for now, because in most cases extra setup-teardown won't happen due to autoreordering, and worst-case scenario is tests slowdown, while option 2 risks breaking some tests in-the-wild (I'm almost certain that I'll find out at my worksite that the new version of pytest broke some of the millions of pytest tests.)

[RonnyPfannschmidt]

teardown when the next item doesnt use is worse than failure at first glance

the failure is a acceptable first step as it turns a silent error into a visible one, but the real issue is that teardown_towards should tear it down in the teardown of test_b as test_c has indeed different parameters

[Anton3]

but the real issue is that teardown_towards should tear it down in the teardown of test_b as test_c has indeed different parameters

There is no issue there:

• With approach 1, teardown of test_b will do nothing (the fixture is already torn down), and test_c will setup it.
• With approach 2, test_b will fail without finishing the fixture, while for test_c no extra teardown-setup work will happen.

The issue is only in test_a -> test_b transition.

[Anton3]

But I got the idea, you think failing explicitly is better than the current state of teardown in setup phase. Will go on with this approach.

[RonnyPfannschmidt]

the goal should be to eventually e able to do it in teardown_towards

but bascially if a fixture has to be removed in the setup phase something went wrong

i do fear that i am missing some edge cases here where the teardown is correct

but my current understanding is that the teardown ought to happen in the teardown phase of the prior test item

[Anton3]

@RonnyPfannschmidt ready for review

[Anton3]

@RonnyPfannschmidt I've moved fixture finalizers management to SetupState. Looks a bit cleaner now. Ready for review

[RonnyPfannschmidt]

Thanks for moving it into setupstate

That removes my biggest concern

I did another skim and now it looks solid at first glance to me

As this necessary is something big ill have to allocate some time for a deeper read

Thank you

[RonnyPfannschmidt]

this looks correct to me now

I’m tempted to suggest figuring how to make the finalizer handles a bit less opaque - to ease debugging the data (should be a followup)

i'd like a second pair of eyes on it and i think i have to run a local test

based on the current code and integration in setupstate i'm confident this is good to go

my due diligence needs some more work

thanks for getting this started and powering through the follow-ups
this was hard and important work and I’m grateful someone actually went down that rabbit hole -

[Anton3]

@RonnyPfannschmidt I've reworked the Callable into a public FinalizerHandle class. Please resolve review comments that have been fixed

[Anton3]

Please someone else review this PR! @RonnyPfannschmidt considers this OK as-is, but an additional pair of eyes would be welcome

[RonnyPfannschmidt]

@nicoddemus @bluetech ping on this one - i'll also try to take another looks tis week but im stretched very thin amt

[Anton3]

I've skimmed through fixture-related issues updated in the last 2 years:

• runner: re-raise pytest.exit inside ExceptionGroups (fixes #13650) #13736

  This PR fixes ignoring pytest.exit via wrapping in ExceptionGroup. Probably need to add some tests for that

  This PR only ensures that its changes don't block the other PR (current situation is not worsened). The root cause of that issue is not touched in this PR.

• Numerous issues regarding controlling fixture cache state dynamically:

  • Pytest session-scoped fixture is re-initialized unexpectedly #13755
  • Add new fixture cache_result param (default True) to ignore fixture cache and re-execute on each usage of the fixture #12814
  • Allow to dynamically create fixture in plugin #12376
  • RFC. Support for resetting function-scoped fixtures  #12596

  As long as we agree that finalizers for custom caches should be executed by SetupState, this PR should not block anything

[Anton3]

@nicoddemus @bluetech Could you please take a look at the PR?

[RonnyPfannschmidt]

i did another round of walk-trough and approve this - i'd still like another pair of critical eyes as this one is a important enhancement that introduces a new mechanism

[Anton3]

@The-Compiler Could you perhaps take a look at the PR?

[Anton3]

@nicoddemus @bluetech gentle ping?

[nicoddemus]

Sorry I won't be able to review this properly.

@RonnyPfannschmidt if you have looked at it thoroughly and are confident with the result, feel free to go ahead and merge it.

[bluetech]

I will try to review this.

If you can split the changes to smaller atomic commits (if possible) that would help a lot.

Also, if you can update the pull request summary to explain what is the problem being fixed here that would also help. Currently it lists the code changes, which don't explain the rationale, and links to issues, which I'm sure explain the problems but are a bit hard to sift through. So a clear description would help a lot.

[Anton3]

@bluetech I'm afraid the main change in fixtures.py and runner.py is more-or-less monolithic, apart from publishing FinalizerHandle. I don't believe inventing new intermediate implementations, intentionally partially incorrect, is a good idea.

I've written a detailed description. Walking through the tests and linked issues again was actually useful, I've found that #2043 is actually irrelevant to this PR.

For now, please try reading through the PR as-is and let me know if anything is unclear. One more thing I could try is adding comments throughout the PR, describing what problems are being solved. This may or may not prove to be necessary.

[bluetech]

@Anton3 I understand if it can't be broken down. Thanks for adding the description. I will try to take a look this weekend.

[Anton3]

@bluetech friendly reminder 👀

[Anton3]

@bluetech weekly ping 🤔

[bluetech]

Hey @Anton3,

Thanks for the impressive work!

I am still going through this PR.

First, I split the pytest_fixture_post_finalizer changes to #14275, please have a look at my comments there. If the changes there are merged, I will ask you to rebase here.

In the meantime please see the small comments below (still not a review of the main changes).

[bluetech]

Here we throw away the FinalizerHandle, so I figure it's better not to allocate it. Maybe inline _append_finalizer?

[bluetech]

Please add a small docstring to the non-private methods of SetupState. SetupState is not a public API, but I mean as documentation for the internal users of it.

It would also be good to discuss the fixture stuff in SetupState's docstring, it currently only discusses the setup/teardown for nodes.

[bluetech]

Suggested change

	self._fixture_finalizers.pop(fixturedef)
	del self._fixture_finalizers[fixturedef]

[bluetech]

It would be good to explain here what is a "stale fixture".

[bluetech]

I assume the list() here is necessary because the dict can change? Can the situation where it changes cause problems?

[Anton3]

It actually can, thanks for discovering! Fixed and added test_getfixturevalue_parametrized_dependency_order

[bluetech]

Suggested change

	If a test forces teardown of a parametrized fixture, e.g. using ``request.getfixturevalue()``, it instead fails. An example of such test:
	If a test forces teardown of a parametrized fixture, e.g. using :func:`request.getfixturevalue() <pytest.FixtureRequest.getfixturevalue>`, it instead fails. An example of such test:

[bluetech]

Maybe also show the explanation part?

This could happen because the current test requested the previously parametrized fixture dynamically via 'getfixturevalue' and did not provide a parameter for the fixture.
Either provide a parameter for the fixture, or make fixture 'foo' statically reachable from the current test, e.g. by adding it as an argument to the test function.

[bluetech]

Is this change necessary? If not, I'd leave it as-is, it works better with "find usages" in an IDE, and to reduce the diff.

[bluetech]

Please remove this change, the missing type annotation here is intentional.

[Anton3]

Then mypy complains in TopRequest.addfinalizer:

src/_pytest/fixtures.py:711: error: Returning Any from function declared to return "FinalizerHandle"  [no-any-return]

Note that I'm only adding this type annotation to TopRequest, not to FixtureRequest.

[bluetech]

Here we now have request == self._cached_request (at least running pytets's tests with this assert passes). So we can remove the request parameter, but I need to think a bit more if that's the way.