Skip to content

Make the UI suite stop failing for reasons unrelated to the change under test - #41

Merged
m96-chan merged 2 commits into
mainfrom
fix-35-keyboard-resize-flake
Sep 15, 2026
Merged

m96-chan merged 2 commits into
mainfrom
fix-35-keyboard-resize-flake

Conversation

@m96-chan

@m96-chan m96-chan commented Sep 14, 2026

Copy link
Copy Markdown
Owner

Three UI tests failed in one day, on three different pull requests, for three reasons that had nothing to do with what those pull requests changed:

failure
#35 a tap delivered while the credential sheet animated in was dropped, so the sheet stayed up and Reconnect never came
hostName No matches found on a text field whose sheet had not presented yet
Settings Timed out while synthesizing event after the app sat not idle for half a minute

All three are the same mistake. waitForExistence returns as soon as an element is in the accessibility tree — which is before it has arrived where it is going and before it can be touched. The question worth asking is isHittable, and these tests mostly were not asking it.

What changed

XCUIElement.tapWhenReady / typeWhenReady, and the duplicated add-a-host preamble in KeyboardResizeUITests and iOSSHUITests — where two of the three landed — now goes through them:

app.buttons["addHost"].tapWhenReady()
app.textFields["hostName"].typeWhenReady("Keyboard resize")

The idiom was already in the suite twice, discovered independently: TailscaleImportUITests.bringIntoView does it by hand for one element, and KeyboardResizeUITests already waited on isHittable before tapping accessory buttons. This makes it shared rather than rediscovered per file.

CI retries: -retry-tests-on-failure -test-iterations 3. Not because a flaky test is acceptable, but because one blocking an unrelated pull request is worse — this pull request was stopped three times by failures that were not its own. Only failing tests are retried, a genuinely broken test fails all three attempts, and the result bundle records which needed a retry, so this reports flakiness rather than hiding it. A green run that took three goes is worth looking at.

The original #35 change, unchanged in substance

Reading the cancel path ruled out the half of #35 that would have been a product bug: submitCredential(_:requestID:attemptID:) guards on isVisible, but activeSheet = nil runs outside that guard, so even a dropped guard dismisses the sheet, and the dismissal resumes the credential continuation with nil. There is no path where the phase gets stuck while the tap has landed — which is what pointed at the tap not landing.

The Reconnect check also moved from waitForExistence to waitUntil, so a failure keeps a screenshot: the next occurrence will say whether the sheet is still up or the phase never moved.

What this is not

Neither half is a root-cause fix, and it is worth being plain about that.

tapWhenReady makes the tests tolerate an app that is slow to settle rather than explaining why it is. The Settings failure in particular — thirty seconds of "Check for interrupting elements" followed by a synthesis timeout — may well be the app's fault rather than the test's, and this does not investigate that. The retries buy time to find out without the suite blocking everything meanwhile.

#35 should stay open. Eight consecutive standalone iPad runs passed against the unchanged test, so the original flake never reproduced here; at the reported rate that has about a 23% chance, which clears nothing.

Verified

