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_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 fixed + replied

Why removal handles are needed:

• They solve the memory leak of potentially thousands of stale finalizers in higher-scoped fixtures and nodes, as per Refactor fixture finalization #4871
• They prevent the issue I described in test_stale_finalizer_not_invoked

In #4871 they also suggest that ideally we should avoid fixture finalizers and instead plan fixture setup-teardown at collection stage. This is impossible, because, as explained here, custom implementations of pytest_runtestloop can reorder, remove and duplicate tests last second before running them.

---

An alternative implementation without finalizers would be the following:

1. For each FixtureDef, collect a set of dependents
2. At teardown stage, first compute a set of what FixtureDefs need to be recomputed on their own
3. Then take a transitive closure of the set using (1)

---

Still, I can't see a non-ad-hoc way of avoiding the pile-up of finalizers of parametrized fixtures in nodes. Removal handles as currently in the PR are the most straightforward solution that I see.

A solution for nodes without handles would be to remember the node for which we have a finalizer currently registered and not readd it on fixture recomputation. The drawback would be incorrect finalizer ordering:

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

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

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

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

The order of fixture teardown on module exit will be: bar, foo - as if the order of registration was foo, bar.

But the instance of foo that was first is gone, and the current instance of foo was setup after bar. So the order of fixture teardown should be foo, bar.

AFAICS the only way the correct order can be achieved is by removing the finalizers of foo at its teardown.

[RonnyPfannschmidt]

ok, i need to reiterate the handles idea a bit more fir my own understaniding

i think the fact that its currently a global variable is part of my confusion

ideally the complete details on caching/recreation and current fixture closure turn into the responsibility of the setup state
currently they are all over the place

but that is sooo out of scope for this change

i do believe having the handles as one variable may create possibly neglectable issues wit in process pytest runs for testing pytest/pytest plugins - off hand it looks to me as if we could leave handles in place by accident if the pytest under test gets really broken - i suspect that this could be resolved by having a weakref.WeakKeyDictionary which would automatically clean up

[Anton3]

Alternatively, there is the following compromise:

• For inter-fixturedef dependencies, use the solution from the previous comment without finalizers and handles
• For fixturedef-node finalizers, allow them to pile up. As noted in Refactor fixture finalization #4871, the most troubling is that currently all those finalizers refer to separate SubRequest objects. In this PR, I move _current_request to FixtureDef, so there may be no more than 1 instance of cached SubRequest per FixtureDef

What do you think?

Edit: After thinking for a while, I still believe handles are a cleaner solution, because this approach

1. Leaves no garbage in nodes
2. Always performs teardown in reverse-setup order, as described here
3. Keeps the correct order of fixture teardown w.r.t. user finalizers

[Anton3]

I don't think weakref can be used with handles. If we drop a handle, that does not mean that we no longer want the finalizer to run, unless we want to break the current finalizers API for users. What dropping a handle currently means in the PR is that we don't want the ability to remove the finalizer.

Edit: actually, a handle could keep a weakref to FinalizerId, but that's pinching pennies.

If handles are kept past their intended scope by mistake, then finalizers are eventually invoked and cleared, and handles are left owning a bunch of FinalizerId which own nothing.

[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.