Skip to content

Screen shot false test - #661

Draft
hannesa2 wants to merge 1 commit into
masterfrom
ScreenShotFalseTest
Draft

Screen shot false test#661
hannesa2 wants to merge 1 commit into
masterfrom
ScreenShotFalseTest

Conversation

@hannesa2

@hannesa2 hannesa2 commented Sep 16, 2026

Copy link
Copy Markdown
Contributor

It's about to detect some changes in screenshot tests

@hannesa2
hannesa2 marked this pull request as draft September 16, 2026 07:30
@hannesa2 hannesa2 added the WIP Work in progress label Sep 17, 2026
@hannesa2
hannesa2 force-pushed the ScreenShotFalseTest branch 2 times, most recently from 0dfb5c2 to a6ee394 Compare September 18, 2026 05:14
This was referenced Sep 18, 2026
Closed
Merged
@hannesa2
hannesa2 force-pushed the ScreenShotFalseTest branch 2 times, most recently from 2d2a6b7 to 0ea6072 Compare September 18, 2026 07:17
@hannesa2 hannesa2 added the pipeline CI pipeline label Sep 18, 2026
@goneng

goneng commented Sep 18, 2026

Copy link
Copy Markdown
Collaborator

Sorry for pushing an empty commit to your test PR - it is only there to
trigger a run, no source change, and revert it whenever you like.

What it is for: on #658 I added a step that prints shasum -a 256 of the
baseline screenshots, because two baseline cache entries turned up under the
same key (baseline-screenshots-a921dbd6...-arm64, 292110 and 292108 bytes)
when this PR and #666 both missed the cache on the same base this morning.
Either that two-byte delta is archive 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 reports differences the PR
did not cause, which is most of what #658 set out to fix.

Master moved to a9961719 since your last run here, so this should be a cache
miss and build a fresh baseline, which is what produces the hashes.

It tells us something either way:

  • If it builds a baseline, base.sha is tracking master, and the printed
    hashes can be compared against the next rebuild on the same base.
  • If it hits the cache, base.sha is still reporting a921dbd6 while
    master is a9961719, which would mean the baseline is built one merge
    behind and Let the terminal application be chosen in the preferences #667 would show up as a difference this PR did not make.

The step prints the base commit it used, so the log says which.

@goneng

goneng commented Sep 18, 2026

Copy link
Copy Markdown
Collaborator

Correction to the above: the empty commit alone would not have printed
anything. This branch forked from 1070d6e8, before the shasum step landed
on #658, so it was running the workflow without it. I have cherry-picked that
step here as e534621d; both commits are throwaway and yours to drop.

goneng added a commit that referenced this pull request Sep 18, 2026
@goneng
Take the baseline commit from the merge, not the event payload
9e1e478 
Compare against the commit the pull request is actually being merged
into, so intervening merges to master stop counting as its changes.

* github.event.pull_request.base.sha is master as GitHub recorded it
  when the pull request last changed base, not the base branch's head.
  On run 35328811182 it read a921dbd while master was a996171, so the
  baseline was a merge behind and #667 counted as #661's work.
* The checkout is refs/pull/N/merge, whose first parent is the commit
  the merge was computed against. Resolve it with rev-parse HEAD^1 and
  use that for the gate, the cache key and the baseline checkout.
* Print both values, so the next run says whether they agree.
hannesa2 pushed a commit that referenced this pull request Sep 18, 2026
@goneng @hannesa2
Take the baseline commit from the merge, not the event payload
633e5d3 
Compare against the commit the pull request is actually being merged
into, so intervening merges to master stop counting as its changes.

* github.event.pull_request.base.sha is master as GitHub recorded it
  when the pull request last changed base, not the base branch's head.
  On run 35328811182 it read a921dbd while master was a996171, so the
  baseline was a merge behind and #667 counted as #661's work.
* The checkout is refs/pull/N/merge, whose first parent is the commit
  the merge was computed against. Resolve it with rev-parse HEAD^1 and
  use that for the gate, the cache key and the baseline checkout.
* Print both values, so the next run says whether they agree.
hannesa2 pushed a commit that referenced this pull request Sep 19, 2026
@goneng @hannesa2
Take the baseline commit from the merge, not the event payload
ac3e146 
Compare against the commit the pull request is actually being merged
into, so intervening merges to master stop counting as its changes.

* github.event.pull_request.base.sha is master as GitHub recorded it
  when the pull request last changed base, not the base branch's head.
  On run 35328811182 it read a921dbd while master was a996171, so the
  baseline was a merge behind and #667 counted as #661's work.
* The checkout is refs/pull/N/merge, whose first parent is the commit
  the merge was computed against. Resolve it with rev-parse HEAD^1 and
  use that for the gate, the cache key and the baseline checkout.
* Print both values, so the next run says whether they agree.
@hannesa2
hannesa2 force-pushed the ScreenShotFalseTest branch 3 times, most recently from dd80056 to 7a9e594 Compare September 19, 2026 08:08
This was referenced Sep 19, 2026
Merged
Merged
Merged
goneng added a commit to goneng/gitx that referenced this pull request Sep 19, 2026
@goneng
Let labelled pull requests keep their superseded CI runs
068437f 
Cancelling a superseded run throws away the artifacts it would have
uploaded, which is the very thing the screenshot experiments in gitx#658,
gitx#661 and gitx#681 exist to look at, and it makes each variant wait for the
running build before it can start. Raised by hannesa2 on gitx#623.

- Read cancel-in-progress from the pull request's labels, so that a
  pull request labelled keep-runs runs every push to completion
