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

Make pytest.warns() re-emit all non-matching warnings #10937

Merged
merged 4 commits into from
Jul 1, 2023

Conversation

reaganjlee
Copy link
Contributor

@reaganjlee reaganjlee commented Apr 24, 2023

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?

@RonnyPfannschmidt
Copy link
Member

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

@reaganjlee
Copy link
Contributor Author

reaganjlee commented Apr 25, 2023

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

@RonnyPfannschmidt
Copy link
Member

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

@reaganjlee
Copy link
Contributor Author

reaganjlee commented Jun 6, 2023

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

@Zac-HD
Copy link
Member

Zac-HD commented Jun 20, 2023

Hey Reagan - let's try to get this merged soon!

  • Rebase on the latest main branch
  • I think that we should re-emit all and only warnings which do not match (by type and possibly regex pattern).
  • We'll need to fix the mypy warnings from pre-commit ci; add a changelog entry, and make sure you're in the authors list

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.

@Zac-HD Zac-HD changed the title pytest.warns() re-emit all captured warnings on exit Make pytest.warns() re-emit all non-matching warnings Jun 20, 2023
Copy link
Member

@Zac-HD Zac-HD left a 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!

src/_pytest/recwarn.py Outdated Show resolved Hide resolved
src/_pytest/recwarn.py Outdated Show resolved Hide resolved
src/_pytest/recwarn.py Outdated Show resolved Hide resolved
src/_pytest/recwarn.py Outdated Show resolved Hide resolved
@reaganjlee reaganjlee marked this pull request as ready for review June 23, 2023 09:23
Copy link
Member

@nicoddemus nicoddemus left a 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. 👍

changelog/9288.breaking.rst Outdated Show resolved Hide resolved
src/_pytest/recwarn.py Outdated Show resolved Hide resolved
src/_pytest/recwarn.py Outdated Show resolved Hide resolved
Copy link
Member

@The-Compiler The-Compiler left a 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.

@Zac-HD
Copy link
Member

Zac-HD commented Jun 29, 2023

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.
Copy link
Member

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.

Copy link
Member

@Zac-HD Zac-HD Jun 29, 2023

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.

@nicoddemus
Copy link
Member

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

Agreed, we can do any adjustments later after main is adapted. 👍

@Zac-HD Zac-HD enabled auto-merge July 1, 2023 04:03
@Zac-HD Zac-HD merged commit 679bd6f into pytest-dev:main Jul 1, 2023
@reaganjlee reaganjlee deleted the re-emit branch July 1, 2023 08:23
@pllim
Copy link
Contributor

pllim commented Jul 3, 2023

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 pytest.warns) is emitted, not necessarily that it has to be the only warning. If we want to make sure that it is the only warning, we check the len later. I suspect this change would break multiple downstream packages when released. Is there a way to get the old behavior back? Or did I misdiagnose the downstream failures?

@Zac-HD
Copy link
Member

Zac-HD commented Jul 3, 2023

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 -Werror (that's why it's documented as a breaking change!), but we think ceasing to hide unrelated warnings is worth making that change in Pytest 8.0 - see #11126 for the release plan.

To ignore any other warnings, without asserting that there must be other warnings, you can use the stdlib warnings.catch_warnings() + warnings.simplefilter("ignore") functions as we document here. I contributed a bit of API sugar so from Python 3.11 you can use with catch_warnings(action="ignore"):, but it might be a while before you can rely on that everywhere.


I did notice skimming through the failures that this test (source here) could be affected by #11129 - up until now, a warns() within a raises() did literally nothing at all 😓. This gzip warning also looks real, though I don't know enough about FITS to tell for most of the rest.

@pllim
Copy link
Contributor

pllim commented Jul 3, 2023

Thanks for the clarification! I guess I'll have to go in and clean stuff up. 😸

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.

6 participants