-
-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
[MRG][feature] Change warns signature to mimic the raises call #2711
[MRG][feature] Change warns signature to mimic the raises call #2711
Conversation
@nicoddemus Before diving into the code, I would like to get some feedback with the functionality itself. Could you check the tests I've written and ping to the appropriated developers to provide feedback before writing the actual feature. Maybe you guys have different view of the problem (like what you pointed in #2708) |
testing/test_recwarn.py
Outdated
|
||
def test_message_using(self): | ||
source = "warnings.warn('my runtime warning', RuntimeWarning)" | ||
pytest.warns(RuntimeWarning, source, message='my runtime warning') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warns should not replicate the old call style , and even more so for the execute code snippets style, its just an accident of history we should stay clear of
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so, warnings should always be used within a context manager?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've changed test_message_using
in favor of test_message
. (The using was a copy-paste error)
44e4e2a
to
705c214
Compare
Hi @massich sorry for not responding earlier, this one fell through the cracks. The tests look good to me. The question of whatever we should match a single warning or error out vs match any of the warnings, I think the second is more practical so I would go for it. I definitely agree we should support only the context manager case and treat function-form and (specially) string-eval forms as history. cc @flub @hpk42 @The-Compiler would you guys like to share your opinions here? |
705c214
to
47e17cb
Compare
47e17cb
to
d8ecca5
Compare
I don't really have an opinion on this, FWIW - I treat all warnings as errors 😉 |
I got |
Hi, tests seem fine but I do t see any for the message kwarg. Should that not have been included according to the description? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is looking great!
This needs some docs and a changelog entry, aside my other minor comments.
with pytest.warns(UserWarning, match=r'must be \d+$'): | ||
warnings.warn("value must be 42", UserWarning) | ||
|
||
with pytest.raises(pytest.fail.Exception): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please include a case which matches with match
but the warning raised is wrong:
with pytest.raises(pytest.fail.Exception):
with pytest.warns(UserWarning, match=r'must be \d+$'):
warnings.warn("value must be 42", RuntimeWarning)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done.
_pytest/recwarn.py
Outdated
elif self.match_expr is not None: | ||
for r in self: | ||
if issubclass(r.category, self.expected_warning): | ||
if compile(self.match_expr).search(str(r.message)): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is minor, but you are shadowing the builtin compile
. I suggest changing to re.compile
and import re
instead.
8a77dfa
to
aa6a670
Compare
The changelog of the feature branch is outdated. Souldn't it have a 3.3.0 section? |
@massich we use towncrier to stop the merge pain - please add a news fragment file in the changelog folder |
Woo thats cool. Thanks. |
well done, thanks 👍 |
fixes #2708 providing warns with
message
andmatch
parameters (similarly topytest.raises
. see #2227)