Returning fig is incompatible with pytest 7.2 (when test uses marker but pytest-mpl not installed)

[pllim]

As @WilliamJamieson discovered in astropy/astropy#13892 , we now see PytestReturnNotNoneWarning with pytest 7.2 that will eventually become an exception in future releases. If there is no way to opt out (asking at pytest-dev/pytest#10427), can this plugin still work without return fig?

[nicoddemus]

Hey, pytest maintainer here.

Here is the related code in pytest:

https://github.com/pytest-dev/pytest/blob/f07017f91b6af5a7c6942868cb61aea72be4ac31/src/_pytest/python.py#L188-L205

I see pytest-mpl uses a hookwrapper over pytest_runtest_call, what might be needed is to make a hookwrapper over pytest_pyfunc_call which also sets the return value to None.

https://pluggy.readthedocs.io/en/stable/#wrappers

[WilliamJamieson]

I don't think there is an issue here. If pytest-mpl is installed, the "figure" tests in astropy don't produce the PytestReturnNotNoneWarning.

The issue in astropy/astropy#13892 appears to be that one of the CI jobs which uses remote-data (unskipping the figure tests) does install have pytest-mpl. All the tests at issue use this wrapper: https://github.com/astropy/astropy/blob/8a3cd43d45722dbb2fbc03e63bfef61320ac1b0e/astropy/tests/figures/helpers.py#L8-L44, which apparently does not require pytest-mpl even though it seems to be using it (https://github.com/astropy/astropy/blob/8a3cd43d45722dbb2fbc03e63bfef61320ac1b0e/astropy/tests/figures/helpers.py#L29-L32).

There are two options:

1. Add a skip in this wrapper so that if pytest-mpl is not installed, then these tests get skipped.
2. Add pytest-mpl as a dependency to the CI jobs which use remote-data.

My preference is for option 1.

[pllim]

My vote is also 1 since I don't find these tests very useful without this plugin.

[WilliamJamieson]

The only thing that those tests do is exercise some of the astropy.visualization code meaning it "tests" that it doesn't raise an error in matplotlib. But the circle CI (the one that uses pytest-mpl) also runs these tests, but actually tests the resulting figures.

[pllim]

Sure, but CircleCI one is the true purpose of those tests. I think "just see if it crash or not" is partly covered by doctest anyway. Though if you are relying on the job without pytest-mpl to calculate coverage... 💭

[pllim]

Then again, (2) isn't a foolproof solution. Anyone running these tests with remote data locally but didn't install pytest-mpl would have the same problem.

[astrofrog]

I would be fine with 1

[WilliamJamieson]

Alright, here is a weird pytest-mpl issue:

1. If I don't have pytest-mpl installed import pytest_mpl fails as expected.
2. If I then pip install pytest-mpl and then do a import pytest_mpl it works (what HAS_PYTEST_MPL does).
3. However, if I then pip uninstall pytest-mpl and then don import pytest_mpl it works again. Moreover, the skips for no pytest-mpl do not occur and we get the error from pytest that we saw before (This is wrt the astropy branch Fix testing for pytest 7.2 support astropy/astropy#13892).
4. Reinstalling with pip install pytest-mpl solves the failing unit tests.

I have to create a completely new environment in order for the import pytest_mpl to start failing again...

[pllim]

#183 (comment)

FWIW, I cannot reproduce this problem but I use conda and you don't. 🤷

[ConorMacBride]

As already noted, this issue only occurs when pytest-mpl is not installed. I've opened a PR to close this issue. It adds the following to the docs:

If pytest-mpl is not installed, the image comparison tests will cause pytest to show a warning, PytestReturnNotNoneWarning. Installing pytest-mpl will solve this issue. Alternativly, the image comparison tests can be deselected by running pytest with -m "not mpl_image_compare".