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

Close all plotters in testing, bump pytest-pyvista #4114

Merged
merged 9 commits into from
Mar 20, 2023

Conversation

MatthewFlamm
Copy link
Contributor

@MatthewFlamm MatthewFlamm commented Mar 10, 2023

Overview

Attempt to resolve problems in pyvista/pytest-pyvista#51 and #4103

This PR also includes version bump for pytest-pyvista to prevent leaking scope for verify_image_cache which can happen when a plotter is closed after a failed test.

@github-actions github-actions bot added the maintenance Low-impact maintenance activity label Mar 10, 2023
@codecov
Copy link

codecov bot commented Mar 10, 2023

Codecov Report

Merging #4114 (cdedba7) into main (7f5d36b) will increase coverage by 0.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main    #4114      +/-   ##
==========================================
+ Coverage   95.56%   95.57%   +0.01%     
==========================================
  Files          94       95       +1     
  Lines       20213    20292      +79     
==========================================
+ Hits        19316    19394      +78     
- Misses        897      898       +1     

@MatthewFlamm MatthewFlamm changed the title yield verify_image_cache for testing Close all plotters in testing Mar 11, 2023
@MatthewFlamm MatthewFlamm marked this pull request as ready for review March 11, 2023 13:57
akaszynski
akaszynski previously approved these changes Mar 11, 2023
Copy link
Member

@akaszynski akaszynski left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks!

@MatthewFlamm
Copy link
Contributor Author

MatthewFlamm commented Mar 11, 2023

In testing over in pytest-pyvista, the changes here make things worse if we do not also include the changes from pyvista/pytest-pyvista#51. See pyvista/pytest-pyvista#49 for comparison

@akaszynski
Copy link
Member

In testing over in pytest-pyvista, the changes here make things worse if we do not also include the changes from pyvista/pytest-pyvista#51. See pyvista/pytest-pyvista#49 for comparison

Do we need another release of pytest-pyvista before merging this?

@MatthewFlamm
Copy link
Contributor Author

Do we need another release of pytest-pyvista before merging this?

Sorry for not being very clear or prescriptive. I would recommend waiting on this one until we can merge in a version bump soon after. This one alone makes the errors even worse, presumably due to not cleaning up well enough in the other package.

I'm not sure if we should fix the image regressions in the other package first, or separate out the downstream testing piece and merge in the guards first.

@MatthewFlamm
Copy link
Contributor Author

pyvista/pytest-pyvista#49 should be ready now. The image regression errors were caused by ubuntu version mismatches. I somehow find myself in these rabbit holes related to a tangential topics lately. I recommend releasing a new version there and then merging this along with a separate PR for pytest-pyvista version bump.

@MatthewFlamm MatthewFlamm changed the title Close all plotters in testing Close all plotters in testing, bump pytest-pyvista Mar 19, 2023
@akaszynski akaszynski merged commit cacdc45 into main Mar 20, 2023
@akaszynski akaszynski deleted the maint/try-yielding-verify-image-cache branch March 20, 2023 15:39
@akaszynski akaszynski mentioned this pull request Apr 30, 2023
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maintenance Low-impact maintenance activity
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants