Add FixtureArgKey class to represent fixture deps in fixtures.py

[sadra-barikbin]

• To add FixtureArgKey class to represent fixture dependencies.
• To base fixture argkeys on param values rather than on indices unless the value isn't hashable in which case we fallback to param index. Improvement: Base FixtureArgKeys on param values if possible, not param indices #11271

[RonnyPfannschmidt]

my understanding is, that this order ought not to change

[sadra-barikbin]

Looking at fixtureargkeys of these 8 tests, due to basing them on indices, currently we have:

# tests' fixture deps:
<Function test_one[arg1v1-arg2v1]> {('arg1', 0, PosixPath('/tmp/pytest-of-sadra/pytest-0/test_5190/issue_519.py')): None}
<Function test_one[arg1v1-arg2v2]> {('arg1', 1, PosixPath('/tmp/pytest-of-sadra/pytest-0/test_5190/issue_519.py')): None}
<Function test_one[arg1v2-arg2v1]> {('arg1', 2, PosixPath('/tmp/pytest-of-sadra/pytest-0/test_5190/issue_519.py')): None}
<Function test_one[arg1v2-arg2v2]> {('arg1', 3, PosixPath('/tmp/pytest-of-sadra/pytest-0/test_5190/issue_519.py')): None}
<Function test_two[arg1v1-arg2v1]> {('arg1', 0, PosixPath('/tmp/pytest-of-sadra/pytest-0/test_5190/issue_519.py')): None}
<Function test_two[arg1v1-arg2v2]> {('arg1', 1, PosixPath('/tmp/pytest-of-sadra/pytest-0/test_5190/issue_519.py')): None}
<Function test_two[arg1v2-arg2v1]> {('arg1', 2, PosixPath('/tmp/pytest-of-sadra/pytest-0/test_5190/issue_519.py')): None}
<Function test_two[arg1v2-arg2v2]> {('arg1', 3, PosixPath('/tmp/pytest-of-sadra/pytest-0/test_5190/issue_519.py')): None}

in which test_one[arg1v1-arg2v1]> and test_one[arg1v1-arg2v2]> unexpectedly don't have a common dependency while they do depend on arg1="arg1v1". This is also the case for test_one[arg1v2-arg2v1]> and test_one[arg1v2-arg2v2]>, etc.

By basing fixtureargkeys on values, we would have:

# tests' fixture deps:
<Function test_one[arg1v1-arg2v1]> {('arg1', "arg1v1", PosixPath('/tmp/pytest-of-sadra/pytest-0/test_5190/issue_519.py')): None}
<Function test_one[arg1v1-arg2v2]> {('arg1', "arg1v1", PosixPath('/tmp/pytest-of-sadra/pytest-0/test_5190/issue_519.py')): None}
<Function test_one[arg1v2-arg2v1]> {('arg1', "arg1v2", PosixPath('/tmp/pytest-of-sadra/pytest-0/test_5190/issue_519.py')): None}
<Function test_one[arg1v2-arg2v2]> {('arg1', "arg1v2", PosixPath('/tmp/pytest-of-sadra/pytest-0/test_5190/issue_519.py')): None}
<Function test_two[arg1v1-arg2v1]> {('arg1', "arg1v1", PosixPath('/tmp/pytest-of-sadra/pytest-0/test_5190/issue_519.py')): None}
<Function test_two[arg1v1-arg2v2]> {('arg1', "arg1v1", PosixPath('/tmp/pytest-of-sadra/pytest-0/test_5190/issue_519.py')): None}
<Function test_two[arg1v2-arg2v1]> {('arg1', "arg1v2", PosixPath('/tmp/pytest-of-sadra/pytest-0/test_5190/issue_519.py')): None}
<Function test_two[arg1v2-arg2v2]> {('arg1', "arg1v2", PosixPath('/tmp/pytest-of-sadra/pytest-0/test_5190/issue_519.py')): None}

