Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork7.9k
Fixed the overdeletion of source images for failing tests#30284
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:main
Are you sure you want to change the base?
Conversation
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. |
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