Make high-scope fixtures teardown at last dependent test

[sadra-barikbin]

Closes #3806

[RonnyPfannschmidt]

That being said, a opt in for over eager teardown is potentially a nice to have

[sadra-barikbin]

@RonnyPfannschmidt , sorry for radio silence. I chose another solution which has the additional advantage that is independent of the reordering algorithm. In the last solution in which I made use of pytest_runtest_teardown, I had the assumption that when the next item is not dependent on a fixture and the current item is, current item is the last dependent item which was not quite the case with existing reordering algorithm.

In the new solution I use the global information being collected before reordering to determine the last item which is dependent on a fixture. Since those information is collected only for parametrized tests, I was not able to cover the situations like your example above so I extended the case to collect fixture information of non-parametrized tests, naturally resulting in their being considered into reordering as well.

During writing tests for this feature, I came up with some improvements in fixtures and parametrization stuff as well:

• Get reordering algorithm use parameter values as fixture key when possible: Upon computing value of a fixture, first cache_key is examined to see if the new request has the same parameter or not. This key is parameter value in the first place but in reordering algorithm, fixture arg keys are constructed of parameter indices which make trouble in certain scenarios e.g. in newly added test_basing_fixture_argkeys_on_param_values_rather_than_on_param_indices_2 test.
• Improving param index setting in Metafunc.parametrize and add_funcarg_pseudo_fixture_def: In certain scenarios like newly added test test_parametrize_with_duplicate_values, param indices was not set in a proper way. It was improved.
• Brginging pseudo-fixture register to Metafunc.parametrize: As is proposed in the add_funcarg_pseudo_fixture_def comments, I moved its logic to Metafunc.parametrize.

[RonnyPfannschmidt]

At first glance those seem great, unfortunately I'm not sure when i can commit the time to do this proper justice

[sadra-barikbin]

At first glance those seem great, unfortunately I'm not sure when i can commit the time to do this proper justice

OK. Could I do anything to help you with that?

[sadra-barikbin]

@RonnyPfannschmidt , Is there any chance that this PR make progress? I would be grateful if you or another maintainer puts it in his/her schedule.

[RonnyPfannschmidt]

@nicoddemus @bluetech ping on this one,

[nicoddemus]

Hi folks.

Thanks for the ping.

First of all thanks a lot @sadra-barikbin for tackling this -- this is definitely one of the hairiest and most complex parts of the code base, so I appreciate your courage and tenacity to work on this.

However, to be honest, I'm not sure this optimization is worth the risk of breaking downstream test suites: the fixture teardown code is sensitive and very complex, making it hard to reason about and nearly impossible to foresee possible breakages; no wonder this PR has been sitting here for so long -- it is a complex piece of code, and central to how fixtures are handled in pytest.

Also there's contention about this being a breaking change, as we cannot be sure if there are users that rely on fixtures tearing down only at the end of their scope, as opposed to when they are no longer declared/needed.

Having said that, if @RonnyPfannschmidt thinks this is worth pursing, I will try my best to take some time aside and review it, however I'm very hesitant to "touch" this part of the code unless we are solving serious bugs -- which I believe is not the case here, as this would be an optimization only.

[sadra-barikbin]

@nicoddemus , you're welcome!
As for its being breaking, It seems that the early teardown only makes trouble for the downstream repos that have not used Pytest in a proper way. That is they define a scoped fixture that its objective is to change the global state rather than possibly yielding a value and instead of using autouse, they activate it by declaring it in the args of the first test in the scope or the first test that needs the global state to be altered.

Since it seems that the optimization benefit of the early teardown is remarkable, another approach is to have it as an optional feature to let users choose depending on their testsuite.

"Late Setup" combined with "Early Teardown" could be a beautiful motto for pytest by the way :)

[nicoddemus]

As for its being breaking, It seems that the early teardown only makes trouble for the downstream repos that have not used Pytest in a proper way. That is they define a scoped fixture that its objective is to change the global state rather than possibly yielding a value and instead of using autouse, they activate it by declaring it in the args of the first test in the scope or the first test that needs the global state to be altered.

Hi @sadra-barikbin sorry for the delay on this topic. Unfortunately the code is complex (but not fault to your code, it was already complex), and I'm not 100% convinced this change is good. In theory it seems OK, but this is a big behavior change and will likely cause problems downstreams.

While I appreciate that to correct that users could use autouse, I know at least one case where this would not work, which is the app fixture in pytest-qt: it is an autouse session fixture, and as it is written today, it needs to live as long as the process, otherwise it will cause a crash. It could be changed to keep the app instance in a local variable, and that would work, but turning it autouse would mean the QApplication would be created very early, which might be surprising.

But TBH I would not like to take this decision only by myself, so I would like to ask other maintainers what is their opinion on this, so let us gather more opinions:

