Convert test decorators to pytest fixtures #7973
Conversation
eef58c5 to
e54ec77
Compare
|
So I've been thinking, instead of |
| @@ -1476,7 +1476,6 @@ def _jupyter_nbextension_paths(): | |||
| default_test_modules = [ | |||
| 'matplotlib.tests.test_png', | |||
| 'matplotlib.tests.test_units', | |||
| 'matplotlib.tests.test_widgets', | |||
There was a problem hiding this comment.
When switching to the automatic fixture and dropping any @cleanups, the tests fail with nose because they don't get that setup done. However, see next comment.
There was a problem hiding this comment.
Sorry for being dense, but why does this variable even exist? Why do we have a default whitelist of tests?
There was a problem hiding this comment.
It's used for matplotlib.test(); in the next PR I just set it to the top-level modules.
There was a problem hiding this comment.
Do we need to keep matplotlib.test()? Even if we do, can we just make matplotlib.test() fully rely on pytest's test discovery?
There was a problem hiding this comment.
Yes, that's why I pass only the top-level modules; but we still need to pass something because this works on the installed version and we need to hit both matplotlib and mpl_toolkits. I wouldn't be against deprecating matplotlib.test() and the top-level tests.py.
There was a problem hiding this comment.
I don't get it.
After the general conversion, do we expect nosetests to correctly run the tests?
- If the answer is yes, I give up (I appreciate your efforts in making this work but conversion to a test suite that supports two runners is way too hard for me to follow properly).
- If the answer is no, we already broke one way of running the tests. I honestly cannot imagine why we would need to keep backcompat for another one, and would thus just kill
matplotlib.test()(... andtests.pyat the same time).
There was a problem hiding this comment.
The answer to the first question is no as far as our own tests are concerned. But matplotlib.test() and tests.py will work with pytest in #7974. I have no idea how many people may or may not be using that method, so I don't know whether we should remove it or not.
There was a problem hiding this comment.
So we already broke backcompat on our tests by having a requirement on pytest, and by making it not possible to run the test suite by running nosetests. I simply do not understand why we can't just break it a bit more to get rid of old warts... that we're planning to get rid of anyways.
There was a problem hiding this comment.
Again, I'm not against it, but I think this discussion really belongs on #7974 where that code is actually changed.
|
So while I was out, I've been thinking about this PR, and I think it might be a bit of a bear, maintenance-wise. Removing all the
This gives us a slight reduction on conflicts when writing and backporting tests, with the downside that This is in these two branches, pytest-fixture2 and pytest2. What do people think? |
| @@ -83,8 +82,8 @@ def create_figure(): | |||
|
|
|||
| # test compiling a figure to pdf with xelatex | |||
| @needs_xelatex | |||
| @cleanup(style='classic') | |||
| @switch_backend('pgf') | |||
| @pytest.mark.style('default') | |||
There was a problem hiding this comment.
Hi @QuLogic
This is the place were I noticed that we were changing stylesheet. Can you confirm this is not normal?
There was a problem hiding this comment.
Or explain to me why it runs fine :)
There was a problem hiding this comment.
so @dopplershift explained it to me, and I now understand.
There was a problem hiding this comment.
For those reading along at home, the outer decorator that was here was being ignored and the last time we generated the images for these test we accidentally switched to the default style.
This was 'fixed' as part of the pytest migration so we need to switch the mark to 'default' style.
|
I am am 👍 on just dropping cleanup, we have alot of conflicts anyway and that is an easy one to resolve. |
|
In that case, I believe this requires a small rebase to delete some new |
Any non-default styles can be specified with
`@pytest.mark.style('name')`.
Non default backends can be specified by marking test functions with
`@pytest.mark.backend('backend')`.
Note that because switch_backend calls `matplotlib.testing.setup`, and
it appears _below_ the previous `@cleanup` decorator in the PGF tests,
the specified 'classic' style is not actually used for those tests.
e54ec77 to
05f4b16
Compare
|
To move things along I am going to merge this. |
I'm pre-opening this PR to verify that tests continue to build correctly. However, it still depends on #7935.This PR converts the test decorators
@cleanupand@switch_backendinto a single pytest fixture that does both things at the same time. In order to set a different style or backend, tests must be marked with@pytest.mark.style('name')or@pytest.mark.backend('name').Due to the way
matplotlib.testing.setupworks, it turns out that 'default' is the style used for any tests not decorated. Since this fixture uses 'classic', some additional tests needed to be marked by the 'default' style.Additionally, because
switch_backendcallsmatplotlib.testing.setup, the PGF tests (which use@cleanup('classic')do not actually use that style, so they've been set to 'default' here.