Test functions that return non-None should raise a warning/error

[maxrothman]

Consider this test:

# 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.

[nicoddemus]

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.

[maxrothman]

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!

[nicoddemus]

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. 😁

[nicoddemus]

(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)

[Zac-HD]

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

[nicoddemus]

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.

[Zac-HD]

Didn't realise it was that soon! Warning in 6.0 then, and error in 7.0 I guess 🙂

[asottile]

I'm a little hesitant about this one, I've seen a few testsuites where there's like

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)

[asottile]

(though, I do think this will be less painful than the don't-call-fixtures change)

[RonnyPfannschmidt]

I am guilty of such testsuites myself and I think it's warranted to extract operational components to function

[maxrothman]

I am guilty of such testsuites myself and I think it's warranted to extract operational components to function

I extract common components into tests all the time, but I’d argue that the better design would be to have both tests call a shared function, rather than piggyback one test off of another, and that pytest should encourage better design. In any case, do you think such usage is common? If not, does the config flag address this concern?

[RonnyPfannschmidt]

my note is about refactoring to common functions instead of running other test functions

[gnikonorov]

I think the option given by @Zac-HD sounds like a good path forward

[symonk]

+1 from me, I cannot think of much use in returning from a test in what I would deem 'good practice'.

[RonnyPfannschmidt]

there is one specialc ase - whch is pytest-twisted and deferreds

[nicoddemus]

there is one specialc ase - whch is pytest-twisted and deferreds

Indeed, but if I understand correctly, those tests are marked in a way that plugins will use pytest_pyfunc_call to intercept them and do something special with the return value.

The proposal seem to apply only for "normal" test functions, handled by pytest itself. Correct?

[Zac-HD]

Same for pytest-asyncio and pytest-trio, if I remember correctly. So we should confirm that

• our new check for non-None return is 'outside' the plugin layer that handles async tests
• the various plugins pytest-{asyncio,trio,twisted} either contain an equivalent check or pass through the return value

[graingert]

there is one specialc ase - whch is pytest-twisted and deferreds

Indeed, but if I understand correctly, those tests are marked in a way that plugins will use pytest_pyfunc_call to intercept them and do something special with the return value.

The proposal seem to apply only for "normal" test functions, handled by pytest itself. Correct?

Should be covered by a collections.abc.Awaitable check.

Edit: Yup deferreds are already checked for here:

pytest/src/_pytest/python.py

Line 185 in 5cfd7c0

if hasattr(result, "__await__") or hasattr(result, "__aiter__"):

[asottile]

interestingly enough, unittest in cpython now warns in this situation: python/cpython#27748

[graingert]

Can we upgrade the coroutine warning to error sooner now?