From 88634bd086f7520b70c1d1615baee6f4c7e9ffaa Mon Sep 17 00:00:00 2001 From: Cheukting Date: Fri, 13 May 2022 18:54:33 +0100 Subject: [PATCH 01/16] Give warning when test function return other than None --- src/_pytest/python.py | 10 ++++++++++ src/_pytest/warning_types.py | 7 +++++++ 2 files changed, 17 insertions(+) diff --git a/src/_pytest/python.py b/src/_pytest/python.py index cd951939e7a..e793721c6f9 100644 --- a/src/_pytest/python.py +++ b/src/_pytest/python.py @@ -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 @@ -192,6 +194,14 @@ 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( + "Test function returning {result}, do you mean to use `assert` instead or `return`?".format( + result=result + ) + ) + ) return True diff --git a/src/_pytest/warning_types.py b/src/_pytest/warning_types.py index ac79bb53acc..aec767cdcc1 100644 --- a/src/_pytest/warning_types.py +++ b/src/_pytest/warning_types.py @@ -42,6 +42,13 @@ class PytestCollectionWarning(PytestWarning): __module__ = "pytest" +@final +class PytestReturnNotNoneWarning(PytestWarning): + """Warning emitted when a test function is returning value other than None.""" + + __module__ = "pytest" + + class PytestDeprecationWarning(PytestWarning, DeprecationWarning): """Warning class for features that will be removed in a future version.""" From 460410b2f8bf472a94f8a93f785a0a62347b352b Mon Sep 17 00:00:00 2001 From: Cheukting Date: Fri, 13 May 2022 21:38:50 +0100 Subject: [PATCH 02/16] Adding test --- testing/acceptance_test.py | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/testing/acceptance_test.py b/testing/acceptance_test.py index 92661b0f23a..18bdc0076e1 100644 --- a/testing/acceptance_test.py +++ b/testing/acceptance_test.py @@ -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(["*PytestReturnNotNoneWarning: Test function returning*"]) From c06605a12ca209ae1cedb3d8d6f33327ccbc7b77 Mon Sep 17 00:00:00 2001 From: Cheukting Date: Fri, 13 May 2022 21:46:58 +0100 Subject: [PATCH 03/16] Adding changelog --- changelog/7337.improvement.rst | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog/7337.improvement.rst diff --git a/changelog/7337.improvement.rst b/changelog/7337.improvement.rst new file mode 100644 index 00000000000..e2e4e5e8374 --- /dev/null +++ b/changelog/7337.improvement.rst @@ -0,0 +1 @@ +Warns for test functions that return non-None. Now in `pytest_pyfunc_call` after the `async_warn_and_skip` it will check for if the return is something other than None. From 238a5cae9c6dd31ea14e557b6c7542ac77b7bc89 Mon Sep 17 00:00:00 2001 From: Cheukting Date: Fri, 13 May 2022 21:48:25 +0100 Subject: [PATCH 04/16] Adding myself --- AUTHORS | 1 + 1 file changed, 1 insertion(+) diff --git a/AUTHORS b/AUTHORS index 86a814a137b..6cecb0d56f8 100644 --- a/AUTHORS +++ b/AUTHORS @@ -63,6 +63,7 @@ Ceridwen Charles Cloud Charles Machalow Charnjit SiNGH (CCSJ) +Cheuk Ting Ho Chris Lamb Chris NeJame Chris Rose From 7855fdefd25fb6cb0dc36a693ac608c0df0850b4 Mon Sep 17 00:00:00 2001 From: Cheukting Date: Fri, 13 May 2022 22:23:17 +0100 Subject: [PATCH 05/16] update changelog, warning msg and warning inherit --- changelog/7337.improvement.rst | 2 +- src/_pytest/python.py | 5 ++--- src/_pytest/warning_types.py | 14 +++++++------- 3 files changed, 10 insertions(+), 11 deletions(-) diff --git a/changelog/7337.improvement.rst b/changelog/7337.improvement.rst index e2e4e5e8374..1df95b61228 100644 --- a/changelog/7337.improvement.rst +++ b/changelog/7337.improvement.rst @@ -1 +1 @@ -Warns for test functions that return non-None. Now in `pytest_pyfunc_call` after the `async_warn_and_skip` it will check for if the return is something other than None. +Warns for test functions that return non-None. diff --git a/src/_pytest/python.py b/src/_pytest/python.py index e793721c6f9..699535620bd 100644 --- a/src/_pytest/python.py +++ b/src/_pytest/python.py @@ -197,9 +197,8 @@ def pytest_pyfunc_call(pyfuncitem: "Function") -> Optional[object]: elif result is not None: warnings.warn( PytestReturnNotNoneWarning( - "Test function returning {result}, do you mean to use `assert` instead or `return`?".format( - result=result - ) + f"Expected None, but the test 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 diff --git a/src/_pytest/warning_types.py b/src/_pytest/warning_types.py index aec767cdcc1..ebfeb87d842 100644 --- a/src/_pytest/warning_types.py +++ b/src/_pytest/warning_types.py @@ -42,13 +42,6 @@ class PytestCollectionWarning(PytestWarning): __module__ = "pytest" -@final -class PytestReturnNotNoneWarning(PytestWarning): - """Warning emitted when a test function is returning value other than None.""" - - __module__ = "pytest" - - class PytestDeprecationWarning(PytestWarning, DeprecationWarning): """Warning class for features that will be removed in a future version.""" @@ -62,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. From 03c1e51f8e419ff52c04cf8b914ea4c6db098a07 Mon Sep 17 00:00:00 2001 From: Cheukting Date: Fri, 13 May 2022 22:42:46 +0100 Subject: [PATCH 06/16] update test --- testing/acceptance_test.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/testing/acceptance_test.py b/testing/acceptance_test.py index 18bdc0076e1..cbda9934d50 100644 --- a/testing/acceptance_test.py +++ b/testing/acceptance_test.py @@ -1302,4 +1302,6 @@ def test_stuff(): """ ) res = testdir.runpytest() - res.stdout.fnmatch_lines(["*PytestReturnNotNoneWarning: Test function returning*"]) + res.stdout.fnmatch_lines( + ["*PytestReturnNotNoneWarning: Expected None, but the test returned*"] + ) From de60ac1713ea2bbe6be0d733d0c12204f1d44a71 Mon Sep 17 00:00:00 2001 From: Cheuk Ting Ho Date: Sun, 15 May 2022 09:39:09 +0100 Subject: [PATCH 07/16] Update src/_pytest/python.py Co-authored-by: Florian Bruhin --- src/_pytest/python.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/_pytest/python.py b/src/_pytest/python.py index 699535620bd..90e884d21ae 100644 --- a/src/_pytest/python.py +++ b/src/_pytest/python.py @@ -198,7 +198,7 @@ def pytest_pyfunc_call(pyfuncitem: "Function") -> Optional[object]: warnings.warn( PytestReturnNotNoneWarning( f"Expected None, but the test returned {result!r}, which will be an error in a " - "future version of Pytest. Did you mean to use `assert` instead of `return`?" + "future version of pytest. Did you mean to use `assert` instead of `return`?" ) ) return True From 175d0c745056543971f7a16c9c035712da1b445d Mon Sep 17 00:00:00 2001 From: Cheukting Date: Sun, 15 May 2022 09:45:42 +0100 Subject: [PATCH 08/16] import NoReturnWarning --- src/pytest/__init__.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/pytest/__init__.py b/src/pytest/__init__.py index 777d3774064..c1634e296bc 100644 --- a/src/pytest/__init__.py +++ b/src/pytest/__init__.py @@ -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 @@ -127,6 +128,7 @@ "PytestDeprecationWarning", "PytestExperimentalApiWarning", "PytestRemovedIn8Warning", + "PytestReturnNotNoneWarning", "Pytester", "PytestPluginManager", "PytestUnhandledCoroutineWarning", From 0c832cafbc6b446034573840bb88d521425b1889 Mon Sep 17 00:00:00 2001 From: Cheukting Date: Sun, 15 May 2022 09:49:20 +0100 Subject: [PATCH 09/16] adding reference --- doc/en/reference/reference.rst | 3 +++ 1 file changed, 3 insertions(+) diff --git a/doc/en/reference/reference.rst b/doc/en/reference/reference.rst index d082697258c..69e8690e73e 100644 --- a/doc/en/reference/reference.rst +++ b/doc/en/reference/reference.rst @@ -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: From a1f26dd0353d9538733c5002c3608e8f1b3ca336 Mon Sep 17 00:00:00 2001 From: Cheukting Date: Sun, 15 May 2022 10:32:03 +0100 Subject: [PATCH 10/16] deprecated reference --- doc/en/deprecations.rst | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/doc/en/deprecations.rst b/doc/en/deprecations.rst index 91944b758f9..88865d1f74e 100644 --- a/doc/en/deprecations.rst +++ b/doc/en/deprecations.rst @@ -252,6 +252,16 @@ or ``pytest.warns(Warning)``. See :ref:`warns use cases` for examples. + +Returning non-None value in test functions +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +.. deprecated:: 7.0 + +:class:`pytest.PytestReturnNotNoneWarning` will be warn if a test function return values other than None. +It is to avoid typo of an `assert` statement as `return` in a test function makes it passes the test without warning or failing, regardless of if the test should have pass or not. + + The ``--strict`` command-line option ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ From ff3d9b26c7ef39c3eb49e54171e5b389d1df3f14 Mon Sep 17 00:00:00 2001 From: Cheuk Ting Ho Date: Tue, 17 May 2022 14:54:02 +0100 Subject: [PATCH 11/16] Update src/_pytest/python.py Co-authored-by: Bruno Oliveira --- src/_pytest/python.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/_pytest/python.py b/src/_pytest/python.py index 90e884d21ae..4d1f722e474 100644 --- a/src/_pytest/python.py +++ b/src/_pytest/python.py @@ -197,7 +197,7 @@ def pytest_pyfunc_call(pyfuncitem: "Function") -> Optional[object]: elif result is not None: warnings.warn( PytestReturnNotNoneWarning( - f"Expected None, but the test returned {result!r}, which will be an error in a " + 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`?" ) ) From c0b368df83168cf4f8ca751a6b7320a8440c9e3c Mon Sep 17 00:00:00 2001 From: Cheuk Ting Ho Date: Tue, 17 May 2022 15:08:38 +0100 Subject: [PATCH 12/16] Update acceptance_test.py --- testing/acceptance_test.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/testing/acceptance_test.py b/testing/acceptance_test.py index cbda9934d50..7b74f158d39 100644 --- a/testing/acceptance_test.py +++ b/testing/acceptance_test.py @@ -1303,5 +1303,5 @@ def test_stuff(): ) res = testdir.runpytest() res.stdout.fnmatch_lines( - ["*PytestReturnNotNoneWarning: Expected None, but the test returned*"] + ["*Did you mean to use `assert` instead of `return`?*"] ) From d8c52240995968c5a7ad3e15321c134c7fd2adba Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Tue, 17 May 2022 14:09:51 +0000 Subject: [PATCH 13/16] [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --- testing/acceptance_test.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/testing/acceptance_test.py b/testing/acceptance_test.py index 7b74f158d39..c7139b538b2 100644 --- a/testing/acceptance_test.py +++ b/testing/acceptance_test.py @@ -1302,6 +1302,4 @@ def test_stuff(): """ ) res = testdir.runpytest() - res.stdout.fnmatch_lines( - ["*Did you mean to use `assert` instead of `return`?*"] - ) + res.stdout.fnmatch_lines(["*Did you mean to use `assert` instead of `return`?*"]) From fc92f5ef3e97764d497af7a6d0fa10ab9b2d3ab9 Mon Sep 17 00:00:00 2001 From: Bruno Oliveira Date: Tue, 17 May 2022 11:43:43 -0300 Subject: [PATCH 14/16] Improve changelog and docs --- changelog/7337.improvement.rst | 2 +- doc/en/deprecations.rst | 29 +++++++++++++++++++++++++++-- 2 files changed, 28 insertions(+), 3 deletions(-) diff --git a/changelog/7337.improvement.rst b/changelog/7337.improvement.rst index 1df95b61228..74d98d9b6c9 100644 --- a/changelog/7337.improvement.rst +++ b/changelog/7337.improvement.rst @@ -1 +1 @@ -Warns for test functions that return non-None. +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`. diff --git a/doc/en/deprecations.rst b/doc/en/deprecations.rst index 88865d1f74e..23bcb2fba91 100644 --- a/doc/en/deprecations.rst +++ b/doc/en/deprecations.rst @@ -258,8 +258,33 @@ Returning non-None value in test functions .. deprecated:: 7.0 -:class:`pytest.PytestReturnNotNoneWarning` will be warn if a test function return values other than None. -It is to avoid typo of an `assert` statement as `return` in a test function makes it passes the test without warning or failing, regardless of if the test should have pass or not. +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 From aecdc3917b8a6b6761c9297f989235a2b8ceaa79 Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Tue, 17 May 2022 14:44:44 +0000 Subject: [PATCH 15/16] [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --- doc/en/deprecations.rst | 28 +++++++++++++++++----------- 1 file changed, 17 insertions(+), 11 deletions(-) diff --git a/doc/en/deprecations.rst b/doc/en/deprecations.rst index 23bcb2fba91..ab76a520390 100644 --- a/doc/en/deprecations.rst +++ b/doc/en/deprecations.rst @@ -258,17 +258,20 @@ Returning non-None value in test functions .. deprecated:: 7.0 -A :class:`pytest.PytestReturnNotNoneWarning` is now emitted if a test function returns something other than `None`. +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], - ]) + @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 @@ -278,11 +281,14 @@ 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], - ]) + @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 From 78e0b67ca75142057d4d22a2cc51b947c9a1df04 Mon Sep 17 00:00:00 2001 From: Cheuk Ting Ho Date: Fri, 20 May 2022 11:19:49 +0100 Subject: [PATCH 16/16] Update doc/en/deprecations.rst Co-authored-by: Florian Bruhin --- doc/en/deprecations.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/doc/en/deprecations.rst b/doc/en/deprecations.rst index ab76a520390..3bbd29bb509 100644 --- a/doc/en/deprecations.rst +++ b/doc/en/deprecations.rst @@ -256,7 +256,7 @@ See :ref:`warns use cases` for examples. Returning non-None value in test functions ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ -.. deprecated:: 7.0 +.. deprecated:: 7.2 A :class:`pytest.PytestReturnNotNoneWarning` is now emitted if a test function returns something other than `None`.