Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

TST: force test with shared test image to run in serial #24081

Merged
merged 1 commit into from
Oct 4, 2022

Conversation

tacaswell
Copy link
Member

PR Summary

The tests test_hexbin_empty and test_hexbin_log_empty use the same target image, however if they the tests ended up being run in different workers when running the tests in parallel they will step on each other if one of the test starts writing the output file while the other is trying to read the test image.

By putting them in the same test they will save and evaluate in order.

I verified I could break both tests.

The other option here is to make a copy of the file (which should be free at the git level due to content based addressing). I am happy to switch to that if the consensus that is a better approach happy to switch (I was very much on the fence).

This should fix the current rash of transient failures. We are running -n 2 on CI so we need to get unlucky that the tests go to different workers and that they run close enough in time to matter which is why we only see in 1-2 jobs per CI run and re-running seems to reliably fix it.

PR Checklist

Tests and Styling

  • Has pytest style unit tests (and pytest passes).
  • Is Flake 8 compliant (install flake8-docstrings and run flake8 --docstring-convention=all).

The tests test_hexbin_empty and test_hexbin_log_empty use the same target
image, however if they the tests ended up being run in different workers when
running the tests in parallel they will step on each other if one of the test
starts writing the output file while the other is trying to read the test
image.

By putting them in the same test they will save and evaluate in order.
@tacaswell tacaswell added this to the v3.7.0 milestone Oct 2, 2022
Copy link
Member

@timhoffm timhoffm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think either way is good enough here. Please add a comment that they have to be together, so that nobody comes along and splits the test later. - Thinking about it, given the need for a note to prevent future breakage and related complexity/mental load, maybe copying the image is the easier solution.

Theoretically, the best solution would be to solve this on the test infrastructure level. Conceptually, there should not be a problem with multiple tests using the same reference image - the reference image should be read-only. But maybe fixing this is more effort than it's worth. It's not that common that multiple tests generate the same image.

@tacaswell
Copy link
Member Author

#16735 is some prior art on this (where we discussed adding file locking).

I remember we had a PR that was adding some randomness to the initial file written, but I am not able to find it. However I think even if we fixed that we would still have some issues if multiple failed because of the way we name the expected / diff files.

Maybe we need some logic to audit the names and make sure we do not have the reused files between tests (I think we know enough as part of the collection phase to determine that).

@QuLogic QuLogic merged commit 76a5711 into matplotlib:main Oct 4, 2022
@tacaswell tacaswell deleted the fix_shared_test_image branch October 4, 2022 13:44
@QuLogic QuLogic modified the milestones: v3.7.0, v3.6.2 Oct 17, 2022
@QuLogic
Copy link
Member

QuLogic commented Oct 17, 2022

meeseeksdev backport to v3.6.x

meeseeksmachine pushed a commit to meeseeksmachine/matplotlib that referenced this pull request Oct 17, 2022
QuLogic added a commit that referenced this pull request Oct 17, 2022
…081-on-v3.6.x

Backport PR #24081 on branch v3.6.x (TST: force test with shared test image to run in serial)
@ksunden ksunden mentioned this pull request Feb 20, 2023
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants