pytest
pytest copied to clipboard
pytest-dev
Undocumented removal of `PytestReturnNotNoneWarning`
It looks like https://github.com/pytest-dev/pytest/pull/12346 got merged without reference to some of the previous discussions, e.g. https://github.com/pytest-dev/pytest/issues/10465.
With pytest 8.4, this change causes workflows to fail if they return non-None values (which can be useful for debugging interactively by running the file directly as well as via pytest).
At minimum, the deprecation documentation does not say that this feature was removed: https://docs.pytest.org/en/stable/deprecations.html#returning-non-none-value-in-test-functions
However, other users mentioned other issues with the current implementation as well, e.g. trying to print the full return value can cause problems with large objects or binary data (see example below).
In terms of a solution, I would like to ask that there be a config option to skip this (similar to --doctest-ignore-import-errors). I agree that newbie users should be protected from this gotcha -- but I don't think that means this feature should be forcibly removed for everyone!
Here's an example of a test that causes an implicit pytest error as it tries to print the object being returned:
def test_bad_repr():
class BadRepr:
def __repr__(self):
raise Exception('Bad repr')
bad_repr = BadRepr()
return bad_repr
which fails with:
====================================================== FAILURES ======================================================
___________________________________________________ test_bad_repr ____________________________________________________
self = <[Exception('Bad repr') raised in repr()] BadRepr object at 0x72b1c228cc20>
def __repr__(self):
> raise Exception('Bad repr')
E Exception: Bad repr
test_bad_repr.py:6: Exception
============================================== short test summary info ===============================================
FAILED test_bad_repr.py::test_bad_repr - Exception: Bad repr
================================================= 1 failed in 0.07s ==================================================
This is a pretty confusing failure, as at no point is it clear from the error why the object repr is being called in the first place, and this error is raised instead of the return-not-None test failure.
This has affected networkx as well: networkx/networkx#8090
In networkx's case specifically, it's a bad interaction between the pytest-mpl plugin and the pytest policy that tests must not return. The pytest-mpl mpl_image_compare relies on returning an image which is then subsequently compared to a reference image.
Hi folks,
I agree that the current state is unfortunate.
I think we should keep the warning indefinitely (not as a deprecation). However, I don't think we need an explicit option to suppress it, seems like using filterwarnings should be enough.
When returning the warning it should have a new message, and the deprecation docs should mention that it's not deprecated. This was the previous message
PytestReturnNotNoneWarning(
f"Expected None, but {pyfuncitem.nodeid} returned {result!r}, which will be an error in a "
"future version of pytest. Did you mean to use `assert` instead of `return`?"
)
and I think we want coroutines to stay as an error, so the logic will be something like:
- None -> No error
- awaitable -> error
- otherwise -> warning
This kind of makes me think that a config option might be a neater way of allowing non-None return values, especially as there may be newbies that don't run with -Werror and accidentally return values instead of doing assert or whatever. But I don't have a strong opinion either way.
In a perfect world I think there may be even better ways of achieving things instead of returning values, but /shrug
Very happy with the plan to revert back to the warning and remove the deprecation notice!
For the warning/error message, maybe something like this would make it more robust in case the repr is very long or raises an exception?
PytestReturnNotNoneWarning(
f"Tests are expected to return `None`, but {pyfuncitem.nodeid} returned {type(result)}."
"Did you mean to use `assert` instead of `return`?"
)
If this warning is targeted at newbie users, could make it even more explicit:
PytestReturnNotNoneWarning(
f"Test outputs are not checked so tests are expected to return `None`, "
"but {pyfuncitem.nodeid} returned {type(result)}. "
"Did you mean to use `assert` instead of `return`?"
)
Can anybody estimate when a version of pytest with PytestReturnNotNoneWarning added back will be released? If it's far in the future, I'll need to pin a previous version of pytest.
