fix(appium): match Android checked state - #154
Conversation
Parse the Android checked attribute separately from selected and use it for checked selectors. Add a regression where the two states deliberately disagree.\n\nFixes devicelab-dev#153.
Follow-up to #154, which fixed this on the Appium driver and left UIAutomator2 and DeviceLab with the same bug — `checked:` behaving differently depending on which driver ran the flow is worse than it being wrong everywhere. All three compared `checked:` against the unrelated `selected` state, and none of them parsed the Android `checked` attribute at all. A Switch that is on but not focused reports checked="true" selected="false", which is the ordinary case, so the selector could not match it — and would happily match a different, unchecked element that happened to be selected. Test fixture makes the two disagree in both directions, on both drivers. Credit to @nt-ben-leblond, whose #153 identified the comparison and whose #154 carries the fix this extends.
|
Merged — thank you. Clean diagnosis, a fixture that makes checked and selected disagree in both directions, and a real-device run; that's about as complete as a bug fix gets. One note on scope, in case it comes up: you added the checked case to the Android parse path only, which is right. The second case "selected" in that file is the XCUITest path, and iOS has no checked attribute — it reports switch state in value. I've followed up in fa63a3b extending the same fix to the UIAutomator2 and DeviceLab drivers, which had the identical comparison and also never parsed the attribute. checked: behaving differently depending on which driver ran the flow seemed worse than And thanks for flagging TestListAvailableAVDs_NoEmulatorBinary — that failure is ours, not yours. It asserts no emulator is on PATH, so it fails on any machine that has the Android SDK installed. It's on the list. |
* fix(report): a step's label wins over its selector in the HTML report `label:` is the author saying, in their own words, what a step is for. In the HTML report it was consulted last — after the selector, the text and the direction — so it only ever appeared on steps that had none of those to show instead. That produced the reported symptom exactly: in one flow, a labelled extendedWaitUntil displayed its label while a labelled tapOn showed its selector and a labelled assertVisible showed its matched text. Same syntax, three different outcomes, and no way to tell from the YAML which you would get (devicelab-dev#150). The label now comes first. JUnit and Allure already preferred it, so this also ends the HTML report disagreeing with the other two about what a step is called. Verified on a generated report by running the report's own extractKeyValue over real flow data: before, three of four labelled steps showed a derived value; after, all four show the label, including non-ASCII text. Reported by @SmileYang966. * fix(selectors): anchors mean something, and regex queries stop 404ing Two faults, one root: the WDA query path and the page-source path did not agree on what a text selector means, and neither agreed with Maestro (devicelab-dev#151). A regex selector was interpolated verbatim into WDA's NSPredicate, so `^SIGN OUT$` asked for a literal caret via `CONTAINS[c]` and every query 404'd — six failed round trips per step before the page-source path resolved it anyway. Regex selectors now go straight to page source. NSPredicate does have MATCHES, but its ICU dialect is not Go's, and a third opinion about what a pattern means is the last thing this needed. Measured on a simulator, an anchored selector goes from 2.6s to 44ms. The costly half was quieter. The page-source matcher compiled every regex with (?i), so an anchored pattern could not distinguish what it was written to distinguish: `^SIGN OUT$` matched a "Sign out" row exactly as readily as the "SIGN OUT" button, and whichever appeared first in the page source won. The reporter had a flow tap the row behind an open dialog, report success, and fail several steps later somewhere unrelated. Regex selectors are now matched case-sensitively on all four drivers, as Maestro does. Scope of the behaviour change is narrower than it sounds: `looksLikeRegex` only fires on actual metacharacters, so plain selectors like `tapOn: "sign out"` are untouched and still match case-insensitively. Only patterns written deliberately as regexes change, which is exactly where the old behaviour was wrong. A pattern that wants insensitivity can still ask: `(?i)^sign out$`. Verified on a simulator both ways: `^welcome back$` passed against text reading "Welcome Back" on 1.1.25, and now fails. Reported by @Ahlisen, with a reproducer and a diagnosis that were both exactly right. * perf(flutter): stop re-serialising the widget tree on every poll The Flutter fallback re-fetched the app's entire widget tree continuously for the whole of every element search — 2.9MB twice a second on the reporting user's app, for as long as the search ran. Steps that took under 700ms with the fallback disabled took 4-9s with it on (devicelab-dev#152). Two causes, both fixed. The widget dump was fetched up front on every poll, though it is consulted only when the semantics search finds nothing. Searches the semantics tree could answer — the common case — paid megabytes for a result they never used. It is now fetched after that search misses. The poll loop had no delay at all. It spun as fast as the VM Service would answer, so a search running to timeout hammered the connection for the entire find window while contending with the app it was inspecting. It now paces at 300ms, chosen to stay under the inner driver's 1s poll so Flutter still answers first when it can. A tree that has not repainted cannot start matching, so polling faster than the app can change buys nothing. Measured on a real Flutter app over one 6-second search: 362 widget-tree fetches before, 46 after. Reported by @Ahlisen, with measurements. The second half of that report — a tapOn reporting success without issuing a tap — is not addressed here and remains open. * fix(appium): match Android checked state (devicelab-dev#154) The page-source parser dropped the Android checked attribute, so state filtering compared checked: against the unrelated selected state. A switch whose checked and selected disagree — the normal case for a Switch that is on but not focused — could not be matched at all. Scoped to the Android parse path; XCUITest has no checked attribute and reports switch state in value. * fix(android): checked: reads the checked state on the native drivers too Follow-up to devicelab-dev#154, which fixed this on the Appium driver and left UIAutomator2 and DeviceLab with the same bug — `checked:` behaving differently depending on which driver ran the flow is worse than it being wrong everywhere. All three compared `checked:` against the unrelated `selected` state, and none of them parsed the Android `checked` attribute at all. A Switch that is on but not focused reports checked="true" selected="false", which is the ordinary case, so the selector could not match it — and would happily match a different, unchecked element that happened to be selected. Test fixture makes the two disagree in both directions, on both drivers. Credit to @nt-ben-leblond, whose devicelab-dev#153 identified the comparison and whose devicelab-dev#154 carries the fix this extends. * build: -trimpath in the Makefile too The commit that introduced this covered the release script and said it covered the Makefile as well; the Makefile change was left unstaged. Without it a `make build` binary still carries the build machine's absolute paths, which is the disclosure devicelab-dev#149 arrived with. * fix(android): require id and text on the same element in both drivers (devicelab-dev#158) The finder returns on the first strategy that matches, and id strategies were emitted independently of text strategies — so whenever the id matched, the text was never read, and assertVisible with a right id and a wrong text passed green. Composing the constraints as fragments means no query can match on one attribute alone. Android counterpart of the iOS fix in devicelab-dev#130. * build: compile for Windows `GOOS=windows go build ./...` failed on one line, in the iOS driver — the last place a Windows user would look: pkg/driver/devicelab_ios/setup.go:275: unknown field Setpgid in struct literal of type syscall.SysProcAttr `Setpgid` is Unix-only; the Windows SysProcAttr has no such field. It was the sole occurrence in the repository, so a single build-tagged helper is the whole fix: the Unix build keeps putting xcodebuild in its own process group, and the Windows build gets a no-op. Nothing is lost by that no-op. Windows has no process groups in the POSIX sense, and this package drives xcodebuild against iOS simulators, so it never runs there. The point is only that the package compiles — which is what the rest of maestro-runner needs in order to build for a platform where the Android and web drivers work perfectly well. The Makefile has had a build-windows target throughout, so this was a target nobody could run. All six targets cross-compile now: darwin, linux and windows on arm64 and amd64. Reported by @RieBi, with the fix. * fix(appium): require id and text on the same element The third driver with devicelab-dev#157's bug, after the two devicelab-dev#158 fixed. findElementDirect tries the id first and returns on the first match, so a selector naming both an id and a text answered on the id alone and never read the text — an assertVisible with the right id and a wrong text passed green. Routed to page source when both are given, which is where a selector's fields are all checked together. That is the same route state filters already take a few lines above, for the same reason: the direct queries each speak for one attribute, and no combination of them can express "the element with both". It costs a page-source fetch on those selectors. The alternative was a chained UiSelector for Android and a combined predicate for iOS — a third dialect of selector composition alongside the fragments devicelab-dev#158 introduced, to be kept in agreement with them forever. Correctness through the matcher everything else already agrees with is worth the round trip. Follows @RieBi's devicelab-dev#157 and devicelab-dev#158, which found and fixed the same fault in UIAutomator2 and DeviceLab. * fix(flow): parse flow files with Windows line endings A flow file with CRLF failed on its first line: invalid steps: yaml: unmarshal errors: line 1: cannot unmarshal !!map into []yaml.Node The document splitter divided on "\n", leaving a trailing "\r" on every line. Its separator test compares the untrimmed line against "---" — deliberately, to tell a real document break from an indented "---" inside a block scalar — and "---\r" fails that. So the break was never found, the file parsed as a single document, and the header's map was handed to the step list. Windows checks out with core.autocrlf=true by default, which made this every flow on Windows rather than an edge case. Normalising CRLF before the split fixes it, and a test pins that a "---" inside a block scalar is still content rather than a separator. Also builds the expected path in TestScriptEngine_ResolvePath with filepath.Join instead of a literal "/". ResolvePath joins with the host separator, so the hard-coded form failed on Windows against the identical answer spelled with backslashes. Both found by @RieBi while verifying the Windows build in devicelab-dev#159, along with the end-to-end confirmation that Android flows do run there. --------- Co-authored-by: Om Narayan <omnarayan@robustest.com> Co-authored-by: Ben LeBlond <ben.leblond@ninjatrader.com> Co-authored-by: Sviatoslav Zubar <riebisv@gmail.com>
Summary
Fix Android Appium
checked:selectors so they read the hierarchycheckedattribute instead of the unrelatedselectedstate.Type of Change
Changes Made
checkedstate inParsedElement.Selector.Checkedwith the parsed checked state.Related Issues
Fixes #153
Testing
make testpasses locallyPassed locally:
go test ./pkg/driver/appium -count=1go test -race ./pkg/driver/appium -count=1go vet ./pkg/driver/appiumgo build .checked: truebefore the tap andchecked: falseafter it: 80/80 steps passed on Pixel 8 / Android 14 — https://app.saucelabs.com/tests/e95b01337f2145fd8c02d14c5cd0990fThe repository-wide
go test ./...run encountered an unrelated environment-sensitive failure inTestListAvailableAVDs_NoEmulatorBinarybecause this workstation has Android AVDs installed, then was bounded while the browser/CDP package ran its first-time Chromium tests. The complete changed Appium package passes, including under the race detector.Checklist
Additional Notes
The production change is two behaviors: parse
checked, then filter against it. The regression exercises the same XML shape observed through Appium on Sauce Labs (checked=true,selected=false).