Allow using warnings.warn() with a Warning#11959
Merged
bluetech merged 2 commits intopytest-dev:mainfrom Feb 16, 2024
Merged
Conversation
7031cbd to
b9d756f
Compare
In preparation for next commit.
Test: `warnings.warn()` expects that its first argument is a `str` or a `Warning`, but since 9454fc3 `pytest.warns()` no longer allows `Warning` instances unless the first argument the `Warning` was initialized with is a `str`. Furthermore, if the `Warning` was created without arguments then `pytest.warns()` raises an unexpected `IndexError`. The new tests reveal the problem. Fix: `pytest.warns()` now allows using `warnings.warn()` with a `Warning` instance, as is required by Python, with one exception. If the warning used is a `UserWarning` that was created by passing it arguments and the first argument was not a `str` then `pytest.raises()` still considers that an error. This is because if an invalid type was used in `warnings.warn()` then Python creates a `UserWarning` anyways and it becomes impossible for `pytest` to figure out if that was done automatically or not. [ran: rebased on previous commit]
b9d756f to
0475b1c
Compare
bluetech
approved these changes
Feb 16, 2024
Member
bluetech
left a comment
There was a problem hiding this comment.
Thanks for the patch and the clear description.
I added a commit and rebase yours on top to make things a bit clearer, but the logic is the same except I removed the isinstance(wrn, Warning) check which I believe was unnecessary and made the typing not work out.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
warnigns.warn()expects its first argument to be astror aWarning(see alsoWarningMessagein typeshed) and using a different type can cause difficult to diagnose errors (as was reported in #10865):Recently #11804 tried to help address this problem by making
pytest.warns()check the type that was passed towarnings.warn(), but the checks are now too strict:Both uses are explicitly permitted by Python. Furthermore, in the second case the
IndexErroris completely unexpected.With my patch
pytest.warns()allowswarnings.warn()to be used with aWarninginstance, except if aUserWarningis created by passing it arguments and the first argument is not astr. This is because if an invalid type was used inwarnings.warn()then Python creates aUserWarninganyways and it becomes impossible forpytestto figure out if that was done automatically or not. However, even with that shortcomingpytest.warns()still behaves much better than it does right now.Closes #11954
Here is a quick checklist that should be present in PRs.