fix(runner): bound XCTest's idle wait around alert activation - #2610
Conversation
Size Report
Startup median (7 runs, lower is better):
|
|
Reviewed at a41899b. The change can report an alert as activated when it should refuse, and this PR's own CI run shows that symptom.
The same test also fails on #2603, which does not touch the runner, but at a different place (lines 75 and 80, an action-count mismatch). So the #2546 flake does not explain this run, and it looks related to this change. |
|
Reviewed at a41899b. The Smoke failure looks caused by this change. Wrapping The new comment at Smoke Tests fails in Next: restore the post-tap margin on the alert path and get that test green repeatedly. |
a41899b to
afafe1a
Compare
|
Both findings are right and the Smoke failure at The alert path now bounds only the pre-event wait. The comment names both waits and which caller needs which. It previously described only the pre-synthesis gate, which is exactly how the dropped post-event wait went unnoticed. The enum carries the reasoning per case, and the alert call site says why the settle stays. Loop evidence on the final head, dedicated The mutation that makes the pinned behaviour fail is the one this review found: passing |
|
Reviewed at afafe1a. The alert path now bounds only the pre-event idle wait, the gesture, type and swipe callers keep the old skip-both behaviour, and both earlier findings are addressed. One gap is left. No test reaches The live runs in the PR are author-reported. The original ~13 s pre-synthesis stall still cannot be provoked on demand, so it stays open whether that stall is gone. Typecheck & Package fails with TS2353 on Next: the busy-app deadline test. |
afafe1a to
f94c74f
Compare
|
Rebased onto
Also unblocked: the red Typecheck & Package job was main's own |
f94c74f to
6a9e275
Compare
|
Added at The fixture can now hold an alert screen open in the state that cost the command its deadline: Reverting Two things learned while tuning it, both in the fixture's comment: a repeating main-thread timer and a bare At this head, runner built from the rebased tree: the nine alert XCTests passed 9/9 in two runs and the new test passed two further single runs (5.6 s, 6.1 s). A third suite run reported eight passed with zero failed and failed the session aggregate after On your second point: agreed, and unchanged by this test. It guards the defect shape — deadline exhausted while the button lands later — not the ~13 s pre-synthesis stall itself, which still cannot be provoked on demand here. Whether that particular stall is gone stays open.
|
|
Reviewed at 6a9e275, as a follow-up to the review at f94c74f. The new busy-app test gives the deadline fix a regression guard, but its last check can never pass on its own. In RunnerTests+AlertObservationTests.swift#L82-L89, line 82 always expects The new test is not in the There are no conflicts. Next: fix the conditional assert and the fixture comment. |
An alert button is read as hittable before it is tapped, so XCTest's own wait for the app to idle before synthesizing that tap adds nothing the runner has not already checked. Its default outlives the command, so the tap lands after the caller was told the alert timed out (#2546). The helper that bounds that wait was named for the scroll path that owned it; gesture, type and swipe already went through it, and alert activation now does too, so it is named for the wait it bounds. Each caller states which of XCTest's two waits it gives up: the gesture, type and swipe paths skip both, because their next step is a poll of their own, while alert activation drops only the pre-event wait. Its verification reads the alert this tap replaced, and an alert that dismisses in order to present an identical replacement passes through a moment with no alert at all — a first read landing in that moment reported a dismissal nothing had proved.
Reverting the alert path to a plain `activateElement` kept every test green, so the #2546 failure had no guard: the deadline tests returned before activation, and the ones that reached it used a 10 s timeout on a fixture that had already settled. The fixture now animates without end until an alert button is answered, which is the state that made XCTest hold the event past the caller's deadline in the field. A test that reaches activation there and is given 6 s reports what actually happens: the response comes back in well under a second of activation, and if it ever reports `ALERT_DEADLINE_EXCEEDED` again the fixture says whether a button was activated behind that answer. Reverting the seam costs the run 18 s and produces exactly the original defect — `ALERT_DEADLINE_EXCEEDED` alongside `First actions: 1`. Only a `UIView` animation counts here: a repeating main-thread timer and a `CABasicAnimation` both left the app looking idle and the reverted path still passed.
…nswered The label check ran twice over the same response: it always expected `First actions: 1`, and the block after it expected `First actions: 0` only when the answer was a deadline failure, so the second could never pass and a correct refusal failed anyway. Now the assertion follows the answer — an accepted alert must show the button press, a refused one must not — and the comment said "main thread busy" where the fixture runs a repeating animation with the main thread free. The test also joins the targeted list in `.github/workflows/ios.yml`, which is where the alert regressions it guards are selected.
6a9e275 to
5dae723
Compare
|
Fixed at The label check now follows the answer instead of asserting both shapes over one response: let recordedActions = app.staticTexts["agent-device-alert-actions"].label
if response.ok {
XCTAssertEqual(recordedActions, "First actions: 1; replacement actions: 0")
} else {
XCTAssertEqual(
recordedActions,
"First actions: 0; replacement actions: 0",
"a caller told about an expired deadline must not have a button activated behind it"
)
}
The test is in the Re-ran the mutation at this head, runner built from the tree: reverting On the Smoke failure: agreed it is not this PR's — Also worth knowing while you look at this PR: |
|
Reviewed at 5dae723, as a follow-up to the review at 6a9e275. The label check now branches on Smoke Tests and Coverage are still running at this head; the earlier depth-frontier Smoke failure is outside the files this PR changes, so it looks unrelated. There are no conflicts. Ready for human review. |
thymikee
added
the
ready-for-human
|
1 participant
Summary
The runner bounded XCTest's idle wait around alert activation so a bounded command cannot be answered as a failure for a tap the device performed afterwards (#2546). The hittable read that gates alert activation is that activation's readiness check, so XCTest's pre-synthesis wait adds nothing to it and can cost more than the command has.
Each caller now names which of XCTest's two waits it gives up, through
RunnerInteractionIdleWaits: the gesture, type and swipe paths skip both, because their next step is a poll of their own, while alert activation drops only the pre-event wait and keeps the post-event settle. Its verification reads the alert this tap replaced, and an alert that dismisses in order to present an identical replacement passes through a moment with no alert at all — a first read landing in that moment reported a dismissal nothing had proved. The helper is also renamed from the scroll-only name that gesture, type and swipe were already using.## ValidationTested at
6a9e275e19(rebased onorigin/main).pnpm check:affected --rungreen at this head; it selectsxctest-selectionandpackaged-runner-swift.Apex CI Smokesimulator, runner built with unit tests included: the nine alert-observation XCTests passed 9/9 in two consecutive runs, andtestAlertCannotProveAnIdenticalReplacementAndDoesNotActivateIt— the one this PR's Smoke run failed — passed in two more single-test runs. A third run reported eight passed with zero failed and failed the session aggregate, the same post-testxcodebuildanomaly as on the previous head.testAlertActivationIgnoresAnAppThatNeverSettlesBeforeTheDeadlineis the guard for iOS runner smoke: alert-observation XCTests are red or flaky on main #2546: the fixture animates until an alert button is answered, and withhandleAlertreverted to the plainactivateElementcall that run waits 18.1 s, answersALERT_DEADLINE_EXCEEDEDwith the button activated behind it, and trips the assertion that forbids exactly that. With this change it answers within a second of activation.a41899ba59is this change's: skipping the post-event wait let the first observation land in the dismiss-then-represent gap and answerok: true. Restoring the post-tap settle is what closes it, and the alert tests that cover shared-title and identical replacement cases run in the same loop.