Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork7.9k
update testing helpers#16990
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
update testing helpers#16990
Uh oh!
There was an error while loading.Please reload this page.
Conversation
555514a toa51bab7Compare- `@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 to30f7b65Compare| @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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
But now you don't make the warning about FreeType. Why did thepytest import check have to move into this function and not just stay in_init_tests?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
You do still get the import warning about FreeType because the pytest configure calls_init_tests
| matplotlib._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 thepytest import into this function is so this function doesn't need to blindly captureImportError and canreturn -1 forpytest 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@QuLogic thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
terencehonlesMay 13, 2020 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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 thetest 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