Skip to content

Test settings - #519

Merged
hannesa2 merged 6 commits into
masterfrom
TestSettings
Sep 25, 2026
Merged

Test settings#519
hannesa2 merged 6 commits into
masterfrom
TestSettings

Conversation

@hannesa2

@hannesa2 hannesa2 commented Mar 1, 2026

Copy link
Copy Markdown
Contributor

No description provided.

@hannesa2 hannesa2 added the pipeline CI pipeline label Mar 1, 2026
@hannesa2
hannesa2 force-pushed the TestSettings branch 8 times, most recently from faff0af to e3583e7 Compare March 2, 2026 05:40
@hannesa2

hannesa2 commented Mar 2, 2026

Copy link
Copy Markdown
Contributor Author

This needs all too long

@hannesa2
hannesa2 force-pushed the TestSettings branch 2 times, most recently from 177feed to a314cf9 Compare March 16, 2026 06:43
@goneng goneng mentioned this pull request Sep 19, 2026
Merged
@hannesa2
hannesa2 force-pushed the TestSettings branch 4 times, most recently from 26992b9 to 831bc23 Compare September 19, 2026 20:05
@hannesa2 hannesa2 added the test Test related label Sep 19, 2026
@hannesa2
hannesa2 force-pushed the TestSettings branch 3 times, most recently from 53770bb to f8cb789 Compare September 20, 2026 07:36
@hannesa2
hannesa2 marked this pull request as draft September 20, 2026 20:18
@hannesa2
hannesa2 force-pushed the TestSettings branch 3 times, most recently from da7ac98 to 217676d Compare September 23, 2026 04:15
@goneng

goneng commented Sep 23, 2026

Copy link
Copy Markdown
Collaborator

@hannesa2 a few notes from going through this one, in case they help.

The screenshot diffs look like noise. Since the PR only touches test
files, the app under test is the same as on its base. Comparing the old
and new panels pixel by pixel:

  • history-view.png: 89 pixels, all inside a 24x24 triangle at the
    window's top-left corner.
  • commit-context-menu.png: anti-aliasing differences in the details
    pane text behind the menu. Nothing visible by eye.

The two Settings tests don't seem to find the window. In the arm64
job of your latest run (35817630526) both log Preferences window not found and save no screenshot, but still pass, since nothing is asserted
after waitForWindow. They take 118s and 125s, against 10-34s for the
other screenshot tests, and the UI test run grows from about 86s to
326s. Asserting that the Preferences window exists would make them fail
fast instead of passing without a screenshot.

You can probably drop the PBGitWindowWiringTests hunk.
commitController being nil in -historyListSelecting: is #703, which
#702 fixed at the source by turning PBShowStageView off in -setUp,
and your branch already includes it. With that preference off there is
no async chain left to wait for: -awakeFromNib calls
-selectCurrentBranch, which reads the current branch synchronously,
and the row selection that follows loads the history view before
-setUp returns. The wiring tests have not failed in CI since #702 was
merged. The same hunk is also in your #692.

@goneng goneng mentioned this pull request Sep 23, 2026
Draft
@hannesa2
Test settings
0399c41
@hannesa2
Address review feedback on Settings/history tests
18cdeb6 
- Drop the PBGitWindowWiringTests -view workaround: #702 already
  turns off PBShowStageView in -setUp, so -selectCurrentBranch runs
  synchronously and commitController is wired before -setUp returns.
- Assert the Preferences window actually appears in both Settings
  screenshot tests, so they fail fast instead of silently passing
  without a screenshot when the window can not be found.
@hannesa2
Fix Settings screenshot tests to find the Preferences panel
71b13f2 
Asserting prefsWindow.exists (added in the previous commit) turned up a
real bug: DBPrefsWindowController's window is an NSPanel with styleMask
(Titled | Closable | Miniaturizable) and no Resizable bit. AppKit
reports that combination to the accessibility API with subrole
AXDialog, so XCUITest classifies it as XCUIElementTypeDialog rather
than XCUIElementTypeWindow. It never showed up in `self.app.windows`
no matter how long the tests waited, which is exactly why they
previously "passed" after 118-125s without ever taking a screenshot.

Switch testSettingsGeneralTabScreenshot and
testSettingsIntegrationTabScreenshot (and their shared helpers) to
look the panel up via `self.app.dialogs` instead of
`self.app.windows`. Both tests now genuinely open the Preferences
window, click the right tab, and capture a real screenshot in ~38s.
@hannesa2
hannesa2 marked this pull request as ready for review September 24, 2026 08:55
@hannesa2

