Skip to content

Add a check in check_figures_equal that the test did not accidentally plot on non-fixture figures #18581

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.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
anntzer opened this issue Sep 25, 2020 · 6 comments
Closed

Add a check in check_figures_equal that the test did not accidentally plot on non-fixture figures #18581

anntzer opened this issue Sep 25, 2020 · 6 comments
Labels
New feature topic: testing
Milestone
v3.4.0

Comments

@anntzer
Copy link
Contributor

anntzer commented Sep 25, 2020 •
edited
Loading

Problem

check_figures_equal relies on the test plotting onto fig_test and fig_ref, not elsewhere (e.g. on newly instantiated figures, as noticed in #18579).

Proposed Solution

It should be reasonable to also fail the test if the test function creates new pyplot figures to catch errors as in #18579.

Additional context and prior art

Instead, we could also change check_figures_equal to just not pass the two figures in and always expect the test function to create exactly two pyplot figures itself and check these, but then it's a bit arbitrary which figure is the reference and which is the test (although that only really (barely) matters for display purposes in triage_tests.py), and feels a bit dirty (even ignoring the API break, which is as always manageable)...
@timhoffm
Copy link
Member

Alternatively, and maybe simpler: Check that the reference figure is not empty.

@anntzer
Copy link
Contributor Author

anntzer commented Sep 26, 2020

"Empty" is tricky to check, and actually I think there are check_figures_equal tests where I check that some commands are equivalent to not drawing anything :)
Checking that there's no new pyplot figures is not actually particularly hard, it's just a matter of checking plt.get_fignums() or plt.get_figlabels().

@tacaswell
Copy link
Member

Also related, I have been thinking about ways to eliminate pyplot from the tests all together. We currently have tests create some global state and then harvest that state in the decorators and hope they line up. It would be better to inject ether some structure via the image comparison decorator that will provide and array (or dict?) of the correct number of Figure objects to match the number of reference images we have or to inject a namespace that provides just enough of the pyplot API to create figures and then have the decorator look at the (localish) state from that rather that all of pyplot.

These thoughts are only barely formed, but externalizing in case anyone has similar ideas / wants to push on them...

@QuLogic
Copy link
Member

QuLogic commented Sep 28, 2020 •
edited
Loading

image_comparison has an array of expected file names from which it checks that there's the right number of figures.

@dstansby dstansby mentioned this issue Dec 27, 2020
Merged
7 tasks
@dstansby
Copy link
Member

Not sure if #19179 fully closes this, but it might @anntzer?

@anntzer
Copy link
Contributor Author

anntzer commented Dec 29, 2020

I think so, thanks for the PR.

@anntzer anntzer closed this as completed Dec 29, 2020
@QuLogic QuLogic added this to the v3.4.0 milestone Jan 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
New feature topic: testing
Projects
None yet
Milestone
v3.4.0
Development

No branches or pull requests
5 participants
@tacaswell @QuLogic @anntzer @timhoffm @dstansby