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

Remove prune_dependency_tree and reuse getfixtureclosure logic #11243

Open
wants to merge 19 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
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
7 changes: 4 additions & 3 deletions src/_pytest/assertion/rewrite.py
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
from pathlib import Path
from pathlib import PurePath
from typing import Callable
from typing import DefaultDict
from typing import Dict
from typing import IO
from typing import Iterable
Expand Down Expand Up @@ -668,9 +669,9 @@ def __init__(
else:
self.enable_assertion_pass_hook = False
self.source = source
self.scope: tuple[ast.AST, ...] = ()
self.variables_overwrite: defaultdict[
tuple[ast.AST, ...], Dict[str, str]
self.scope: Tuple[ast.AST, ...] = ()
self.variables_overwrite: DefaultDict[
Tuple[ast.AST, ...], Dict[str, str]
] = defaultdict(dict)

def run(self, mod: ast.Module) -> None:
Expand Down
80 changes: 38 additions & 42 deletions src/_pytest/fixtures.py
Original file line number Diff line number Diff line change
Expand Up @@ -314,33 +314,6 @@ class FuncFixtureInfo:
# sequence is ordered from furthest to closes to the function.
name2fixturedefs: Dict[str, Sequence["FixtureDef[Any]"]]

def prune_dependency_tree(self) -> None:
"""Recompute names_closure from initialnames and name2fixturedefs.

Can only reduce names_closure, which means that the new closure will
always be a subset of the old one. The order is preserved.

This method is needed because direct parametrization may shadow some
of the fixtures that were included in the originally built dependency
tree. In this way the dependency tree can get pruned, and the closure
of argnames may get reduced.
"""
closure: Set[str] = set()
working_set = set(self.initialnames)
while working_set:
argname = working_set.pop()
# Argname may be smth not included in the original names_closure,
# in which case we ignore it. This currently happens with pseudo
# FixtureDefs which wrap 'get_direct_param_fixture_func(request)'.
# So they introduce the new dependency 'request' which might have
# been missing in the original tree (closure).
if argname not in closure and argname in self.names_closure:
closure.add(argname)
if argname in self.name2fixturedefs:
working_set.update(self.name2fixturedefs[argname][-1].argnames)

self.names_closure[:] = sorted(closure, key=self.names_closure.index)


class FixtureRequest(abc.ABC):
"""The type of the ``request`` fixture.
Expand Down Expand Up @@ -961,7 +934,6 @@ def _eval_scope_callable(
return result


@final
class FixtureDef(Generic[FixtureValue]):
"""A container for a fixture definition.

Expand Down Expand Up @@ -1104,6 +1076,26 @@ def __repr__(self) -> str:
)


class IdentityFixtureDef(FixtureDef[FixtureValue]):
def __init__(
self,
fixturemanager: "FixtureManager",
argname: str,
scope: Union[Scope, _ScopeName, Callable[[str, Config], _ScopeName], None],
sadra-barikbin marked this conversation as resolved.
Show resolved Hide resolved
*,
_ispytest: bool = False,
):
super().__init__(
fixturemanager,
"",
argname,
lambda request: request.param,
scope,
None,
_ispytest=_ispytest,
)


def resolve_fixture_function(
fixturedef: FixtureDef[FixtureValue], request: FixtureRequest
) -> "_FixtureFunc[FixtureValue]":
Expand Down Expand Up @@ -1486,10 +1478,11 @@ def getfixtureinfo(
initialnames = deduplicate_names(autousenames, usefixturesnames, argnames)

direct_parametrize_args = _get_direct_parametrize_args(node)

names_closure, arg2fixturedefs = self.getfixtureclosure(
arg2fixturedefs: Dict[str, Sequence[FixtureDef[Any]]] = {}
names_closure = self.getfixtureclosure(
parentnode=node,
initialnames=initialnames,
arg2fixturedefs=arg2fixturedefs,
ignore_args=direct_parametrize_args,
)

Expand Down Expand Up @@ -1533,30 +1526,33 @@ def getfixtureclosure(
self,
parentnode: nodes.Node,
initialnames: Tuple[str, ...],
arg2fixturedefs: Dict[str, Sequence[FixtureDef[Any]]],
ignore_args: AbstractSet[str],
) -> Tuple[List[str], Dict[str, Sequence[FixtureDef[Any]]]]:
) -> List[str]:
Copy link
Member

Choose a reason for hiding this comment

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

I would prefer to keep returning a brand new arg2fixturedefs, and let the caller merge it with other dict if they want, rather than passing in a dict and fill it inside. This makes it clear what's input/output.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But the second fixture closure computation needs arg2fixturedefs as input, otherwise it should compute it again which is expensive according to docstring:

# ........ we also populate arg2fixturedefs mapping
# for the args missing therein so that the caller can reuse it and does
# not have to re-discover fixturedefs again for each fixturename
# (discovering matching fixtures for a given name/node is expensive).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How about this?

def getfixtureclosure(
        self,
        parentnode: nodes.Node,
        initialnames: Tuple[str, ...],
        arg2fixturedefs: Union[Dict[str, Sequence[FixtureDef[Any]]], None],
        ignore_args: Sequence[str] = (),
    ) -> Tuple[List[str], Dict[str, Sequence[FixtureDef[Any]]]]:

If arg2fixturedefs is none, we create one, populate it and return it. Otherwise we just use it and return it.

# Collect the closure of all fixtures, starting with the given
# fixturenames as the initial set. As we have to visit all
# factory definitions anyway, we also return an arg2fixturedefs
# mapping so that the caller can reuse it and does not have
# to re-discover fixturedefs again for each fixturename
# initialnames containing function arguments, `usefixture` markers
# and `autouse` fixtures as the initial set. As we have to visit all
# factory definitions anyway, we also populate arg2fixturedefs mapping
# for the args missing therein so that the caller can reuse it and does
# not have to re-discover fixturedefs again for each fixturename
# (discovering matching fixtures for a given name/node is expensive).

parentid = parentnode.nodeid
fixturenames_closure = list(initialnames)

arg2fixturedefs: Dict[str, Sequence[FixtureDef[Any]]] = {}
lastlen = -1
while lastlen != len(fixturenames_closure):
lastlen = len(fixturenames_closure)
for argname in fixturenames_closure:
if argname in ignore_args:
continue
if argname in arg2fixturedefs:
continue
fixturedefs = self.getfixturedefs(argname, parentid)
if fixturedefs:
arg2fixturedefs[argname] = fixturedefs
if argname not in arg2fixturedefs:
fixturedefs = self.getfixturedefs(argname, parentid)
if fixturedefs:
arg2fixturedefs[argname] = fixturedefs
else:
fixturedefs = arg2fixturedefs[argname]
if fixturedefs and not isinstance(fixturedefs[-1], IdentityFixtureDef):
for arg in fixturedefs[-1].argnames:
if arg not in fixturenames_closure:
fixturenames_closure.append(arg)
Expand All @@ -1570,7 +1566,7 @@ def sort_by_scope(arg_name: str) -> Scope:
return fixturedefs[-1]._scope

fixturenames_closure.sort(key=sort_by_scope, reverse=True)
return fixturenames_closure, arg2fixturedefs
return fixturenames_closure

def pytest_generate_tests(self, metafunc: "Metafunc") -> None:
"""Generate new tests based on parametrized fixtures used by the given metafunc"""
Expand Down
68 changes: 41 additions & 27 deletions src/_pytest/python.py
Original file line number Diff line number Diff line change
Expand Up @@ -59,10 +59,10 @@
from _pytest.deprecated import check_ispytest
from _pytest.deprecated import INSTANCE_COLLECTOR
from _pytest.deprecated import NOSE_SUPPORT_METHOD
from _pytest.fixtures import FixtureDef
from _pytest.fixtures import FixtureRequest
from _pytest.fixtures import _get_direct_parametrize_args
from _pytest.fixtures import FuncFixtureInfo
from _pytest.fixtures import get_scope_node
from _pytest.fixtures import IdentityFixtureDef
from _pytest.main import Session
from _pytest.mark import MARK_GEN
from _pytest.mark import ParameterSet
Expand Down Expand Up @@ -479,8 +479,6 @@
)
fixtureinfo = definition._fixtureinfo

# pytest_generate_tests impls call metafunc.parametrize() which fills
# metafunc._calls, the outcome of the hook.
metafunc = Metafunc(
definition=definition,
fixtureinfo=fixtureinfo,
Expand All @@ -489,23 +487,32 @@
module=module,
_ispytest=True,
)
methods = []

def prune_dependency_tree_if_test_is_directly_parametrized(metafunc: Metafunc):
# Direct (those with `indirect=False`) parametrizations taking place in
# module/class-specific `pytest_generate_tests` hooks, a.k.a dynamic direct
# parametrizations using `metafunc.parametrize`, may have shadowed some
# fixtures, making some fixtures no longer reachable. Update the dependency
# tree to reflect what the item really needs.
# Note that direct parametrizations using `@pytest.mark.parametrize` have
# already been considered into making the closure using the `ignore_args`
# arg to `getfixtureclosure`.
if metafunc._has_direct_parametrization:
metafunc._update_dependency_tree()

Check warning on line 501 in src/_pytest/python.py

View check run for this annotation

Codecov / codecov/patch

src/_pytest/python.py#L501

Added line #L501 was not covered by tests

methods = [prune_dependency_tree_if_test_is_directly_parametrized]
if hasattr(module, "pytest_generate_tests"):
methods.append(module.pytest_generate_tests)
if cls is not None and hasattr(cls, "pytest_generate_tests"):
methods.append(cls().pytest_generate_tests)

# pytest_generate_tests impls call metafunc.parametrize() which fills
# metafunc._calls, the outcome of the hook.
self.ihook.pytest_generate_tests.call_extra(methods, dict(metafunc=metafunc))

if not metafunc._calls:
yield Function.from_parent(self, name=name, fixtureinfo=fixtureinfo)
else:
# Direct parametrizations taking place in module/class-specific
# `metafunc.parametrize` calls may have shadowed some fixtures, so make sure
# we update what the function really needs a.k.a its fixture closure. Note that
# direct parametrizations using `@pytest.mark.parametrize` have already been considered
# into making the closure using `ignore_args` arg to `getfixtureclosure`.
fixtureinfo.prune_dependency_tree()

for callspec in metafunc._calls:
subname = f"{name}[{callspec.id}]"
yield Function.from_parent(
Expand Down Expand Up @@ -1180,12 +1187,8 @@
return "-".join(self._idlist)


def get_direct_param_fixture_func(request: FixtureRequest) -> Any:
return request.param


# Used for storing pseudo fixturedefs for direct parametrization.
name2pseudofixturedef_key = StashKey[Dict[str, FixtureDef[Any]]]()
name2pseudofixturedef_key = StashKey[Dict[str, IdentityFixtureDef[Any]]]()


@final
Expand Down Expand Up @@ -1232,6 +1235,9 @@
# Result of parametrize().
self._calls: List[CallSpec2] = []

# Whether it's ever been directly parametrized, i.e. with `indirect=False`.
self._has_direct_parametrization = False

def parametrize(
self,
argnames: Union[str, Sequence[str]],
Expand Down Expand Up @@ -1372,26 +1378,22 @@
if node is None:
name2pseudofixturedef = None
else:
default: Dict[str, FixtureDef[Any]] = {}
default: Dict[str, IdentityFixtureDef[Any]] = {}

Check warning on line 1381 in src/_pytest/python.py

View check run for this annotation

Codecov / codecov/patch

src/_pytest/python.py#L1381

Added line #L1381 was not covered by tests
name2pseudofixturedef = node.stash.setdefault(
name2pseudofixturedef_key, default
)
arg_directness = self._resolve_args_directness(argnames, indirect)
for argname in argnames:
if arg_directness[argname] == "indirect":
continue
self._has_direct_parametrization = True
if name2pseudofixturedef is not None and argname in name2pseudofixturedef:
fixturedef = name2pseudofixturedef[argname]
else:
fixturedef = FixtureDef(
fixturemanager=self.definition.session._fixturemanager,
baseid="",
argname=argname,
func=get_direct_param_fixture_func,
scope=scope_,
params=None,
unittest=False,
ids=None,
fixturedef = IdentityFixtureDef(
self.definition.session._fixturemanager,
argname,
scope_,
_ispytest=True,
)
if name2pseudofixturedef is not None:
Expand Down Expand Up @@ -1557,6 +1559,18 @@
pytrace=False,
)

def _update_dependency_tree(self) -> None:
definition = self.definition
assert definition.parent is not None
fm = definition.parent.session._fixturemanager
fixture_closure = fm.getfixtureclosure(

Check warning on line 1566 in src/_pytest/python.py

View check run for this annotation

Codecov / codecov/patch

src/_pytest/python.py#L1563-L1566

Added lines #L1563 - L1566 were not covered by tests
parentnode=definition,
initialnames=definition._fixtureinfo.initialnames,
arg2fixturedefs=definition._fixtureinfo.name2fixturedefs,
ignore_args=_get_direct_parametrize_args(definition),
)
definition._fixtureinfo.names_closure[:] = fixture_closure

Check warning on line 1572 in src/_pytest/python.py

View check run for this annotation

Codecov / codecov/patch

src/_pytest/python.py#L1572

Added line #L1572 was not covered by tests


def _find_parametrized_scope(
argnames: Sequence[str],
Expand Down
Loading