Alternative approaches for PytestReturnNotNoneWarning

[cliffckerr]

Like others (here and here), I was surprised with the new warnings in pytest 7.2 when test functions return values other than None. Dating since the original issue (#7337), there was quite a bit of debate about whether this was a good idea.

Personally, making something raise an exception not because it's an error, but simply because some users may get confused by it, feels somewhat inconsistent with other decisions made in Python. For example, these Python "gotchas" could raise exceptions, but don't, because sometimes you do actually mean to do these things -- as is the case with returning non-None from a test.

I'd propose to (a) keep the warning, but leave it as a warning indefinitely (so filterwarnings can be used), and/or (b) add a config flag under the pytest-warnings section for --allow-return-not-none.

[nicoddemus]

Seems reasonable.

We are following unittest's lead in this topic, if they keep it always a warning, we definitely should follow suit.

[RomeshA]

I'd be in favor of keeping it as a warning too. While having tests return values something other than None might be a questionable practice, it doesn't actually make the test invalid, there isn't actually anything wrong with the test and it can be used with pytest just fine. Displaying a warning rather than raising an error is in keeping with the purpose of assisting the user - helping them in case what they wrote isn't what they intended, but staying out of the way if it is what they intended

[Kodiologist]

Making this an error would be problematic for Hy, which implements implicit returns, so e.g. a test that ends with a function call will pass on whatever the function returns, not None. Having to add a lot of trailing Nones just to keep pytest happy would be silly.

[yifeim]

This took me by surprise. I thought something was wrong in my codes. I would appreciate a simple option to silence these warnings for previously published codes. Better yet, the pytest team could be a little more considerate of their users by telling us that this was a new change that they introduced as of <version number@date>.

[xapple]

To solve the issue this silly warning is causing I just wrap the return statements of my functions like this:

     if "pytest" not in sys.modules: return result

[jrounds]

I feel this warning is overly opiniated, but a more constructive criticism is the warning prints the value of the return which in certain binary data cases can make bamboo logs unreadable due to text spam. It seems like it shouldn't print the value of the return because pytest is not prepared to properly format all possible return values.

[RonnyPfannschmidt]

@jrounds this is primarily about helping people to avoid grave mistakes that are observable in the wild

The typical case for those is devoid of other tools that help as its precisely for the aspiring test writer that is not yet up2date with all the surrounding best practise

Im happy to discuss ways to make this easier for people that know what they are doing

But it's going to be hard to just get me to agree on removing footgun safely for people that don't know or only think they know

[Kodiologist]

What footgun are we trying to protect against, though? Is the concern that users will return an expression instead of asserting it as they meant to? That should be the kind of thing one catches by just checking that the test fails when it should fail and succeeds when it should succeed, which is something one needs to do anyway because it catches a lot of other basic mistakes.

[RonnyPfannschmidt]

Beginners don't always fit what you believe ough to be

[The-Compiler]

Let me note that Python's unittest.py does the same too: python/cpython#85494 (I believe this was one of the original motivations for pytest to follow).

As for:

Im happy to discuss ways to make this easier for people that know what they are doing

I feel like an filterwarnings = ignore::pytest.PytestReturnNotNoneWarning is already rather trivial?

[xapple]

Where would you put the string filterwarnings = ignore::pytest ..., inside the pytest configuration file? I'd like to be able to solve this issue.

[The-Compiler]

Yes, it's a config option, see the configuration docs for details. You can also use -W ignore::pytest.PytestReturnNotNoneWarning if you prefer a CLI argument.

[nicoddemus]

I think we can settle then that this will never become an error then?

If so, I think all we need to do is to change its superclass from PytestRemovedIn8Warning to PytestWarning:

pytest/src/_pytest/warning_types.py

Line 59 in fd56968

class PytestReturnNotNoneWarning(PytestRemovedIn8Warning):

[RonnyPfannschmidt]

👍

[fraserstark]

I've created a PR for this if it's still needed.