Fix a tiny bug in python.py::_find_parametrized_scope

[sadra-barikbin]

Currently, when there are multiple fixturedefs for a param, _find_parametrized_scope picks the farthest one, while it should pick the nearest one.

[bluetech]

Thanks, as best as I can understand, this was a bug, at least didn't work as written in the docstring, and the fix makes it work as intended.

I tried to create an integration test for it, and came up with this:

import pytest

@pytest.fixture(scope="module")
def fix(request):
    assert request.scope == "module"
    return "module"

class TestIt:
    @pytest.fixture(scope="class")
    def fix(self, request, fix):
        assert request.scope == "class"
        return "class"

    @pytest.mark.parametrize("fix", ["function"], indirect=True)
    def test_it(self, fix):
        print(fix)

Without your fix, the request.scope is "module" in both fix's. After your fix, the request.scope is "class" in both. It would be good to add this test, WDYT?

I am confused why the request.scope in the module-level fixture is "class", that seems incorrect? (Maybe the sub-SubRequest forgets to change the scope or something?) I ran out of time to look at it, but I think it's a separate issue, so it shouldn't prevent us from getting this fix in, but should probably remove this assert from the test.

---

A couple of procedural comments:

The commit message "Fix a tiny typo" sounds like this is fixing some doc comment or such, but this actually fixes a logic bug, so it would be better to use a commit message like:

python: fix scope assignment for indirect parameter sets

Previously, when assigning a scope for a fully-indirect parameter set,
when there are multiple fixturedefs for a param (i.e. same-name fixture
chain), the highest scope was used, but it should be the lowest scope,
since that's the effective scope of the fixture.

Also since this is a bugfix we should add a changelog entry, see "write a changelog entry" here.

[sadra-barikbin]

Regarding an integration test, how is to add this one which checks where _find_parametrized_scope actually affects, i.e. reordering and subrequest._check_scope in FixtureRequest::_compute_fixture_value?

def test_reordering_with_scopeless_and_just_indirect_parametrization(self, pytester: Pytester) -> None:
    pytester.makeconftest(
        """
        import pytest

        @pytest.fixture(scope="package")
        def fixture1():
            pass
        """
    )
    pytester.makepyfile(
        """
        import pytest

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

        @pytest.fixture(scope="module")
        def fixture1(fixture0):
            pass

        @pytest.mark.parametrize("fixture1", [0], indirect=True)
        def test_0(fixture1):
            pass
                
        @pytest.fixture(scope="module")
        def fixture():
            pass
                
        @pytest.mark.parametrize("fixture", [0], indirect=True)
        def test_1(fixture):
            pass

        def test_2():
            pass

        class Test:
            @pytest.fixture(scope="class")
            def fixture(self):
                pass

            @pytest.mark.parametrize("fixture", [0], indirect=True)
            def test_3(self, fixture):
                pass
        """
    )
    result = pytester.runpytest("-v")
    assert result.ret == 0
    result.stdout.fnmatch_lines(
        [
            "*test_0*",
            "*test_1*",
            "*test_2*",
            "*test_3*",
        ]
    )

Regarding resuest.scope in the module-level fixture, it's not the same as fixturedef.scope as the item's callspec.arg2scope overrides it:

pytest/src/_pytest/fixtures.py

Lines 684 to 695 in b8b7433

	scope = fixturedef._scope
	try:
	callspec = funcitem.callspec
	except AttributeError:
	callspec = None
	if callspec is not None and argname in callspec.params:
	param = callspec.params[argname]
	param_index = callspec.indices[argname]
	# If a parametrize invocation set a scope it will override
	# the static scope defined with the fixture function.
	with suppress(KeyError):
	scope = callspec._arg2scope[argname]

[bluetech]

I pushed a little edit of the commit message and changelog as discussed in my previous comment, before merging.

Thanks for improving the test, I just have a final comment on it, let me know what you think.

[bluetech]

I think this is important for testing this particular code path:

Suggested change

	def fixture(self):
	def fixture(self, fixture):

This change might make test3 to be reordered before test2, in order to share to module-scoped fixture -- I'm not sure (maybe it will only happen once we reorder based on param value and not param index).

[sadra-barikbin]

I'll add it but it does not change the order since the only produced fixture key of test_3 is FixtureArgKey('fixture', 0, 'test_module', 'Test') and the only produced fixture key of test_1 is FixtureArgKey('fixture', 0, 'test_module', None). Basing reordering on param value does not change it either as the module-scoped fixture is shadowed behind the class-scoped one for test_3. In other words we only pick fixturedefs[-1] to produce a key.

If you think we should consider shadowed-but-used fixtures into reordering as well, one way to that end is to check if fixturedefs[-1] itself uses fixture or not so that we could produce a key for it as well. A general solution to considering shadowed-but-used fixtures was among the items in #11234 (Considering used shadowed fixture dependencies for reordering as well. This was done by changing fixturemanager::getfixtureclosure algo from BFS to DFS.) but I removed it as I thought it would not get accepted by the team. We could move forward with that as well.

[bluetech]

I'll add it but it does not change the order since the only produced fixture key of test_3 is FixtureArgKey('fixture', 0, 'test_module', 'Test') and the only produced fixture key of test_1 is FixtureArgKey('fixture', 0, 'test_module', None)

Right, I forgot about the item_cls.

Regarding reordering based on shadowed-but-used fixtures, naively it makes sense to me -- they are used after all so why should they be ignored?

but I removed it as I thought it would not get accepted by the team. We could move forward with that as well.

Perhaps after the current batch of changes we can look into it and think more deeply about it.

[bluetech]

@sadra-barikbin BTW I just sent you an invite to become a pytest contributor, it doesn't impose any burden on you, just allows you to handle issues, review PRs etc if you'd like to.

[sadra-barikbin]

@sadra-barikbin BTW I just sent you an invite to become a pytest contributor, it doesn't impose any burden on you, just allows you to handle issues, review PRs etc if you'd like to.

Thank you! It’ll be a pleasure.