Merge 26ae39866a into 04cf8db1d7

AUTHORS

@@ -322,6 +322,7 @@ Thomas Grainger
 Thomas Hisch
 Tim Hoffmann
 Tim Strazny
+Tobias Deiminger
 Tom Dalton
 Tom Viner
 Tomáš Gavenčiak

changelog/8914.bugfix.rst

@@ -0,0 +1,4 @@
+If fixtures had been indirectly parameterized via test function, e.g. using the
+``@pytest.mark.parametrize(indirect=True)`` marker, reordering of tests for the least possible fixture setup/teardown
+cycles did not work. Optimized test groups can now be determined either explicitly by passing parameter ids, or
+implicitly if the parameter value is hashable.

src/_pytest/fixtures.py

@@ -5,6 +5,7 @@ import sys
 import warnings
 from collections import defaultdict
 from collections import deque
+from collections.abc import Hashable
 from contextlib import suppress
 from pathlib import Path
 from types import TracebackType
@@ -248,21 +249,21 @@ def get_parametrized_fixture_keys(item: nodes.Item, scope: Scope) -> Iterator[_K
         pass
     else:
         cs: CallSpec2 = callspec
-        # cs.indices.items() is random order of argnames.  Need to
+        # cs.param_keys.items() is random order of argnames.  Need to
         # 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, param_key in sorted(cs.param_keys.items()):
             if cs._arg2scope[argname] != scope:
                 continue
             if scope is Scope.Session:
-                key: _Key = (argname, param_index)
+                key: _Key = (argname, param_key)
             elif scope is Scope.Package:
-                key = (argname, param_index, item.path.parent)
+                key = (argname, param_key, item.path.parent)
             elif scope is Scope.Module:
-                key = (argname, param_index, item.path)
+                key = (argname, param_key, item.path)
             elif scope is Scope.Class:
                 item_cls = item.cls  # type: ignore[attr-defined]
-                key = (argname, param_index, item.path, item_cls)
+                key = (argname, param_key, item.path, item_cls)
             else:
                 assert_never(scope)
             yield key
@@ -601,6 +602,7 @@ class FixtureRequest:
         except (AttributeError, ValueError):
             param = NOTSET
             param_index = 0
+            param_key = ""
             has_params = fixturedef.params is not None
             fixtures_not_supported = getattr(funcitem, "nofuncargs", False)
             if has_params and fixtures_not_supported:
@@ -640,13 +642,14 @@ class FixtureRequest:
                 fail(msg, pytrace=False)
         else:
             param_index = funcitem.callspec.indices[argname]
+            param_key = funcitem.callspec.param_keys[argname]
             # If a parametrize invocation set a scope it will override
             # the static scope defined with the fixture function.
             with suppress(KeyError):
                 scope = funcitem.callspec._arg2scope[argname]
 
         subrequest = SubRequest(
-            self, scope, param, param_index, fixturedef, _ispytest=True
+            self, scope, param, param_index, param_key, fixturedef, _ispytest=True
         )
 
         # Check if a higher-level scoped fixture accesses a lower level one.
@@ -731,6 +734,7 @@ class SubRequest(FixtureRequest):
         scope: Scope,
         param: Any,
         param_index: int,
+        param_key: Hashable,
         fixturedef: "FixtureDef[object]",
         *,
         _ispytest: bool = False,
@@ -741,6 +745,7 @@ class SubRequest(FixtureRequest):
         if param is not NOTSET:
             self.param = param
         self.param_index = param_index
+        self.param_key = param_key
         self._scope = scope
         self._fixturedef = fixturedef
         self._pyfuncitem = request._pyfuncitem
@@ -1012,10 +1017,10 @@ class FixtureDef(Generic[FixtureValue]):
 
         my_cache_key = self.cache_key(request)
         if self.cached_result is not None:
-            # note: comparison with `==` can fail (or be expensive) for e.g.
-            # numpy arrays (#6497).
             cache_key = self.cached_result[1]
-            if my_cache_key is cache_key:
+            # Note: Comparison with `==` may be implemented as (possibly expensive)
+            # deep by-value comparison. See _pytest.python.SafeHashWrapper for details.
+            if my_cache_key == cache_key:
                 if self.cached_result[2] is not None:
                     _, val, tb = self.cached_result[2]
                     raise val.with_traceback(tb)
@@ -1032,7 +1037,7 @@ class FixtureDef(Generic[FixtureValue]):
         return result
 
     def cache_key(self, request: SubRequest) -> object:
-        return request.param_index if not hasattr(request, "param") else request.param
+        return request.param_key
 
     def __repr__(self) -> str:
         return "<FixtureDef argname={!r} scope={!r} baseid={!r}>".format(

src/_pytest/python.py

@@ -9,6 +9,7 @@ import types
 import warnings
 from collections import Counter
 from collections import defaultdict
+from collections.abc import Hashable
 from functools import partial
 from pathlib import Path
 from typing import Any
@@ -927,6 +928,38 @@ def hasnew(obj: object) -> bool:
     return False
 
 
+@attr.s(auto_attribs=True, eq=False, slots=True)
+class SafeHashWrapper:
+    """Wrap an arbitrary type so that it becomes comparable with guaranteed constraints.
+
+    Constraints:
+    - SafeHashWrapper(a) == SafeHashWrapper(b) will never raise an exception
+    - SafeHashWrapper(a) == SafeHashWrapper(b) will always return bool
+      (oddly some inner types wouldn't, e.g. numpy.array([0]) == numpy.array([0]) returns List)
+    - SafeHashWrapper(a) is always hashable
+    - if SafeHashWrapper(a) == SafeHashWrapper(b),
+      then hash(SafeHashWrapper(a)) == hash(SafeHashWrapper(b))
+
+    It works by falling back to identity compare in case constraints couldn't be met otherwise.
+    """
+
+    obj: Any
+
+    def __eq__(self, other: object) -> bool:
+        if isinstance(self.obj, Hashable) and isinstance(other, Hashable):
+            try:
+                res = self.obj == other
+                return bool(res)
+            except Exception:
+                pass
+        return self.obj is other
+
+    def __hash__(self) -> int:
+        if isinstance(self.obj, Hashable):
+            return hash(self.obj)
+        return hash(id(self.obj))
+
+
 @final
 @attr.s(frozen=True, auto_attribs=True, slots=True)
 class IdMaker:
@@ -974,6 +1007,27 @@ class IdMaker:
                     id_suffixes[id] += 1
         return resolved_ids
 
+    def make_parameter_keys(self) -> Iterable[Dict[str, Hashable]]:
+        """Make hashable parameter keys for each ParameterSet.
+
+        For each ParameterSet, generates a dict mapping each parameter to its key.
+
+        This key will be considered (along with the arguments name) to determine
+        if parameters are the same in the sense of reorder_items() and the
+        FixtureDef cache. The key is guaranteed to be hashable and comparable.
+        It's not intended for printing and therefore not ASCII escaped.
+        """
+        for idx, parameterset in enumerate(self.parametersets):
+            if parameterset.id is not None:
+                # ID provided directly - pytest.param(..., id="...")
+                yield {argname: parameterset.id for argname in self.argnames}
+            elif self.ids and idx < len(self.ids) and self.ids[idx] is not None:
+                # ID provided in the IDs list - parametrize(..., ids=[...]).
+                yield {argname: self.ids[idx] for argname in self.argnames}
+            else:
+                # ID not provided - generate it.
+                yield self._parameter_keys_from_parameterset(parameterset, idx)
+
     def _resolve_ids(self) -> Iterable[str]:
         """Resolve IDs for all ParameterSets (may contain duplicates)."""
         for idx, parameterset in enumerate(self.parametersets):
@@ -992,6 +1046,20 @@ class IdMaker:
                     for val, argname in zip(parameterset.values, self.argnames)
                 )
 
+    def _parameter_keys_from_parameterset(
+        self, parameterset: ParameterSet, idx: int
+    ) -> Dict[str, Hashable]:
+        """Make parameter keys for all parameters in a ParameterSet."""
+        param_keys: Dict[str, Hashable] = {}
+        for val, argname in zip(parameterset.values, self.argnames):
+            evaluated_id = self._idval_from_function(val, argname, idx)
+            if evaluated_id is not None:
+                param_keys[argname] = evaluated_id
+            else:
+                # Wrapping ensures val becomes comparable and hashable.
+                param_keys[argname] = SafeHashWrapper(val)
+        return param_keys
+
     def _idval(self, val: object, argname: str, idx: int) -> str:
         """Make an ID for a parameter in a ParameterSet."""
         idval = self._idval_from_function(val, argname, idx)
@@ -1076,6 +1144,8 @@ class CallSpec2:
     # arg name -> arg value which will be passed to a fixture of the same name
     # (indirect parametrization).
     params: Dict[str, object] = attr.Factory(dict)
+    # arg name -> parameter key.
+    param_keys: Dict[str, Hashable] = attr.Factory(dict)
     # arg name -> arg index.
     indices: Dict[str, int] = attr.Factory(dict)
     # Used for sorting parametrized resources.
@@ -1095,9 +1165,12 @@ class CallSpec2:
         marks: Iterable[Union[Mark, MarkDecorator]],
         scope: Scope,
         param_index: int,
+        param_set_keys: Dict[str, Hashable],
     ) -> "CallSpec2":
