Skip to content
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.

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

Fix a tiny bug in python.py::_find_parametrized_scope #11277

Merged
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
2 changes: 2 additions & 0 deletions changelog/11277.bugfix.rst
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
Fixed a bug that when there are multiple fixtures for an indirect parameter,
the scope of the highest-scope fixture is picked for the parameter set, instead of that of the one with the narrowest scope.
2 changes: 1 addition & 1 deletion src/_pytest/fixtures.py
Original file line number Diff line number Diff line change
Expand Up @@ -492,7 +492,7 @@ def node(self):
node: Optional[Union[nodes.Item, nodes.Collector]] = self._pyfuncitem
elif scope is Scope.Package:
# FIXME: _fixturedef is not defined on FixtureRequest (this class),
# but on FixtureRequest (a subclass).
# but on SubRequest (a subclass).
node = get_scope_package(self._pyfuncitem, self._fixturedef) # type: ignore[attr-defined]
else:
node = get_scope_node(self._pyfuncitem, scope)
Expand Down
4 changes: 2 additions & 2 deletions src/_pytest/python.py
Original file line number Diff line number Diff line change
Expand Up @@ -1516,7 +1516,7 @@ def _find_parametrized_scope(
if all_arguments_are_fixtures:
fixturedefs = arg2fixturedefs or {}
used_scopes = [
fixturedef[0]._scope
fixturedef[-1]._scope
for name, fixturedef in fixturedefs.items()
if name in argnames
]
Expand Down Expand Up @@ -1682,7 +1682,7 @@ class Function(PyobjMixin, nodes.Item):
:param config:
The pytest Config object.
:param callspec:
If given, this is function has been parametrized and the callspec contains
If given, this function has been parametrized and the callspec contains
meta information about the parametrization.
:param callobj:
If given, the object which will be called when the Function is invoked,
Expand Down
62 changes: 62 additions & 0 deletions testing/python/metafunc.py
Original file line number Diff line number Diff line change
Expand Up @@ -151,6 +151,7 @@ class DummyFixtureDef:
module_fix=[DummyFixtureDef(Scope.Module)],
class_fix=[DummyFixtureDef(Scope.Class)],
func_fix=[DummyFixtureDef(Scope.Function)],
mixed_fix=[DummyFixtureDef(Scope.Module), DummyFixtureDef(Scope.Class)],
),
)

Expand Down Expand Up @@ -187,6 +188,7 @@ def find_scope(argnames, indirect):
)
== Scope.Module
)
assert find_scope(["mixed_fix"], indirect=True) == Scope.Class

def test_parametrize_and_id(self) -> None:
def func(x, y):
Expand Down Expand Up @@ -1503,6 +1505,66 @@ def test_it(x): pass
result = pytester.runpytest()
assert result.ret == 0

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):
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@sadra-barikbin sadra-barikbin Aug 6, 2023

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

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


class TestMetafuncFunctionalAuto:
"""Tests related to automatically find out the correct scope for
Expand Down
Loading