fix(ios): find a popover dismiss region in one query instead of reading every element - #2901
Conversation
0b82660 to
ee75e9b
Compare
|
Applied the review notes at ee75e9b. (1) One marker pattern now serves the query, the title filter, and alert observation. (2) The fixture has 500 labels: without the fix the absent-alert test takes 282 s against its 5 s cap; with it, 0.37–0.39 s. A new test covers a window that is itself the marker. That test showed XCTest's |
ee75e9b to
19584de
Compare
Size Report
Startup median (7 runs, lower is better):
|
|
Rebased onto main at |
19584de to
cb62ac2
Compare
|
At cb62ac2, the fix direction looks right, but the regression test meant to prove it likely can't. In RunnerTests+AlertTests.swift, testAlertResolutionFindsAWindowThatIsItselfTheDismissPopupMarker (https://github.com/callstack/agent-device/blob/cb62ac2/apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/UnitTests/RunnerTests+AlertTests.swift#L177) launches a fixture where only the window's own accessibilityIdentifier is "Dismiss popup" and the sole on-screen button is identifier "agent-device-close-popover", which doesn't match the marker pattern. Given that fixture, resolveAlert should return nil and the XCTUnwrap should throw, so this test looks like it currently fails against the PR's own implementation. If that's right, the case it's meant to cover — a window that is itself the marker — isn't actually shown to work, and if CI is misconfigured or this test gets skipped, the fix could ship broken. Does firstDismissPopupWindow gain a query that matches the window itself, or only one that matches a descendant containing the marker? Apple's XCUIElementQuery.h documents containingPredicate: as matching descendants only, not self, so the self-matching case needs its own query next to containing(). I did not build or run the iOS target to check this; the live-sim validation in the PR covered the absent-alert path, not this marker-on-window-itself path. CI is still pending. The only checks in question sit inside the single Smoke Tests job, which runs both the targeted iOS runner XCTest regressions (this PR's own gates commit adds -only-testing entries for the three new alert-resolution unit tests here) and the fixture-backed E2E smoke that covers the #2491 scenario this PR fixes, so the job is queued rather than failed and no log yet confirms or refutes the test failure above. There are no known conflicts. Before merge, someone needs to run testAlertResolutionFindsAWindowThatIsItselfTheDismissPopupMarker, either locally or by reading the completed Smoke Tests log, and confirm it passes; per the containingPredicate: documentation it should not, which would mean firstDismissPopupWindow needs a self-matching query added alongside containing(). |
…ng every element alert get/accept/dismiss with no alert walked every descendant of every window, one XCTest round trip per element plus a retry cycle for each element that vanished mid-walk. On the WebView lab it outran the 10 s alert budget and kept the runner main thread busy past 30 s, so the following wait saw RUNNER_BUSY on every poll until its deadline (smoke:webview-remote-content, #2491).
…ded-screen margin The query predicate, the alert-title filter, and the post-activation observation now all read one pattern. The crowded fixture grows to 500 labels so the absent alert test fails by a wide margin without the query (282 s against a 5 s cap), and a new test covers a window that is itself the marker, which the single containing query also returns.
cb62ac2 to
1416066
Compare
|
Correction to my earlier comment: the Smoke run on cb62ac2 shows The one failing test in that run was I rebased this PR onto main at 1416066 (no conflicts, and main did not change the runner or ios.yml since the old base). #2902 is rebased on top of it. CI is running again on both. |
thymikee
added
the
ready-for-human
thymikee
left a comment
thymikee
left a comment
There was a problem hiding this comment.
Reviewed at 1416066, including the later gate-selection commit. No inline findings. The predicate fixes the unbounded descendant walk, the shared marker pattern keeps the query/title/observation predicates from drifting, and the 500-label mutation plus live 10/10 evidence pin the original pathology. The window-self test is valuable because the containing behavior is otherwise easy to misread as a child-only match.
|
|
Reviewed the delta since cb62ac2 at 1416066. The one-query search for the popover dismiss region looks correct, and nothing from the earlier review is still open. The earlier CI failure at cb62ac2 ( |
|
Re the window-marker concern: I ran I also changed only the fixture window identifier to |
1 participant
Summary
Fixes
smoke:webview-remote-content→wait for the WebView page to expose its link(#2491 row 1, run 35982745770). A runner restart timed out the landmark wait, so the scenario sentalert get. With no alert up, the runner then read every descendant of every window to find a popover dismiss region. Each element cost one XCTest round trip, and each element the loading page removed cost a 4 s retry. After the 10 s watchdog the runner stayed busy 38 s; every wait poll spent the #2863 resend window (~6 s) onRUNNER_BUSY.Now one predicate query per window set finds the region. Its query, the alert-title filter, and alert observation share one marker pattern. Three iOS XCTests on a 500-label fixture check it: an absent alert resolves in under half the alert budget; a trimmed, mixed-case marker on a button resolves; a window that is itself the marker resolves. Complements #2897.
Validation
Head
ee75e9b9ae:pnpm check:affected --runpassed (exit 0).firstDismissPopupWindowrestored): the absent test fails at 282 s against its 5 s bound (500 labels). With the fix: 0.37–0.39 s over 3 runs.alert get→ page wait:MAIN_THREAD_TIMEOUTat 10.2 s; the wait failed withwait_capture_stalledafter 20 s; the runner stayed busy 21.9 s.ALERT_NOT_FOUNDin 0.18–1.2 s; the wait passed in <0.7 s.Residual: the runner restart is a separate signature.