cc @The-Compiler @asottile @RonnyPfannschmidt @bluetech

[sadra-barikbin]

@nicoddemus , thank you for the response.
I didn't understand why this feature causes problem to pytest-qt. Is not _qapp_instance a global variable? So when it's set at qapp setup, it remains alive till the end of the process. Early teardown has nothing to do with qapp fixture since qapp is not a yieldy fixture that for example does _qapp_instance = None in its teardown statements. Am I wrong?

[nicoddemus]

@sadra-barikbin you are right, I misremembered the code, indeed it would not be a problem for pytest-qt, since then we have changed that to a global variable. 👀

[RonnyPfannschmidt]

I'm aware of multiple testsuites that wouldn't break or need substancially more time if session scope fixtures where to tear down early/eagerly

[sadra-barikbin]

I'm aware of multiple testsuites that wouldn't break or need substancially more time if session scope fixtures where to tear down early/eagerly

@RonnyPfannschmidt , Could you please set an example? Anyway my last resort, then, would be to ask if we could add the feature as an optional one? This way, the pytest and the downstreams won't break and also the hidden bugs would get caught and resolved through time.

[RonnyPfannschmidt]

I'm absolutely happy to have this as opt in

It will take a while to set up some examples, I'm currently on paternity leave and severely overestimated the time I could have for opensource

[The-Compiler]

For what it's worth, I tested this with the qutebrowser testsuite, and I end up seeing a session-scoped testdata_scheme fixture being set up and then torn down again around every single testcase.

The code is here: https://github.com/qutebrowser/qutebrowser/blob/52c7ec57c1dc8607e253866ca03ef5af7193b03b/tests/helpers/fixtures.py#L175-L275

I tried creating a minimal example:

import pytest

session_fixt_did_run = False

@pytest.fixture(scope="session")
def session_fixt():
    print("Session init")

    global session_fixt_did_run
    assert not session_fixt_did_run
    session_fixt_did_run = True

    yield

    print("Session teardown")


@pytest.fixture
def setup_fixt(qtbot, session_fixt):
    pass


@pytest.fixture
def fixt_a(setup_fixt):
    yield


@pytest.fixture
def fixt_b(setup_fixt):
    yield


@pytest.fixture(params=["a", "b"])
def fixt(request):
    if request.param == "a":
        request.getfixturevalue("fixt_a")
    else:
        request.getfixturevalue("fixt_b")


def test_a(fixt):
    pass


def test_b(fixt):
    pass

but that seems to work fine, surprisingly - then again, in my real code, it also seems to weirdly depend on which files I run.

---

Not sure what to do here - for a far simpler example, this now also runs fixt twice, despite being a session fixture:

import pytest

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

def test_fixt(fixt):
    pass

def test_nofixt(request):
    request.getfixturevalue("fixt")

and a session fixture running multiple times seems like a clear violation of expectations. For me, it broke because testdata_scheme actually modifies global application-wide state which can only be done once.

I feel like this is a fundamental problem with this approach that can't be solved - IMHO, this should be opt-in, and probably stay opt-in.

[asottile]

sorry I'm slow to the party -- I was also playing around with this a bit and noticed that it will cause fixtures to be run more than once breaking some invariants due to request.getfixturevalue. I also ran into scenarios where this cut off fixtures earlier leading to some pretty surprising side-effects (requests breaking out of a mock sandbox, etc.)

it definitely can't be on by default with that, and I'm also hesitant to even make an option for this (it'll be under-exercised, not something we generally recommend, and is complexity we'd have to carry indefinitely)

so I'm more -1

[nicoddemus]

Thanks everyone for the feedback.

IMHO, while we definitely appreciate the effort and care put in by @sadra-barikbin, I think we should reject the patch and the feature itself as "won't fix".

I would rather not having this at all, rather than opt-in, given the complexity and maintenance burden involved in having two "high-scope fixture teardown "modes".

[sadra-barikbin]

Hi, sorry for delay.

Having Qutebrowser as a huge stress test, I came to make a change in the way the feature finds the last dependent item on a fixture. Some improvements, bug fixes and refactors were also done.
Regarding request.getfixturevalue, @The-Compiler and @asottile are right as it's a dynamic dependency introducer and we have no way to find it out at collection time. It seems to me as the only blocker to this feature. But request.getfixturevalue itself is a thing that pytest has already asked user to use cautiously. We have in its docstring:

Declaring fixtures via function argument is recommended where possible.
But if you can only decide whether to use another fixture at test
setup time, you may use this function to retrieve it inside a fixture
or test function body.
This method can be used during the test setup phase or the test run
phase, but during the test teardown phase a fixture's value may not
be available.

If early teardown feature gets added to pytest, we could add proper warning to the docstring that using this method might make some high-scoped fixtures get called more than once.

