Skip to content

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)
# 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
@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