Instantiate fixtures by scope order in test function requests

[nicoddemus]

This is a stab at fixing #2405.

This fixes the reproducible example posted by @jaraco:

import pytest

data = {}

@pytest.fixture(scope='module')
def clean_data():
    data.clear()

pytestmark = pytest.mark.usefixtures('clean_data')

class TestDataValues:
    @pytest.fixture(scope='class', autouse=True)
    def add_data(cls):
        data.update(value=True)

    def test_value(self):
        assert data.get('value')

And also this one which I used to debug the problem:

import pytest

data = {}

@pytest.fixture(scope='module')
def clean_data():
    data.clear()

@pytest.fixture(autouse=True)
def add_data():
    data.update(value=True)

@pytest.mark.usefixtures('clean_data')
def test_value():
    assert data.get('value')

All tests pass (which is encouraging), but given that this might alter the order of evaluation of fixtures (albeit it seems to actually fix it) we might want to target features instead.

Still a WIP: needs more tests, changelog, some docstrings, etc.

[coveralls]

Coverage increased (+0.05%) to 92.88% when pulling 59e7fd4 on nicoddemus:2405-scope-fixture-order into d03e389 on pytest-dev:features.

[nicoddemus]

I think this might break suites by accident: even though the test function is declared (f1, m1), pytest will sort by scope so it will actually instantiate unrelated fixtures in m1, f1 order.

It seems this is actually our expectation and was the root cause for #2405.

[nicoddemus]

Temporarily using a separate module for those tests because it makes switching between master and this branch to run tests.

[nicoddemus]

The only test in this module which passes on master is this one when use_mark=False: in this case, both m1 and f1 are autouse, which we do sort by scope on master:

pytest/_pytest/fixtures.py

Lines 1017 to 1019 in 1b53538

	autousenames.sort(
	key=lambda x: self._arg2fixturedefs[x][-1].scopenum)
	return autousenames

[okken]

I have not reviewed the code.
I am commenting to say that I want this PR to go through.
I think the current behavior of file order overriding scope order is insane and a bug.
I get numerous questions about it, and I always tell people to create artificial dependencies between fixtures that need to run in a certain order.

The general mental model that people have for fixtures is that they are run in scope order.

[nicoddemus]

@okken thanks for the feedback!

[RonnyPfannschmidt]

this needs to go in, its yet another indicator that fixtures have a really really bad internal base

[nicoddemus]

It seems we are leaning on merging this, still needs changelog, doc updates to document the behavior and rebase it to features.

@RonnyPfannschmidt do you think the tests I put in place are enough?

[RonnyPfannschmidt]

@nicoddemus given what we are trying to model we can only aim for best efford, and it seems like the tests cover the behaviours we understand and want to have

i would hope at some point we can have unittests for this, but right now what you did is great

[ceridwen]

Was the order that fixtures were expected to run just not defined before? @okken, when you say "I think the current behavior of file order overriding scope order is insane and a bug," do you mean that fixtures are currently executed using file order? If so, how does that work exactly?

Scope order is probably a good heuristic for how expensive a fixture is, as all else being equal, users will give more expensive fixtures higher scopes so they're run less often. Unfortunately, I can imagine situations where there are lower-scoped fixtures that are more expensive than session-scoped fixtures, say with a session-scoped fixture that makes a connection to a test database but class- or module-scoped fixtures that do some complicated and slow operations to get the test database into the right state. For an inexpensive fixture, though, there's always the option of tightening the scope so it gets run more often, which doesn't cost that much because it's cheap, so it doesn't take priority over a more-expensive fixture with a lower scope, while it may not be possible to broaden the scope of an expensive fixture. Since it makes the common use-case better while still allowing the uncommon use-case (with some additional work), I think this is a good compromise.

Edited (had the wrong configuration on the toy example):
@nicoddemus, as you said, this doesn't fix #3161. I'm guessing this is because this only affects the initial fixture order, not how fixtures are reordered after autouse fixtures are introduced? Are you planning on tackling that here or should I be looking at creating a patch on top of this branch?

[nicoddemus]

Hey @ceridwen thanks for the feedback.

I'm guessing this is because this only affects the initial fixture order, not how fixtures are reordered after autouse fixtures are introduced?

Yes, your guess sounds right to me.

Are you planning on tackling that here or should I be looking at creating a patch on top of this branch?

I'm not planning to tackle #3161 here, it definitely should be a separate PR. Feel free to start working on it. The consensus so far here and in the mailing list is that the change is correct, so I will most likely finish this PR on Monday and we can merge it.

[ceridwen]

I'll wait until the merge. The biggest thing I need is agreement on what the correct order should be. I don't know the history of reorder_atscope() but the function is overcomplicated and hard to understand, and I have no idea what order it's supposed to produce, if there ever was such a thing.

[nicoddemus]

Folks I think this is now ready for another round of reviews. 👍

[RonnyPfannschmidt]

from my pov we can merge