@The-Compiler, if you add a test that directly depends on session_fixt before test_a or test_b, it raises error as the feature considers the test as the last dependent item on session_fixt while test_b is the last one. Concerning Qutebrowser, request.getfixturevalue in tests/helpers/fixtures.py::web_tab causes some tests to fail because the feature couldn't find the last dependent item as expected and a fixture is setup again and raises error. Apart from that, with the newest changes, only stubs and tmp_path_factory are setup again due to request.getfixturevalues used in tests/unit/utils/test_version.py::TestChromiumVersion::test_simulated and pytest-bdd respectively.

@asottile , regarding the second problem you encountered, I would be grateful if you provide a little more details so that I examine it. Maybe, with the newest change, they get solved.

@nicoddemus ,the third approach would be not to have this feature and reduce this PR to some improvements and refactors.

[nicoddemus]

@nicoddemus ,the third approach would be not to have this feature and reduce this PR to some improvements and refactors.

Oh that would definitely be welcome! I suggest to open a separate PR in that case then.

[sadra-barikbin]

Hi everybody,

Here are the improvements that came along the early teardown feature:

Improvement	Corresponding PR
Remove funcargs attribute from Callspec to unify parameters of the test item, as funcargs are transformed into pseudo-fixtures.	#11220
Remove fixtures.py::add_funcarg_pseudo_fixture_def and care for transforming funcargs to pseudo fixturedefs right in MetaFunc.parametrize.	#11220
Make FixtureArgKey to represent fixture param by its value rather than its index if possible, as does FixtureDef::cache_key .	#11231
When parametrizing with multiple parametersets (or multiple tuples of params), index of parameters in a parameter set is determined by their index in the existing values of the parameter, not by the index of their parameter set in the parameter set list. This results in better identifying dependencies of tests, thus better reordering.	#11220
Creating FixtureArgKey for representing fixture dependencies.	#11231
Taking nonparametrized tests into consideration for reordering as well. Beforehand, only parametrized tests were considered, by retrieving their fixture dependecies.
Considering used shadowed fixture dependencies for reordering as well. This was done by changing fixturemanager::getfixtureclosure algo from BFS to DFS.
Remove fixtures.py::FuncFixtureInfo::remove_dependecy_tree and move its responsibility to very FixtureManager::getfixtureclosure. I note that populating arg2fixturedefs is done only once.
Pruning dependency tree is done only if we have a metafunc.parametrize call within module-specific or class-specific pytest_generate_tests hooks.
Fix a few typos and do a few small improvements

Taking the last approach, which ones are worth keeping?

By the way, what is the final decision? @RonnyPfannschmidt @nicoddemus @The-Compiler @asottile

• To have early teardown as an optional feature.🥳
• Not to have early teardown but keep the improvements listed above.🤓

[RonnyPfannschmidt]

I'm under the impression we Will want to have many of the improvements in any case

In particular since landing them would reduce the scope of the actual feature and help to figure it out

I'll try to make some time in the evening to give this due diligence

I believe we also want to have a discussion on intentionally early/late teardown in general as well as the relationships with xdist, as the whole mess around nextitem, runtestprotocol, workload and metadata about fixtures, parameters and marks sums up a ball of yarns that makes the feature as currently implemented something that creates more anxiety than necessary

I believe its very important to drive some internal improvements of pytest to ensure this feature can shed the details that cause us anxiety

I strongly would prefer safe early teardown myself, but we also have to take the fact into account that even with your lovely enhancements the pytest Mechanic's around nextitem, teardown, Parametrize and xdist make it very hard to go for safe as of now

But i am certain that i want to progress towards making the safe more easy

The necessary changes as well as the problems we found are great indicators and help for progressing the internals

[bluetech]

@sadra-barikbin Some of these changes sound interesting (some overlap also with #9350 and #9420). If you can spare the effort, I recommend submitting these changes in separate PRs. Since the fixture internals are pretty hard to grok, it's best if the PRs are small and with an "explain like I'm 5" approach :) And I'd also do the pure improvements first, before the more controversial/potentially breaking changes. Personally I'd be happy to review anything which simplifies the fixtures code.

[sadra-barikbin]

I made a PR for 1st, 2nd and 4th items in the above list. I combined them as they had overlap with each other. Please let me know if I'm in the wrong direction. 🙏

[nicoddemus]

Thanks @sadra-barikbin, to be clear, we definitely appreciate all the effort and patience you are putting in this topic! 🎉

Answering to your question, for now I would vote to not have the feature at all, but keep the improvements. As commented, even so is paving the way for simpler code, and perhaps we can rethink about introducing this feature in the future.

I think you can move that list to a an issue for tracking if you like, and we can close this PR for now?

[sadra-barikbin]

Yes. Anyway I'm willing to help if it is decided to introduce the feature in the future.