The whole UI suite on both idioms locally — iPhone 8 passing, iPad 7 passing — simulators shut down after. The branch is rebased onto main (post-#42), so the push was a force.

🤖 Generated with Claude Code

m96-chan and others added 2 commits September 15, 2026 10:50
`KeyboardResizeUITests` fails on an iPad about one run in six, timing out
waiting for a "Reconnect" button after cancelling the credential prompt.

Reading the cancel path rules out the half of #35 that would have been a product
bug. `submitCredential(_:requestID:attemptID:)` guards on `isVisible` and on the
request still being current, but `activeSheet = nil` runs outside that guard, so
a dropped guard still dismisses the sheet, and the dismissal resumes the
credential continuation with nil through `credentialSheetDidDismiss`. The
connection then reports `.disconnected` and `TerminalScreen` draws the button.
There is no path where the phase gets stuck while the tap has landed — which
leaves the tap not landing.

`waitForExistence` returns while the sheet is still animating in. On an iPad the
credential prompt is a form sheet that travels further and settles later than the
iPhone's full-height one, so the window in which a tap is delivered to a view
still in motion — and dropped — is wider there. That matches the shape of the
report: iPad only, intermittent, and stuck forever rather than slow, because the
sheet simply stays up.

Every other button this test taps already waits for `isHittable` first
(`dismiss.isHittable`, `proceed.isHittable`). Cancel did not. Now it does.

Two supporting changes. The Reconnect check goes through `waitUntil` instead of
`waitForExistence` so a failure keeps a screenshot: the next occurrence will say
whether the sheet is still up or the phase never moved, which is what this commit
had to infer by reading. And `waitUntil`'s deadline goes from five seconds to
twenty — it is how long the test waits before giving up, not a claim about how
fast anything has to be, and five is short on a runner that has been building for
half an hour and is on its second simulator.

**This is not a demonstrated fix.** Eight consecutive standalone runs on an iPad
passed against the unchanged test, so the flake did not reproduce here at all; at
the reported rate that has about a 23% chance of happening, so it clears nothing.
What this does is close the one mechanism that reading can reach. #35 stays open
until CI has run the iPad destination green enough times to mean something.

Verified not to have broken it: four iterations on each idiom, all green.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…rest

Three UI tests failed in one day, on three different pull requests, for three
reasons that had nothing to do with what those pull requests changed:

  #35        a tap delivered while the credential sheet animated in was dropped
  hostName   "No matches found" on a text field whose sheet had not presented
  Settings   "Timed out while synthesizing event" after the app sat not idle

All three are the same mistake. `waitForExistence` returns as soon as an element
is in the accessibility tree, which is before it has arrived where it is going
and before it can be touched. The question worth asking is `isHittable`, and
these tests mostly were not asking it — `XCUIElement.tapWhenReady` and
`typeWhenReady` do, and the duplicated add-a-host preamble in
`KeyboardResizeUITests` and `iOSSHUITests`, where two of the three landed, now
goes through them.

`TailscaleImportUITests` already had a `bringIntoView` doing this by hand for
one element, and `KeyboardResizeUITests` was already waiting on `isHittable`
before tapping the accessory buttons. This makes the idiom shared rather than
rediscovered per file.

CI also retries: `-retry-tests-on-failure -test-iterations 3`. Not because a
flaky test is acceptable, but because one blocking an unrelated pull request is
worse — #41 was stopped three times by failures that were not its own. Only
failing tests are retried, a genuinely broken test fails all three attempts, and
the result bundle records which ones needed a retry, so this reports flakiness
rather than hiding it. A green run that took three goes is worth looking at.

Neither half is a root-cause fix. `tapWhenReady` makes the tests tolerate an app
that is slow to settle rather than explaining why it is — the Settings failure
in particular may be the app's fault and not the test's. The retries buy time to
find that out without the suite blocking everything in the meantime.

Verified: the whole UI suite on both idioms, iPhone 8 and iPad 7, all passing.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@m96-chan
m96-chan force-pushed the fix-35-keyboard-resize-flake branch from 3100771 to 97df18f Compare September 15, 2026 02:01
@m96-chan m96-chan changed the title Wait for the credential sheet to settle before cancelling it (#35) Make the UI suite stop failing for reasons unrelated to the change under test Sep 15, 2026
@m96-chan
m96-chan merged commit 1732e8e into main Sep 15, 2026
6 checks passed
@m96-chan
m96-chan deleted the fix-35-keyboard-resize-flake branch September 15, 2026 02:32
@m96-chan

Copy link
Copy Markdown
Owner Author

Review pass found two defects in the helper this pull request adds

Both fixed in 0b98d45, both in code added by the commit before it.

waitUntilHittable was busy-waiting, not polling. It used waitForExistence(timeout: 0.1) as the throttle, and that returns immediately when the element already exists — which is exactly the case the helper is written for: present but not yet touchable. So the loop spun on back-to-back accessibility snapshots for the full twenty seconds. Querying the app as hard as possible is a poor way to wait for it to settle, and one of the three failures this helper exists to prevent was the app failing to report itself idle. It sleeps now.

tapWhenReady recorded a failure and returned. continueAfterFailure is true by default, so the caller carried on: typeWhenReady would type into nothing and raise a second, unrelated "no keyboard focus" error on top of the real one, and every later helper in the test would burn its own twenty seconds doing the same — three times over, now that CI retries. Both helpers throw instead, so a test stops at the first unmet precondition and the report is about what actually went wrong.

TailscaleImportUITests.bringIntoView had the same non-throttle, predating this branch. Fixed alongside rather than left as the one place still doing it.

The previous run

Green, with no retries used18m32s, all six jobs, and not one failed line in the log. So tapWhenReady carried both idioms on the first attempt rather than -retry-tests-on-failure covering for it. That is the result worth having; the retries are there for the next unrelated flake, not for this one.

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