hannesa2 commented Sep 24, 2026

Copy link
Copy Markdown
Contributor Author

@goneng
I did my best, if you have suggestion, I'm fine, when you direct commit into the pull request

@hannesa2
hannesa2 requested a lite review from Copilot September 24, 2026 08:57
Copilot AI reviewed Sep 24, 2026
View reviewed changes

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

Ensure missing tabs cause test failure before clicking or capturing screenshots.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 1 Medium severity

Open (1)
What changed in this PR

Adds UI screenshot coverage for the Preferences window’s General and Integration panes.

Changes:

  • Opens Preferences via menu with keyboard fallback.
  • Adds dialog and toolbar-tab discovery helpers.
  • Captures pane-specific screenshots.

The tab lookup must fail or wait when tabs are absent; otherwise the default pane may be captured under the wrong name.

File Summary
GitXUITests/​GitXScreenshotTests.m Adds Preferences navigation, tab selection, and screenshot tests.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread GitXUITests/GitXScreenshotTests.m Outdated
Comment on lines +319 to +326
XCUIElement *btn = [self findPrefsTabButton:@"General" inWindow:prefsWindow];
if (btn.exists) {
[btn click];
[NSThread sleepForTimeInterval:0.6];
// Re-fetch — title changed to "General" after click
prefsWindow = self.app.dialogs.firstMatch;
} else {
NSLog(@"[GitXScreenshotTests] General toolbar button not found");
@goneng
Open Preferences with its key equivalent in the screenshot tests
5b3c048 
Reach the Preferences window by pressing Command-, rather than walking
the GitX menu, the same way the view switching moved to Command-1 and
Command-2.

- Drop the menu walk: the item is named "Settings..." on current macOS,
  so the lookup for "Preferences..." always ran out its 5 second wait,
  and each menu query took 5-15 seconds on the CI runner, about 75
  seconds per test in all.
- Keep the Command-, the walk already fell back to as the only path.
@goneng
Fail the Settings screenshot tests when the tab button is missing
546f101 
Make a missing General or Integration toolbar button fail the test
instead of saving whichever pane is showing under that pane's name.

- Wait up to 5 seconds for the button, then assert it exists before
  clicking it.
- This matters because the Preferences window reopens on the pane it
  last showed, so once the Integration test has run, the General test
  would otherwise capture the Integration pane.
@goneng
Keep the Preferences date sample fixed in the screenshot tests
e1730a6 
Let the UI tests pin the date the General pane's date-format sample
shows, so its screenshot stops changing every minute and flagging every
later pull request as a screenshot difference.

- Read GITX_UITEST_NOW, in seconds since 1970, in place of the current
  time when building the sample, next to the existing GITX_UITEST_REPO
  hook.
- Set it to 2026-01-01 12:00 UTC for every screenshot test launch.
@hannesa2

hannesa2 commented Sep 24, 2026

Copy link
Copy Markdown
Contributor Author

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

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

commit-context-menu.pngscreenshot

scripts/Screenshots-arm64/settings-general.png (new screenshot)
screenshot

scripts/Screenshots-arm64/settings-integration.png (new screenshot)
screenshot

@goneng

goneng commented Sep 24, 2026 •
edited
Loading

Copy link
Copy Markdown
Collaborator

@hannesa2 thanks, I pushed three small commits on top:

  • the Settings tests now open Preferences with Cmd-, instead of searching the menus,
  • fail when the 'General' or 'Integration' tab button is missing
    instead of saving whichever pane is showing, and
  • pin the 'General' pane's date sample with a new GITX_UITEST_NOW launch variable
    (2026-01-01 12:00 UTC) so that screenshot no longer changes every minute.

The two Settings tests went from 99s and 117s to 26s and 25s, and
the whole UI test run from 318s to 146s.
The commit-context-menu diff in the bot comment is the same text-rendering noise as before,
and the two Settings screenshots show as new only because master has no setpoint for them yet.

@hannesa2 hannesa2 removed wontfix pipeline CI pipeline labels Sep 25, 2026
@hannesa2
hannesa2 merged commit b10f42f into master Sep 25, 2026
3 checks passed
@hannesa2
hannesa2 deleted the TestSettings branch September 25, 2026 05:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot
Copilot review effort
Applies to this pull request for everyone.Learn more about Copilot code review.
Copilot left review comments

Assignees

No one assigned

Labels

test Test related

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@hannesa2 @goneng