Fixed the overdeletion of source images for failing tests #30284
+22
−27
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
I originally was thinking this should be a Python script, and I'm wondering if that's starting to be the better option now. |
|
Heh, shell scripting can certainly be more difficult to read than Python, but I don't think a Python version would be more straightforward. The bug here was in the logic of the approach, and not really affected by the choice of language. I'm of course biased because I just wrote this, but I'd suggest merging this updated shell script because it definitely works. Someone in the future can replace this with a Python version if a benefit is identified. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
PR summary
This PR further fixes the deletion of all images that have passed tests before uploading the artifact (see #27882 and #30188). #27882 had separated calls to delete PNGs, PDFs, SVGs, etc., but that approach is flawed because all non-PNG files are converted to PNGs for comparison. That is, there is never a difference PDF, only a difference PDF-converted-to-PNG, which means that the code would always delete PDFs due to the lack of difference PDFs. So, after the fix of #30188, the uploaded artifact would have expected/actual PDF-converted-to-PNG for failing PDF tests, but were missing the expected/actual PDF.
Fixing this required a significant rewrite of the shell script. I've tested this extensively in an unrelated PR with a bunch of failing image tests.
PR checklist