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

MAINT: Add GC tests #66

Merged
merged 36 commits into from
Jan 9, 2023
Merged

MAINT: Add GC tests #66

merged 36 commits into from
Jan 9, 2023

Conversation

larsoner
Copy link
Contributor

Locally this passes. Shouldn't be merged until PyVista merges pyvista/pyvista#958

@codecov
Copy link

codecov bot commented Oct 23, 2020

Codecov Report

Merging #66 (0cd0112) into main (1ad53e9) will increase coverage by 0.09%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main      #66      +/-   ##
==========================================
+ Coverage   97.22%   97.31%   +0.09%     
==========================================
  Files           8        8              
  Lines         648      670      +22     
  Branches       81       82       +1     
==========================================
+ Hits          630      652      +22     
  Partials       18       18              

@larsoner
Copy link
Contributor Author

Okay this is ready for review, the 3.7 Linux build correctly picks up the GC tests and they pass:

Including plotting tests (ALLOW_PLOTTING=true)
Including garbage collection tests (GC_TEST=true)

Then once pyvista/pyvista#958 is in we can merge!

@larsoner
Copy link
Contributor Author

(The windows 3.6 is just a download timeout error, I don't have permissions to restart it)

@GuillaumeFavelier
Copy link
Contributor

I restarted Azure but it seems like Github Actions is still not happy

@GuillaumeFavelier
Copy link
Contributor

BTW @pyvista/developers can we allow @larsoner to restart the CIs?

* upstream/main: (125 commits)
  Bump check-jsonschema from 0.17.1 to 0.20.0 (pyvista#261)
  Bump pre-commit from 2.20.0 to 2.21.0 (pyvista#263)
  Bump matplotlib from 3.5.2 to 3.6.2 (pyvista#262)
  Bump isort from 5.10.1 to 5.11.4 (pyvista#264)
  Bump pre-commit-hooks from 4.3.0 to 4.4.0 (pyvista#260)
  Bump flake8 from 4.0.1 to 6.0.0 (pyvista#251)
  Bump actions/checkout from 2 to 3 in /.github/workflows (pyvista#246)
  Bump pylint from 2.13.0 to 2.15.9 (pyvista#256)
  Bump peter-evans/create-pull-request from 3 to 4 in /.github/workflows (pyvista#249)
  Bump codecov/codecov-action from 2 to 3 in /.github/workflows (pyvista#247)
  Bump actions/setup-python from 2 to 4 in /.github/workflows (pyvista#248)
  Bump numpy from 1.21.6 to 1.24.1 (pyvista#257)
  Bump ipython from 7.34.0 to 8.8.0 (pyvista#259)
  Update pre-commit hooks (pyvista#231)
  BUG: Safe app window close (pyvista#258)
  update pre-commit hooks (pyvista#228)
  Bump pyvista from 0.35.2 to 0.36.0 (pyvista#217)
  Bump imageio from 2.20.0 to 2.21.0 (pyvista#216)
  Bump sphinx from 5.0.2 to 5.1.1 (pyvista#213)
  Bump imageio from 2.19.5 to 2.20.0 (pyvista#214)
  ...
@larsoner
Copy link
Contributor Author

larsoner commented Jan 6, 2023

Okay finally green, ready for review/merge from my end @tkoyama010 !

@akaszynski
Copy link
Member

This is the longest I've ever seen a PR been open... and still make it to review/merge.

Comment on lines +794 to +797
# TODO: Need to fix this allow_bad_gc:
# - the actors are not cleaned up in the non-empty scene case
# - the q_key_press leaves a lingering vtkUnsignedCharArray referred to by
# a "managedbuffer" object
Copy link
Member

Choose a reason for hiding this comment

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

Let's open a follow-up issue for this.

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. Just need a follow up issue opened for the remaining GC issue and we're good to go.

@larsoner
Copy link
Contributor Author

larsoner commented Jan 9, 2023

Good idea @akaszynski, done in #270 so I'll merge!

@larsoner larsoner merged commit a783c20 into pyvista:main Jan 9, 2023
@larsoner larsoner deleted the mem branch January 9, 2023 15:02
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