Skip to content

Add FixtureArgKey class to represent fixture deps in fixtures.py #11231

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
bluetech merged 6 commits into from
Aug 2, 2023
Merged

Add FixtureArgKey class to represent fixture deps in fixtures.py #11231

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
81464b3
Implement the feature
sadra-barikbin Jul 19, 2023
2c270b8
Improve a test
sadra-barikbin Jul 21, 2023
e653fa9
Fix a tiny bug in a test
sadra-barikbin Jul 21, 2023
fdc1b10
Apply comments
sadra-barikbin Aug 1, 2023
cd4a26a
Separate PR#11271 from this PR
sadra-barikbin Aug 1, 2023
5b7ebee
Apply comments
sadra-barikbin Aug 2, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
46 changes: 28 additions & 18 deletions src/_pytest/fixtures.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -239,11 +239,17 @@ def getfixturemarker(obj: object) -> Optional["FixtureFunctionMarker"]:
)


# Parametrized fixture key, helper alias for code below.
_Key = Tuple[object, ...]
@dataclasses.dataclass(frozen=True)
class FixtureArgKey:
argname: str
param_index: int
scoped_item_path: Optional[Path]
item_cls: Optional[type]


def get_parametrized_fixture_keys(item: nodes.Item, scope: Scope) -> Iterator[_Key]:
def get_parametrized_fixture_keys(
item: nodes.Item, scope: Scope
) -> Iterator[FixtureArgKey]:
"""Return list of keys for all parametrized arguments which match
the specified scope."""
assert scope is not Scope.Function
Expand All @@ -256,21 +262,25 @@ def get_parametrized_fixture_keys(item: nodes.Item, scope: Scope) -> Iterator[_K
# cs.indices.items() is random order of argnames. Need to
Comment thread
sadra-barikbin marked this conversation as resolved.
Outdated
# sort this so that different calls to
# get_parametrized_fixture_keys will be deterministic.
for argname, param_index in sorted(cs.indices.items()):
for argname in sorted(cs.indices):
if cs._arg2scope[argname] != scope:
continue

item_cls = None
if scope is Scope.Session:
key: _Key = (argname, param_index)
scoped_item_path = None
elif scope is Scope.Package:
key = (argname, param_index, item.path)
scoped_item_path = item.path.parent
Copy link
Copy Markdown
Member

@bluetech bluetech Aug 2, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Copy Markdown
Contributor Author

@sadra-barikbin sadra-barikbin Aug 2, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Copy Markdown
Contributor Author

@sadra-barikbin sadra-barikbin Aug 2, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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*",
        ]
    )

Copy link
Copy Markdown
Contributor Author

@sadra-barikbin sadra-barikbin Aug 2, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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*",
        ]
    )

Copy link
Copy Markdown
Member

@bluetech bluetech Aug 2, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Copy Markdown
Contributor Author

@sadra-barikbin sadra-barikbin Aug 2, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Copy Markdown
Contributor Author

@sadra-barikbin sadra-barikbin Aug 2, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

elif scope is Scope.Module:
key = (argname, param_index, item.path)
scoped_item_path = item.path
elif scope is Scope.Class:
scoped_item_path = item.path
item_cls = item.cls # type: ignore[attr-defined]
key = (argname, param_index, item.path, item_cls)
else:
assert_never(scope)
yield key

param_index = cs.indices[argname]
yield FixtureArgKey(argname, param_index, scoped_item_path, item_cls)


