Repository: pytest-dev/pytest
Pull request: Give warning when test function return other than None
<!--
Thanks for submitting a PR, your contribution is really appreciated!

Here is a quick checklist that should be present in PRs.

- [ ] Include documentation when adding new features.
- [X] Include new tests or update existing tests when applicable.
- [X] Allow maintainers to push and squash when merging my commits. Please uncheck this if you prefer to squash the commits yourself.

If this change fixes an issue, please:

- [X] Add text like ``closes #XYZW`` to the PR description and/or commits (where ``XYZW`` is the issue number). See the [github docs](https://help.github.com/en/github/managing-your-work-on-github/linking-a-pull-request-to-an-issue#linking-a-pull-request-to-an-issue-using-a-keyword) for more information.

Unless your change is trivial or a small documentation fix (e.g., a typo or reword of a small section) please:

- [X] Create a new changelog file in the `changelog` folder, with a name like `<ISSUE NUMBER>.<TYPE>.rst`. See [changelog/README.rst](https://github.com/pytest-dev/pytest/blob/main/changelog/README.rst) for details.

  Write sentences in the **past or present tense**, examples:

  * *Improved verbose diff output with sequences.*
  * *Terminal summary statistics now use multiple colors.*

  Also make sure to end the sentence with a `.`.

- [X] Add yourself to `AUTHORS` in alphabetical order.
-->

Adding a warning in `pytest_pyfunc_call` after the `async_warn_and_skip` will check if the return is something other than `None`.

Closes #7337; see that issue for details.

CC @Zac-HD 

Problem statement:
Test functions that return non-None should raise a warning/error
Consider this test:

```python
# The function we're testing
def foo(a: int, b: int) -> int:
  return a * 3 + b

@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
```

Did you spot the error? The second parametrization has a typo, `2 * 3 + 3` is 9, not 8. But this test actually doesn't test anything, because it returns the assertion rather than asserting it. This is a common enough mistake, and it wouldn't normally be a problem except that it can silently cause false positives in test suites.

I propose that test functions that return anything except None fail with a message that cues users that they probably meant to assert rather than return. This feature could be disabled (or enabled, if there are backwards-compatibility issues) via a config flag if necessary.

Hints from issue discussion:
Hi @maxrothman,

Thanks for the suggestion. I appreciate where you are coming from, but I don't think this is a good idea:

* It is somewhat implicit to me that returning `False` from a test function would cause the test to fail. Also, it would fail with what message? `assert` statements are the bread and butter of tests in pytest after all.
* I've seen many times users using plain `return` to stop a test in the middle of the execution (for example, in a parametrized test one of the parameters only goes through the middle of the test). This would cause those test suites to fail.

So while I see that a few users might get confused by that, I don't think it is common enough to create a special case for it and also possibly breaking backward compatibility, so 👎 from me.

This is also a good opportunity to advocate users to see their tests fail first before fixing code, to ensure they are not testing a false positive.

But again, thanks for the suggestion and taking the time to write about it.
Responses inline:

> * It is somewhat implicit to me that returning `False` from a test function would cause the test to fail. Also, it would fail with what message? `assert` statements are the bread and butter of tests in pytest after all.

I think we agree? I'm not suggesting that returning the result of a test should be made valid, I'm suggesting that returning **at all** from a test function is probably a typo and thus should fail loudly with a message indicating that the user probably made a typo.

> * I've seen many times users using plain `return` to stop a test in the middle of the execution (for example, in a parametrized test one of the parameters only goes through the middle of the test). This would cause those test suites to fail.

From my suggestion (emphasis added):
> I propose that test functions that return **anything except None** fail with a message that cues users that they probably meant to assert rather than return.

Plain returns would be allowed, since a plain return implicitly returns `None`.
 
> So while I see that a few users might get confused by that, I don't think it is common enough to create a special case for it and also possibly breaking backward compatibility, so 👎 from me.

If backward-compatibility is an issue (which I suspect it wouldn't be, since no one should be returning anything but None from tests) this feature could be downgraded to a warning (or upgraded from a warning to an error) by a config flag.

> But again, thanks for the suggestion and taking the time to write about it.

Thank you for quickly responding! I hope my comment cleared up some confusion and that you'll reconsider your 👎. In either case, thank you for helping to maintain pytest!
Indeed, my mistake, thanks for clarifying. 

I'm still not convinced that this is common enough to warrant an error/warning however, but I'm changing my vote to -0.5 now that the proposal is clearer to me.

If others provide more use cases or situations where this error is common then I'm not against adding a warning for tests which return non-None. 😁 
(I've changed the title of the issue to better describe the proposal, feel free to edit it further or revert it in case you disagree)
I'm +1 on this - in fact Hypothesis already makes non-`None` returns from wrapped tests into errors.

- it catches some trivial user errors, as noted in the OP
- more importantly, it also catches problems like definining generators as tests (or async test functions), where calling the function doesn't actually execute the test body.  On this basis we could e.g. explicitly recommend `pytest-asyncio` or `pytest-trio` if an awaitable object is returned.

I'd probably do this as a warning to start with, have an option to disable the warning, and then upgrade it to an error (still disabled by the same option) in pytest 6.0
Thanks @Zac-HD for the feedback. 👍 

> I'd probably do this as a warning to start with, have an option to disable the warning, and then upgrade it to an error (still disabled by the same option) in pytest 6.0

Probably a bit late for 6.0, given that 5.5 won't happen so users wouldn't have a chance to see the warning.

Didn't realise it was that soon! Warning in 6.0 then, and error in 7.0 I guess :slightly_smiling_face: 
I'm a little hesitant about this one, I've seen a few testsuites where there's like

```python
def test_one():
    # do some stuff
    # assert some stuff
    return some_info()

def test_two():
    res = test_one()
    # do more things
    # assert more things
```

I'm not saying they're good tests, but forbidding return will break them (the test works fine and returning *something* isn't breaking its correctness)
(though, I do think this will be less painful than the don't-call-fixtures change)
I am guilty of such testsuites myself and I think it's warranted to extract operational components to function

Commit message: Adding myself

Patch to review:
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
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.
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."""
 
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*"])


Code review comments:
diff_hunk: @@ -42,6 +42,13 @@ class PytestCollectionWarning(PytestWarning):
     __module__ = "pytest"
 
 
+@final
+class PytestReturnNotNoneWarning(PytestWarning):
path: src/_pytest/warning_types.py
text: @user1:
Let's inherit from `PytestDeprecationWarning`, since we want to make this an error in future.

@author:
Sure

diff_hunk: @@ -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
+                )
path: src/_pytest/python.py
text: @user1:
```suggestion
                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`?"
```

diff_hunk: @@ -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.
path: changelog/7337.improvement.rst
text: @user1:
I'd drop the second sentence here, since it's describing internal implementation details.