in which <Function test_one[arg1v1-arg2v1]> correctly has common dependency with both <Function test_one[arg1v1-arg2v2]> and <Function test_two[arg1v1-arg2v1]>. This changes the reordering output thus affecting test_issue519. Am I correct?

[RonnyPfannschmidt]

scope ordering gets broken up wrong

the parameter sets order is more important that the test name, so all test of a particular parameterization must come before the next test
so the order change here is incorrect and to be considered a bug

[sadra-barikbin]

Shouldn't the initial ordering be preserved, given that all fixture dependencies are applied in reordering?

test_two[arg1v1-arg2v1] shares dependency FixtureArgKey(argname="arg1", param_value="arg1v1") with both test_one[arg1v1-arg2v1] and test_one[arg1v1-arg2v2] so all the three come together, but between test_one[arg1v1-arg2v2] and test_two[arg1v1-arg2v1], the former should come first as it comes first in the initial ordering as well.

[sadra-barikbin]

arg2=arg2v1 is not considered into reordering as its scope is function.

[sadra-barikbin]

But in that case and also in general, isn't better that user declare the parameter as a high-scoped one so that all tests reading a specific file, be close to each other?

[RonnyPfannschmidt]

If repeated setup is needed that's neither intuitive nor what people do

[sadra-barikbin]

Then, what's the reason that user could set scope for direct parametrizations?

To set another example that the combination of basing arg keys on param index and add_funcarg_pseudo_fixturedef's implementation produce unwanted results, by swapping arg2 values for test_two we would have a whole different ordering

import pytest

@pytest.mark.parametrize("arg2", ["arg2v1", "arg2v2"])
@pytest.mark.parametrize("arg1", ["arg1v1", "arg1v2"], scope='module')
def test_one(arg1, arg2):
    pass

@pytest.mark.parametrize("arg2", ["arg2v2", "arg2v1"])
@pytest.mark.parametrize("arg1", ["arg1v1", "arg1v2"], scope='module')
def test_two(arg1, arg2):
    pass

<Function test_one[arg1v1-arg2v1]>
  <Function test_two[arg1v1-arg2v2]>
  <Function test_one[arg1v1-arg2v2]>
  <Function test_two[arg1v1-arg2v1]>
  <Function test_one[arg1v2-arg2v1]>
  <Function test_two[arg1v2-arg2v2]>
  <Function test_one[arg1v2-arg2v2]>
  <Function test_two[arg1v2-arg2v1]>

[bluetech]

IMO the new order is more intuitive. I feel that having the same test definition (i.e. the base un-parametrized test) should have a stronger ordering affect than sharing a function-scoped parameterset.

I understand @RonnyPfannschmidt comments re. the filesystem caching example, but I also agree with @sadra-barikbin that for such cases the user should set a higher scope.

[RonnyPfannschmidt]

sometimes the user is not ableto set a higher scope (when using fixtures that have to recreate for each test)

the alternative would be to introduce a new fixture that is indeed of higher scope and passed in as the "parameter" for another fixture (which we currently cant do nicely)

if we can make a higher scope parameterize of a function scoped fixture work my concern would be obsoleted

let me reassure you that the new way proposed here is indeed more beautiful, more clean and more maintainable, but i'm under the impression it also breaks some "dirty wins"

as of now its unclear how to spell those "dirty wins cleanly,

pytest-lazyfixtures has a potential way out by allowing fixtures to be spelled there and when integrating it in core parametrize support could be added

then we would be able to spell something like

inputs = pytest.fixture.from_constants([1,2,3,4], scope="module")

@pytest.fixture
def something(request):
   return request.param

@pytest.mark.parametrize('something', inputs, indirect=True)
def mytest(something):
   pass

[bluetech]

@sadra-barikbin It would be very helpful for review to split this PR to two:

1. The first PR would introduce FixtureArgKey to replace the _Key tuple currently used, but will make no functional/behavior/logic changes, i.e. just a refactor.
2. The second PR will be based on the first one and introduce the Hashable changes.

The reason is that the first PR can be a nice cleanup on its own, while the second would be easier to consider if the diff is minimal.

[bluetech]

