Deprecate expected_warnings in favor of pytest.warns

Contributor & Development Discussion scikit-image
1

Is there a reason we are still using our own custom skimage._shared._warnings.expected_warnings instead of pytest’s tooling? Currently, I don’t see functionality that can’t be replaced. I propose to remove this to save some maintenance cost down the line.

I’ll wait at least a week for input and if no concerns are raised I’ll start a PR then.

2

I think that decorator dates back to nose days! :joy:

Separately, some core devs felt burned by the nose->pytest transition and were reluctant to use pytest tooling directly, instead importing into skimage._shared.testing and using that as an adapter — that way if we had to migrate again we could do it.

However, I always thought that was overkill. Pytest is now standard across the ecosystem and I feel comfortable depending directly on it. So I’m :+1: on using pytest.warns directly.

3

If pytest supports all the functionality we need, +1 on using it directly.

4

I am in favor of this. In several prior PRs I had done a similar thing for testing.raises → pytest.raises, but only in the files that were already being modified for the PR. Another general refactoring I have done periodically was to remove inhertance from unittest.TestCase for class-based tests. There are likely still remaining cases of each of those that could be addressed as well.

5

Just had a more thorough look into this. Unfortunately it seems that pytest.warns does swallow unexpected warnings as long as it finds the expected one. E.g.

import warnings
import pytest
from skimage._shared._warnings import expected_warnings

def foo():
    warnings.warn("bar", category=UserWarning)
    warnings.warn("baz", category=RuntimeWarning)

with pytest.warns(RuntimeWarning):
    foo()  # UserWarning "bar" is silently ignored

with expected_warnings(["baz"]):
    foo()  # ValueError: Unexpected warning: bar

Furthermore, expected_warnings provides this additional functionality that Pytest is lacking

Finally, you can use |\A\Z in a pattern to signify it as optional.

Only disadvantage I see right now is that our expected_warnings can’t test the warning type and is a maintenance burden.