Improved fixture reuse by new param keys that can be derived from API ids

[haxtibal]

This is a less trivial but (arguably) more powerful alternative to #9350. It fixes #8914. For a review, please follow the commit series and messages.

Situation: Both reorder_items and FixtureDef.execute must decide and agree about whether fixture instances can be reused. The obvious is to decide by comparing parameter values. Sadly none of prm1 == prm2, prm1 is prm2 or hash(prm1) == hash(prm2) is generally suited to decide about "are parameters the same?":

• dicts are not hashable
• numpy arrays don't compare to bool, and are not hashable either
• objects compare and hash by identity, but a user may expect reuse only if it's equal by value

Using the parameter index instead of value doesn't work either, because, well, see #8914.

Idea: Give users the option to explicitly control equality of parameters by leveraging parameter ids. Under the hood we introduce a param_key to support fixture reordering and caching. The key is built according to the following rules

1. If parameter ids have been passed via API, use that parameter id as key.
2. Else, if a parameter value is hashable, use that parameter value as key.
3. Else, fallback to a parameter values identity.

Example 1: Use id= to control equality of the unhashable dict parameters (rule 1 applies).

@pytest.fixture(scope="session")
def foo(request.param):
    pass  # goal: minimize nrof setup/teardown
@pytest.mark.parametrize("foo", [pytest.param({"data": "a"}, id="0"), pytest.param({"data": "b"}, id="1")], indirect=True)
def test1(foo):
    pass
@pytest.mark.parametrize("foo", [pytest.param({"data": "b"}, id="1"), pytest.param({"data": "a"}, id="0")], indirect=True)
def test2(foo):
    pass

Example 2: No need to use id= or ids= if the parameter is hashable by value (rule 2 applies).

@pytest.fixture(scope="session")
def foo(request.param):
    pass  # goal: minimize nrof setup/teardown
@pytest.mark.parametrize("foo", ["a", "b"], indirect=True)
def test1(foo):
    pass
@pytest.mark.parametrize("foo", ["b", "a"], indirect=True)
def test2(foo):
    pass

Also related: #244, #5693, #6497, #6541.

[bluetech]

@haxtibal Thanks a lot for creating this separate PR with nice commits and all!

I'm looking forward to reviewing this PR, when I have the time available. Right now we are mostly focused on getting the 7.0 release out the door, just wanted to write this so you don't get discouraged :)

[bluetech]

I reviewed everything, though I ran out of time toward the end. The commit separation and commit messages were very helpful.

[haxtibal]

Regarding the the problem with the duplicates, when there are conflicts in make_unique_test_ids pytest disambiguates them with a suffix. This should probably also be done for parameter keys then, since duplicates probably don't make sense within the same group of parameter sets.

For make_parameter_keys, leaving duplicates duplicated is at the heart of what makes the new feature work: Only duplicates hash to the same value, and same hash will in later stages be the criterion to skip all but one duplicate for the sake of fixture s/t optimization. Does this reasoning make sense?

[haxtibal]

@bluetech Thanks for taking your time! That was a really helpful review.

Most of the remarks are hopefully done, and the commit series is rebased on latest main. I left conversations open where response from you would be good.

[haxtibal]

if you agree, I can cherry-pick it to main already, to reduce the size of the remaining PR.

Just applied your recent comments. Sure, cherry-pick it, or in case you find some more bits you'd like to change, it's fine for me if you amend the commit to spare another review iteration on "Refactor idmaker functions into class IdMaker".

[bluetech]

(I rebased on top of the cherry-picked commit)

[bluetech]

Gave this another (quick) look.

First, the example in the first commit message is not very fitting, because @pytest.mark.parametrize() parametrizations are function scoped, so they don't get a chance to be reused, i.e. not relevant for this change. I suggest the following example instead (prints "apple" twice):

import pytest

@pytest.fixture(params=["apple", "orange"], ids=["fruit", "fruit"], scope="module")
def a_fruit(request):
    return request.param

def test_fruits(a_fruit):
    print(a_fruit)

This makes me think, if we shouldn't add a "deduplication" step to make_parameter_keys, similar to the one in make_unique_parameterset_ids, to prevent such cases? Because I mean, it does not make much sense for someone to duplicate an id within a single parametrization (basically, for a single test)? The mechanism here is only useful for better managing the lifetime of higher-scoped indirect fixtures across tests.

