From 4f67a59c74389ec766b165ee37e16fc337485c5a Mon Sep 17 00:00:00 2001 From: Dave Craig Date: Sat, 6 Jun 2026 16:54:28 +0100 Subject: [PATCH] Fix failure in Japanese large screen test SwipeAndTap is never going to be perfect because the isVisible test is unreliable - a target can be visible but can't be tapped :-( This fixes the specific Japanese issue where the nudge was moving the target offscreen. The script now detects that and nudges it back! Of course it could be nudged to be visible but not tappable, so this is all a bit sketchy. --- .github/workflows/run-maestro-tests.yaml | 19 +++++++----- docs/developers/maestro-tests.md | 38 +++++++++++++++++------- maestro/SwipeAndTap.yaml | 29 ++++++++++++++++++ 3 files changed, 68 insertions(+), 18 deletions(-) diff --git a/.github/workflows/run-maestro-tests.yaml b/.github/workflows/run-maestro-tests.yaml index 87d4d6b6e..e77db400e 100644 --- a/.github/workflows/run-maestro-tests.yaml +++ b/.github/workflows/run-maestro-tests.yaml @@ -199,14 +199,17 @@ jobs: mkdir maestro_outputs # Run every flow even if an earlier one fails, then fail the step at # the end if any of them failed. - status=0 - $HOME/.maestro/bin/maestro test --format=junit --output=report1.xml --test-output-dir=maestro_outputs --no-ansi maestro/Onboarding.yaml || status=1 - $HOME/.maestro/bin/maestro test --format=junit --output=report2.xml --test-output-dir=maestro_outputs --no-ansi maestro/HomePage.yaml || status=1 - $HOME/.maestro/bin/maestro test --format=junit --output=report3.xml --test-output-dir=maestro_outputs --no-ansi maestro/LocationDetails.yaml || status=1 - $HOME/.maestro/bin/maestro test --format=junit --output=report4.xml --test-output-dir=maestro_outputs --no-ansi maestro/PlacesNearby.yaml || status=1 - $HOME/.maestro/bin/maestro test --format=junit --output=report5.xml --test-output-dir=maestro_outputs --no-ansi maestro/MarkersAndRoutes.yaml || status=1 - $HOME/.maestro/bin/maestro test --format=junit --output=report6.xml --test-output-dir=maestro_outputs --no-ansi maestro/RouteCreation.yaml || status=1 - exit $status + # + # This MUST stay a single line. android-emulator-runner runs each + # script line in its own "sh -c" (see the comment above), so a + # "status" variable accumulated across separate lines, with a trailing + # "exit $status", does not work: each line gets a fresh shell, the + # "|| status=1" makes the failing line still exit 0, and the final + # "exit $status" sees an empty variable and exits 0 - which left the + # step (and the whole run) green even when every flow failed. Keeping + # the loop and the exit in one shell makes the non-zero status stick. + # To add a flow, add its name (without .yaml) to the list below. + status=0; for flow in Onboarding HomePage LocationDetails PlacesNearby MarkersAndRoutes RouteCreation; do $HOME/.maestro/bin/maestro test --format=junit --output="maestro_outputs/report-$flow.xml" --test-output-dir=maestro_outputs --no-ansi "maestro/$flow.yaml" || status=1; done; exit $status # Artifact names must be unique across the matrix, so suffix them with the # combination (e.g. maestro-reports-35-large-ja-JP). diff --git a/docs/developers/maestro-tests.md b/docs/developers/maestro-tests.md index 9ad29e13e..498d2e73a 100644 --- a/docs/developers/maestro-tests.md +++ b/docs/developers/maestro-tests.md @@ -151,6 +151,17 @@ A few small reusable flows take parameters via `env:` and are called with is tappable. Instead `SwipeAndTap` swipes the scrollable region in small steps until the element appears, then nudges it clear of the edge so it's not clipped when tapped. + + The nudge swipe can accidentally land on an **inner scrollable list** (rather + than the outer page) and scroll a fully-visible target back out of view — this + is what happened with the audio-beacon list in `Onboarding.yaml`: the nudge + scrolled "Current" out from under the tap, so no beacon was selected and the + continue button stayed disabled. To handle this generically, after nudging, + `SwipeAndTap` re-checks that the target is still visible and, if it is not, + reverses the nudge (the opposite swipe) to restore it. A normally + partially-clipped target stays visible after the nudge, so the reverse is + skipped for it and the behaviour is unchanged — no per-call configuration is + needed. - **`Wait.yaml`** — a fixed pause that never fails the flow. It waits for an element that intentionally never exists, with `optional: true`, so it simply burns the `timeout` (in ms) and carries on. Used to let map tiles or network @@ -385,19 +396,26 @@ For each matrix combination it then: status=1`), and the step fails at the end if any flow failed: ```bash - status=0 - maestro test --format=junit --output=report1.xml \ - --test-output-dir=maestro_outputs --no-ansi maestro/Onboarding.yaml || status=1 - maestro test --format=junit --output=report2.xml \ - --test-output-dir=maestro_outputs --no-ansi maestro/HomePage.yaml || status=1 - # ... and so on through FullScreenMap.yaml - exit $status + status=0; for flow in Onboarding HomePage LocationDetails PlacesNearby MarkersAndRoutes RouteCreation; do \ + maestro test --format=junit --output="maestro_outputs/report-$flow.xml" \ + --test-output-dir=maestro_outputs --no-ansi "maestro/$flow.yaml" || status=1; \ + done; exit $status ``` + This is deliberately a **single shell line**: `android-emulator-runner` runs + each line of the `script` in its own `sh -c`, so a `status` variable spread + across separate lines (with a trailing `exit $status`) is lost — every line + gets a fresh shell, the `|| status=1` makes the failing line exit 0 anyway, + and the final `exit` sees an empty variable and exits 0. That previously left + the step — and the whole run — **green even when every flow failed**. Keeping + the loop and the `exit` in one shell makes the non-zero status stick. + Running every flow regardless of earlier failures means one broken flow no longer hides the results of the flows after it — but note the suite is still [stateful and ordered](#the-suite-is-stateful-and-ordered), so a flow that - fails part-way can still leave later flows without the state they expect. + fails part-way can still leave later flows without the state they expect (as + happens when `Onboarding` fails: every later flow then fails too because the + app never reaches the onboarded home screen). 4. Uploads the `maestro_outputs` reports as an artifact, and on failure also uploads `app/emulator.log` (a full `logcat` capture) to help diagnose what @@ -406,5 +424,5 @@ For each matrix combination it then: The Maestro step uses `continue-on-error: true` so the report-upload steps always run; a final step re-raises the failure to mark the workflow red. -When you add a new flow, add a matching `maestro test ... || status=1` line in -the correct position so CI runs it. +When you add a new flow, add its name (without the `.yaml` extension) to the +`for flow in …` list in the correct position so CI runs it. diff --git a/maestro/SwipeAndTap.yaml b/maestro/SwipeAndTap.yaml index 2f84afbd8..49ce0ab78 100644 --- a/maestro/SwipeAndTap.yaml +++ b/maestro/SwipeAndTap.yaml @@ -16,6 +16,12 @@ appId: org.scottishtecharmy.soundscape # scrollable part of the screen in small steps until the element appears, nudge # it clear of the edge, then tap. # +# The nudge swipe can land on an inner scrollable list (e.g. the audio beacon +# list) rather than the outer page; that scrolls a fully-visible target back out +# of view. So after nudging we check the target is still visible and, if not, +# reverse the nudge to put it back - a partially-clipped target stays visible and +# is left as nudged. +# # Note: swipe start/end coordinates can't be supplied via ${} variables (Maestro # parses them as literals and throws a number-format error), so the per-direction # coordinates are hard-coded below and selected with `when` conditions. @@ -89,5 +95,28 @@ appId: org.scottishtecharmy.soundscape start: 50%, 60% end: 50%, 75% +# If the nudge landed on an inner scrollable list it can scroll a fully-visible +# target back out of view. If the target is no longer visible, reverse the nudge +# (the opposite swipe) to restore it. A normally partially-clipped target stays +# visible after the nudge, so this reverse is skipped for it. +- runFlow: + when: + true: "${output.swipeFound == 1 && output.swipeDir == 'up'}" + notVisible: + id: ${id} + commands: + - swipe: + start: 50%, 60% + end: 50%, 75% +- runFlow: + when: + true: "${output.swipeFound == 1 && output.swipeDir == 'down'}" + notVisible: + id: ${id} + commands: + - swipe: + start: 50%, 75% + end: 50%, 60% + - tapOn: id: ${id}