pytest.warns multiple argument handling
#11917
Conversation
src/_pytest/recwarn.py
Outdated
| w.message.__class__, # type: ignore[arg-type] | ||
| w.filename, | ||
| w.lineno, | ||
| message=w.message, # type: ignore[arg-type] |
There was a problem hiding this comment.
w is of type warnings.WarningMessage, which isn't a Warning instance whereas w.message is.
I added this above this line to check
print(f"type(w) is: {type(w)}") # <class 'warnings.WarningMessage'>
print(f"type(w) is subclass of Warning: {issubclass(type(w), Warning)}") # False
print(f"type(w.message) is: {type(w.message)}") # <class 'test_recwarn.TestWarns.test_multiple_arg_custom_warning.<locals>.CustomWarning'>
print(f"type(w.message) is subclass of Warning: {issubclass(type(w.message), Warning)}") # TrueThere was a problem hiding this comment.
Indeed, sorry for the confusion.
So if I understand things correctly, WarningMessage.message can be either a message str or a full Warning, hence the arg-type mypy error. BUT, the old code seems to have assumed that w.message is always a Warning, so maybe it's true here. Still, maybe it'd be safer to appease mypy and write it like this:
if isinstance(w.message, Warning):
message: Union[Warning, str] = w.message
category: Optional[Type[Warning]] = None
else:
message = w.message
category = w.category
warnings.warn_explicit(
message,
category,
...
)There was a problem hiding this comment.
Yes that makes sense!
There was a problem hiding this comment.
Hm, I'm having a bit of trouble with the mypy and the requirements for warn_explicit() (without just type ignoring). Would you mind taking a look?
There was a problem hiding this comment.
After trying it and reading the warn_explicit code, I think we should be good with just this:
message=w.message,
category=w.category,
There was a problem hiding this comment.
LGTM!
Left a small suggest, other than that we also need a changelog, please create changelog/11906.bugfix.rst with something along these lines:
Fix regression using :func:`pytest.warns` with custom warning subclasses which have more than one parameter in their `__init__`.
testing/test_recwarn.py
Outdated
| @@ -477,3 +477,14 @@ def test_catch_warning_within_raise(self) -> None: | |||
| with pytest.raises(ValueError, match="some exception"): | |||
| warnings.warn("some warning", category=FutureWarning) | |||
| raise ValueError("some exception") | |||
|
|
|||
| def test_multiple_arg_custom_warning(self) -> None: | |||
There was a problem hiding this comment.
Let's just add a docstring to mention the original issue for future reference (I know we can eventually find it via git-blame, but often things move around, code is refactored/reformatted, so I think it pays to just slap the issue number along with the test):
| def test_multiple_arg_custom_warning(self) -> None: | |
| def test_multiple_arg_custom_warning(self) -> None: | |
| """pytest.warns with custom warning subclass with multiple arguments (#11906).""" |
|
@reaganjlee There is a conflict, can you rebase on latest main please? |
| @@ -336,7 +332,9 @@ def found_str(): | |||
|
|
|||
| @staticmethod | |||
| def _validate_message(wrn: Any) -> None: | |||
| if not isinstance(msg := wrn.message.args[0], str): | |||
There was a problem hiding this comment.
After making the other changes, the code failed some tests from #11804 that solved #10865, where the incorrect arguments then defaulted to becoming UserWarnings. However, the implementation also caught the custom CustomWarning class (where wrn.message.args[0] = int from CustomWarning's __init__, which this looked to fix.
This looks related to #11954
There was a problem hiding this comment.
Hm, this looks like it could be its own PR potentially. Another implementation: #11959
|
Merged through #11991, thanks @reaganjlee! |
Fixes #11906