+        """Extend an existing callspec with new parameters during multiple invocation of Metafunc.parametrize."""
         funcargs = self.funcargs.copy()
         params = self.params.copy()
+        param_keys = self.param_keys.copy()
         indices = self.indices.copy()
         arg2scope = self._arg2scope.copy()
         for arg, val in zip(argnames, valset):
@@ -1111,10 +1184,12 @@ class CallSpec2:
             else:
                 assert_never(valtype_for_arg)
             indices[arg] = param_index
+            param_keys[arg] = param_set_keys[arg]
             arg2scope[arg] = scope
         return CallSpec2(
             funcargs=funcargs,
             params=params,
+            param_keys=param_keys,
             arg2scope=arg2scope,
             indices=indices,
             idlist=[*self._idlist, id],
@@ -1284,7 +1359,7 @@ class Metafunc:
             if generated_ids is not None:
                 ids = generated_ids
 
-        ids = self._resolve_parameter_set_ids(
+        ids, parameters_keys = self._resolve_parameter_set_ids(
             argnames, ids, parametersets, nodeid=self.definition.nodeid
         )
 
@@ -1297,17 +1372,18 @@ class Metafunc:
         # of all calls.
         newcalls = []
         for callspec in self._calls or [CallSpec2()]:
-            for param_index, (param_id, param_set) in enumerate(
+            for param_index, (param_id, parameterset, param_set_keys) in enumerate(
-                zip(ids, parametersets)
+                zip(ids, parametersets, parameters_keys)
             ):
                 newcallspec = callspec.setmulti(
                     valtypes=arg_values_types,
                     argnames=argnames,
-                    valset=param_set.values,
+                    valset=parameterset.values,
                     id=param_id,
-                    marks=param_set.marks,
+                    marks=parameterset.marks,
                     scope=scope_,
                     param_index=param_index,
+                    param_set_keys=param_set_keys,
                 )
                 newcalls.append(newcallspec)
         self._calls = newcalls
@@ -1323,9 +1399,8 @@ class Metafunc:
         ],
         parametersets: Sequence[ParameterSet],
         nodeid: str,
-    ) -> List[str]:
+    ) -> Tuple[List[str], List[Dict[str, Hashable]]]:
         """Resolve the actual ids for the given parameter sets.
-
         :param argnames:
             Argument names passed to ``parametrize()``.
         :param ids:
@@ -1337,7 +1412,9 @@ class Metafunc:
             The nodeid of the definition item that generated this
             parametrization.
         :returns:
-            List with ids for each parameter set given.
+            Tuple, where
+            1st entry is a list with ids for each parameter set given used to name test invocations, and
+            2nd entry is a list with keys to support distinction of parameters to support fixture reuse.
         """
         if ids is None:
             idfn = None
