Skip to content

Make test_stem less flaky. #16735

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
Merged
merged 1 commit into from
Mar 13, 2020
Merged

Make test_stem less flaky. #16735

merged 1 commit into from
Mar 13, 2020

Conversation

QuLogic
Copy link
Member

@QuLogic QuLogic commented Mar 11, 2020

PR Summary

Since parametrizing the test allows it to run in parallel, this makes it flaky, as one process can overwrite the test result image of another.

Our standard way for dealing with tests that use the same baseline image is to pass duplicate filenames to image_comparison, because that is serialized.

This is basically the same as #16656 for test_stem.

PR Checklist

  • Has Pytest style unit tests
  • Code is Flake 8 compliant
  • [N/A] New features are documented, with examples if plot related
  • [N/A] Documentation is sphinx and numpydoc compliant
  • [N/A] Added an entry to doc/users/next_whats_new/ if major new feature (follow instructions in README.rst there)
  • [N/A] Documented in doc/api/api_changes.rst if API changed in a backward-incompatible way

@QuLogic
Make test_stem less flaky.
2e20058 
Since parametrizing the test allows it to run in parallel, this makes it
flaky, as one process can overwrite the test result image of another.

Our standard way for dealing with tests that use the same baseline image
is to pass duplicate filenames to `image_comparison`, because that is
serialized.
@QuLogic QuLogic added this to the v3.2.1 milestone Mar 11, 2020
@timhoffm
Copy link
Member

Would this be fixed by #16693?

@QuLogic
Copy link
Member Author

QuLogic commented Mar 11, 2020

No, that's only for check_figures_equal, not image_comparison. And in that case, the filename doesn't actually matter since it doesn't correspond with checked-in files, so it can be changed at will.

@timhoffm
Copy link
Member

timhoffm commented Mar 11, 2020 •
edited
Loading

Sorry about the confusion with check_figures_equal.

This approach here unrolls the parametrization into a helper loop. Exchanging the declarative parametrization for to code logic makes it less readable. But what bothers me more is that pytest will now not give the information which of the parameters failed.

I Would prefer to solve this in the administrative logic if possible. I quickly searched for disabling parallelization for some tests, but didn’t find anything. Could we set a lock based on the filename in image_comparison? That would prevent simultaneous access to the test file. AFAICS a good test could still overwrite a bad test image result so that you see the failure but may not have the offending test image anymore. But that‘s no difference in the manual solution proposed here.

Alternatively/additionally, could image_comparison carry a counter and number the test image stem.png, stem1.png, ... ?

@QuLogic
Copy link
Member Author

QuLogic commented Mar 12, 2020

We could use a lock, Cartopy uses filelock for this purpose. But I think that might be a bit large for 3.2.x, so how about we merge this (I can retarget directly if needed) and try that out for 3.3.0?

@timhoffm
Copy link
Member

I don‘t see a problem backporting a lock to 3.2.1. It‘s an internal bug fix. But if you want to bring this explicit fix back I’m also fine with that.

@QuLogic
Copy link
Member Author

QuLogic commented Mar 12, 2020

It's not just a lock, but also a new dependency. And also 3.2.1 is scheduled for Friday, and that will take some time to test, I think.

@QuLogic QuLogic mentioned this pull request Mar 13, 2020
Merged
6 tasks
@tacaswell tacaswell merged commit 4160365 into matplotlib:master Mar 13, 2020
meeseeksmachine pushed a commit to meeseeksmachine/matplotlib that referenced this pull request Mar 13, 2020
@tacaswell @meeseeksmachine
Backport PR matplotlib#16735: Make test_stem less flaky.
f456138
@meeseeksmachine meeseeksmachine mentioned this pull request Mar 13, 2020
Merged
QuLogic added a commit that referenced this pull request Mar 13, 2020
@QuLogic
Merge pull request #16760 from meeseeksmachine/auto-backport-of-pr-16…
9da2546 
…735-on-v3.2.x

Backport PR #16735 on branch v3.2.x (Make test_stem less flaky.)
@QuLogic QuLogic deleted the flaky-test_stem branch March 14, 2020 06:24
@tacaswell tacaswell mentioned this pull request Oct 3, 2022
Merged
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@timhoffm timhoffm timhoffm approved these changes

Assignees
No one assigned
Labels
topic: testing
Projects
None yet
Milestone
v3.2.1
Development

Successfully merging this pull request may close these issues.
3 participants
@QuLogic @timhoffm @tacaswell