# Algorithm for sorting on a per-parametrized resource setup basis.
Expand All @@ -280,12 +290,12 @@ def get_parametrized_fixture_keys(item: nodes.Item, scope: Scope) -> Iterator[_K


def reorder_items(items: Sequence[nodes.Item]) -> List[nodes.Item]:
argkeys_cache: Dict[Scope, Dict[nodes.Item, Dict[_Key, None]]] = {}
items_by_argkey: Dict[Scope, Dict[_Key, Deque[nodes.Item]]] = {}
argkeys_cache: Dict[Scope, Dict[nodes.Item, Dict[FixtureArgKey, None]]] = {}
items_by_argkey: Dict[Scope, Dict[FixtureArgKey, Deque[nodes.Item]]] = {}
for scope in HIGH_SCOPES:
d: Dict[nodes.Item, Dict[_Key, None]] = {}
d: Dict[nodes.Item, Dict[FixtureArgKey, None]] = {}
argkeys_cache[scope] = d
item_d: Dict[_Key, Deque[nodes.Item]] = defaultdict(deque)
item_d: Dict[FixtureArgKey, Deque[nodes.Item]] = defaultdict(deque)
items_by_argkey[scope] = item_d
for item in items:
keys = dict.fromkeys(get_parametrized_fixture_keys(item, scope), None)
Expand All @@ -301,8 +311,8 @@ def reorder_items(items: Sequence[nodes.Item]) -> List[nodes.Item]:

def fix_cache_order(
item: nodes.Item,
argkeys_cache: Dict[Scope, Dict[nodes.Item, Dict[_Key, None]]],
items_by_argkey: Dict[Scope, Dict[_Key, "Deque[nodes.Item]"]],
argkeys_cache: Dict[Scope, Dict[nodes.Item, Dict[FixtureArgKey, None]]],
items_by_argkey: Dict[Scope, Dict[FixtureArgKey, "Deque[nodes.Item]"]],
) -> None:
for scope in HIGH_SCOPES:
for key in argkeys_cache[scope].get(item, []):
Expand All @@ -311,13 +321,13 @@ def fix_cache_order(

def reorder_items_atscope(
items: Dict[nodes.Item, None],
argkeys_cache: Dict[Scope, Dict[nodes.Item, Dict[_Key, None]]],
items_by_argkey: Dict[Scope, Dict[_Key, "Deque[nodes.Item]"]],
argkeys_cache: Dict[Scope, Dict[nodes.Item, Dict[FixtureArgKey, None]]],
Comment thread
sadra-barikbin marked this conversation as resolved.
items_by_argkey: Dict[Scope, Dict[FixtureArgKey, "Deque[nodes.Item]"]],
scope: Scope,
) -> Dict[nodes.Item, None]:
if scope is Scope.Function or len(items) < 3:
return items
ignore: Set[Optional[_Key]] = set()
ignore: Set[Optional[FixtureArgKey]] = set()
items_deque = deque(items)
items_done: Dict[nodes.Item, None] = {}
scoped_items_by_argkey = items_by_argkey[scope]
Expand Down
38 changes: 38 additions & 0 deletions testing/python/fixtures.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4536,3 +4536,41 @@ def test_fixt(custom):
result.assert_outcomes(errors=1)
result.stdout.fnmatch_lines([expected])
assert result.ret == ExitCode.TESTS_FAILED


@pytest.mark.xfail(
Comment thread
sadra-barikbin marked this conversation as resolved.
Outdated
reason="It isn't differentiated between direct `fixture` param and fixture `fixture`. Will be"
"solved by adding `baseid` to `FixtureArgKey`."
)
def test_reorder_with_high_scoped_direct_and_fixture_parametrization(
pytester: Pytester,
):
pytester.makepyfile(
"""
import pytest

@pytest.fixture(params=[0, 1], scope='module')
def fixture(request):
pass

def test_1(fixture):
pass

def test_2():
pass

@pytest.mark.parametrize("fixture", [1, 2], scope='module')
def test_3(fixture):
pass
"""
)
result = pytester.runpytest("--collect-only")
result.stdout.re_match_lines(
[
r" <Function test_1\[0\]>",
r" <Function test_1\[1\]>",
r" <Function test_2>",
r" <Function test_3\[1\]>",
r" <Function test_3\[2\]>",
]
)