Repository: matplotlib/matplotlib
Pull request: Deleting all images that have passed tests before upload
<!--
Thank you so much for your PR!  To help us review your contribution, please check
out the development guide https://matplotlib.org/devdocs/devel/index.html
-->

## PR summary
<!-- Please provide at least 1-2 sentences describing the pull request in detail
(Why is this change required?  What problem does it solve?) and link to relevant
issues and PRs.

Also please summarize the changes in the title, for example, "Raise ValueError on
non-numeric input to set_xlim" and avoid non-descriptive titles such as "Addresses
issue #8576".
-->
Supersedes PR #27881 (I'm so sorry for creating unnecessary traffic on this repo.)
Attempts to close #23400 and potentially complete what PR #19466 intended to do
Included a bash script right before the uploading part of the code in the *tests.yml* file to delete all files(.png, .svg, .eps, .pdf) that have not failed in the *result_images* directory. Have tested this using a dummy bash file and the downloaded result_images folder. Please let me know if this approach seems correct, or is blatantly wrong :).

## PR checklist
<!-- Please mark any checkboxes that do not apply to this PR as [N/A].-->

- [x] "closes #0000" is in the body of the PR description to [link the related issue](https://docs.github.com/en/issues/tracking-your-work-with-issues/linking-a-pull-request-to-an-issue)
- [x] new and changed code is [tested](https://matplotlib.org/devdocs/devel/testing.html)
- [N/A] *Plotting related* features are demonstrated in an [example](https://matplotlib.org/devdocs/devel/document.html#write-examples-and-tutorials)
- [N/A] *New Features* and *API Changes* are noted with a [directive and release note](https://matplotlib.org/devdocs/devel/api_changes.html#announce-changes-deprecations-and-new-features)
- [N/A] Documentation complies with [general](https://matplotlib.org/devdocs/devel/document.html#write-rest-pages) and [docstring](https://matplotlib.org/devdocs/devel/document.html#write-docstrings) guidelines

<!--We understand that PRs can sometimes be overwhelming, especially as the
reviews start coming in.  Please let us know if the reviews are unclear or
the recommended next step seems overly demanding, if you would like help in
addressing a reviewer's comments, or if you have been waiting too long to hear
back on your PR.-->


Problem statement:
Only upload failed images on failure
Currently when a job fails on GHA we upload all 109 MB across ~7k files which takes a surprisingly long time.  What we really want is just the images that failed and the computed difference.

This is labeled as "good first issue" because there are no API designs here (it is all configuring CI).

Steps:

 - sort out how to identify the failed test images (there is a systematic naming convention)
 -  explain that to the uploader workflow (or just delete all of the non-failed fails!)

Hints from issue discussion:
@andrzejnovak actually did this in #19466, but we closed it when I accidentally did the upload carte-blanche in a different PR. I had some slight reservations about where the subsetting was in that PR. It seems our tests must have ballooned though, because it was only 50 MB at that point.
I'm a beginner and I would like to work on this.
Fine for me if someone else wants to work on this or reuse my old PR. Probably some guidance from @jklymak on the direction would be helpful.
Has anyone started to work on this ?

Would love to take it and also would like to know how can I follow the closed PR to make it work or get an Idea?
@N3XT14 Please have a look at the linked PR and then ask any specific questions you have. 
Hi @tacaswell. I looked at the linked PR and understood it well but what I am trying to find out is the linked issue with that PR which I couldn't find.
I need it so that I can understand where the problem is, how do I reproduce it, and then work on fixing it.
@N3XT14 The PR was linking a failed test job, think https://github.com/matplotlib/matplotlib/actions/runs/4679682790 for example, github just removes old actions understandably. 

EDIT: Heh, things ballooned quite nicely, a nifty half a gig for every failed PR :D 
@N3XT14 If that PR still looks good, then the task is to rebase in on current main, fix anything that is failing, verify it is doing what we want, and then shepherd it through review process.
Hi, I would love to start working on this issue, can I get assigned to it? thanks.
@lukas-mar you are welcome to submit a pull request for any issue, but we do not assign them.  Please see our contributors’ guide:
https://matplotlib.org/devdocs/devel/index.html

Commit message: Deleting all images that have passed tests before upload

Modified code to delete all types of files which have not failed

Minor extension handling

Checking if file exists before deleting

Did not delete directories

Patch to review:
diff --git a/.github/workflows/tests.yml b/.github/workflows/tests.yml
index a825b4cf263b..d24c1533d476 100644
--- a/.github/workflows/tests.yml
+++ b/.github/workflows/tests.yml
@@ -318,6 +318,35 @@ jobs:
             --maxfail=50 --timeout=300 --durations=25 \
             --cov-report=xml --cov=lib --log-level=DEBUG --color=yes
 
+      - name: Cleanup non-failed image files
+        run: |
+          function remove_files() {
+              local extension=$1
+              find ./result_images -type f -name "*-expected*.$extension" | while read file; do
+                  if [[ $file == *"-expected_pdf"* ]]; then
+                      base=${file%-expected_pdf.$extension}_pdf
+                  elif [[ $file == *"-expected_eps"* ]]; then
+                      base=${file%-expected_eps.$extension}_eps
+                  elif [[ $file == *"-expected_svg"* ]]; then
+                      base=${file%-expected_svg.$extension}_svg
+                  else
+                      base=${file%-expected.$extension}
+                  fi
+                  if [[ ! -e "${base}-failed-diff.$extension" ]]; then
+                      if [[ -e "$file" ]]; then
+                          rm "$file"
+                          echo "Removed $file"
+                      fi
+                      if [[ -e "${base}.$extension" ]]; then
+                          rm "${base}.$extension"
+                      fi
+                      echo "Removed $file"
+                  fi
+              done
+          }
+
+          remove_files "png"; remove_files "svg"; remove_files "pdf"; remove_files "eps";
+
       - name: Filter C coverage
         run: |
           if [[ "${{ runner.os }}" != 'macOS' ]]; then


Code review comments:
diff_hunk: @@ -318,6 +318,35 @@ jobs:
             --maxfail=50 --timeout=300 --durations=25 \
             --cov-report=xml --cov=lib --log-level=DEBUG --color=yes
 
+      - name: Cleanup non-failed image files
+        run: |
+          function remove_files() {
+              local extension=$1
+              find ./result_images -type f -name "*-expected*.$extension" | while read file; do
+                  if [[ $file == *"-expected_pdf"* ]]; then
+                      base=${file%-expected_pdf.$extension}_pdf
+                  elif [[ $file == *"-expected_eps"* ]]; then
+                      base=${file%-expected_eps.$extension}_eps
+                  elif [[ $file == *"-expected_svg"* ]]; then
+                      base=${file%-expected_svg.$extension}_svg
+                  else
+                      base=${file%-expected.$extension}
+                  fi
+                  if [[ ! -e "${base}-failed-diff.$extension" ]]; then
+                      if [[ -e "$file" ]]; then
+                          rm "$file"
+                          echo "Removed $file"
+                      fi
+                      if [[ -e "${base}.$extension" ]]; then
+                          rm "${base}.$extension"
+                      fi
+                      echo "Removed $file"
path: .github/workflows/tests.yml
text: @user1:
```suggestion
                      echo "Removed ${base}.$extension"
```

?

@author:
Fixed that. Another thing - Should I delete all empty folders inside the *result_images* folder? Or leave it as it is

@user1:
I think leaving empty directories is not something worth holding the PR over, but if it is easy no reason not to do it.

@author:
I guess this works well as it is, don't want to break anything in the process