-
-
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
Make pytest.warns()
re-emit all non-matching warnings
#10937
Conversation
this one is tricky, as the key question to solve here is whether to remove one or all matched warnings unlike the case of only checking, if a match exists, now we need a decision on what to re-emmit i believe the possibly desired use-cases are all, unmatched, "all but first match" and its going to be hard to decide a good api for this |
Since we want to reduce number of refactors (and potentially, number of optional arguments), we can assume that if a user specifies the particular class they'd like to catch warnings from, then they would use this same warning type for similar checks, leading us to filter that exception class and remitting only the different, unexpected exceptions from the one specified? If there is a specific match given, then ignore this and do the usual re-emit besides the one/s found. This may or may not still lead to some of the existing, unrelated tests failing, though I can implement this soon and see how that goes |
I'm currently not able to allocate some time to write the details down to reiterate the options My current recommendation would be start with a implementation that filters all matching warnings and then Iterating on documentation for a future api where the other use cases can be served |
Let me know if there are any documentation issues to fix for this. And if this works, I can fix up the code to match |
Hey Reagan - let's try to get this merged soon!
and I think that's it; your docs look good to me and I don't think we need to handle any additional complexity here. |
pytest.warns()
re-emit all non-matching warnings
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.
As you note, we'll need a changelog, docs, and adding you to the authors list (🎊); after those, fixing the mypy CI errors, and comments below I think we'll be ready to merge!
31695a6
to
bec07d8
Compare
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.
Thanks @reaganjlee and everyone for the comments.
Left a few smaller comments too, other than that LGTM. 👍
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 don't think I'll be able to review this - I barely ever use warnings (given my main project is an application rather than a library), so I'd rather leave this to someone else.
Thanks @nicoddemus, all done! Since Python 3.7 is now end-of-life, we should probably test on a newer version of PyPy (3.8 through 3.10 are now supported) and remove the old CI jobs. I'd rather not add that into this long-running PR though 😅 |
|
||
if not (exc_type is None and exc_val is None and exc_tb is None): | ||
# We currently ignore missing warnings if an exception is active. | ||
# TODO: fix this, because it means things are surprisingly order-sensitive. |
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.
Not sure what this TODO means here... can we clarify it? Also not clear if this is something meant to be done still in this PR, or later.
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.
Ah, that's a note-to-self about rebasing #11129, which I'll do immediately after this is merged.
Agreed, we can do any adjustments later after |
I think this broke downstream tests reported at astropy/astropy#15015 . Our intention of the affected tests are to make sure a particular warning (that we are checking with |
I think you're correct in tracing the change to this PR. We're aware that this will lead to more warnings being emitted, which can make tests fail with To ignore any other warnings, without asserting that there must be other warnings, you can use the stdlib I did notice skimming through the failures that this test (source here) could be affected by #11129 - up until now, a |
Thanks for the clarification! I guess I'll have to go in and clean stuff up. 😸 |
PR Description:
This issue fixes part of issue #9288 by allowing
pytest.warns()
to emit all unmatched warnings.One question I have with the design is how to handle the exceptions once we re-emit them. Having
pytest.warns()
calls return all exceptions will require refactoring of other tests. There doesn't seem to be a way to check whether the code is within a WarningsChecker frame, so a way to solve this is an optional-argument for re-emitting?