Give warning when test function return other than None #9956

Cheukting · 2022-05-13T20:51:44Z

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

Zac-HD

Looks good to me - if you can confirm that this doesn't trigger when used with pytest-twisted, I'll be happy to merge 🥳

src/_pytest/warning_types.py

src/_pytest/python.py

changelog/7337.improvement.rst

Cheukting · 2022-05-13T21:25:02Z

Looks good to me - if you can confirm that this doesn't trigger when used with pytest-twisted, I'll be happy to merge 🥳

I saw there is a test file pytest_twisted_integration.py. Does it test pytest-twisted? Or do I have to create new tests?

Zac-HD · 2022-05-13T21:25:51Z

Looks good to me - if you can confirm that this doesn't trigger when used with pytest-twisted, I'll be happy to merge 🥳

I saw there is a test file pytest_twisted_integration.py. Does it test pytest-twisted? Or do I have to create new tests?

Without looking, I'd have gone and installed pytest-twisted, ran the inlineCallbacks and ensureDeferred examples to check that they work with the main branch and also (without warnings) on this PR. It looks like that test file does pretty much exactly that though, so if it's passing we're all good 💪

Cheukting · 2022-05-13T21:48:48Z

Seems fnmatch_lines works differently in Python3.7 and Python 3.8, This one https://github.com/pytest-dev/pytest/runs/6429987533?check_suite_focus=true#step:5:85 fails at 3.8 but passes at 3.7

Zac-HD

Looks good to me! I'll give others a few days to review, but inclined to merge if I don't hear any objections.

Thanks for the patch, Cheuk 🤩

The-Compiler

Thanks @Cheukting, that seems quite useful! There's a bit of documentation and an addition to the public API missing - after that, this seems good to merge to me!

src/_pytest/python.py

The-Compiler · 2022-05-14T10:47:26Z

src/_pytest/warning_types.py

@@ -55,6 +55,13 @@ class PytestRemovedIn8Warning(PytestDeprecationWarning):
    __module__ = "pytest"


+@final
+class PytestReturnNotNoneWarning(PytestDeprecationWarning):


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.

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?

This should be a warning, not a deprecation warning

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

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

Co-authored-by: Florian Bruhin <me@the-compiler.org>

src/_pytest/python.py

Co-authored-by: Bruno Oliveira <nicoddemus@gmail.com>

for more information, see https://pre-commit.ci

changelog/7337.improvement.rst

doc/en/deprecations.rst

for more information, see https://pre-commit.ci

nicoddemus

Thanks a lot @Cheukting for the contribution! 👍

The-Compiler

I'm always a bit confused about how deprecations work in pytest - I feel like we should document this better somewhere... @nicoddemus @bluetech @RonnyPfannschmidt am I correct with my comment? Should PytestReturnNotNoneWarning really be its own class, or should it be an instance of PytestRemovedIn8Warning (or PytestRemovedIn9Warning even?) like some others in src/_pytest/deprecated.py?

Apologies for the delay and somewhat bumpy road @Cheukting!

doc/en/deprecations.rst

RonnyPfannschmidt · 2022-05-20T09:04:55Z

It should be a normal warning

The new class should be a subclass of user warning

RonnyPfannschmidt · 2022-05-20T09:05:35Z

Also, good work, I'd love to get it in

Zac-HD · 2022-05-20T09:28:15Z

It should be a normal warning

The new class should be a subclass of user warning

I disagree; IMO this should be a deprecation warning. I'd like to make it an error in future, and it seems like CPython is also going that way with a deprecation warning in python/cpython#27748 (for 3.11).

RonnyPfannschmidt · 2022-05-20T09:54:27Z

in that case, lets go for Removed in pytest 8

Co-authored-by: Florian Bruhin <me@the-compiler.org>

The-Compiler

This seems ready now - @RonnyPfannschmidt @nicoddemus?

graingert · 2022-06-01T16:33:11Z

in that case, lets go for Removed in pytest 8

NotNoneWarning should subclass RemovedIn8Warning https://github.com/pytest-dev/pytest/pull/10012/files#diff-a7738833b05551853edda7933c165b6d7a41d521afb130fdc68631bd9129531eR59

graingert · 2022-06-01T16:36:19Z

in that case, lets go for Removed in pytest 8

It's not possible to subclass RemovedIn8Warning because it's decorated with @final

…0196) As discussed in #9956 (comment). Also added PytestRemovedIn8Warning to the reference docs.

Cheukting added 4 commits May 13, 2022 18:54

Give warning when test function return other than None

88634bd

Adding test

460410b

Adding changelog

c06605a

Adding myself

238a5ca

Zac-HD reviewed May 13, 2022

View reviewed changes

src/_pytest/warning_types.py Outdated Show resolved Hide resolved

src/_pytest/python.py Outdated Show resolved Hide resolved

changelog/7337.improvement.rst Outdated Show resolved Hide resolved

update changelog, warning msg and warning inherit

7855fde

Cheukting changed the title ~~[WIP] Give warning when test function return other than None~~ Give warning when test function return other than None May 13, 2022

Cheukting force-pushed the warning_return_non_none branch from b6fdec5 to ffad3f8 Compare May 13, 2022 21:37

update test

03c1e51

Cheukting force-pushed the warning_return_non_none branch from ffad3f8 to 03c1e51 Compare May 13, 2022 21:42

