-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
MAINT: Deal with pytest 8.0 #12376
MAINT: Deal with pytest 8.0 #12376
Conversation
* upstream/main: MAINT: Check for shadowing and mutable defaults (mne-tools#12380) Bump actions/cache from 3 to 4 (mne-tools#12374) MAINT: Work around pytest issue (mne-tools#12377) [pre-commit.ci] pre-commit autoupdate (mne-tools#12378)
@@ -110,7 +110,7 @@ full = [ | |||
|
|||
# Dependencies for running the test infrastructure | |||
test = [ | |||
"pytest!=8.0.0rc1,!=8.0.0rc2", | |||
"pytest>=8.0.0rc2", |
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.
@drammock we have to do this if we want to do:
with pytest.warns(...), pytest.warns(...):
because on <8
the inner context manager won't reemit the warnings but on >=8
it will.
Alternatively I could change all instances to with _record_warnings(), pytest.warns(...)
(i.e., a single pytest.warns
being caught) but then we lose the option to have extra specificity.
I don't love either option really... a third option would be to have a with also_warns(...), pytest.warns(...)
where also_warns == pytest.warns
for 8+ and also_warns == _record_warnings
(i.e., noop) when on <8
. But this is custom code again, at least for a while until we do want to pin to 8+ then we can substitute pytest.warns
for all second_warns
. Maybe that's the safest option here?
Self-merging, now that 8.0 is out we need this |
Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>
See pytest-dev/pytest#10937 (comment), TL;DR is that
pytest.warns
in 8.0 will re-emit any unmatched warnings. So in effect our options are, for every call where this happens (about 100 according to this run), do one or a combination of:warns(...)
variant with areemit=False
defaultI don't like (3) because the more we specialize our code the more hard it becomes to work with and come from other packages. (2) is the easiest and probably most appropriate for some cases actually where we get multiple warnings -- like in this first
test_misc.py
we get multiple "event missing" warnings we don't care about and rather than catch a bunch of them, just make sure the one we care about does get emitted. (In the volume source estimate cases this could be dozens of warnings, so impractical there as well.) And (1) could be ideal in some cases.So I think my plan is to do a combination of (1) and (2) based on judgment of the ~85 currently failing tests. It'll take some time but hopefully end up readable. Okay @drammock ?
Closes #12372