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

MAINT: Deal with pytest 8.0 #12376

Merged
merged 10 commits into from
Jan 29, 2024
Merged

MAINT: Deal with pytest 8.0 #12376

merged 10 commits into from
Jan 29, 2024

Conversation

larsoner
Copy link
Member

@larsoner larsoner commented Jan 22, 2024

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:

  1. Chain multiple context managers to catch all warnings
  2. Add an outer context manager just to swallow other warnings
  3. Roll our own warns(...) variant with a reemit=False default

I 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

* 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",
Copy link
Member Author

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?

@larsoner larsoner added this to the 1.7 milestone Jan 24, 2024
@larsoner
Copy link
Member Author

Self-merging, now that 8.0 is out we need this

@larsoner larsoner merged commit 990ce18 into mne-tools:main Jan 29, 2024
29 checks passed
@larsoner larsoner deleted the pytest branch January 29, 2024 16:09
snwnde pushed a commit to snwnde/mne-python that referenced this pull request Mar 20, 2024
Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>
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.

MAINT: pip-pre failures
1 participant