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

Add testing for doc images generated by sphinx build #5718

Merged
merged 81 commits into from
May 28, 2024

Conversation

user27182
Copy link
Contributor

@user27182 user27182 commented Mar 2, 2024

Overview

Add initial testing of images in the documentation. This is related to #5410, but does not (yet?) involve pytest-pyvista. This is a first proof of concept to experiment with how this kind of testing may be achieved.

Details

The test works by collecting all images from pyvista/doc/_build/_images (after building the documentation) and comparing them to a collection of cached images.

Unlike the testing in test/plotting, this test suite does not use plot. Instead, pv.compare_images is used directly.

The number of png images collected from the build is approx. 2000, and their collective size is approx 250MB. To address concerns about file size, the build images are first pre-processed and resized to have a maximum size of 400x400 pixels (the same size as images in tests/plotting/image_cache. The files are also saved as JPG (lossy) to further reduce the file size of the images.

With resizing and saving as JPG, the 250MB is reduced to approx 20MB, which may be a reasonably small enough size to store the images directly as-is in the pyvista repository. For comparison, the plotting/image_cache has about 470 files and uses about 12MB.

The tests will not be triggered by calling pytest generally. Instead, they must be triggered explicitly, e.g. with
pytest tests/doc/tst_doc_images.py, and the build images must already be in pyvista/doc/_build/_images

TODO:

  • Integrate tests into a new Test Documentation workflow the existing docs workflow
  • Fix failed cases (caused by use of random number generators)

Copy link

codecov bot commented Mar 2, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.58%. Comparing base (5bb9ed0) to head (a5d629d).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #5718      +/-   ##
==========================================
- Coverage   96.99%   90.58%   -6.41%     
==========================================
  Files         141      141              
  Lines       24666    24666              
==========================================
- Hits        23925    22344    -1581     
- Misses        741     2322    +1581     

@user27182
Copy link
Contributor Author

I did not intend to yet request a review, sorry! that was an accidental click

@user27182 user27182 marked this pull request as draft March 2, 2024 06:34
@user27182
Copy link
Contributor Author

First rest results not too bad for 2000 test cases, only 34 failures:
image
Some of the failures were expected, and caused by missing images since I used an older build from a few weeks ago to populate the initial cache.

@user27182
Copy link
Contributor Author

user27182 commented Mar 2, 2024

This kind of testing is already helping to highlight some issues with the docs. Having all the images dumped into a flat folder makes it really easy to go through them all manually. Scrolling through, I see a few blank images, e.g.
https://docs.pyvista.org/version/stable/api/core/_autosummary/pyvista.Camera.view_frustum.html#pyvista.Camera.view_frustum

EDIT: I think in this case it's probably supposed to be blank since nothing is plotted, but why have an image at all if there's nothing to see? Either way, it's helpful to review

@user27182
Copy link
Contributor Author

Looks like the flaky Capsule and Frog Tissue tests are going to be a problem with this PR. The builds keep failing because of either one of these.

I think these should be special- cased, e.g. convert the errors into warning for these two. Or maybe add a fallback image, e.g. if it fails for one image, try again with another that is known to work.

@user27182
Copy link
Contributor Author

New checks for flaky tests are working. What would be a failure is now downgraded to a warning. See recent test output: https://github.com/pyvista/pyvista/actions/runs/9231333767/job/25401044191?pr=5718#step:14:88

@user27182
Copy link
Contributor Author

@tkoyama010 let me know if you want me to temporarily remove the images again to review the code changes

@tkoyama010
Copy link
Member

@tkoyama010 let me know if you want me to temporarily remove the images again to review the code changes

Yes, please. I am happy that CI passes now :)

@tkoyama010
Copy link
Member

@pyvista-bot preview

@pyvista-bot
Copy link
Contributor

CONTRIBUTING.rst Outdated Show resolved Hide resolved
tkoyama010
tkoyama010 previously approved these changes May 26, 2024
Copy link
Member

@tkoyama010 tkoyama010 left a comment

Choose a reason for hiding this comment

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

With resizing and saving as JPG, the 250MB is reduced to approx 20MB, which may be a reasonably small enough size to store the images directly as-is in the pyvista repository. For comparison, the plotting/image_cache has about 470 files and uses about 12MB.

Thanks for your thoughts on this, I have decided that 20MB is an acceptable size given the tradeoff with being able to test.
I give my approval to this at this time. Thank you for waiting so long. Please revert the image.

@tkoyama010 tkoyama010 enabled auto-merge (squash) May 26, 2024 16:02
@tkoyama010
Copy link
Member

@pyvista-bot LGTM

Copy link
Contributor

@pyvista-bot pyvista-bot left a comment

Choose a reason for hiding this comment

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

✅ Approving this PR because tkoyama010 said so in here :shipit:

@tkoyama010
Copy link
Member

Providing an approval since I'm required to review all GitHub Action changes. My review is limited in scope to the changes of the docs.yml GitHub Actions workflow file. Please ping/re-request review if needed but I want to make sure someone else is able to review the rest of the changes here

I will merge this.

@tkoyama010 tkoyama010 merged commit 21b16aa into pyvista:main May 28, 2024
27 checks passed
@user27182 user27182 deleted the testing/doc_images branch May 28, 2024 13:25
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

5 participants