Apply comments, rebase and a few improvements

src/_pytest/fixtures.py

@@ -63,7 +63,6 @@ from _pytest.pathlib import bestrelpath
 from _pytest.scope import _ScopeName
 from _pytest.scope import HIGH_SCOPES
 from _pytest.scope import Scope
-from _pytest.stash import StashKey
 
 
 if TYPE_CHECKING:
@@ -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."""
@@ -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

src/_pytest/python.py

@@ -14,6 +14,7 @@ from functools import partial
 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
@@ -59,7 +60,10 @@ from _pytest.config.argparsing import Parser
 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
@@ -77,6 +81,7 @@ from _pytest.pathlib import parts
 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
@@ -493,13 +498,8 @@ class PyCollector(PyobjMixin, nodes.Collector):
         if not metafunc._calls:
             yield Function.from_parent(self, name=name, fixtureinfo=fixtureinfo)
         else:
-            # Add funcargs() as fixturedefs to fixtureinfo.arg2fixturedefs.
+            # Dynamic direct parametrization may have shadowed some fixtures,
-            fm = self.session._fixturemanager
+            # so make sure we update what the function really needs.
-            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.
             fixtureinfo.prune_dependency_tree()
 
             for callspec in metafunc._calls:
@@ -1116,11 +1116,8 @@ class CallSpec2:
     and stored in item.callspec.
     """
 
-    # arg name -> arg value which will be passed to the parametrized test
+    # arg name -> arg value which will be passed to a fixture or pseudo-fixture
-    # function (direct parameterization).
+    # of the same name. (indirect or direct parametrization respectively)
-    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).
     params: Dict[str, object] = dataclasses.field(default_factory=dict)
     # arg name -> arg index.
     indices: Dict[str, int] = dataclasses.field(default_factory=dict)
@@ -1134,7 +1131,6 @@ class CallSpec2:
     def setmulti(
         self,
         *,
-        valtypes: Mapping[str, "Literal['params', 'funcargs']"],
         argnames: Iterable[str],
         valset: Iterable[object],
         id: str,
@@ -1142,24 +1138,16 @@ class CallSpec2:
         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}")
