Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

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.

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

Merged
tacaswell merged 1 commit intomatplotlib:masterfromQuLogic:flaky-test_stem
Mar 13, 2020

Conversation

QuLogic
Copy link
Member

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 toimage_comparison, because that is serialized.

This is basically the same as#16656 fortest_stem.

PR Checklist

  • Has Pytest style unit tests
  • Code isFlake 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

Since parametrizing the test allows it to run in parallel, this makes itflaky, as one process can overwrite the test result image of another.Our standard way for dealing with tests that use the same baseline imageis to pass duplicate filenames to `image_comparison`, because that isserialized.
@QuLogicQuLogic added this to thev3.2.1 milestoneMar 11, 2020
@timhoffm
Copy link
Member

Would this be fixed by#16693?

@QuLogic
Copy link
MemberAuthor

No, that's only forcheck_figures_equal, notimage_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 reacted with thumbs up emoji

@timhoffm
Copy link
Member

timhoffm commentedMar 11, 2020
edited
Loading

Sorry about the confusion withcheck_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 inimage_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, couldimage_comparison carry a counter and number the test imagestem.png,stem1.png, ... ?

@QuLogic
Copy link
MemberAuthor

We could use a lock, Cartopy usesfilelock 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
MemberAuthor

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.

timhoffm reacted with thumbs up emoji

@tacaswelltacaswell merged commit4160365 intomatplotlib:masterMar 13, 2020
meeseeksmachine pushed a commit to meeseeksmachine/matplotlib that referenced this pull requestMar 13, 2020
QuLogic added a commit that referenced this pull requestMar 13, 2020
…735-on-v3.2.xBackport PR#16735 on branch v3.2.x (Make test_stem less flaky.)
@QuLogicQuLogic deleted the flaky-test_stem branchMarch 14, 2020 06:24
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@timhoffmtimhoffmtimhoffm approved these changes

Assignees
No one assigned
Projects
None yet
Milestone
v3.2.1
Development

Successfully merging this pull request may close these issues.

3 participants
@QuLogic@timhoffm@tacaswell

[8]ページ先頭

©2009-2025 Movatter.jp