Skip to content

Commit

Permalink
Warn when test functions return other than None (#9956)
Browse files Browse the repository at this point in the history
Closes #7337
  • Loading branch information
Cheukting committed May 25, 2022
1 parent 31f9e5b commit c988e49
Show file tree
Hide file tree
Showing 8 changed files with 75 additions and 0 deletions.
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):
"""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`?*"])

0 comments on commit c988e49

Please sign in to comment.