Skip to content

Reintroduce PytestReturnNotNoneWarning #13495

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jun 16, 2025

Conversation

nicoddemus
Copy link
Member

Since this warning is meant to be permanent, added proper documentation to the assert section in the docs.

Fixes #13477

@psf-chronographer psf-chronographer bot added the bot:chronographer:provided (automation) changelog entry is part of PR label Jun 7, 2025
@nicoddemus
Copy link
Member Author

Do we consider this a bugfix that is OK to enter in the next patch release, or should we wait for 8.5?

@RonnyPfannschmidt
Copy link
Member

We should backport as we correct a regression

Since this warning is meant to be permanent, added proper documentation to the `assert` section in the docs.

Fixes pytest-dev#13477
@nicoddemus nicoddemus force-pushed the 13477-none-warning branch from 3c8dff2 to 1e1c68d Compare June 7, 2025 12:52
Copy link
Member

@Pierre-Sassoulas Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, I think it's a bug fix to be backported

@nicoddemus nicoddemus added the backport 8.4.x apply to PRs at any point; backports the changes to the 8.4.x branch label Jun 7, 2025
@nicoddemus
Copy link
Member Author

I'm getting the same failure on main locally.

Testing main on #13496 to confirm the failure is unrelated to these changes.

@nicoddemus
Copy link
Member Author

Blocked by #13497, will wait for us to fix that before merging this.

@RonnyPfannschmidt
Copy link
Member

The related issue is possibly something larger to fix so id prefer we dont block on it

)
def test_foo(a, b, result):
assert foo(a, b) == result
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should maybe have some blurb on why you might want to return values?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this will only confuse readers. The official pytest documentation should continue to advise against returning anything from test functions. If plugins wish to implement their functionality using return values, they can describe their rationale in their own documentation.

Copy link

@cliffckerr cliffckerr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good -- thanks! Apologies if this comment is off the mark, but do we need to separately handle the await case (#11372) that was the original rationale for changing the warning to an error?

@nicoddemus
Copy link
Member Author

nicoddemus commented Jun 16, 2025

Apologies if this comment is off the mark, but do we need to separately handle the await case (#11372) that was the original rationale for changing the warning to an error?

If I understand correctly your question, no. The state of affairs will be:

  • async tests will always fail if not handled by a plugin.
  • Normal tests will always generate a warning if they return non-None.

@nicoddemus nicoddemus merged commit 53f05c4 into pytest-dev:main Jun 16, 2025
54 of 58 checks passed
Copy link

patchback bot commented Jun 16, 2025

Backport to 8.4.x: 💚 backport PR created

✅ Backport PR branch: patchback/backports/8.4.x/53f05c44d9530c4ac5ce5804ef75fe61713d46d8/pr-13495

Backported as #13527

🤖 @patchback
I'm built with octomachinery and
my source is open — https://github.com/sanitizers/patchback-github-app.

patchback bot pushed a commit that referenced this pull request Jun 16, 2025
Since this warning is meant to be permanent, added proper documentation to the `assert` section in the docs.

Fixes #13477

(cherry picked from commit 53f05c4)
nicoddemus added a commit that referenced this pull request Jun 17, 2025
Since this warning is meant to be permanent, added proper documentation to the `assert` section in the docs.

Fixes #13477

(cherry picked from commit 53f05c4)

Co-authored-by: Bruno Oliveira <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport 8.4.x apply to PRs at any point; backports the changes to the 8.4.x branch bot:chronographer:provided (automation) changelog entry is part of PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Undocumented removal of PytestReturnNotNoneWarning
5 participants