Skip to content
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

Do not make use of pytest.warns(None) #868

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

s-t-e-v-e-n-k
Copy link

pytest.warns(None) is an anti-pattern, and there are easier ways to accomplish both cases with how the code is leveraging pytest.warns(). Firstly, it can match on a specific warning and will assert that the warning is raised -- and it doesn't need to be used if we want to assert code raises no warnings, the standard library can do that directly. Furthermore, pytest.warns(None) is an error in Pytest 8 and above.

s-t-e-v-e-n-k and others added 2 commits December 16, 2024 14:47
pytest.warns(None) is an anti-pattern, and there are easier ways to
accomplish both cases with how the code is leveraging pytest.warns().
Firstly, it can match on a specific warning and will assert that the
warning is raised -- and it doesn't need to be used if we want to assert
code raises no warnings, the standard library can do that directly.
Furthermore, pytest.warns(None) is an error in Pytest 8 and above.
@clintonsteiner
Copy link
Collaborator

This is no longer needed due to similar test fixes which have been merged

@s-t-e-v-e-n-k
Copy link
Author

with warnings.catch_warnings() is not sufficient, since it will swallow any warnings emitted, which is not what the test case is trying to accomplish. Setting the warnings filter to error will raise them as exceptions, like so:

with warnings.catch_warnings():
            warnings.simplefilter("error")

I'm happy to rebase this PR to just make that change.

@clintonsteiner
Copy link
Collaborator

Good callout, please do so

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants