Fix screenshot compare - #658

Really nice work to start from - the step gating is careful in the places that are easy to get wrong
(using outcome rather than conclusion under continue-on-error, and
letting a failed baseline skip the comparison instead of failing the PR),
and the comments explain the reasoning rather than the mechanics.
The structure below is all yours, I only moved two things inside it.

Sorry for pushing to your branch instead of leaving notes.
It looked faster than a long thread, but say the word and I'll drop the commit so
you can do it your way.

What it changes:

• The baseline is built from the PR's base commit rather than tag 1.5.8, so
  the diff is about this PR instead of everything merged since the release.
• The fixture clone now drops every ref but the fixture branch.
  The history view counts commits reachable from any ref, so the fixture was
  still moving with master and a cached baseline no longer matched it.
  That is why the run on 88d5480 still flagged all four screenshots even though
  nothing UI-related changed.
• The baseline build is skipped for PRs that touch no UI path, which is about half of them.
• The committed PNGs are removed, since nothing compares against them any more.

I replayed the fixture change over six months of master: the old clone drifts
from 3081 to 3312 reachable commits, the new one stays at 3043 throughout.

Nice ! It's very promising.

I made some tests #666 and #661

• Both changes on left side bar texts and detect them ✅
• Only Screen shot false test2 #666 changes popup after run of Screen shot false test #661, it detects all ✅ but now even this PR, and Screen shot false test #661 comes with same screenshot 🛑

That means, that we need a downloadlink/name separation per pull request.

Btw, I rebased all 3 pull requests (this, #666, #661). Let's see how a parallel test behaves, and who is winning the last screenshot publish (which is the current open issue)

I removed this warning

Good catch, and I think it is simpler than a race. The upload has no per-PR
namespace at all: screenShotCompare.sh:113 sends each diff image under its
bare basename, into one flat directory shared by every PR, so
.../uploads/00-commit-context-menu.png is a single global slot. Whoever
finishes last wins, and every older comment then renders their pictures. So
there is no arbitration to get right - nothing should be competing for that
name.

Two lines fix it, in the shape the same file already uses for error
screenshots at 144-145:

newName="pr${PR}-${emulatorApi}-${f}"
mv "${f}" "$newName"

The mv matters: curl sends the basename as the stored name, so the file has
to be renamed on disk for the URL to change - which is probably why it got
commented out. $PR is already set at line 47 and $emulatorApi comes from
the workflow, so nothing else changes. It lives in your screenShotScript
repo; happy to open a PR there if that is easier than patching it yourself.

It also fixes something my gate introduced: if a PR posts a comment and a
later push drops its UI change, the comparison is skipped and the old comment
never gets cleaned up. Right now that stranded comment shows someone else's
images; with per-PR names it just shows its own, slightly stale ones.

One bit of housekeeping: the comment on this PR from 09-16 predates the fix
and will never be auto-deleted, since the comparison no longer runs here.
Worth deleting by hand.

And thanks for running #661 and #666 - those closed the thing I could not
verify from the outside. Both runs went cache-miss, built a baseline, then hit
the cache on the rerun and still detected the change correctly. That was
exactly the path the old design got wrong.

Opened it as hannesa2/screenShotScript#1 so you can
just merge it rather than retype the two lines. It is the first PR that repo
has had, so it may not notify you the way you are used to.

Once it is in, GitX needs the submodule pin moved off 0b1a330 to pick it up.
Happy to do that here, or leave it to you.

Added one diagnostic step (977fe1e5), which needs a word of explanation
since it looks like debug noise on its own.

Two baseline cache entries turned up under the same key
(baseline-screenshots-a921dbd6...-arm64), saved 05:30 and 06:12, at 292110
and 292108 bytes. That is from #661 and #666 both missing the cache on the
same base and both building a baseline. Either the two-byte delta is just
archive/compression noise between runner images, or the baseline screenshots
are not byte-stable between two builds of the same commit - and if it is the
second, every cache miss will report differences the PR did not cause, which
is most of what this branch set out to fix.

So the step prints shasum -a 256 of the baseline PNGs on both paths, cached
and freshly built, plus which one it was and the base commit.

What to compare: two runs that both missed the cache on the same base -
a hit only proves the cache round-trips bytes. That is exactly the 05:30 /
06:12 situation, so next time master moves and both test PRs run, their
printed hashes line up for comparison. Same hashes means the delta was
archive noise and we can forget it; different hashes means the screenshots
themselves drift and the cache is unsound.

Worth knowing why it has to be measured this way: the UI-path gate means a PR
that changes no UI skips the comparison entirely, so there is never a "these
two should be identical" run to just look at.

It only prints when a baseline exists, so it stays quiet on PRs like this one.
Happy to drop the step once the question is settled, or leave it - it is cheap
and the hashes are useful the next time a screenshot result looks wrong.

Unfortunately it doesn't detect anything anymore

• Screen shot false test #661
• Screen shot false test2 #666

maybe the retention of old screenshots is because they are now specifically created with different names/paths. want me to investigate?

maybe the retention of old screenshots is because they are now specifically created with different names/paths. want me to investigate?

Yes please ! ❤️

I added here DEBUG_INFO in screenshot script, now it should be easier to investigate

OK - helpful observations - found 2 related bugs 😃 - on it.

Investigated the comment pile-up. Two separate things, one in the script and
one here.

The script one is the cause of what you spotted, and it is not the new
file names. screenShotCompare.sh runs under zsh, whose echo expands
backslash escapes, so line 74

oldCommentsList=$(echo $oldCommentsJson | jq '...')

turns every \n inside a comment body into a real newline in the middle of a
JSON string, and jq rejects the payload. The || echo "" then leaves an empty
list, so the cleanup reports "nothing to do" while the old comments stay. It
only bites once a pull request carries a comment written by a person - the
ones this script posts use <br/> and contain no escapes, which is why it
worked until now. On #661 the trigger was my own long comments yesterday.

Fixed in hannesa2/screenShotScript#1 (printf '%s'
instead of echo, plus the same in the DEBUG_INFO dump). I replayed the full
deletion chain against the live payloads: it now finds all 7 stale comments on
#661 and both on #666, and correctly leaves the human ones alone. Your pins
are on the exact SHA b1a7454, so nothing moves under your running tests
until you bump them.

