-
Notifications
You must be signed in to change notification settings - Fork 443
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
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
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 |
I did not intend to yet request a review, sorry! that was an accidental click |
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. 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 |
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 |
@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 :) |
@pyvista-bot preview |
There was a problem hiding this 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, theplotting/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.
This reverts commit 0f7e360.
@pyvista-bot LGTM |
There was a problem hiding this 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
I will merge this. |
It works well in docstring review. Thanks! |
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 useplot
. 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 intests/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, theplotting/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. withpytest tests/doc/tst_doc_images.py
, and the build images must already be inpyvista/doc/_build/_images
TODO:
a newthe existing docs workflowTest Documentation
workflow