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

Allow using warnings.warn() with a Warning #11959

Merged
merged 2 commits into from Feb 16, 2024

Conversation

eerovaher
Copy link
Contributor

warnigns.warn() expects its first argument to be a str or a Warning (see also WarningMessage in typeshed) and using a different type can cause difficult to diagnose errors (as was reported in #10865):

>>> import warnings
>>> with warnings.catch_warnings():
...     warnings.filterwarnings("ignore", "test")
...     warnings.warn("Warning!")
...     warnings.warn(RuntimeWarning())
...     warnings.warn(0)
... 
<stdin>:3: UserWarning: Warning!
<stdin>:4: RuntimeWarning: 
Traceback (most recent call last):
  File "<stdin>", line 5, in <module>
TypeError: expected string or bytes-like object

Recently #11804 tried to help address this problem by making pytest.warns() check the type that was passed to warnings.warn(), but the checks are now too strict:

>>> import warnings
>>> import pytest
>>> with pytest.warns(Warning):
...     warnings.warn(RuntimeWarning(["warning", "context"]))
... 
Traceback (most recent call last):
    ...
    raise TypeError(
TypeError: Warning message must be str, got ['warning', 'context'] (type list)
>>> with pytest.warns(Warning):
...     warnings.warn(RuntimeWarning())
... 
Traceback (most recent call last):
    ...
IndexError: tuple index out of range

Both uses are explicitly permitted by Python. Furthermore, in the second case the IndexError is completely unexpected.

With my patch pytest.warns() allows warnings.warn() to be used with a Warning instance, except if a UserWarning is created by passing it arguments and the first argument is not a str. 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. However, even with that shortcoming pytest.warns() still behaves much better than it does right now.

Closes #11954

Here is a quick checklist that should be present in PRs.

  • Include documentation when adding new features.
  • Include new tests or update existing tests when applicable.
  • Allow maintainers to push and squash when merging my commits. Please uncheck this if you prefer to squash the commits yourself.

bluetech and others added 2 commits February 16, 2024 13:41
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]
Copy link
Member

@bluetech bluetech left a comment

Choose a reason for hiding this comment

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

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.

@bluetech bluetech merged commit 6ef0cf1 into pytest-dev:main Feb 16, 2024
24 checks passed
@eerovaher eerovaher deleted the warn-message-type branch February 16, 2024 20:50
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.

pytest 8.1.0.dev gives TypeError: Warning message must be str
2 participants