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.

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

Move fixtures.py::add_funcarg_pseudo_fixture_def to Metafunc.parametrize #11220

Merged
merged 5 commits into from
Aug 9, 2023
+167 −130
Merged

Move fixtures.py::add_funcarg_pseudo_fixture_def to Metafunc.parametrize #11220

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
96be57a
Apply comments, rebase and a few improvements
sadra-barikbin Jul 28, 2023
40b2c09
A few improvements
sadra-barikbin Aug 1, 2023
8b59725
Apply comments
sadra-barikbin Aug 2, 2023
2853331
Merge branch 'main' into Improvement-move-add-pseudo-funcarg-to-metaf…
sadra-barikbin Aug 9, 2023
c7f4f91
Update src/_pytest/python.py
bluetech Aug 9, 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
88 changes: 0 additions & 88 deletions src/_pytest/fixtures.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,6 @@
from _pytest.scope import _ScopeName
from _pytest.scope import HIGH_SCOPES
from _pytest.scope import Scope
from _pytest.stash import StashKey


if TYPE_CHECKING:
Expand Down Expand Up @@ -147,89 +146,6 @@ def get_scope_node(
assert_never(scope)


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


def add_funcarg_pseudo_fixture_def(
collector: nodes.Collector, metafunc: "Metafunc", fixturemanager: "FixtureManager"
) -> None:
import _pytest.python

# This function will transform all collected calls to functions
# if they use direct funcargs (i.e. direct parametrization)
# because we want later test execution to be able to rely on
# an existing FixtureDef structure for all arguments.
# XXX we can probably avoid this algorithm if we modify CallSpec2
# to directly care for creating the fixturedefs within its methods.
if not metafunc._calls[0].funcargs:
# This function call does not have direct parametrization.
return
# Collect funcargs of all callspecs into a list of values.
arg2params: Dict[str, List[object]] = {}
arg2scope: Dict[str, Scope] = {}
for callspec in metafunc._calls:
for argname, argvalue in callspec.funcargs.items():
assert argname not in callspec.params
callspec.params[argname] = argvalue
arg2params_list = arg2params.setdefault(argname, [])
callspec.indices[argname] = len(arg2params_list)
arg2params_list.append(argvalue)
if argname not in arg2scope:
scope = callspec._arg2scope.get(argname, Scope.Function)
arg2scope[argname] = scope
callspec.funcargs.clear()

# Register artificial FixtureDef's so that later at test execution
# time we can rely on a proper FixtureDef to exist for fixture setup.
arg2fixturedefs = metafunc._arg2fixturedefs
for argname, valuelist in arg2params.items():
# If we have a scope that is higher than function, we need
# to make sure we only ever create an according fixturedef on
# a per-scope basis. We thus store and cache the fixturedef on the
# node related to the scope.
scope = arg2scope[argname]
node = None
if scope is not Scope.Function:
node = get_scope_node(collector, scope)
if node is None:
# If used class scope and there is no class, use module-level
# collector (for now).
if scope is Scope.Class:
assert isinstance(collector, _pytest.python.Module)
node = collector
# If used package scope and there is no package, use session
# (for now).
elif scope is Scope.Package:
node = collector.session
else:
assert False, f"Unhandled missing scope: {scope}"
if node is None:
name2pseudofixturedef = None
else:
default: Dict[str, FixtureDef[Any]] = {}
name2pseudofixturedef = node.stash.setdefault(
name2pseudofixturedef_key, default
)
if name2pseudofixturedef is not None and argname in name2pseudofixturedef:
arg2fixturedefs[argname] = [name2pseudofixturedef[argname]]
else:
fixturedef = FixtureDef(
fixturemanager=fixturemanager,
baseid="",
argname=argname,
func=get_direct_param_fixture_func,
scope=arg2scope[argname],
params=valuelist,
unittest=False,
ids=None,
_ispytest=True,
)
arg2fixturedefs[argname] = [fixturedef]
if name2pseudofixturedef is not None:
name2pseudofixturedef[argname] = fixturedef


def getfixturemarker(obj: object) -> Optional["FixtureFunctionMarker"]:
"""Return fixturemarker or None if it doesn't exist or raised
exceptions."""
Expand Down Expand Up @@ -355,10 +271,6 @@ def reorder_items_atscope(
return items_done


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


@dataclasses.dataclass(frozen=True)
class FuncFixtureInfo:
"""Fixture-related information for a fixture-requesting item (e.g. test
Expand Down
98 changes: 72 additions & 26 deletions src/_pytest/python.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
from pathlib import Path
from typing import Any
from typing import Callable
from typing import cast
from typing import Dict
from typing import final
from typing import Generator
Expand Down Expand Up @@ -59,7 +60,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 FuncFixtureInfo
from _pytest.fixtures import get_scope_node
from _pytest.main import Session
from _pytest.mark import MARK_GEN
from _pytest.mark import ParameterSet
Expand All @@ -77,6 +81,7 @@
from _pytest.pathlib import visit
from _pytest.scope import _ScopeName
from _pytest.scope import Scope
from _pytest.stash import StashKey
from _pytest.warning_types import PytestCollectionWarning
from _pytest.warning_types import PytestReturnNotNoneWarning
from _pytest.warning_types import PytestUnhandledCoroutineWarning
Expand Down Expand Up @@ -493,13 +498,8 @@ def _genfunctions(self, name: str, funcobj) -> Iterator["Function"]:
if not metafunc._calls:
yield Function.from_parent(self, name=name, fixtureinfo=fixtureinfo)
else:
# Add funcargs() as fixturedefs to fixtureinfo.arg2fixturedefs.
fm = self.session._fixturemanager
fixtures.add_funcarg_pseudo_fixture_def(self, metafunc, fm)

# Add_funcarg_pseudo_fixture_def may have shadowed some fixtures
# with direct parametrization, so make sure we update what the
# function really needs.
# Dynamic direct parametrization may have shadowed some fixtures,
sadra-barikbin marked this conversation as resolved.
Show resolved Hide resolved
# so make sure we update what the function really needs.
fixtureinfo.prune_dependency_tree()

for callspec in metafunc._calls:
Expand Down Expand Up @@ -1116,11 +1116,8 @@ class CallSpec2:
and stored in item.callspec.
"""

# arg name -> arg value which will be passed to the parametrized test
# function (direct parameterization).
funcargs: Dict[str, object] = dataclasses.field(default_factory=dict)
# arg name -> arg value which will be passed to a fixture of the same name
# (indirect parametrization).
# arg name -> arg value which will be passed to a fixture or pseudo-fixture
# of the same name. (indirect or direct parametrization respectively)
params: Dict[str, object] = dataclasses.field(default_factory=dict)
# arg name -> arg index.
indices: Dict[str, int] = dataclasses.field(default_factory=dict)
Expand All @@ -1134,32 +1131,23 @@ class CallSpec2:
def setmulti(
self,
*,
valtypes: Mapping[str, "Literal['params', 'funcargs']"],
sadra-barikbin marked this conversation as resolved.
Show resolved Hide resolved
argnames: Iterable[str],
valset: Iterable[object],
id: str,
marks: Iterable[Union[Mark, MarkDecorator]],
scope: Scope,
param_index: int,
) -> "CallSpec2":
funcargs = self.funcargs.copy()
params = self.params.copy()
indices = self.indices.copy()
arg2scope = self._arg2scope.copy()
for arg, val in zip(argnames, valset):
if arg in params or arg in funcargs:
if arg in params:
raise ValueError(f"duplicate {arg!r}")
bluetech marked this conversation as resolved.
Show resolved Hide resolved
valtype_for_arg = valtypes[arg]
if valtype_for_arg == "params":
params[arg] = val
elif valtype_for_arg == "funcargs":
funcargs[arg] = val
else:
assert_never(valtype_for_arg)
params[arg] = val
indices[arg] = param_index
arg2scope[arg] = scope
return CallSpec2(
funcargs=funcargs,
params=params,
indices=indices,
_arg2scope=arg2scope,
Expand All @@ -1178,6 +1166,14 @@ def id(self) -> str:
return "-".join(self._idlist)


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


# Used for storing artificial fixturedefs for direct parametrization.
sadra-barikbin marked this conversation as resolved.
Show resolved Hide resolved
name2pseudofixturedef_key = StashKey[Dict[str, FixtureDef[Any]]]()


@final
class Metafunc:
"""Objects passed to the :hook:`pytest_generate_tests` hook.
Expand Down Expand Up @@ -1319,8 +1315,6 @@ def parametrize(

self._validate_if_using_arg_names(argnames, indirect)

arg_values_types = self._resolve_arg_value_types(argnames, indirect)

# Use any already (possibly) generated ids with parametrize Marks.
if _param_mark and _param_mark._param_ids_from:
generated_ids = _param_mark._param_ids_from._param_ids_generated
Expand All @@ -1335,6 +1329,59 @@ def parametrize(
if _param_mark and _param_mark._param_ids_from and generated_ids is None:
object.__setattr__(_param_mark._param_ids_from, "_param_ids_generated", ids)

# Add funcargs as fixturedefs to fixtureinfo.arg2fixturedefs by registering
# artificial FixtureDef's so that later at test execution time we can rely
sadra-barikbin marked this conversation as resolved.
Show resolved Hide resolved
# on a proper FixtureDef to exist for fixture setup.
arg2fixturedefs = self._arg2fixturedefs
node = None
# If we have a scope that is higher than function, we need
# to make sure we only ever create an according fixturedef on
# a per-scope basis. We thus store and cache the fixturedef on the
# node related to the scope.
if scope_ is not Scope.Function:
bluetech marked this conversation as resolved.
Show resolved Hide resolved
collector = cast(nodes.Node, self.definition.parent)
sadra-barikbin marked this conversation as resolved.
Show resolved Hide resolved
node = get_scope_node(collector, scope_)
if node is None:
# If used class scope and there is no class, use module-level
# collector (for now).
if scope_ is Scope.Class:
assert isinstance(collector, _pytest.python.Module)
node = collector
# If used package scope and there is no package, use session
# (for now).
elif scope_ is Scope.Package:
node = collector.session
else:
assert_never(scope_) # type: ignore[arg-type]
sadra-barikbin marked this conversation as resolved.
Show resolved Hide resolved
if node is None:
name2pseudofixturedef = None
else:
default: Dict[str, FixtureDef[Any]] = {}
name2pseudofixturedef = node.stash.setdefault(
name2pseudofixturedef_key, default
)
arg_values_types = self._resolve_arg_value_types(argnames, indirect)
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 it would be helpful for clarity if (in separate PR) we change _resolve_arg_value_types like so:

  • Rename to _resolve_arg_directness - "arg value type" is pretty generic/non-specific and doesn't say much.
  • Change the Literal return value from Literal['params', 'funcargs'] to Literal['direct', 'indirect'].

Then the check becomes if arg_directness[argname] == "direct": continue which is pretty self-explanatory I think.

Note: we can do it later, you don't have to do it.

for argname in argnames:
if arg_values_types[argname] == "params":
continue
if name2pseudofixturedef is not None and argname in name2pseudofixturedef:
fixturedef = name2pseudofixturedef[argname]
else:
fixturedef = FixtureDef(
Copy link
Member

Choose a reason for hiding this comment

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

this particular usage indicates to me that we may want to investigate having a "constant" definitions that could be used to represent params both from parametrize, and also declaring fixtures that repressent sets of values

details are for a followup that may want to also integrate pytest-lazyfixtures

Copy link
Contributor Author

Choose a reason for hiding this comment

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

By "constant" you mean stateless? A fixturedef that just returns its input?

Copy link
Member

Choose a reason for hiding this comment

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

Exact

fixturemanager=self.definition.session._fixturemanager,
baseid="",
argname=argname,
func=get_direct_param_fixture_func,
scope=scope_,
params=None,
Copy link
Member

Choose a reason for hiding this comment

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

This is now params=None, where before it was params=valuelist. This made me confused somewhat.

My mental model for before was the basically desugar this:

@pytest.mark.parametrize("x", [1])
def test_it(x): pass

to this:

@pytest.fixture(params=[1])
def x(request):
    return request.param

def test_it(x): pass

but with params=None that makes less sense now. And indeed, if we change to params=None in the current code, all tests still pass, i.e. it wasn't needed even before.

I guess then that the correct mental model for the desugaring is indirect parametrization of the x fixture? I.e. this:

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

@pytest.mark.parametrize("x", [1], indirect=["x"])
def test_it(x): pass

Ran out of time to look into it today but will appreciate your insights :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right. It is the test that holds param values in its callspec and the fixtures that it depends on, use them at test execution time. The only usage of fixturedef.params is in fixtures.py::pytest_generate_tests that the directly parametrized fixture hand the params to the test that uses it. By directly parametrized fixture, I mean this:

@pytest.fixture(params=[1])
def x(request):
    return request.param

unittest=False,
ids=None,
_ispytest=True,
)
if name2pseudofixturedef is not None:
name2pseudofixturedef[argname] = fixturedef
arg2fixturedefs[argname] = [fixturedef]

# Create the new calls: if we are parametrize() multiple times (by applying the decorator
# more than once) then we accumulate those calls generating the cartesian product
# of all calls.
Expand All @@ -1344,7 +1391,6 @@ def parametrize(
zip(ids, parametersets)
):
newcallspec = callspec.setmulti(
valtypes=arg_values_types,
argnames=argnames,
valset=param_set.values,
id=param_id,
Expand Down
4 changes: 2 additions & 2 deletions testing/example_scripts/issue_519.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,13 +22,13 @@ def checked_order():
assert order == [
("issue_519.py", "fix1", "arg1v1"),
("test_one[arg1v1-arg2v1]", "fix2", "arg2v1"),
("test_two[arg1v1-arg2v1]", "fix2", "arg2v1"),
Copy link
Member

Choose a reason for hiding this comment

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

As I said in https://github.com/pytest-dev/pytest/pull/11231/files#r1278270549 IMO this change makes sense (though @RonnyPfannschmidt may still disagree).

But I would like to understand what exactly in this PR is causing this change? @sadra-barikbin do you think you can explain it?

Copy link
Member

Choose a reason for hiding this comment

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

i left another comment, i think we can choose to opt for the new ordering if we ensure to add the tools i mentioned there eventually

Copy link
Contributor Author

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

Choose a reason for hiding this comment

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

Current ordering stems from the way add_funcarg_pseudo_fixture_def assigns index to params. It assigns i to the index of all of the params in ith callspec of the metafunc. In our example, test_one and test_two, each have 4 callspecs and two params (arg1 and arg2). We assign index 0 to arg1 and arg2 of test_one[arg1v1,arg2v1] and also to those of test_two[arg1v1,arg2v1]. We assign index 1 to arg1 and arg2 of test_one[arg1v1,arg2v2] and also to those of test_two[arg1v1,arg2v2] and so on. As a result, ith callspecs of the two metafuncs would have common fixture keys. In our example, kth test_one pulls kth test_two towards itself, so we would have a one-two-one-two... pattern. Also test_one (also test_two ones) items would have no common fixture key because their index of arg1 and arg2 is 0,1,2 and 3.

The new way assigns i to the index of param j in all callspecs that use ith value of j. So the decision is made on a per-param basis. In our example, we would assign index 0 to arg1 of the items that use arg1v1 and index 1 to arg1 of the items that use arg1v2 and so on. As a result, if two callspecs have the ith value of the param j, they have a common fixture key.

The new way is improved in determining what ith value of the param is when there are multiple parametersets in #11257 .

Copy link
Member

Choose a reason for hiding this comment

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

After reading your comment and looking more closely at the issue_519.py file, I think I understood some things:

First, I'm really not used to the pytest_generate_tests hook, so I rewrote the test using pytest.mark.parametrize. I also changed some of the names to make things clearer to me:

Rewritten issue_519.py 
import pprint
from typing import List
from typing import Tuple

import pytest


@pytest.fixture(scope="session")
def checked_order():
    order: List[Tuple[str, str, str]] = []

    yield order
    pprint.pprint(order)


@pytest.fixture(scope="module")
def fixmod(request, a, checked_order):
    checked_order.append((request.node.name, "fixmod", a))
    yield "fixmod-" + a


@pytest.fixture(scope="function")
def fixfun(request, fixmod, b, checked_order):
    checked_order.append((request.node.name, "fixfun", b))
    yield "fixfun-" + b + fixmod


@pytest.mark.parametrize("b", ["b1", "b2"], scope="function")
@pytest.mark.parametrize("a", ["a1", "a2"], scope="module")
def test_one(fixfun, request):
    print()
    # print(request._pyfuncitem.nodeid, request._pyfuncitem.callspec)


@pytest.mark.parametrize("b", ["b1", "b2"], scope="function")
@pytest.mark.parametrize("a", ["a1", "a2"], scope="module")
def test_two(fixfun, request):
    print()
    # print(request._pyfuncitem.nodeid, request._pyfuncitem.callspec)

This made me realize that the apparent niceness of the previous ordering, which orders test_one[arg1v1-arg2v1] next to test_two[arg1v1-arg2v1], thus saving a setup, is pretty arbitrary. This can be shown by making the values of the function-scoped parameter (arg2/b) different in test_one and test_two:

-@pytest.mark.parametrize("b", ["b1", "b2"], scope="function")
+@pytest.mark.parametrize("b", ["b11", "b12"], scope="function")
 @pytest.mark.parametrize("a", ["a1", "a2"], scope="module")
 def test_one(fixfun, request):
     print()
     # print(request._pyfuncitem.nodeid, request._pyfuncitem.callspec)
 
 
-@pytest.mark.parametrize("b", ["b1", "b2"], scope="function")
+@pytest.mark.parametrize("b", ["b21", "b22"], scope="function")
 @pytest.mark.parametrize("a", ["a1", "a2"], scope="module")
 def test_two(fixfun, request):
     print()

The ordering stays the same, which now doesn't make sense.

I also figured that the entire param_index thing was much more useful/natural in the pre-parametrize times, when only pytest_generate_tests/metafunc was used for parametrizing. With pytest_generate_tests you often use the same parameter set for different tests, as is done in the issue_519.py file. But with parametrize, which is mostly done for a single test-function only, it is much less common, I think.

("test_one[arg1v1-arg2v2]", "fix2", "arg2v2"),
("test_two[arg1v1-arg2v1]", "fix2", "arg2v1"),
("test_two[arg1v1-arg2v2]", "fix2", "arg2v2"),
("issue_519.py", "fix1", "arg1v2"),
("test_one[arg1v2-arg2v1]", "fix2", "arg2v1"),
("test_two[arg1v2-arg2v1]", "fix2", "arg2v1"),
("test_one[arg1v2-arg2v2]", "fix2", "arg2v2"),
("test_two[arg1v2-arg2v1]", "fix2", "arg2v1"),
("test_two[arg1v2-arg2v2]", "fix2", "arg2v2"),
]

Expand Down
Loading