Fix unguarded == comparison in fixtures.

[kohr-h]

Here's an attempt to fix the equality checking issue in fixtures.

To do:

• AUTHORS file
• Tests for the fix

Closes: #6497

[RonnyPfannschmidt]

this should warn that a fragile cache key was used,
pytest should at some point require parameters to be reasonably stable/hash-able
accidentally and strangely modifiable parameters have led to many horrendous errors as aftermath of accidential modification

[kohr-h]

Ok, I'll add a warning.
Regarding the TypeError case, is that even desirable? Classes have to actively disallow ==, if unimplemented it defaults to is, which would be okay. Should we consider a class that raises TypeError on __eq__ as pathological? Besides, AttributeError would also be an option.

[nicoddemus]

Perhaps we should straight out error out instead? Warnings have a tendency to get ignored, and we should avoid letting users running test suites with problematic setups.

So I suggest we raise an appropriate error pointing to the problematic node id instead.

Regarding the TypeError case

We should actually handle any exception I think (using Exception)

[kohr-h]

Just for clarity, @nicoddemus do you mean to also error out in the NumPy array case? After thinking about it for a second, I agree that's the most reasonable solution. Besides brittleness, another reason is that every == operation with an array computes a new boolean array, so with large arrays as parameters, the comparisons can get quite expensive.
I suggest to include a hint in the error message to use scope='function' for NumPy arrays or other types where if a == b: can raise an exception. The docs would ideally also mention that case.

[nicoddemus]

Just for clarity, @nicoddemus do you mean to also error out in the NumPy array case? After thinking about it for a second, I agree that's the most reasonable solution. Besides brittleness, another reason is that every == operation with an array computes a new boolean array, so with large arrays as parameters, the comparisons can get quite expensive.

Yes that's my reasoning as well: we should straight out discourage parametrization using types that don't have standard/buggy == operators.

I suggest to include a hint in the error message to use scope='function' for NumPy arrays or other types where if a == b: can raise an exception. The docs would ideally also mention that case.

Not sure, this problem is not related to scoping at all, I'm afraid. We could however add a link in the exception message to a section in the docs where this is explained, probably using numpy arrays as example and possible workarounds.

[nicoddemus]

Thanks @RonnyPfannschmidt those are good points.

I'm still leaning towards just breaking it in the next feature release though (don't think this will affect many users), but I'm also OK about deprecating it.

[nicoddemus]

Btw, please get the final word then so @kohr-h can finish the PR.

[asottile]

what if we did something more clever with request.param below in def cache_key -- I think we could probably use object identity (id(...)) for request.param

[kohr-h]

if we remove support for them it breaks test-suites that are technically working

Hm, with the current release test suites are already broken, so the question is whether you prefer to add a (somewhat dirty) workaround or rather keep failing, but with a better error message. Or would it make sense to re-enable a broken feature and deprecate it at the same time?

@asottile You mean replace == with is basically?

[kohr-h]

Quick testing shows that is instead of == could work. I'll do a push and see how the test suite does.

[kohr-h]

Works with is, at least the existing tests. What do you think?
And what would be a good place for a test case for this issue?

[kohr-h]

And what would be a good place for a test case for this issue?

Maybe around here?

pytest/testing/python/fixtures.py

Line 1105 in 2c32dad

def test_funcarg_parametrized_and_used_twice(self, testdir):

[kohr-h]

Now with test.

[nicoddemus]

Thanks @kohr-h!

Pushed minor improvements and fixed linting.

As we only use the comparison for caching, I don't think this will have any adverse effects. 👍

[blueyed]

Thanks, looks good.
Please squash it though (@nicoddemus can do it).

[nicoddemus]

Please squash it though (@nicoddemus can do it).

Just did, thanks for catching it.

[asottile]

[blueyed]

Please squash it though (@nicoddemus can do it).
Just did, thanks for catching it.

Has not been done/pushed?

JFI: I'm out of here (given the animated GIF).

[nicoddemus]

Has not been done/pushed?

My bad, done. 😅

[kohr-h]

Ah, just saw that this is merged, 👍 -- and that the conversation took a funny turn..