- Leave every other pull request cancelling as it does now, and master
  and tag builds untouched, since their group is keyed per run
@hannesa2 hannesa2 changed the title Screen shot false test keep-runs: Screen shot false test Sep 19, 2026
hannesa2 pushed a commit that referenced this pull request Sep 19, 2026
@goneng @hannesa2
Take the baseline commit from the merge, not the event payload
7b63d78 
Compare against the commit the pull request is actually being merged
into, so intervening merges to master stop counting as its changes.

* github.event.pull_request.base.sha is master as GitHub recorded it
  when the pull request last changed base, not the base branch's head.
  On run 35328811182 it read a921dbd while master was a996171, so the
  baseline was a merge behind and #667 counted as #661's work.
* The checkout is refs/pull/N/merge, whose first parent is the commit
  the merge was computed against. Resolve it with rev-parse HEAD^1 and
  use that for the gate, the cache key and the baseline checkout.
* Print both values, so the next run says whether they agree.
@hannesa2 hannesa2 changed the title keep-runs: Screen shot false test Screen shot false test Sep 19, 2026
@hannesa2 hannesa2 removed the WIP Work in progress label Sep 23, 2026
@hannesa2
hannesa2 force-pushed the ScreenShotFalseTest branch 4 times, most recently from cd88062 to 3a2e351 Compare September 27, 2026 17:16
@hannesa2

hannesa2 commented Sep 28, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

@goneng I don't get it why the tests are failing here. Do you have any idea ?

image

@goneng

goneng commented Sep 29, 2026

Copy link
Copy Markdown
Collaborator

@hannesa2 apologies for the delayed response.

The failures are unit tests, not screenshot tests, and two of the three are caused by the rename itself.

PBGitSidebarWorktreeGroupTests looks the sidebar group up by its literal title:

GitXTests/PBGitSidebarWorktreeGroupTests.m:286: [self groupOfSidebar:sidebar titled:@"Branches"]
GitXTests/PBGitSidebarWorktreeGroupTests.m:330: [self groupOfSidebar:sidebar titled:@"Branches"]

With the group now called "Branches Test" that returns nil, so one test asserts "the sidebar has no BRANCHES group" and the other reads toolTip off nil and gets (null). Only @"Branches" is asserted anywhere - Remotes, Tags and Stashes are not referenced by any test.

The third failure, PBGitWorktreeManagementTests testALockedWorktreeCannotBeRemovedAndSaysWhy, is unrelated: it timed out after 20s waiting for git worktree lock state. Master at this PR's exact base commit (049507f1) ran green 26 minutes before your run, so that one is runner flake.

Intel is green because BuildPR.yml:106 gates the unit-test step on matrix.abi == 'arm64' - the x86_64 job runs no tests at all.

The part that matters for what you were testing: the job's steps 12 through 27 are all skipped. Every screenshot step sits behind "Run unit tests", which failed, so the baseline build never started and the comparison never ran. The PR has not actually exercised the screenshot diff yet.

Simplest fix: drop the Branches rename and keep the other three. That leaves a visible, screenshot-detectable change with no unit test touched. Classes is in the ui-paths filter, so the comparison will run once the unit tests pass.

Worth noting separately, and not this PR's job: those two tests match the sidebar group by display string, so any future legitimate rename or localization of "Branches" breaks them.

@hannesa2
hannesa2 force-pushed the ScreenShotFalseTest branch 4 times, most recently from 3f679c2 to 89d046f Compare September 30, 2026 06:09
@hannesa2 hannesa2 mentioned this pull request Sep 30, 2026
Merged
@hannesa2
hannesa2 force-pushed the ScreenShotFalseTest branch 2 times, most recently from 1c7b2bd to 38f9b66 Compare October 1, 2026 04:58
@hannesa2 hannesa2 mentioned this pull request Oct 1, 2026
Merged
@hannesa2
hannesa2 force-pushed the ScreenShotFalseTest branch from 38f9b66 to 226d751 Compare October 1, 2026 05:40
@hannesa2
hannesa2 force-pushed the ScreenShotFalseTest branch from 226d751 to 3b428cc Compare October 28, 2026 06:20
@github-actions github-actions Bot deleted a comment from hannesa2 Oct 28, 2026
@hannesa2
hannesa2 force-pushed the ScreenShotFalseTest branch from c7ceb82 to 421bf3e Compare October 28, 2026 07:06
@gitx gitx deleted a comment from github-actions Bot Oct 28, 2026
@hannesa2
hannesa2 force-pushed the ScreenShotFalseTest branch 2 times, most recently from d2fbef7 to ab5b7e8 Compare October 29, 2026 05:41
@hannesa2 hannesa2 mentioned this pull request Oct 29, 2026
Merged
@hannesa2
hannesa2 force-pushed the ScreenShotFalseTest branch 4 times, most recently from f91e6f5 to a78a8d0 Compare November 5, 2026 08:18
@hannesa2
hannesa2 force-pushed the ScreenShotFalseTest branch from a78a8d0 to ebff74d Compare November 5, 2026 19:30
@hannesa2

hannesa2 commented Nov 5, 2026

Copy link
Copy Markdown
Contributor Author

Screenshot differs: emulatorApi=arm64 with 4 screenshot(s)

setpoint (old) | diff | actual screenshot (new)

commit-context-menu.pngscreenshot

history-view.pngscreenshot

repository-folder-open.pngscreenshot

staging-view.pngscreenshot

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

pipeline CI pipeline wontfix

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@hannesa2 @goneng