The second one is in this workflow, and it arrived with forceFail=true.
Archive screenshots diffs has no always(), so now that the compare step
exits non-zero on a difference, the diff images are dropped in exactly the
runs that produced them. Run 35431177919 shows it: step 26 failure, step 27
skipped.

       - name: Archive screenshots diffs
-        if: ${{ env.variableSet != '' && matrix.abi == 'arm64' && github.event_name == 'pull_request' && steps.stage-baseline.outcome == 'success' }}
+        if: ${{ always() && env.variableSet != '' && matrix.abi == 'arm64' && github.event_name == 'pull_request' && steps.stage-baseline.outcome == 'success' }}

Leaving this one for you rather than pushing to your branch again.

On whether failing is the right call - I would keep it non-blocking for
now, and I think your instinct is right. Not because the mechanism is wrong,
but because of #676: three of the four screenshots are byte-identical, so a
one-line sidebar change reports as four failures. Blocking on that teaches
people to ignore it. Worth reconsidering once the tests capture four distinct
views.

One good piece of news while I was in the logs. The #661 baseline was built
from 423af8d2 and the earlier one from a921dbd6, two separate builds of
two different commits, and the screenshots are byte-identical
(b29eb7bb..., e39cb94e...). So the baselines are reproducible and the
two-byte cache difference was the tarball compression, not the images. That
was the last open worry about false positives on a cache miss.

@hannesa2 , I have not pushed the above BuildPR.yml change to ScreenshotCompare as it is a branch you are working on.
Please edit the line and commit.
Thanks!

@goneng Thanks a lot, I followed you on both suggestions

Looks really good now, thanks a lot !
I merged hannesa2/screenShotScript#1, applied the merge to master here, rebased both test pull request and when all goes well, I'll merge this

💃🏼 🕺🏼