Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork7.9k
Convert test decorators to pytest fixtures#7973
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
Uh oh!
There was an error while loading.Please reload this page.
Conversation
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
what's happening there?
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
It's used formatplotlib.test(); in the next PR I just set it to the top-level modules.
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.
Do weneed to keepmatplotlib.test()? Even if we do, can we just makematplotlib.test() fully rely on pytest's test discovery?
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.
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 bothmatplotlib andmpl_toolkits. I wouldn't be against deprecatingmatplotlib.test() and the top-leveltests.py.
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 don't get it.
After the general conversion, do we expectnosetests 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
The answer to the first question is no as far as our own tests are concerned. Butmatplotlib.test() andtests.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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
So wealready broke backcompat on our tests by having a requirement on pytest, and by making it not possible to run the test suite by runningnosetests. 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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 andpytest2. 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Or explain to me why it runs fine :)
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.
so@dopplershift explained it to me, and I now understand.
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.
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 conflictsanyway 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`, andit appears _below_ the previous `@cleanup` decorator in the PGF tests,the specified 'classic' style is not actually used for those tests.To move things along I am going to merge this. |
Uh oh!
There was an error while loading.Please reload this page.
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 testsnot 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')donot actually use that style, so they've been set to 'default' here.