And that made me further think, if we shouldn't just completely do away with falling back on comparing values (with the entire SafeHashWrapper business it entails), and instead always generate string IDs (basically, call _idval in _parameter_keys_from_parameterset). This will mostly keep the existing behavior (since _idval_from_argname eventually falls back to the parameterset idx), but still allow the user to override with an explicit ID (which we could recommend they do).

This stuff is pretty entangled and complicated so I may be confused or missing something, but WDYT?

[haxtibal]

@bluetech Sorry for delay in response.

the example in the first commit message is not very fitting

Agreed, my example not only missed scope="module", but also contained syntax errors.

I suggest the following example [...]

Agreed. That's better suited.

it does not make much sense for someone to duplicate an id within a single parametrization (basically, for a single test)?

Agreed. I'd suggest to print a warning or error message then, and maybe even bail out early, instead of deduplicating silently?

The mechanism here is only useful for better managing the lifetime of higher-scoped indirect fixtures across tests.

Agreed. Right now I'm slightly puzzled why direct parametrization (as in my commit example) is subject to fixture lookup at all - as opposed to unconditionally take values from CallSpec2.funcargs. Because, as you said, there's just no way to optimize something for direct parametrization. It's not new behavior from this PR (I hope), I just need to think twice to maybe understand the original intent.

And that made me further think, if we shouldn't just completely do away with falling back on comparing values (with the entire SafeHashWrapper business it entails), and instead always generate string IDs (basically, call _idval in _parameter_keys_from_parameterset). This will mostly keep the existing behavior (since _idval_from_argname eventually falls back to the parameterset idx), but still allow the user to override with an explicit ID (which we could recommend they do).

This stuff is pretty entangled and complicated so I may be confused or missing something, but WDYT?

We can't simply rely on _idval, as the string it generates is not guaranteed to be unique within fixture scope. If we do what you suggested we can get incorrect parametrization:

import pytest

@pytest.fixture(scope="module")
def fruitfix(request):
    print(f"setup fruitfix with {request.param}")
    return request.param

@pytest.mark.parametrize("fruitfix", [(1, 2), (3, 4)], indirect=True)
def test_fruits_1(fruitfix):
    print(f"test1 using {fruitfix}")

@pytest.mark.parametrize("fruitfix", [(3, 4), (5, 6)], indirect=True)
def test_fruits_2(fruitfix):
    print(f"test2 using {fruitfix}")

Output with _evaluate_idval_function, or SafeHashWrapper if the former is None (the PRs current approach):

setup fruitfix with (1, 2)
test1 using (1, 2)
setup fruitfix with (3, 4)
test1 using (3, 4)
test2 using (3, 4)
setup fruitfix with (5, 6)
test2 using (5, 6)

Output with using _idval as suggested (note how tuple (5, 6) is not passed to test2):

setup fruitfix with (1, 2)
test1 using (1, 2)
test2 using (1, 2)
setup fruitfix with (3, 4)
test1 using (3, 4)
test2 using (3, 4)

This is because _idval ~~only generates unique value representations for scalar parameters like int, float, double. But not for e.g. tuples, where it would~~ falls back to argument names + counter for structured types. Further, it would stringify scalar types str("1.23") and float(1.23) to the same id "1.23". Therefore _idval suffers from a similar problem as using the index (the strategy before this PR).

What we could do was to use _evaluate_idval_function instead of _idval, and fallback to id() right away instead of SafeHashWrapper. This would run correctly, but we lose fixture optimization ~~for tuple types~~. The advice to the user would then be simply: "Provide ids, else you won't get optimized".

[haxtibal]

but WDYT?

Sorry for the edits in yesterdays answer. They make decision even simpler: We can trade the current SafeHashWrapper approach for a documentation statement "Provide ids, else your parametrized fixtures won't get optimized".

My personal opinion is to keep SafeHashWrapper and the implied compare-by-value. When I came to pytest I was under the impression that reuse of parametrized fixtures is an advertised feature and found it to be the most outstanding one compared to other testing frameworks (although it mostly didn't work because of a bug). As a user, I would like if it works without custom annotations where possible.

But I can understand if you want to leave the compare-by-value fallback out: It's simpler to document and understand, and it's probably simpler to maintain.

@bluetech It's your turn to decide, I will for sure be fine with either decision.

[nicoddemus]

@bluetech @haxtibal gentle ping to get this rolling again (or close, if deemed best).

[haxtibal]

@bluetech @haxtibal gentle ping to get this rolling again (or close, if deemed best).

I'd still be interested and standing by. @bluetech How about you?