-            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)
             indices[arg] = param_index
             arg2scope[arg] = scope
         return CallSpec2(
-            funcargs=funcargs,
             params=params,
             indices=indices,
             _arg2scope=arg2scope,
@@ -1178,6 +1166,14 @@ class CallSpec2:
         return "-".join(self._idlist)
 
 
+def get_direct_param_fixture_func(request: FixtureRequest) -> Any:
+    return request.param
+
+
+# Used for storing artificial fixturedefs for direct parametrization.
+name2pseudofixturedef_key = StashKey[Dict[str, FixtureDef[Any]]]()
+
+
 @final
 class Metafunc:
     """Objects passed to the :hook:`pytest_generate_tests` hook.
@@ -1319,8 +1315,6 @@ class Metafunc:
 
         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
@@ -1335,6 +1329,59 @@ class Metafunc:
         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
+        # 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:
+            collector = cast(nodes.Node, self.definition.parent)
+            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]
+        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)
+        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(
+                    fixturemanager=self.definition.session._fixturemanager,
+                    baseid="",
+                    argname=argname,
+                    func=get_direct_param_fixture_func,
+                    scope=scope_,
+                    params=None,
+                    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.
@@ -1344,7 +1391,6 @@ class Metafunc:
                 zip(ids, parametersets)
             ):
                 newcallspec = callspec.setmulti(
-                    valtypes=arg_values_types,
                     argnames=argnames,
                     valset=param_set.values,
                     id=param_id,

testing/example_scripts/issue_519.py

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

testing/python/metafunc.py

@@ -33,11 +33,19 @@ class TestMetafunc:
         # on the funcarg level, so we don't need a full blown
         # initialization.
         class FuncFixtureInfoMock:
-            name2fixturedefs = None
+            name2fixturedefs: Dict[str, List[fixtures.FixtureDef[object]]] = {}
 
             def __init__(self, names):
                 self.names_closure = names
 
+        @dataclasses.dataclass
+        class FixtureManagerMock:
+            config: Any
+
+        @dataclasses.dataclass
+        class SessionMock:
+            _fixturemanager: FixtureManagerMock
+
         @dataclasses.dataclass
         class DefinitionMock(python.FunctionDefinition):
             _nodeid: str
@@ -46,6 +54,8 @@ class TestMetafunc:
         names = getfuncargnames(func)
         fixtureinfo: Any = FuncFixtureInfoMock(names)
         definition: Any = DefinitionMock._create(obj=func, _nodeid="mock::nodeid")
+        definition._fixtureinfo = fixtureinfo
+        definition.session = SessionMock(FixtureManagerMock({}))
         return python.Metafunc(definition, fixtureinfo, config, _ispytest=True)
 
     def test_no_funcargs(self) -> None:
@@ -98,7 +108,7 @@ class TestMetafunc:
         # When the input is an iterator, only len(args) are taken,
         # so the bad Exc isn't reached.
         metafunc.parametrize("x", [1, 2], ids=gen())  # type: ignore[arg-type]
-        assert [(x.funcargs, x.id) for x in metafunc._calls] == [
+        assert [(x.params, x.id) for x in metafunc._calls] == [
             ({"x": 1}, "0"),
             ({"x": 2}, "2"),
         ]
@@ -712,8 +722,6 @@ class TestMetafunc:
         metafunc.parametrize("x", [1], indirect=True)
         metafunc.parametrize("y", [2, 3], indirect=True)
         assert len(metafunc._calls) == 2
-        assert metafunc._calls[0].funcargs == {}
-        assert metafunc._calls[1].funcargs == {}
         assert metafunc._calls[0].params == dict(x=1, y=2)
         assert metafunc._calls[1].params == dict(x=1, y=3)
 
@@ -725,8 +733,10 @@ class TestMetafunc:
 
         metafunc = self.Metafunc(func)
         metafunc.parametrize("x, y", [("a", "b")], indirect=["x"])
-        assert metafunc._calls[0].funcargs == dict(y="b")
+        assert metafunc._calls[0].params == dict(x="a", y="b")
-        assert metafunc._calls[0].params == dict(x="a")
+        # Since `y` is a direct parameter, its pseudo-fixture would
+        # be registered.
+        assert list(metafunc._arg2fixturedefs.keys()) == ["y"]
 
     def test_parametrize_indirect_list_all(self) -> None:
         """#714"""
@@ -736,8 +746,8 @@ class TestMetafunc:
 
         metafunc = self.Metafunc(func)
         metafunc.parametrize("x, y", [("a", "b")], indirect=["x", "y"])
-        assert metafunc._calls[0].funcargs == {}
         assert metafunc._calls[0].params == dict(x="a", y="b")
+        assert list(metafunc._arg2fixturedefs.keys()) == []
 
     def test_parametrize_indirect_list_empty(self) -> None:
         """#714"""
@@ -747,8 +757,8 @@ class TestMetafunc:
 
         metafunc = self.Metafunc(func)
         metafunc.parametrize("x, y", [("a", "b")], indirect=[])
-        assert metafunc._calls[0].funcargs == dict(x="a", y="b")
+        assert metafunc._calls[0].params == dict(x="a", y="b")
-        assert metafunc._calls[0].params == {}
+        assert list(metafunc._arg2fixturedefs.keys()) == ["x", "y"]
 
     def test_parametrize_indirect_wrong_type(self) -> None:
         def func(x, y):
@@ -942,9 +952,9 @@ class TestMetafunc:
         metafunc = self.Metafunc(lambda x: None)
         metafunc.parametrize("x", [1, 2])
         assert len(metafunc._calls) == 2
-        assert metafunc._calls[0].funcargs == dict(x=1)
+        assert metafunc._calls[0].params == dict(x=1)
         assert metafunc._calls[0].id == "1"
-        assert metafunc._calls[1].funcargs == dict(x=2)
+        assert metafunc._calls[1].params == dict(x=2)
         assert metafunc._calls[1].id == "2"
 
     def test_parametrize_onearg_indirect(self) -> None:
@@ -959,11 +969,85 @@ class TestMetafunc:
         metafunc = self.Metafunc(lambda x, y: None)
         metafunc.parametrize(("x", "y"), [(1, 2), (3, 4)])
         assert len(metafunc._calls) == 2
-        assert metafunc._calls[0].funcargs == dict(x=1, y=2)
+        assert metafunc._calls[0].params == dict(x=1, y=2)
         assert metafunc._calls[0].id == "1-2"
-        assert metafunc._calls[1].funcargs == dict(x=3, y=4)
+        assert metafunc._calls[1].params == dict(x=3, y=4)
         assert metafunc._calls[1].id == "3-4"
 
+    @pytest.mark.xfail(reason="Will pass upon merging PR#")
+    def test_parametrize_with_duplicate_values(self) -> None:
+        metafunc = self.Metafunc(lambda x, y: None)
+        metafunc.parametrize(("x", "y"), [(1, 2), (3, 4), (1, 5), (2, 2)])
+        assert len(metafunc._calls) == 4
+        assert metafunc._calls[0].indices == dict(x=0, y=0)
+        assert metafunc._calls[1].indices == dict(x=1, y=1)
+        assert metafunc._calls[2].indices == dict(x=0, y=2)
+        assert metafunc._calls[3].indices == dict(x=2, y=0)
+
+    def test_high_scoped_parametrize_reordering(self, pytester: Pytester) -> None:
+        pytester.makepyfile(
+            """
+            import pytest
+
+            @pytest.mark.parametrize("arg2", [3, 4])
+            @pytest.mark.parametrize("arg1", [0, 1, 2], scope='module')
+            def test1(arg1, arg2):
+                pass
+
+            def test2():
+                pass
+
+            @pytest.mark.parametrize("arg1", [0, 1, 2], scope='module')
+            def test3(arg1):
+                pass
+        """
+        )
+        result = pytester.runpytest("--collect-only")
+        result.stdout.re_match_lines(
+            [
+                r"  <Function test1\[0-3\]>",
+                r"  <Function test1\[0-4\]>",
+                r"  <Function test3\[0\]>",
+                r"  <Function test1\[1-3\]>",
+                r"  <Function test1\[1-4\]>",
+                r"  <Function test3\[1\]>",
+                r"  <Function test1\[2-3\]>",
+                r"  <Function test1\[2-4\]>",
+                r"  <Function test3\[2\]>",
+                r"  <Function test2>",
+            ]
+        )
+
+    @pytest.mark.xfail(reason="Will pass upon merging PR#")
+    def test_high_scoped_parametrize_with_duplicate_values_reordering(
+        self, pytester: Pytester
+    ) -> None:
+        pytester.makepyfile(
+            """
+            import pytest
+
+            @pytest.fixture(scope='module')
+            def fixture1(request):
+                pass
+
+            @pytest.fixture(scope='module')
+            def fixture2(request):
+                pass
+
+            @pytest.mark.parametrize("fixture1, fixture2", [("a", 0), ("b", 1), ("a", 2)], indirect=True)
+            def test(fixture1, fixture2):
+                pass
+        """
+        )
+        result = pytester.runpytest("--collect-only")
+        result.stdout.re_match_lines(
+            [
+                r"  <Function test\[a-0\]>",
+                r"  <Function test\[a-2\]>",
+                r"  <Function test\[b-1\]>",
+            ]
+        )
+
     def test_parametrize_multiple_times(self, pytester: Pytester) -> None:
         pytester.makepyfile(
             """
@@ -1503,6 +1587,33 @@ class TestMetafuncFunctional:
         result = pytester.runpytest()
         assert result.ret == 0
 
+    def test_parametrize_module_level_test_with_class_scope(
+        self, pytester: Pytester
+    ) -> None:
+        pytester.makepyfile(
+            """
+            import pytest
+
+            @pytest.fixture
+            def item(request):
+                return request._pyfuncitem
+
+            fixturedef = None
+
+            @pytest.mark.parametrize("x", [0, 1], scope="class")
+            def test_1(item, x):
+                global fixturedef
+                fixturedef = item._fixtureinfo.name2fixturedefs['x'][-1]
+
+            @pytest.mark.parametrize("x", [1, 2], scope="module")
+            def test_2(item, x):
+                global fixturedef
+                assert fixturedef == item._fixtureinfo.name2fixturedefs['x'][-1]
+        """
+        )
+        result = pytester.runpytest()
+        assert result.ret == 0
+
 
 class TestMetafuncFunctionalAuto:
     """Tests related to automatically find out the correct scope for