Skip to content

test: replace fixed 50ms spins with condition waits in shortcut routing tests - #218

Merged
arzafran merged 1 commit into
mainfrom
test/186-deflake-fixed-waits
Jul 30, 2026
Merged

test: replace fixed 50ms spins with condition waits in shortcut routing tests#218
arzafran merged 1 commit into
mainfrom
test/186-deflake-fixed-waits

Conversation

@arzafran

Copy link
Copy Markdown
Member

Second slice of #186 — but not the one the issue proposes. See below.

Why not the issue's proposal

#186 proposes extracting a pure routing decision and retiring the 87 tests. That is blocked, and worth recording rather than rediscovering.

#208 already extracted the only cleanly-pure logic (ShortcutRouting.matchStroke/match) — that's 3 table tests. The other 84 exercise routing decisions whose implementation is ~55 flat if matchConfiguredShortcut(…) { perform effect; return true } branches, with decision and effect fused per branch and no intermediate value to assert against. There is nothing pure left to table-test them against, which is the same wall that made ShortcutRouter not worth attempting in #187.

So this targets the issue's actual cost driver instead.

What this does

59 occurrences of RunLoop.main.run(until: Date(timeIntervalSinceNow: 0.05)). Each burns the full 50ms whether the work took 1ms or 49ms, and fails when it takes 51. That flakiness is precisely why ci-run-unit-tests.sh runs this class serially and absorbs a retry.

The file already diagnosed this. Three sites carry comments reading "a single fixed 0.05s spin is occasionally too short under CPU contention... causing rare flakes. Poll instead of assuming one spin is enough." This finishes what those three started.

Adds a waitUntil helper — polls the condition, returns the moment it holds, fails after 2s with the caller's file/line. 26 sites converted to wait for exactly the state the following assertion checks.

What was deliberately left alone

No assertion was added, removed, or modified. Verified: zero XCTAssert lines on either side of the diff.

27 sites stay as fixed spins, because converting them would be wrong rather than merely unfinished:

  • 15 makeKeyAndOrderFront sites — a plain NSWindow can never genuinely become key in the headless test host; the file documents this. Polling isKeyWindow there would time out on every CI run. These were converted first, then reverted on finding that comment.
  • 5 negative assertions ("this count should not have changed") — the spin's job is to give latent async work a window to misbehave before asserting it didn't. Polling returns instantly and defeats the test.
  • 1 tearDown drain, 2 settle spins, 1 shared close helper, 1 float-accuracy assertion — none reduce to a condition.

Not touching CI yet

The serial isolation, the permanent skip, and the retry in scripts/ci-run-unit-tests.sh all stay. Removing them needs evidence from several green runs, not one. That's the follow-up once this has some history.

Test plan

  • xcodebuild -scheme programa-unit build-for-testing** TEST BUILD SUCCEEDED **, zero error: matches, modified file confirmed in the compile log
  • Zero assertion lines changed
  • Fixed spins 59 → 34 (26 converted; the helper itself contains one)
  • CI green — the 87 tests are their own regression check here

…ng tests

AppDelegateShortcutRoutingTests had 59 occurrences of

    RunLoop.main.run(until: Date(timeIntervalSinceNow: 0.05))

Each burns the full 50ms whether the awaited work took 1ms or 49ms, and fails
when it takes 51. That is why CI runs this class serially and absorbs a retry.

The file already diagnosed this itself: three sites carry comments saying "a
single fixed 0.05s spin is occasionally too short under CPU contention, causing
rare flakes. Poll instead of assuming one spin is enough." This finishes what
those three started.

Adds a waitUntil helper that polls the condition, returns the moment it holds,
and fails with a clear message and the caller's file/line after 2 seconds.
Converts 26 sites to wait for exactly the state the following assertion checks.

No assertion was added, removed, or modified.

27 sites are deliberately left as fixed spins:
- 15 makeKeyAndOrderFront sites, because a plain NSWindow can never genuinely
  become key in the headless test host -- the file documents this, and polling
  isKeyWindow there would time out on every run
- 5 negative assertions, where the spin's job is to give latent async work a
  chance to misbehave before asserting it did not; polling would return
  instantly and defeat the test
- a tearDown drain, two settle spins, a shared close helper, and one
  accuracy-based float assertion, none of which reduce to a condition

Does not touch the CI retry or serial isolation. Removing those needs evidence
from several green runs, not one.

Refs #186
@arzafran
arzafran merged commit 47c4f70 into main Jul 30, 2026
16 of 17 checks passed
@arzafran
arzafran deleted the test/186-deflake-fixed-waits branch July 30, 2026 22:07
arzafran added a commit that referenced this pull request Jul 30, 2026
Un-skipping it (previous commit) answered the question the skip could not: it
does not flake, it fails.

  AppDelegateShortcutRoutingTests.swift:1463: failed - Timed out waiting for
    Cmd+W on the last surface to close the window
  AppDelegateShortcutRoutingTests.swift:1465: XCTAssertNil failed: "<NSWindow: 0x...>"

It waits the full 2s that #218 gave it and the window is still there, so more
time does not help. The shortcut itself dispatches -- the XCTAssertTrue on
debugHandleCustomShortcut just above passes. And headless window closing works
in general: two sibling Cmd+Ctrl+W tests close real windows on the same runner
and are not skipped.

That narrows it to the cascade this test exercises -- close last surface, close
last workspace, close window -- and leaves open whether it is a headless-only
gap or a real bug in a path users rely on.

Restores the skip so CI stays green, with all of that written down. The wiring
is byte-identical to before; the diff is comment-only.

Refs #186
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant