Filter out doctest warning

[max-sixty]

Trying to fix #8537. Not sure it'll work and can't test locally so seeing if it passes CI

[max-sixty]

No luck. Possibly we could add a filter in an autouse fixture for the doctests? Or just skip that test?

[keewis]

I think the issue is that the warning is emitted at test collection time (when executing one of the conftest.py files), so the filterwarnings does not apply there. It also does not appear to be possible to apply a mark to all doctest tests (otherwise we could have used pytest.mark.filterwarnings("error")).

So I guess the only option we have is using the CLI:

pytest --doctest-modules xarray --ignore xarray/tests -Werror -W "ignore:h5py is running against HDF5 1.14.3:UserWarning"

[kmuehlbauer]

Seems to be pinning problem over at conda-forge: conda-forge/h5py-feedstock#122 (see latest comments there).

[keewis]

Seems to be pinning problem over at conda-forge: conda-forge/h5py-feedstock#122 (see latest comments there).

As expected, so there is indeed nothing we can do about this besides silencing the warning.

For now, I've added a ignore to the CLI, but we'll have to come up with a strategy for future cases.

[headtr1ck]

Did you try adding it to xarray/tests/__init__.py? There are already a few ignores.

[max-sixty]

For now, I've added a ignore to the CLI, but we'll have to come up with a strategy for future cases.

Thanks! Closing

Did you try adding it to xarray/tests/__init__.py? There are already a few ignores.

Good point — maybe we can do it at import time. (though not that different from adding to CLI I guess, since the only time it errors is with our CLI arguments that convert warnings to errors — which I think is great overall)

[keewis]

Did you try adding it to xarray/tests/__init__.py? There are already a few ignores.

no, I totally missed that. I agree with @max-sixty though that having the ignore in the same place as where we enable raising warnings is a good property, especially since it then will not be ignored in the tests (although right now that's not so useful since we have hundreds if not thousands of warnings). So we might want to migrate those warnings that should only be ignored in doctest?

The downside, though, is that trying to reproduce CI failures means that we have to copy a potentially very long command... so that would be an argument for keeping them in xarray.tests.__init__.

For now, I've added a ignore to the CLI

Thanks! Closing

what I meant was that I modified this PR, so we'd still have to merge it.

[max-sixty]

what I meant was that I modified this PR, so we'd still have to merge it.

lol, sorry, reopening!

[max-sixty]

having the ignore in the same place as where we enable raising warnings is a good property, especially since it then will not be ignored in the tests (although right now that's not so useful since we have hundreds if not thousands of warnings). So we might want to migrate those warnings that should only be ignored in doctest?

I would say it depends on "do we want to oversee the warnings?"

• for warnings that are temporary and we know we can always ignore ('urllib3.contrib.pyopenssl' module is deprecated, as an example), putting it in the __init__ means we don't have to litter the tests with their ignores. That's often the case with warnings from dependencies, and almost always the case with dependencies of dependencies, where we don't have control.
• and then for more specific warnings — those in xarray, and those in some close dependencies, it makes sense to have them next to the tests, where we want to oversee them

...probably both of those are better than the CLI. Though I'm not sure the __init__ covers the doctests, does xarray.tests.__init__ get run before doctest collections?

[keewis]

does xarray.tests.__init__ get run before doctest collections?

Yes, because we import it in xarray/tests/conftest.py

But yes, I agree it is probably better to put them in xarray.tests. I'd argue that we should ignore warnings that are emitted by imports in xarray.tests, and anything else should be ignored in the tests (unless way too many tests would emit these, in which case it might make sense to also do so in xarray.tests).

Either way, we definitely need to point out why we ignore a certain warning (in general a link to the issue / PR with a short explanation should be enough), and maybe also list criteria that allow removing them?

[max-sixty]

Either way, we definitely need to point out why we ignore a certain warning (in general a link to the issue / PR with a short explanation should be enough), and maybe also list criteria that allow removing them?

Agree!

I'd argue that we should ignore warnings that are emitted by imports in xarray.tests, and anything else should be ignored in the tests (unless way too many tests would emit these, in which case it might make sense to also do so in xarray.tests).

I agree with the direction. Though I would put the bar for "way too many tests would emit these" quite low for things we can't control, or that don't show up at runtime (e.g. PendingDeprecationWarning)

In particular, if it means that we silence more of the warnings, that's great — our test suite is hardly too restrictive about showing warnings which we can't do anything about atm...