Account for changes in fixture dependencies properly

[Anton3]

• Keep track of fixtures requested through a FixtureRequest object
• Add finalizers to dynamically requested fixtures to ensure teardown for dependent fixtures runs first
• Make addfinalizer return a handle that allows to remove the finalizer; use it in FixtureDef
• Make SetupState teardown parametrized fixtures, which parameter changes in nextitem, at teardown stage

Closes: #2043
Closes: #4871
Closes: #5848
Closes: #9287
Closes: #14103
Closes: #14114

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

Could anyone review this please?

[RonnyPfannschmidt]

i just did a preliminary skim of the change

the finalizer handles are a interesting way to sort the dependencies out, unfortunately it creates a very global way to sort it

i haven’t done my own die diligence yet, i hope that there’s potentially a sensible way to integrate the detail handling of this with setupstate,

after my first iteration im getting the impression it should be integrated into finalizers in a more direct manner

this is quite the tricky problem, in particular wit the current architecture
i am of the impression that a missing underlying refactoring/feature addition is making the task you set out to solve very hard

we have to be very careful about the effect of the tech debt incurred by this

[RonnyPfannschmidt]

this is one of the hooks that should never error
if we hide away base-exception here we break quite a bit of the ux

so we absolutely need to handle this hook raising a outcome or a baseexception thats not a normal exception

[Anton3]

Changed to only catching TEST_OUTCOME, like in SetupState.teardown_exact

[RonnyPfannschmidt]

the handles are something i need to dig in deeper, that layer is a bit confusing at first glance

the ideally we only track finalizers in a single way

[Anton3]

I've changed all addfinalizer functions to work in a single way. More on handles in the comment below

[RonnyPfannschmidt]

this looks like a problem - as far as i can tell it could collect garbage in inprocess tests that break pytest intentional for behaviour checks

its also a global mutable storage - something wed absolutely want to avoid

[Anton3]

I think you've read this piece of code incorrectly. This is not a global variable, but a type. I haven't added any completely new state, I've added handles to existing finalizer storages in nodes and fixturedefs

[RonnyPfannschmidt]

Suggested change

	if isinstance(nextitem, Item):
	if nextitem is not None:

[Anton3]

Fixed

[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