Zac-HD approved these changes May 13, 2022

View reviewed changes

The-Compiler requested changes May 14, 2022

View reviewed changes

Cheukting and others added 3 commits May 15, 2022 09:39

Update src/_pytest/python.py

de60ac1

Co-authored-by: Florian Bruhin <me@the-compiler.org>

import NoReturnWarning

175d0c7

adding reference

0c832ca

Cheukting force-pushed the warning_return_non_none branch from 1b589da to bc2df82 Compare May 15, 2022 09:29

deprecated reference

a1f26dd

Cheukting force-pushed the warning_return_non_none branch from bc2df82 to a1f26dd Compare May 15, 2022 09:32

Cheukting requested a review from The-Compiler May 16, 2022 03:29

nicoddemus requested changes May 17, 2022

View reviewed changes

src/_pytest/python.py Outdated Show resolved Hide resolved

nicoddemus reviewed May 17, 2022

View reviewed changes

src/_pytest/python.py Outdated Show resolved Hide resolved

Cheukting and others added 3 commits May 17, 2022 14:54

Update src/_pytest/python.py

ff3d9b2

Co-authored-by: Bruno Oliveira <nicoddemus@gmail.com>

Update acceptance_test.py

c0b368d

[pre-commit.ci] auto fixes from pre-commit.com hooks

d8c5224

for more information, see https://pre-commit.ci

nicoddemus reviewed May 17, 2022

View reviewed changes

changelog/7337.improvement.rst Outdated Show resolved Hide resolved

doc/en/deprecations.rst Outdated Show resolved Hide resolved

nicoddemus and others added 2 commits May 17, 2022 11:43

Improve changelog and docs

fc92f5e

[pre-commit.ci] auto fixes from pre-commit.com hooks

aecdc39

for more information, see https://pre-commit.ci

nicoddemus approved these changes May 17, 2022

View reviewed changes

The-Compiler reviewed May 20, 2022

View reviewed changes

doc/en/deprecations.rst Outdated Show resolved Hide resolved

Update doc/en/deprecations.rst

78e0b67

Co-authored-by: Florian Bruhin <me@the-compiler.org>

The-Compiler approved these changes May 24, 2022

View reviewed changes

nicoddemus merged commit c988e49 into pytest-dev:main May 25, 2022

graingert mentioned this pull request Jul 29, 2022

Update :class:pytest.PytestUnhandledCoroutineWarning to a deprecation; it will raise an error in pytest 8. [SQUASH] #10012

Merged

graingert mentioned this pull request Oct 9, 2022

PytestReturnNotNoneWarning now subclasses PytestRemovedIn8Warning #10196

Merged

nicoddemus pushed a commit that referenced this pull request Oct 9, 2022

PytestReturnNotNoneWarning now subclasses PytestRemovedIn8Warning (#1…

8a40fc5

…0196) As discussed in #9956 (comment). Also added PytestRemovedIn8Warning to the reference docs.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Give warning when test function return other than None #9956

Give warning when test function return other than None #9956

Cheukting commented May 13, 2022 •

edited by Zac-HD

Zac-HD left a comment

Cheukting commented May 13, 2022

Zac-HD commented May 13, 2022

Cheukting commented May 13, 2022 •

edited

Zac-HD left a comment

The-Compiler left a comment

The-Compiler May 14, 2022

Cheukting May 15, 2022

RonnyPfannschmidt May 20, 2022

The-Compiler May 20, 2022

nicoddemus May 25, 2022 •

edited

nicoddemus left a comment

The-Compiler left a comment •

edited

RonnyPfannschmidt commented May 20, 2022

RonnyPfannschmidt commented May 20, 2022

Zac-HD commented May 20, 2022

RonnyPfannschmidt commented May 20, 2022

The-Compiler left a comment

graingert commented Jun 1, 2022

graingert commented Jun 1, 2022

Give warning when test function return other than None #9956

Give warning when test function return other than None #9956

Conversation

Cheukting commented May 13, 2022 • edited by Zac-HD

Zac-HD left a comment

Choose a reason for hiding this comment

Cheukting commented May 13, 2022

Zac-HD commented May 13, 2022

Cheukting commented May 13, 2022 • edited

Zac-HD left a comment

Choose a reason for hiding this comment

The-Compiler left a comment

Choose a reason for hiding this comment

The-Compiler May 14, 2022

Choose a reason for hiding this comment

Cheukting May 15, 2022

Choose a reason for hiding this comment

RonnyPfannschmidt May 20, 2022

Choose a reason for hiding this comment

The-Compiler May 20, 2022

Choose a reason for hiding this comment

nicoddemus May 25, 2022 • edited

Choose a reason for hiding this comment

nicoddemus left a comment

Choose a reason for hiding this comment

The-Compiler left a comment • edited

Choose a reason for hiding this comment

RonnyPfannschmidt commented May 20, 2022

RonnyPfannschmidt commented May 20, 2022

Zac-HD commented May 20, 2022

RonnyPfannschmidt commented May 20, 2022

The-Compiler left a comment

Choose a reason for hiding this comment

graingert commented Jun 1, 2022

graingert commented Jun 1, 2022

Cheukting commented May 13, 2022 •

edited by Zac-HD

Cheukting commented May 13, 2022 •

edited

nicoddemus May 25, 2022 •

edited

The-Compiler left a comment •

edited