test: harden timing-sensitive unit tests for slow CI runners#128
Merged
Conversation
The test waited on a fixed 5s XCTestExpectation for the mock socket server to observe the SSH bootstrap client closing its connection, then immediately read a log file written by a separate process. Under a full serial CI run, the accept-loop thread's GCD scheduling can legitimately lag behind the subprocess round-trip, so the expectation timeout fired first and the log read threw an uncaught file-not-found error on top of it (the '1 unexpected' failure in CI). Give the expectation CI-safe headroom and poll for the log file actually having its expected content instead of reading it the instant the expectation resolves, so a scheduling delay produces one clear assertion failure instead of a cascading crash-like error.
These tests pass locally on fast machines but flake on slower shared CI runners (macos-26 GitHub runners), where fixed-duration waits (single RunLoop spins, tight XCTestExpectation timeouts) starve under a heavily contended scheduler running the full serial suite. Replace fixed sleeps with condition-based polling where a real completion signal exists, and bump fixed timeouts to generous CI-safe ceilings (matching this repo's existing hardening precedent) where the wait is on a real subprocess or background-queue round trip. Fast machines still finish immediately since the poll returns as soon as the condition is true; only a genuinely slow runner needs the extra headroom. Covers: - AppDelegateShortcutRoutingTests: poll for focus repair after sendEvent instead of a single 0.05s spin - BrowserDeveloperToolsVisibilityPersistenceTests: poll for the queued hide transition to actually settle instead of a fixed 2s spin - GhosttySurfaceOverlayTests: poll for overlay mount/dismiss instead of fixed RunLoop spins (drop-zone hide, find-overlay mount and dismiss) - NotificationDockBadgeTests: give the real subprocess spawn and the main-queue drain more CI headroom - TabManagerPullRequestProbeTests: give the git subprocess + probe round trip the same headroom already used by its sibling test - WorkspaceTerminalFocusRecoveryTests: give the focus-reassert wait more than a hair-trigger 1s ceiling
The 8 CI-only failures on loaded macos-26 runners are subprocess/ scheduling starvation: GitMetadataProber's initialWorkspaceGitMetadata Snapshot runs git subprocesses and, for non-main/master branches, a synchronous gh call with a 5.0s timeout before the snapshot is applied. On CI, gh is unauthenticated and can block to the full timeout, and git subprocesses contend with the rest of the serial suite. Scale wait budgets by 4x under CI (GitHub Actions always sets CI=true; local runs stay at 1x): - TabManagerUnitTests.swift: shared waitForCondition helper now scales its timeout internally, covering all 5 TabManagerPullRequestProbeTests failures (default 3.0s and explicit 12.0s call sites alike). - AppDelegateShortcutRoutingTests.swift: scale the two fixed poll deadlines in testWindowSendEventRepairsFocusedTerminalSearchTyping AfterResponderDrift. - WorkspaceRemoteConnectionTests.swift: scale the subprocess/mock-socket timeouts in testSSHBootstrapStartupCommandPassesRemoteInstallScript AsSingleSSHCommand. - TerminalAndGhosttyTests.swift: replace the fixed 0.3s RunLoop spin in testScheduledExternalGeometrySyncWaitsForQueuedLayoutShift with two condition-polling loops (layout shift, then the separately-scheduled external geometry sync settling), each CI-scaled. Also stub gh on PATH for the duration of each TabManagerPullRequestProbeTests test (setUp/tearDown), so these fixtures never depend on a real gh binary or network access, mirroring the existing PATH-stub pattern used by shell-integration tests. Verified the stub actually intercepts a real gh pr list call (added and removed a temporary probe test) before landing it. No production code changed; no assertions weakened.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this fixes
CI has been randomly failing on a rotating set of about a dozen unit tests. They pass every time locally on a fast machine, but fail on the slower shared macos-26 GitHub runners, and which tests fail shifts from run to run. That's the signature of tests racing against a fixed clock instead of waiting for the thing they actually care about.
The one real bug
One of these wasn't just slow, it was actually broken:
CLINotifyProcessIntegrationTests.testSSHBootstrapStartupCommandPassesRemoteInstallScriptAsSingleSSHCommandwas showing up in CI as an "unexpected" failure (basically a crash), not a normal assertion failure.Here's what was happening: the test spawns a fake SSH process, waits up to 5 seconds for a background thread to notice the process closed its socket connection, then immediately reads a log file that a different process wrote to. Under a full serial suite run, that background thread's turn to run on the CPU can get delayed by GCD scheduling, so the 5 second wait would time out. Then the test would barrel ahead and try to read the log file anyway, and since it might not exist yet, that raised an uncaught error - which is what CI was reporting as "1 unexpected" failure.
The fix: give that wait more headroom for CI, and instead of assuming the log file is ready the instant the wait resolves, poll for the file to actually have its expected content before reading it. Now a slow CI runner gets one clear, readable assertion failure instead of a confusing crash-like error, and normally it just passes.
Everything else
The other ~8 tests were legitimately just too impatient: a single fixed RunLoop spin of 0.05-2 seconds, or a tight expectation timeout, checked once and then asserted on immediately. On a loaded CI box that's not always enough time for the async work being tested to finish.
Fixed the same way throughout: poll for the actual condition to become true (with a generous timeout), or bump a fixed timeout to a number with real headroom, following the same pattern already used elsewhere in this test suite for this exact problem. None of the assertions themselves changed, only how long and how we wait before checking them. Fast machines won't notice a difference since the poll returns as soon as the condition is true.
Testing