Thanks again for splitting. I left a couple of comments, with them fixed this PR should be good to merge.

[bluetech]

There's a semantic conflict here, the item.path.parent changed to item.path in main (a21fb87). However, I just realized that that change was wrong, and that item.path.parent is somewhat wrong as well.

Anyway, let's keep it scoped_item_path = item.path here, because changing it is not the purpose of this PR, and I will send a PR to fix it after (the fix will actually be easier with the new FixtureArgKey).

[sadra-barikbin]

Why is item.path.parent wrong? Because packages could contain subdirectories? How is this?

elif scope is Scope.Package:
    if isinstance(item.parent, Package):
        scoped_item_path = item.parent.path
    elif isinstance(item.parent.parent, Package):
        scoped_item_path = item.parent.parent.path # If item is a class method.
    else:
        continue # It's not located in a package.

Also shouldn't we change the class branch as follows:

elif scope is Scope.Class:
    if not item.cls:
        continue
    scoped_item_path = item.path
    item_cls = item.cls

For class-less test functions?

[sadra-barikbin]

As you said, this test fails currently:

def test_reordering_with_package_scoped_parametrization_of_test_in_subdirectory(pytester: Pytester) -> None:
    pytester.makepyfile(
        __init__= "",
        test_a= textwrap.dedent(
        """\
        import pytest

        @pytest.mark.parametrize("arg", [0], scope="package")
        def test_1(arg):
            pass
        
        def test_2():
            pass
        """,
        ),
    )
    subdir = pytester.mkdir("subdirectory")
    subdir.joinpath("test_b.py").write_text(
        textwrap.dedent(
            """\
            import pytest

            @pytest.mark.parametrize("arg", [0], scope="package")
            def test_3(arg):
                pass
            """,
        ),
        encoding="utf-8",
    )
    result = pytester.runpytest("--collect-only")
    result.stdout.fnmatch_lines(
        [
        "*test_1*",
        "*test_3*",
        "*test_2*",
        ]
    )

[sadra-barikbin]

And this also fails for class branch:

def test_reordering_with_class_scoped_parametrization_of_classless_test(pytester: Pytester) -> None:
    pytester.makepyfile(
        """
        import pytest
        
        @pytest.mark.parametrize("arg", [0], scope="class")
        def test_1(arg):
            pass
            
        def test_2():
            pass
        
        @pytest.mark.parametrize("arg", [0], scope="class")
        def test_3(arg):
            pass
    """
    )
    result = pytester.runpytest("--collect-only")
    result.stdout.fnmatch_lines(
        [
        "*test_1*",
        "*test_2*",
        "*test_3*",
        ]
    )

[bluetech]

Why is item.path.parent wrong? Because packages could contain subdirectories? How is this?

Two reasons:

• It can contain subdirectories or other oddities from plugins, we shouldn't assume that the Package.path is the parent.
• There might not be a Package at all, in which case should "fall back" to Session i.e. None.

You got the idea in your code, we need to find the actual Package and use its path. But we shouldn't assume it's the parent or the parent.parent, best to use item.getparent(Package). And if it's None, treat same as Session.

Also shouldn't we change the class branch as follows:

Hmm I don't think it's OK to just skip is it?

As you said, this test fails currently:

Right, I think it should pass. We should add it as a test in the follow up PR.

And this also fails for class branch:

When I run it on current main I get order 1-3-2, which seems good to me. Do you think the order should be 1-2-3 in this case?

[sadra-barikbin]

If we set scoped_item_path to None when the package-scoped test is package-less and do not skip the key, then the test would have common key with all the package-scoped tests that have the same parameter, so they will be gathered together in the reordering. Is this the desired behavior? If we skip the key, the test gets isolated up until it's no longer package-less.
This is also the case for class branch. Do we want class-less tests using a parameter to be gathered altogether in reordering or be isolated?

[sadra-barikbin]

If we skip the key, it might seem more intuitive to the user. He/She says: I've set scope x for the test. The test is not located in a x. So the scope would be ineffective.