From e2c88eaf98e1cc6e4b6afefc4238ca136cd958be Mon Sep 17 00:00:00 2001 From: Tobias Deiminger Date: Thu, 16 Dec 2021 21:02:37 +0100 Subject: [PATCH 1/7] Add IdMaker.make_parameter_keys These parameter keys will later become the unified way how reorder_items and FixtureDefs decide if parameters are equal and can be reused. Order for what to use as key is as follows: 1. If users gave explicitly parameter ids, use them as key. 2. If not explictely given, and the parameter value is hashable, use the parameter value as key. 3. Else, fallback to the parameters identity. NB: Rule 1 gives users ultimate (equallity-telling) power, and with great power comes great responsiblity. One could now do something wired like @pytest.mark.parametrize(fruit, [ pytest.param("apple", id="fruit"), pytest.param("orange", id="fruit"), ] def test_fruits(fruit): pass The user just made "apple" equal to "orange". If that's what they intend is unknown, but probably not. --- src/_pytest/python.py | 68 +++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 68 insertions(+) diff --git a/src/_pytest/python.py b/src/_pytest/python.py index f5b332e68..ba96adaa2 100644 --- a/src/_pytest/python.py +++ b/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 @@ -929,6 +930,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: @@ -976,6 +1009,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): @@ -994,6 +1048,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) From a8151fb26767de8612299e01b645f490298568a2 Mon Sep 17 00:00:00 2001 From: Tobias Deiminger Date: Thu, 16 Dec 2021 21:02:46 +0100 Subject: [PATCH 2/7] Extend CallSpec2 with param_keys This way we make the previously calculated parameter key accessible to reorder_items and FixtureDef. --- src/_pytest/python.py | 29 ++++++++++++++++++++--------- 1 file changed, 20 insertions(+), 9 deletions(-) diff --git a/src/_pytest/python.py b/src/_pytest/python.py index ba96adaa2..bc00a84f2 100644 --- a/src/_pytest/python.py +++ b/src/_pytest/python.py @@ -1146,6 +1146,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. @@ -1165,9 +1167,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): @@ -1181,10 +1186,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], @@ -1354,7 +1361,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 ) @@ -1367,17 +1374,18 @@ class Metafunc: # of all calls. newcalls = [] for callspec in self._calls or [CallSpec2()]: - for param_index, (param_id, param_set) in enumerate( - zip(ids, parametersets) + for param_index, (param_id, parameterset, param_set_keys) in enumerate( + 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 @@ -1393,9 +1401,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: @@ -1407,7 +1414,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 @@ -1421,7 +1430,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, From 94279c0931e28ad655bd4d96a993218c86bffbca Mon Sep 17 00:00:00 2001 From: Tobias Deiminger Date: Thu, 16 Dec 2021 21:03:00 +0100 Subject: [PATCH 3/7] Extend SubRequest with param_key Pick up the value from curent CallSpec2 and assign it to the SubRequest. It's required to make the parameter key accessible in FixtureDef.execute. --- src/_pytest/fixtures.py | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/src/_pytest/fixtures.py b/src/_pytest/fixtures.py index be03fb2a8..cf61cb23c 100644 --- a/src/_pytest/fixtures.py +++ b/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 @@ -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 From c1aaede59b87528ebbcb3a7fa430811c73350ced Mon Sep 17 00:00:00 2001 From: Tobias Deiminger Date: Thu, 16 Dec 2021 21:03:20 +0100 Subject: [PATCH 4/7] Let reorder_items use our new parameter key Fixes test reordering for indirect parameterization (see #8913). Prior to this commit, reorder_items considered the parameter index to tell if a parameter is "the same" and therefore can be shared. Looking at the index causes trouble if there are multiple parametrizations for the same fixture, basically because one index means different things in different parameter lists. This is fixed here by using the recently introduced parameter key as grouping criterion. Caution: The parameter key ends up inside the key of another dict, and therefore must be hashable. CallSpec2.param_keys is crafted sufficiently, it guarantees to contain comparable and hashable values. --- src/_pytest/fixtures.py | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/_pytest/fixtures.py b/src/_pytest/fixtures.py index cf61cb23c..701badd48 100644 --- a/src/_pytest/fixtures.py +++ b/src/_pytest/fixtures.py @@ -249,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 From 5377ff1067f54f24acd40fba66d77b6962a3695f Mon Sep 17 00:00:00 2001 From: Tobias Deiminger Date: Thu, 16 Dec 2021 21:03:27 +0100 Subject: [PATCH 5/7] Let FixtureDef.cache_key use our new parameter key The FixtureDef cache must agree with reorder_items about what parmeters are the same. The new param key must (and can) be compared by value, so we change from "is" to "==" in FixtureDef.execute. --- src/_pytest/fixtures.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/_pytest/fixtures.py b/src/_pytest/fixtures.py index 701badd48..9176493dd 100644 --- a/src/_pytest/fixtures.py +++ b/src/_pytest/fixtures.py @@ -1017,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) @@ -1037,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 "".format( From 7ffa9a99dc43e25e98a5be51790248537d0dabc6 Mon Sep 17 00:00:00 2001 From: Tobias Deiminger Date: Thu, 16 Dec 2021 21:03:49 +0100 Subject: [PATCH 6/7] Add and adapt tests Add tests to assert #8914 is fixed. Tests assures that both reordering and caching work as intended, and demonstrates how one can use parameter ids to decide about equality. Adapting issue_519.checked_order is not cheating. See our discussion at https://github.com/pytest-dev/pytest/pull/9350#pullrequestreview-819514238 --- testing/example_scripts/issue_519.py | 4 +-- testing/python/fixtures.py | 53 ++++++++++++++++++++++++++++ 2 files changed, 55 insertions(+), 2 deletions(-) diff --git a/testing/example_scripts/issue_519.py b/testing/example_scripts/issue_519.py index e44367fca..73437ef7b 100644 --- a/testing/example_scripts/issue_519.py +++ b/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"), ] diff --git a/testing/python/fixtures.py b/testing/python/fixtures.py index 3ce5cb34d..9ff8d3e9f 100644 --- a/testing/python/fixtures.py +++ b/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( """ From 26ae39866ae3410a1fee86e80437c93e670bac37 Mon Sep 17 00:00:00 2001 From: Tobias Deiminger Date: Thu, 16 Dec 2021 21:11:00 +0100 Subject: [PATCH 7/7] Update changelog and AUTHORS --- AUTHORS | 1 + changelog/8914.bugfix.rst | 4 ++++ 2 files changed, 5 insertions(+) create mode 100644 changelog/8914.bugfix.rst diff --git a/AUTHORS b/AUTHORS index 9413f9c2e..2fa019278 100644 --- a/AUTHORS +++ b/AUTHORS @@ -322,6 +322,7 @@ Thomas Grainger Thomas Hisch Tim Hoffmann Tim Strazny +Tobias Deiminger Tom Dalton Tom Viner Tomáš Gavenčiak diff --git a/changelog/8914.bugfix.rst b/changelog/8914.bugfix.rst new file mode 100644 index 000000000..397c1be17 --- /dev/null +++ b/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.