update testing helpers #16990
update testing helpers #16990
Conversation
555514a to
a51bab7
Compare
- `@matplotlib.testing.decorators.image_comparison` now prints the path to the diff image so it is easier to find, so that newcomers know it exists. - update `tests.py` to give more information about how it should be run if matplotlib is not installed already. - update `matplotlib.test` to return -1 instead of raising ImportErrors if there are preconditions not met. This allows the error messages to not be buried by the stack traces, is still a return value which would signify an error, and does not conflict with the range used by `pytest.ExitCode` (0-5) allowing callers to distinguish different pytest error codes from an error starting up pytest - move the `import pytest` check out of `matplotlib._init_tests` because it is only called via `matplotlib.test` and via pytest's configure handler, and it is more appropriate and can be handled more gracefully if it is checked in `matplotlib.test`
a51bab7 to
30f7b65
Compare
|
|
||
| @cbook._delete_parameter("3.2", "switch_backend_warn") | ||
| @cbook._delete_parameter("3.3", "recursionlimit") | ||
| def test(verbosity=None, coverage=False, switch_backend_warn=True, | ||
| recursionlimit=0, **kwargs): | ||
| """Run the matplotlib test suite.""" | ||
| _init_tests() |
There was a problem hiding this comment.
But now you don't make the warning about FreeType. Why did the pytest import check have to move into this function and not just stay in _init_tests?
There was a problem hiding this comment.
You do still get the import warning about FreeType because the pytest configure calls _init_tests
As originally written the _init_tests will be called twice: Once before the logger is set up (here), and then again by pytest (linked above) which by then logging is being captured. I didn't believe _init_tests needed to be called here because it doesn't raise an exception and you're not going to see the logs since they aren't set up at this point. If the warning in _init_tests isn't actually a warning it should be emitted in a way that a user can see it (currently it isn't).
The reason I moved the pytest import into this function is so this function doesn't need to blindly capture ImportError and can return -1 for pytest not being installed. This is all detailed in the PR description if you would like to read that.
There was a problem hiding this comment.
@QuLogic thoughts?
There was a problem hiding this comment.
@QuLogic Does this satisfy your question? I've put this on the call agenda for tomorrow, but if you're ok with this, we don't have to spend time on it.
| import pytest | ||
| except ImportError: | ||
| print("matplotlib.test requires pytest to run.") | ||
| return -1 |
There was a problem hiding this comment.
I am skeptical about the API change, but this function is already returning an error code so any users of this will likely already be expecting to handle error codes so I expect it will do little harm.
There was a problem hiding this comment.
That was basically my position. It wasn't obvious to me why this was "broken" and I thought it wasn't used anymore. It was, but I didn't have the right environment. I then sought to clarify the whole process until getting pytest to run. I was contemplating keeping the same API for this part, but the code would be less clear and as you noted the test function will already return error codes on pytest errors
|
I am happy that the freetype checks are still happening via Thanks @terencehonles ! |
PR Summary
Pulling out of #16922 and expanding to be a bit more helpful (but will probably create additional discussion which I am am providing the context below)
@matplotlib.testing.decorators.image_comparisonnow prints the path to the diff image so it is easier to find, so that newcomers know it exists.tests.pyto give more information about how it should be run if matplotlib is not installed already.matplotlib.testto return -1 instead of raising ImportErrors if there are preconditions not met. This allows the error messages to not be buried by the stack traces, is still a return value which would signify an error, and does not conflict with the range used bypytest.ExitCode(0-5) allowing callers to distinguish different pytest error codes from an error starting up pytestimport pytestcheck out ofmatplotlib._init_testsbecause it is only called viamatplotlib.testand via pytest's configure handler, and it is more appropriate and can be handled more gracefully if it is checked inmatplotlib.testPR Checklist