Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Give warning when test function return other than None #9956

Merged
merged 16 commits into from May 25, 2022
Merged
1 change: 1 addition & 0 deletions AUTHORS
Expand Up @@ -63,6 +63,7 @@ Ceridwen
Charles Cloud
Charles Machalow
Charnjit SiNGH (CCSJ)
Cheuk Ting Ho
Chris Lamb
Chris NeJame
Chris Rose
Expand Down
1 change: 1 addition & 0 deletions changelog/7337.improvement.rst
@@ -0,0 +1 @@
A warning is now emitted if a test function returns something other than `None`. This prevents a common mistake among beginners that expect that returning a `bool` (for example `return foo(a, b) == result`) would cause a test to pass or fail, instead of using `assert`.
41 changes: 41 additions & 0 deletions doc/en/deprecations.rst
Expand Up @@ -252,6 +252,47 @@ or ``pytest.warns(Warning)``.

See :ref:`warns use cases` for examples.


Returning non-None value in test functions
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

.. deprecated:: 7.2

A :class:`pytest.PytestReturnNotNoneWarning` is now emitted if a test function returns something other than `None`.

This prevents a common mistake among beginners that expect that returning a `bool` would cause a test to pass or fail, for example:

.. code-block:: python

@pytest.mark.parametrize(
["a", "b", "result"],
[
[1, 2, 5],
[2, 3, 8],
[5, 3, 18],
],
)
def test_foo(a, b, result):
return foo(a, b) == result

Given that pytest ignores the return value, this might be surprising that it will never fail.

The proper fix is to change the `return` to an `assert`:

.. code-block:: python

@pytest.mark.parametrize(
["a", "b", "result"],
[
[1, 2, 5],
[2, 3, 8],
[5, 3, 18],
],
)
def test_foo(a, b, result):
assert foo(a, b) == result


The ``--strict`` command-line option
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Expand Down
3 changes: 3 additions & 0 deletions doc/en/reference/reference.rst
Expand Up @@ -1130,6 +1130,9 @@ Custom warnings generated in some situations such as improper usage or deprecate
.. autoclass:: pytest.PytestExperimentalApiWarning
:show-inheritance:

.. autoclass:: pytest.PytestReturnNotNoneWarning
:show-inheritance:

.. autoclass:: pytest.PytestUnhandledCoroutineWarning
:show-inheritance:

Expand Down
9 changes: 9 additions & 0 deletions src/_pytest/python.py
Expand Up @@ -77,10 +77,12 @@
from _pytest.pathlib import visit
from _pytest.scope import Scope
from _pytest.warning_types import PytestCollectionWarning
from _pytest.warning_types import PytestReturnNotNoneWarning
from _pytest.warning_types import PytestUnhandledCoroutineWarning

if TYPE_CHECKING:
from typing_extensions import Literal

from _pytest.scope import _ScopeName


Expand Down Expand Up @@ -192,6 +194,13 @@ def pytest_pyfunc_call(pyfuncitem: "Function") -> Optional[object]:
result = testfunction(**testargs)
if hasattr(result, "__await__") or hasattr(result, "__aiter__"):
async_warn_and_skip(pyfuncitem.nodeid)
elif result is not None:
warnings.warn(
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`?"
)
)
return True


Expand Down
7 changes: 7 additions & 0 deletions src/_pytest/warning_types.py
Expand Up @@ -55,6 +55,13 @@ class PytestRemovedIn8Warning(PytestDeprecationWarning):
__module__ = "pytest"


@final
class PytestReturnNotNoneWarning(PytestDeprecationWarning):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be imported in src/pytest/__init__.py and added to __all__ there, since we want e.g. people to be able to ignore it via their config without having to import private API.

It should also be added to doc/en/reference/reference.rst with the other warnings, and probably get a new entry in doc/en/deprecations.rst.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the review, just want to be clear about the deprecations. Since we are already in 7, shall it be deprecated in 8? Or should I say 7?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be a warning, not a deprecation warning

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should it, if we plan to turn it into an error in the future?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Deprecation warning is correct if we want to turn it into an error in the future, which seems the plan for unittest in the stdlib too: #7337 (comment).

python/cpython#27748

"""Warning emitted when a test function is returning value other than None."""

__module__ = "pytest"


@final
class PytestExperimentalApiWarning(PytestWarning, FutureWarning):
"""Warning category used to denote experiments in pytest.
Expand Down
2 changes: 2 additions & 0 deletions src/pytest/__init__.py
Expand Up @@ -69,6 +69,7 @@
from _pytest.warning_types import PytestDeprecationWarning
from _pytest.warning_types import PytestExperimentalApiWarning
from _pytest.warning_types import PytestRemovedIn8Warning
from _pytest.warning_types import PytestReturnNotNoneWarning
from _pytest.warning_types import PytestUnhandledCoroutineWarning
from _pytest.warning_types import PytestUnhandledThreadExceptionWarning
from _pytest.warning_types import PytestUnknownMarkWarning
Expand Down Expand Up @@ -127,6 +128,7 @@
"PytestDeprecationWarning",
"PytestExperimentalApiWarning",
"PytestRemovedIn8Warning",
"PytestReturnNotNoneWarning",
"Pytester",
"PytestPluginManager",
"PytestUnhandledCoroutineWarning",
Expand Down
11 changes: 11 additions & 0 deletions testing/acceptance_test.py
Expand Up @@ -1292,3 +1292,14 @@ def test_no_brokenpipeerror_message(pytester: Pytester) -> None:

# Cleanup.
popen.stderr.close()


def test_function_return_non_none_warning(testdir) -> None:
testdir.makepyfile(
"""
def test_stuff():
return "something"
"""
)
res = testdir.runpytest()
res.stdout.fnmatch_lines(["*Did you mean to use `assert` instead of `return`?*"])