@@ -1351,7 +1428,9 @@ class Metafunc:
         id_maker = IdMaker(
             argnames, parametersets, idfn, ids_, self.config, nodeid=nodeid
         )
-        return id_maker.make_unique_parameterset_ids()
+        return id_maker.make_unique_parameterset_ids(), list(
+            id_maker.make_parameter_keys()
+        )
 
     def _validate_ids(
         self,

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/fixtures.py

@@ -1308,6 +1308,59 @@ class TestFixtureUsages:
         result = pytester.runpytest()
         result.stdout.fnmatch_lines(["*4 passed*"])
 
+    @pytest.mark.parametrize(
+        ("parametrize1", "parametrize2"),
+        [
+            (
+                '"fix", [1, 2], indirect=True',
+                '"fix", [2, 1], indirect=True',
+            ),
+            (
+                '"fix", [1, pytest.param({"data": 2}, id="2")], indirect=True',
+                '"fix", [pytest.param({"data": 2}, id="2"), 1], indirect=True',
+            ),
+            (
+                '"fix", [{"data": 1}, {"data": 2}], indirect=True, ids=lambda d: MyEnum(d["data"])',
+                '"fix", [{"data": 2}, {"data": 1}], indirect=True, ids=lambda d: MyEnum(d["data"])',
+            ),
+            (
+                '"fix", [{"data": 1}, {"data": 2}], indirect=True, ids=[1, "two"]',
+                '"fix", [{"data": 2}, {"data": 1}], indirect=True, ids=["two", 1]',
+            ),
+        ],
+    )
+    def test_reorder_and_cache(
+        self, pytester: Pytester, parametrize1, parametrize2
+    ) -> None:
+        """Test optimization for minimal setup/teardown with indirectly parametrized fixtures. See #8914, #9420."""
+        pytester.makepyfile(
+            f"""
+            import pytest
+            from enum import Enum
+            class MyEnum(Enum):
+                Id1 = 1
+                Id2 = 2
+            @pytest.fixture(scope="session")
+            def fix(request):
+                value = request.param["data"] if isinstance(request.param, dict) else request.param
+                print(f'prepare foo-%s' % value)
+                yield value
+                print(f'teardown foo-%s' % value)
+            @pytest.mark.parametrize({parametrize1})
+            def test1(fix):
+                pass
+            @pytest.mark.parametrize({parametrize2})
+            def test2(fix):
+                pass
+        """
+        )
+        result = pytester.runpytest("-s")
+        output = result.stdout.str()
+        assert output.count("prepare foo-1") == 1
+        assert output.count("prepare foo-2") == 1
+        assert output.count("teardown foo-1") == 1
+        assert output.count("teardown foo-2") == 1
+
     def test_funcarg_parametrized_and_used_twice(self, pytester: Pytester) -> None:
         pytester.makepyfile(
             """