From 17d9d27b935ea75f4c57d36fd289661e163129c9 Mon Sep 17 00:00:00 2001 From: Bulat Galeev Date: Tue, 29 Sep 2026 13:18:10 +0400 Subject: [PATCH 1/4] fix(wda): keep enough connections open for four requests at once The WDA driver reads an element's name, rect, text and displayed in parallel, and a tap looks an element up four ways at once. The client used Go's default transport, which keeps two idle connections per host, so every such burst closed two connections and opened two new ones. On a real iPhone reached through a forward (SSH, then iproxy over USB) a new connection costs about 300 ms, and new ones opened together fail at once with EOF. In one 44-flow run, 297 of 996 element bursts lost exactly two reads (the two new connections), 2 lost one, and none lost a read on a kept connection: 976 EOFs, each sent again by the dropped-connection retry. WebDriverAgent does not close an idle keep-alive connection (FBHTTPServer exempts them from its reaper), so a stale reuse was not the cause. The client now keeps up to eight idle connections per host. In the new tests, 50 bursts of four reads open 4 connections instead of 102, and through a server that drops every new connection past the first four, no read fails (the old client lost 23 reads there, after 196 dropped connections). --- CHANGELOG.md | 3 + pkg/driver/wda/client.go | 23 ++++- pkg/driver/wda/kept_connections_test.go | 115 ++++++++++++++++++++++++ 3 files changed, 140 insertions(+), 1 deletion(-) create mode 100644 pkg/driver/wda/kept_connections_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index e9aca020..7862da16 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Fixed +- **WDA keeps enough idle connections for its own parallel reads.** The driver reads an element's name, rect, text and displayed at once, and a tap looks an element up four ways at once, but Go's default transport keeps two idle connections per host, so every burst closed two connections and opened two new ones. Through a forwarded port to a physical iPhone, new connections opened together fail with EOF and are sent again, which costs time on every step. The WDA client now keeps up to eight. + ## [1.1.28] - 2026-09-30 This release is about **running a Maestro suite and getting Maestro's answer**. Most of it came from running real Maestro suites (DuckDuckGo Android and iOS, React Native's RNTester, React Navigation) side by side with Maestro and fixing each place the two disagreed. The largest of those is selector matching: text and id selectors now match the whole value, as Maestro does, which is a behaviour change — see below. `--driver devicelab` on iOS now runs a new agent, on simulators and on real iPhones. DeviceLab becomes the default Android driver. Every flow gets `DEVICE_UDID`, `-e` stops splitting values at commas, and the Appium driver closes a long list of gaps with the native drivers. diff --git a/pkg/driver/wda/client.go b/pkg/driver/wda/client.go index 56a0d35b..5078bc4e 100644 --- a/pkg/driver/wda/client.go +++ b/pkg/driver/wda/client.go @@ -34,11 +34,32 @@ func NewClient(port uint16) *Client { return &Client{ baseURL: fmt.Sprintf("http://127.0.0.1:%d", port), httpClient: &http.Client{ - Timeout: 60 * time.Second, + Timeout: 60 * time.Second, + Transport: newWDATransport(), }, } } +// wdaIdleConnsPerHost is how many idle connections the client keeps open to WebDriverAgent. +// The driver sends up to four requests at once (an element's name, rect, text and displayed; +// a tap's four lookups), and Go keeps two idle connections per host by default, so every such +// burst closed two connections and opened two new ones. On a real iPhone reached through a +// forward (SSH, then iproxy), a new connection took about 300 ms, and the two opened together +// failed at once with EOF in 297 of 996 bursts of one 44-flow run, always both of them and +// never a kept connection. Kept for every request of a burst, no new ones are needed. +const wdaIdleConnsPerHost = 8 + +// newWDATransport is Go's default transport with room to keep a whole burst's connections. +func newWDATransport() *http.Transport { + base, ok := http.DefaultTransport.(*http.Transport) + if !ok { + base = &http.Transport{Proxy: http.ProxyFromEnvironment} + } + t := base.Clone() + t.MaxIdleConnsPerHost = wdaIdleConnsPerHost + return t +} + // Session management // CreateSession creates a new WDA session. diff --git a/pkg/driver/wda/kept_connections_test.go b/pkg/driver/wda/kept_connections_test.go new file mode 100644 index 00000000..ea0b8652 --- /dev/null +++ b/pkg/driver/wda/kept_connections_test.go @@ -0,0 +1,115 @@ +package wda + +import ( + "fmt" + "net" + "net/http" + "net/http/httptest" + "strconv" + "strings" + "sync" + "sync/atomic" + "testing" + "time" + + "github.com/devicelab-dev/maestro-runner/pkg/core" +) + +// keptConnServer answers an element's four property reads, each after a short delay so the +// four of a burst are in flight together, as they are on a phone. It counts the connections +// it accepted, and with dropNewAfter > 0 closes, without an answer, every new connection past +// that many: a forward whose new connections fail, while kept ones work. +type keptConnServer struct { + server *httptest.Server + accepted int32 + dropped int32 + dropNewAfter int32 +} + +func newKeptConnServer(t *testing.T, dropNewAfter int32) *keptConnServer { + t.Helper() + es := &keptConnServer{dropNewAfter: dropNewAfter} + handler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + time.Sleep(20 * time.Millisecond) + switch { + case strings.HasSuffix(r.URL.Path, "/rect"): + jsonResponse(w, map[string]interface{}{"value": map[string]interface{}{"x": 10, "y": 20, "width": 100, "height": 40}}) + case strings.HasSuffix(r.URL.Path, "/displayed"): + jsonResponse(w, map[string]interface{}{"value": true}) + default: + jsonResponse(w, map[string]interface{}{"value": "Continue"}) + } + }) + es.server = httptest.NewUnstartedServer(handler) + es.server.Config.ConnState = func(c net.Conn, state http.ConnState) { + if state != http.StateNew { + return + } + n := atomic.AddInt32(&es.accepted, 1) + if es.dropNewAfter > 0 && n > es.dropNewAfter { + atomic.AddInt32(&es.dropped, 1) + _ = c.Close() + } + } + es.server.Start() + t.Cleanup(es.server.Close) + return es +} + +// driverFor is a driver whose client is built the way the runner builds it. +func (es *keptConnServer) driverFor(t *testing.T) *Driver { + t.Helper() + _, portText, err := net.SplitHostPort(strings.TrimPrefix(es.server.URL, "http://")) + if err != nil { + t.Fatalf("server address %q: %v", es.server.URL, err) + } + port, _ := strconv.Atoi(portText) + client := NewClient(uint16(port)) + client.baseURL = es.server.URL // the listener is on 127.0.0.1, not localhost's first address + client.sessionID = "s1" + return &Driver{client: client, info: &core.PlatformInfo{Platform: "ios", ScreenWidth: 390, ScreenHeight: 844}} +} + +// Fifty elements' reads, four at a time, open four connections and then keep them, instead of +// closing two and opening two for every element. +func TestElementReadsKeepTheirConnections(t *testing.T) { + es := newKeptConnServer(t, 0) + d := es.driverFor(t) + for i := 0; i < 50; i++ { + if _, err := d.getElementInfo(fmt.Sprintf("E%d", i)); err != nil { + t.Fatalf("element %d: %v", i, err) + } + } + if n := atomic.LoadInt32(&es.accepted); n > 4 { + t.Errorf("50 bursts of four reads opened %d connections, want at most 4", n) + } +} + +// Where a new connection fails (the phone's forward), only the first burst opens any, so the +// reads after it never meet a failing one. +func TestElementReadsSurviveAForwardWhoseNewConnectionsFail(t *testing.T) { + es := newKeptConnServer(t, 4) + d := es.driverFor(t) + var mu sync.Mutex + failed := 0 + for i := 0; i < 50; i++ { + if _, err := d.getElementInfo(fmt.Sprintf("E%d", i)); err != nil { + mu.Lock() + failed++ + mu.Unlock() + } + } + if dropped := atomic.LoadInt32(&es.dropped); dropped != 0 || failed != 0 { + t.Errorf("after the first burst, %d new connections were needed and dropped, and %d reads failed; want none", dropped, failed) + } +} + +func TestTheWDATransportKeepsABurstsConnections(t *testing.T) { + tr := newWDATransport() + if tr.MaxIdleConnsPerHost < 4 { + t.Errorf("MaxIdleConnsPerHost = %d, fewer than the driver's four requests at once", tr.MaxIdleConnsPerHost) + } + if tr.Proxy == nil || tr.DialContext == nil { + t.Error("the WDA transport lost the default transport's proxy or dialer") + } +} From 48922eb7de915190858dcf9695da8e2d39edaec1 Mon Sep 17 00:00:00 2001 From: Bulat Galeev Date: Tue, 29 Sep 2026 13:18:29 +0400 Subject: [PATCH 2/4] fix(executor): retry's maxRetries counts retries, not attempts Maestro runs a retry's commands once and then up to maxRetries more times: `(maxRetries?.toIntOrNull() ?: 1).coerceAtMost(3)`, then `while (attempt <= maxRetries)` from 0 (Orchestra.kt:934-955, with MAX_RETRIES_ALLOWED at 1844 and the "1" default at YamlFluentCommand.kt:614). So `maxRetries: 1` is two attempts, the default is two, and no retry runs more than four times. The value is evaluated first, so ${...} works (Commands.kt:986), and anything that is not an integer counts as 1. The runner ran exactly maxRetries attempts, three when unset, with no cap, so a flow written with `maxRetries: 1` never retried at all. It also failed the step when maxRetries was not an integer. The inline and the file form of retry now count as Maestro does. A negative maxRetries runs no attempt and passes, since Maestro's loop never starts. A value that is not an integer is logged and read as 1. --- CHANGELOG.md | 1 + pkg/executor/flow_runner.go | 74 +++++++++++----- pkg/executor/retry_attempts_test.go | 133 ++++++++++++++++++++++++++++ pkg/executor/scripting_test.go | 14 --- 4 files changed, 186 insertions(+), 36 deletions(-) create mode 100644 pkg/executor/retry_attempts_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index 7862da16..761933de 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed - **WDA keeps enough idle connections for its own parallel reads.** The driver reads an element's name, rect, text and displayed at once, and a tap looks an element up four ways at once, but Go's default transport keeps two idle connections per host, so every burst closed two connections and opened two new ones. Through a forwarded port to a physical iPhone, new connections opened together fail with EOF and are sent again, which costs time on every step. The WDA client now keeps up to eight. +- **`retry` counts retries, not attempts, as Maestro does.** `maxRetries: 1` now runs the commands twice (once, then one retry), an unset `maxRetries` means one retry, and the value is capped at 3. The runner ran exactly `maxRetries` attempts, three when unset and with no cap, so `maxRetries: 1` never retried. A value that is not an integer is logged and read as 1 instead of failing the step. ## [1.1.28] - 2026-09-30 diff --git a/pkg/executor/flow_runner.go b/pkg/executor/flow_runner.go index 77a86bf2..a9cb8f3a 100644 --- a/pkg/executor/flow_runner.go +++ b/pkg/executor/flow_runner.go @@ -5,6 +5,7 @@ import ( "fmt" "os" "path/filepath" + "strconv" "strings" "time" @@ -1091,16 +1092,53 @@ func (fr *FlowRunner) executeRepeat(step *flow.RepeatStep) *core.CommandResult { } } -// executeRetry handles retry step execution. -func (fr *FlowRunner) executeRetry(step *flow.RetryStep) *core.CommandResult { - maxRetries, err := fr.script.ParseIntStrict(step.MaxRetries, 3) - if err != nil { +// maxRetriesAllowed caps retry's maxRetries, as Maestro's MAX_RETRIES_ALLOWED +// does (Orchestra.kt:1844). +const maxRetriesAllowed = 3 + +// retryAttempts is how many times a retry runs its commands: the first run +// plus one per retry. maxRetries is read as Maestro reads it (Orchestra.kt:934 +// and 939-955): unset or not an integer is 1, more than 3 is 3, and a negative +// value runs nothing, because Maestro's `while (attempt <= maxRetries)` never +// starts. +func (fr *FlowRunner) retryAttempts(raw string) int { + maxRetries := 1 + if expanded := fr.script.ExpandVariables(raw); expanded != "" { + if n, err := strconv.Atoi(expanded); err == nil { + maxRetries = n + } else { + logger.Warn("retry: maxRetries %q is not an integer, so it is 1, as in Maestro", expanded) + } + } + if maxRetries > maxRetriesAllowed { + maxRetries = maxRetriesAllowed + } + if maxRetries < 0 { + return 0 + } + return maxRetries + 1 +} + +// retryExhausted is the result of a retry that ran out of attempts. With no +// attempt at all (a negative maxRetries) Maestro's command completes without +// running anything, so that one passes. +func retryExhausted(lastErr error, attempts int) *core.CommandResult { + if attempts == 0 { return &core.CommandResult{ - Success: false, - Error: err, - Message: fmt.Sprintf("retry: invalid 'maxRetries' value: %v", err), + Success: true, + Message: "Retry ran no attempts (maxRetries is negative)", } } + return &core.CommandResult{ + Success: false, + Error: lastErr, + Message: fmt.Sprintf("Retry failed after %d attempts", attempts), + } +} + +// executeRetry handles retry step execution. +func (fr *FlowRunner) executeRetry(step *flow.RetryStep) *core.CommandResult { + attempts := fr.retryAttempts(step.MaxRetries) // Apply env variables with restore defer fr.script.withEnvVars(step.Env)() @@ -1116,12 +1154,12 @@ func (fr *FlowRunner) executeRetry(step *flow.RetryStep) *core.CommandResult { Message: fmt.Sprintf("Failed to parse flow file: %s", filePath), } } - return fr.executeSubFlowWithRetry(*subFlow, maxRetries) + return fr.executeSubFlowWithRetry(*subFlow, attempts) } // Execute inline steps with retry var lastErr error - for attempt := 1; attempt <= maxRetries; attempt++ { + for attempt := 1; attempt <= attempts; attempt++ { if fr.ctx.Err() != nil { return &core.CommandResult{ Success: false, @@ -1148,11 +1186,7 @@ func (fr *FlowRunner) executeRetry(step *flow.RetryStep) *core.CommandResult { } } - return &core.CommandResult{ - Success: false, - Error: lastErr, - Message: fmt.Sprintf("Retry failed after %d attempts", maxRetries), - } + return retryExhausted(lastErr, attempts) } // executeRunFlow handles runFlow step execution. @@ -1721,11 +1755,11 @@ func (fr *FlowRunner) executeSubFlow(subFlow flow.Flow) *core.CommandResult { } } -// executeSubFlowWithRetry executes a sub-flow with retry logic. -func (fr *FlowRunner) executeSubFlowWithRetry(subFlow flow.Flow, maxRetries int) *core.CommandResult { +// executeSubFlowWithRetry runs a sub-flow up to attempts times, until it passes. +func (fr *FlowRunner) executeSubFlowWithRetry(subFlow flow.Flow, attempts int) *core.CommandResult { var lastErr error - for attempt := 1; attempt <= maxRetries; attempt++ { + for attempt := 1; attempt <= attempts; attempt++ { if fr.ctx.Err() != nil { return &core.CommandResult{ Success: false, @@ -1744,11 +1778,7 @@ func (fr *FlowRunner) executeSubFlowWithRetry(subFlow flow.Flow, maxRetries int) lastErr = result.Error } - return &core.CommandResult{ - Success: false, - Error: lastErr, - Message: fmt.Sprintf("Retry failed after %d attempts", maxRetries), - } + return retryExhausted(lastErr, attempts) } // captureArtifacts captures the step screenshot and, when captureHierarchy is diff --git a/pkg/executor/retry_attempts_test.go b/pkg/executor/retry_attempts_test.go new file mode 100644 index 00000000..6fbc9495 --- /dev/null +++ b/pkg/executor/retry_attempts_test.go @@ -0,0 +1,133 @@ +package executor + +import ( + "os" + "path/filepath" + "testing" + + "github.com/devicelab-dev/maestro-runner/pkg/core" + "github.com/devicelab-dev/maestro-runner/pkg/flow" + "github.com/devicelab-dev/maestro-runner/pkg/report" +) + +// Maestro reads maxRetries as retries after the first run, not as attempts: +// `(maxRetries ?: 1).coerceAtMost(3)`, then `while (attempt <= maxRetries)` +// from 0 (Orchestra.kt:934-955). +func TestRetryAttempts_CountsRetriesAsMaestroDoes(t *testing.T) { + se := NewScriptEngine() + defer se.Close() + se.SetVariable("TWO", "2") + fr := &FlowRunner{script: se} + + for _, tc := range []struct { + maxRetries string + want int + }{ + {"", 2}, // unset: one retry + {"0", 1}, // no retry, one run + {"1", 2}, // one retry + {"3", 4}, // the most there can be + {"10", 4}, // capped at three retries + {"-1", 0}, // the loop never starts + {"${TWO}", 3}, // an expression is evaluated first + {"${1 + 1}", 3}, // and so is arithmetic + {"${UNSET_VAR}", 2}, // evaluates to nothing: one retry + {"five", 2}, // not an integer: one retry, not an error + {"1.5", 2}, // toIntOrNull rejects a decimal + } { + if got := fr.retryAttempts(tc.maxRetries); got != tc.want { + t.Errorf("maxRetries %q: %d attempts, want %d", tc.maxRetries, got, tc.want) + } + } +} + +// failingTaps is a driver whose taps fail the first failures times and then +// pass; every other step passes. It counts the taps. +func failingTaps(failures int, taps *int) *mockDriver { + return &mockDriver{executeFunc: func(step flow.Step) *core.CommandResult { + if _, ok := step.(*flow.TapOnStep); ok { + *taps++ + if *taps <= failures { + return &core.CommandResult{Success: false, Error: &testError{msg: "not there yet"}} + } + } + return &core.CommandResult{Success: true} + }} +} + +func retryAroundTap(maxRetries string) *flow.RetryStep { + return &flow.RetryStep{ + BaseStep: flow.BaseStep{StepType: flow.StepRetry}, + MaxRetries: maxRetries, + Steps: []flow.Step{&flow.TapOnStep{BaseStep: flow.BaseStep{StepType: flow.StepTapOn}}}, + } +} + +// `maxRetries: 1` ran the commands once and never retried. +func TestRetry_MaxRetriesOneRetriesOnce(t *testing.T) { + taps := 0 + result := runOneFlow(t, failingTaps(1, &taps), flow.Flow{ + SourcePath: "test.yaml", + Config: flow.Config{Name: "retry once"}, + Steps: []flow.Step{retryAroundTap("1")}, + }) + if result.Status != report.StatusPassed { + t.Errorf("status = %v, want passed: the retry should run the tap a second time", result.Status) + } + if taps != 2 { + t.Errorf("tap ran %d times, want 2", taps) + } +} + +func TestRetry_AttemptsWhenEveryRunFails(t *testing.T) { + for _, tc := range []struct { + maxRetries string + taps int + status report.Status + }{ + {"", 2, report.StatusFailed}, + {"0", 1, report.StatusFailed}, + {"1", 2, report.StatusFailed}, + {"10", 4, report.StatusFailed}, + // A negative maxRetries runs nothing, and Maestro's command then + // completes without an error. + {"-1", 0, report.StatusPassed}, + } { + taps := 0 + result := runOneFlow(t, failingTaps(1000, &taps), flow.Flow{ + SourcePath: "test.yaml", + Config: flow.Config{Name: "retry " + tc.maxRetries}, + Steps: []flow.Step{retryAroundTap(tc.maxRetries)}, + }) + if taps != tc.taps { + t.Errorf("maxRetries %q: tap ran %d times, want %d", tc.maxRetries, taps, tc.taps) + } + if result.Status != tc.status { + t.Errorf("maxRetries %q: status = %v, want %v", tc.maxRetries, result.Status, tc.status) + } + } +} + +// The file form counts the same way. +func TestRetry_FileFormRetriesOnce(t *testing.T) { + dir := t.TempDir() + if err := os.WriteFile(filepath.Join(dir, "tap.yaml"), []byte("appId: com.example.app\n---\n- tapOn: OK\n"), 0o644); err != nil { + t.Fatal(err) + } + taps := 0 + result := runOneFlow(t, failingTaps(1000, &taps), flow.Flow{ + SourcePath: filepath.Join(dir, "main.yaml"), + Config: flow.Config{Name: "retry file"}, + Steps: []flow.Step{&flow.RetryStep{ + BaseStep: flow.BaseStep{StepType: flow.StepRetry}, + MaxRetries: "1", + File: "tap.yaml", + }}, + }) + if taps != 2 { + t.Errorf("tap ran %d times, want 2 (one run and one retry)", taps) + } + if result.Status != report.StatusFailed { + t.Errorf("status = %v, want failed", result.Status) + } +} diff --git a/pkg/executor/scripting_test.go b/pkg/executor/scripting_test.go index 4fc9211e..5e4eaf71 100644 --- a/pkg/executor/scripting_test.go +++ b/pkg/executor/scripting_test.go @@ -660,20 +660,6 @@ func TestExecuteRepeat_InvalidTimes(t *testing.T) { } } -func TestExecuteRetry_InvalidMaxRetries(t *testing.T) { - se := NewScriptEngine() - defer se.Close() - fr := &FlowRunner{ctx: context.Background(), driver: &mockDriver{}, script: se} - - result := fr.executeRetry(&flow.RetryStep{MaxRetries: "five"}) - if result.Success { - t.Error("expected failure for non-numeric maxRetries, got success") - } - if !strings.Contains(result.Message, "invalid 'maxRetries'") { - t.Errorf("message = %q, want it to mention invalid 'maxRetries'", result.Message) - } -} - func TestScriptEngine_withEnvVars(t *testing.T) { se := NewScriptEngine() defer se.Close() From 9b05a9b917518a5dc4438e2cd3c2489eb5f2714f Mon Sep 17 00:00:00 2001 From: Bulat Galeev Date: Tue, 29 Sep 2026 13:20:03 +0400 Subject: [PATCH 3/4] fix(wda): launchApp stops a running app first unless stopApp is false Maestro stops the app before launching it unless the flow says `stopApp: false`. The WDA driver never read stopApp: with a session open and no launch arguments it called WDA's launch on the running app, which only activates it. A relaunch then left the app on the screen it was already on, so a flow checking what survives a restart (a chat's history, or a changed subscription state being read back on launch) was not restarting anything. It now terminates first unless stopApp is false. Launch arguments still force the stop, since they only apply on a real launch. A launchApp that creates the session is unchanged: creating it launches the app fresh. --- CHANGELOG.md | 1 + pkg/driver/wda/commands.go | 9 ++-- pkg/driver/wda/launch_stop_test.go | 81 ++++++++++++++++++++++++++++++ 3 files changed, 88 insertions(+), 3 deletions(-) create mode 100644 pkg/driver/wda/launch_stop_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index 761933de..4572239e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed - **WDA keeps enough idle connections for its own parallel reads.** The driver reads an element's name, rect, text and displayed at once, and a tap looks an element up four ways at once, but Go's default transport keeps two idle connections per host, so every burst closed two connections and opened two new ones. Through a forwarded port to a physical iPhone, new connections opened together fail with EOF and are sent again, which costs time on every step. The WDA client now keeps up to eight. - **`retry` counts retries, not attempts, as Maestro does.** `maxRetries: 1` now runs the commands twice (once, then one retry), an unset `maxRetries` means one retry, and the value is capped at 3. The runner ran exactly `maxRetries` attempts, three when unset and with no cap, so `maxRetries: 1` never retried. A value that is not an integer is logged and read as 1 instead of failing the step. +- **WDA `launchApp` restarts a running app unless `stopApp: false`, as Maestro does.** It only activated the running app, so a relaunch left the app on the screen it was already on, and a flow checking what survives a restart restarted nothing. ## [1.1.28] - 2026-09-30 diff --git a/pkg/driver/wda/commands.go b/pkg/driver/wda/commands.go index 8b3c5c58..df8d335c 100644 --- a/pkg/driver/wda/commands.go +++ b/pkg/driver/wda/commands.go @@ -1203,9 +1203,12 @@ func (d *Driver) launchApp(step *flow.LaunchAppStep) *core.CommandResult { return successResult(fmt.Sprintf("Launched app: %s", bundleID), nil) } - // Terminate the app first so WDA calls launch (not activate), - // which is required for arguments/environment to take effect - if hasArgs { + // Terminate the app first so WDA calls launch (not activate). Arguments and + // environment only take effect on a real launch, and Maestro stops the app + // before launching it unless the flow says `stopApp: false`. Without the stop, + // a relaunch only brought a running app to the front, still where it was, so + // a flow checking what survives a restart tested nothing. + if hasArgs || step.StopApp == nil || *step.StopApp { _ = d.client.TerminateApp(bundleID) } diff --git a/pkg/driver/wda/launch_stop_test.go b/pkg/driver/wda/launch_stop_test.go new file mode 100644 index 00000000..41750386 --- /dev/null +++ b/pkg/driver/wda/launch_stop_test.go @@ -0,0 +1,81 @@ +package wda + +import ( + "net/http" + "net/http/httptest" + "strings" + "sync" + "testing" + + "github.com/devicelab-dev/maestro-runner/pkg/core" + "github.com/devicelab-dev/maestro-runner/pkg/flow" +) + +// launchWithSession runs launchApp against a WDA that already has a session, the +// usual case for every launchApp after a flow's first, and returns the app +// lifecycle calls it made, in order. +func launchWithSession(t *testing.T, step *flow.LaunchAppStep) []string { + t.Helper() + var mu sync.Mutex + var calls []string + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + mu.Lock() + defer mu.Unlock() + switch { + case strings.HasSuffix(r.URL.Path, "/wda/apps/terminate"): + calls = append(calls, "terminate") + case strings.HasSuffix(r.URL.Path, "/wda/apps/launch"): + calls = append(calls, "launch") + case r.URL.Path == "/session" && r.Method == http.MethodPost: + calls = append(calls, "new-session") + } + jsonResponse(w, map[string]interface{}{"value": nil}) + })) + defer server.Close() + + client := &Client{baseURL: server.URL, httpClient: http.DefaultClient, sessionID: "test-session"} + driver := &Driver{client: client, info: &core.PlatformInfo{Platform: "ios"}} + if result := driver.launchApp(step); !result.Success { + t.Fatalf("launchApp failed: %s", result.Message) + } + mu.Lock() + defer mu.Unlock() + return append([]string(nil), calls...) +} + +// Maestro stops the app before launching it by default. A relaunch that only +// activated the running app left it where it was, so a flow asserting what a +// restart preserves passed or failed on the pre-restart screen. +func TestLaunchAppStopsARunningAppFirstByDefault(t *testing.T) { + got := strings.Join(launchWithSession(t, &flow.LaunchAppStep{AppID: "com.test.app"}), ",") + if got != "terminate,launch" { + t.Fatalf("lifecycle calls %q, want terminate,launch", got) + } +} + +func TestLaunchAppStopAppTrueStopsFirst(t *testing.T) { + stop := true + got := strings.Join(launchWithSession(t, &flow.LaunchAppStep{AppID: "com.test.app", StopApp: &stop}), ",") + if got != "terminate,launch" { + t.Fatalf("lifecycle calls %q, want terminate,launch", got) + } +} + +func TestLaunchAppStopAppFalseOnlyBringsTheAppForward(t *testing.T) { + stop := false + got := strings.Join(launchWithSession(t, &flow.LaunchAppStep{AppID: "com.test.app", StopApp: &stop}), ",") + if got != "launch" { + t.Fatalf("lifecycle calls %q, want launch alone", got) + } +} + +// Arguments need a real launch, so they still stop the app even with stopApp: false. +func TestLaunchAppArgumentsStillStopTheApp(t *testing.T) { + stop := false + step := &flow.LaunchAppStep{AppID: "com.test.app", StopApp: &stop, Arguments: map[string]any{"e2e": true}} + got := strings.Join(launchWithSession(t, step), ",") + if got != "terminate,launch" { + t.Fatalf("lifecycle calls %q, want terminate,launch", got) + } +} From 8ccc1043626c74e4e473605106011498a493c17a Mon Sep 17 00:00:00 2001 From: Bulat Galeev Date: Tue, 29 Sep 2026 13:21:39 +0400 Subject: [PATCH 4/4] fix(wda): an element is not visible only when a lookup finds it absent Maestro's notVisible waits for a lookup that ends in ElementNotFound (Orchestra.kt:1066-1082); any other error from the lookup fails the step. assertNotVisible and extendedWaitUntil notVisible treated any failed lookup (err != nil || info == nil) as the element being gone. A page source that could not be read, from a WDA error or a dropped connection, passed the assertion without the screen ever being looked at. The when/while and assertCondition notVisible checks go through assertNotVisible, so they had the same hole. The page-source lookups now return a not-found error when they read the screen and nothing matches (an anchor that is not there included). Only that counts as gone. Any other error is tried again until the timeout, and the step then fails with that error ("Could not check that ... is not visible"). TestAssertNotVisibleWithTimeout served an error for every request and expected a pass; it now serves an empty screen. --- CHANGELOG.md | 1 + pkg/driver/wda/commands.go | 30 +++++-- pkg/driver/wda/driver.go | 8 +- pkg/driver/wda/driver_test.go | 9 +- pkg/driver/wda/notfound.go | 26 ++++++ pkg/driver/wda/notvisible_test.go | 131 ++++++++++++++++++++++++++++++ 6 files changed, 195 insertions(+), 10 deletions(-) create mode 100644 pkg/driver/wda/notfound.go create mode 100644 pkg/driver/wda/notvisible_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index 4572239e..fc71708c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - **WDA keeps enough idle connections for its own parallel reads.** The driver reads an element's name, rect, text and displayed at once, and a tap looks an element up four ways at once, but Go's default transport keeps two idle connections per host, so every burst closed two connections and opened two new ones. Through a forwarded port to a physical iPhone, new connections opened together fail with EOF and are sent again, which costs time on every step. The WDA client now keeps up to eight. - **`retry` counts retries, not attempts, as Maestro does.** `maxRetries: 1` now runs the commands twice (once, then one retry), an unset `maxRetries` means one retry, and the value is capped at 3. The runner ran exactly `maxRetries` attempts, three when unset and with no cap, so `maxRetries: 1` never retried. A value that is not an integer is logged and read as 1 instead of failing the step. - **WDA `launchApp` restarts a running app unless `stopApp: false`, as Maestro does.** It only activated the running app, so a relaunch left the app on the screen it was already on, and a flow checking what survives a restart restarted nothing. +- **WDA `notVisible` passes only when a lookup finds the element absent.** `assertNotVisible` and `extendedWaitUntil: notVisible` treated any failed lookup, such as an unreadable page source or a dropped connection, as the element being gone, so they could pass without the screen being looked at. Other errors are now retried until the timeout, and then fail the step. ## [1.1.28] - 2026-09-30 diff --git a/pkg/driver/wda/commands.go b/pkg/driver/wda/commands.go index df8d335c..023ead3a 100644 --- a/pkg/driver/wda/commands.go +++ b/pkg/driver/wda/commands.go @@ -403,8 +403,10 @@ func (d *Driver) countVisibleMatchesOnce(sel flow.Selector) (int, error) { func (d *Driver) assertNotVisible(step *flow.AssertNotVisibleStep) *core.CommandResult { // Poll with quick checks, waiting for element to disappear. - // Each check is a single lookup (no retries). If element is not found - // at any point, we pass immediately. If still visible at timeout, fail. + // Each check is a single lookup (no retries). If a lookup reads the + // screen and finds no match, we pass immediately. A lookup that could not + // read the screen proves nothing, so polling goes on, as it does while the + // element is still visible, and the step fails at timeout. timeoutMs := step.TimeoutMs if timeoutMs <= 0 { timeoutMs = 5000 @@ -415,11 +417,14 @@ func (d *Driver) assertNotVisible(step *flow.AssertNotVisibleStep) *core.Command for { info, err := d.findElementOnce(step.Selector) - if err != nil || info == nil { + if isNotFound(err) || (err == nil && info == nil) { return successResult("Element is not visible", nil) } if time.Now().After(deadline) { + if err != nil { + return errorResult(err, fmt.Sprintf("Could not check that %s is not visible: %v", selectorDesc(step.Selector), err)) + } return errorResult(fmt.Errorf("element is visible"), fmt.Sprintf("Element should not be visible: %s", selectorDesc(step.Selector))) } @@ -1629,6 +1634,10 @@ func (d *Driver) waitUntil(step *flow.WaitUntilStep) *core.CommandResult { selector = step.NotVisible } + // The last notVisible check that could not read the screen, if the last + // check was one. + var unreadable error + for { select { case <-ctx.Done(): @@ -1639,6 +1648,10 @@ func (d *Driver) waitUntil(step *flow.WaitUntilStep) *core.CommandResult { fmt.Sprintf("Element '%s' not visible within %v", selector.Describe(), timeout), ) } + if unreadable != nil { + return errorResult(unreadable, + fmt.Sprintf("Could not check that '%s' is not visible within %v: %v", selector.Describe(), timeout, unreadable)) + } return errorResult( context.DeadlineExceeded, fmt.Sprintf("Element '%s' still visible after %v", selector.Describe(), timeout), @@ -1651,11 +1664,18 @@ func (d *Driver) waitUntil(step *flow.WaitUntilStep) *core.CommandResult { return successResult("Element became visible", info) } } else { - // Single attempt for not visible check + // Single attempt for not visible check. Only a lookup that + // read the screen and found no match means the element is + // gone; one that could not read it is tried again. info, err := d.findElementOnce(*step.NotVisible) - if err != nil || info == nil { + if isNotFound(err) || (err == nil && info == nil) { return successResult("Element became not visible", nil) } + unreadable = err + if unreadable != nil { + // A refused connection fails at once; do not spin on it. + time.Sleep(50 * time.Millisecond) + } } // HTTP round-trip (~100ms) is natural rate limit, no sleep needed } diff --git a/pkg/driver/wda/driver.go b/pkg/driver/wda/driver.go index 1a9437df..3f7b9b04 100644 --- a/pkg/driver/wda/driver.go +++ b/pkg/driver/wda/driver.go @@ -1181,7 +1181,7 @@ func (d *Driver) resolveRelativeSelector(sel flow.Selector, allElements []*Parse if anchorSelector != nil { anchors := FilterBySelector(allElements, *anchorSelector) if len(anchors) == 0 { - return nil, fmt.Errorf("anchor element not found") + return nil, notFound("anchor element not found") } var matchingCandidates []*ParsedElement @@ -1203,7 +1203,7 @@ func (d *Driver) resolveRelativeSelector(sel flow.Selector, allElements []*Parse // Bounds-based visibility (FilterOutOfBounds already applied above); // XCUITest's `visible="false"` is unreliable on RN testID wrappers. if len(candidates) == 0 { - return nil, fmt.Errorf("no elements match selector") + return nil, notFound("no elements match selector") } // Prioritize clickable/interactive elements @@ -1262,10 +1262,10 @@ func (d *Driver) findElementByPageSourceOnce(sel flow.Selector) (*core.ElementIn if len(candidates) == 0 { if sel.Text != "" { if closest := ClosestTexts(allElements, sel.Text, 3); len(closest) > 0 { - return nil, fmt.Errorf("no elements match selector; closest on-screen texts: %s", strings.Join(closest, ", ")) + return nil, notFound("no elements match selector; closest on-screen texts: %s", strings.Join(closest, ", ")) } } - return nil, fmt.Errorf("no elements match selector") + return nil, notFound("no elements match selector") } // Prioritize clickable/interactive elements diff --git a/pkg/driver/wda/driver_test.go b/pkg/driver/wda/driver_test.go index 9053e16c..c661bccf 100644 --- a/pkg/driver/wda/driver_test.go +++ b/pkg/driver/wda/driver_test.go @@ -4864,7 +4864,14 @@ func TestInputTextTapError(t *testing.T) { func TestAssertNotVisibleWithTimeout(t *testing.T) { server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { w.Header().Set("Content-Type", "application/json") - // Return error to indicate element not found + // The screen reads, and holds nothing matching. (An error from + // /source would not mean "not found"; see notvisible_test.go.) + if strings.HasSuffix(r.URL.Path, "/source") { + jsonResponse(w, map[string]interface{}{ + "value": ``, + }) + return + } jsonResponse(w, map[string]interface{}{ "value": map[string]interface{}{"error": "not found"}, }) diff --git a/pkg/driver/wda/notfound.go b/pkg/driver/wda/notfound.go new file mode 100644 index 00000000..02967712 --- /dev/null +++ b/pkg/driver/wda/notfound.go @@ -0,0 +1,26 @@ +package wda + +import ( + "errors" + "fmt" +) + +// notFoundError is a lookup that read the screen and found nothing matching +// the selector. Any other lookup error means the screen could not be read +// (a failed /source, a hierarchy that would not parse), which says nothing +// about whether the element is there. +type notFoundError struct{ msg string } + +func (e notFoundError) Error() string { return e.msg } + +// notFound returns a notFoundError with a formatted message. +func notFound(format string, args ...interface{}) error { + return notFoundError{msg: fmt.Sprintf(format, args...)} +} + +// isNotFound reports whether err is a lookup that read the screen and found +// no match. Only that result means an element is gone. +func isNotFound(err error) bool { + var nf notFoundError + return errors.As(err, &nf) +} diff --git a/pkg/driver/wda/notvisible_test.go b/pkg/driver/wda/notvisible_test.go new file mode 100644 index 00000000..797e57f3 --- /dev/null +++ b/pkg/driver/wda/notvisible_test.go @@ -0,0 +1,131 @@ +package wda + +import ( + "errors" + "fmt" + "net/http" + "net/http/httptest" + "strings" + "sync" + "testing" + + "github.com/devicelab-dev/maestro-runner/pkg/flow" +) + +const emptyScreenSource = `` + +// unreadableScreenServer answers the first n page-source reads with a server +// error, then serves an empty screen; n < 0 fails every read. WDA's own +// queries never find anything. +func unreadableScreenServer(t *testing.T, failures int) *httptest.Server { + t.Helper() + var mu sync.Mutex + reads := 0 + return httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + if strings.HasSuffix(r.URL.Path, "/source") { + mu.Lock() + reads++ + n := reads + mu.Unlock() + if failures < 0 || n <= failures { + w.WriteHeader(http.StatusInternalServerError) + jsonResponse(w, map[string]interface{}{"value": map[string]interface{}{"error": "unknown error", "message": "snapshot failed"}}) + return + } + jsonResponse(w, map[string]interface{}{"value": emptyScreenSource}) + return + } + w.WriteHeader(http.StatusNotFound) + jsonResponse(w, map[string]interface{}{"value": map[string]interface{}{"error": "no such element"}}) + })) +} + +// A page source that cannot be read says nothing about the element, so +// assertNotVisible does not pass on it. Maestro counts only element-not-found +// as gone (Orchestra.kt:1066-1082). +func TestAssertNotVisibleFailsWhenScreenUnreadable(t *testing.T) { + server := unreadableScreenServer(t, -1) + defer server.Close() + d := createTestDriver(server) + + res := d.assertNotVisible(&flow.AssertNotVisibleStep{ + BaseStep: flow.BaseStep{TimeoutMs: 300}, + Selector: flow.Selector{Text: "Banner"}, + }) + if res.Success { + t.Fatal("assertNotVisible passed although the screen was never read") + } + if !strings.Contains(res.Message, "Could not check") || !strings.Contains(res.Message, "snapshot failed") { + t.Errorf("message should say the check could not be made, and why: %s", res.Message) + } +} + +// A read that fails once is tried again; the element is gone once a read +// succeeds and finds no match. +func TestAssertNotVisiblePassesAfterUnreadableScreen(t *testing.T) { + server := unreadableScreenServer(t, 2) + defer server.Close() + d := createTestDriver(server) + + res := d.assertNotVisible(&flow.AssertNotVisibleStep{ + BaseStep: flow.BaseStep{TimeoutMs: 3000}, + Selector: flow.Selector{Text: "Banner"}, + }) + if !res.Success { + t.Fatalf("assertNotVisible should pass once the screen reads empty: %s", res.Message) + } +} + +// A relative selector whose anchor is not on screen matches nothing: gone. +func TestAssertNotVisibleRelativeAnchorAbsent(t *testing.T) { + server := unreadableScreenServer(t, 0) + defer server.Close() + d := createTestDriver(server) + + res := d.assertNotVisible(&flow.AssertNotVisibleStep{ + BaseStep: flow.BaseStep{TimeoutMs: 300}, + Selector: flow.Selector{Text: "Banner", Below: &flow.Selector{Text: "Header"}}, + }) + if !res.Success { + t.Fatalf("no anchor on screen means no match: %s", res.Message) + } +} + +// extendedWaitUntil notVisible follows the same rule. +func TestWaitUntilNotVisibleFailsWhenScreenUnreadable(t *testing.T) { + server := unreadableScreenServer(t, -1) + defer server.Close() + d := createTestDriver(server) + + sel := flow.Selector{Text: "Banner"} + res := d.waitUntil(&flow.WaitUntilStep{NotVisible: &sel, BaseStep: flow.BaseStep{TimeoutMs: 300}}) + if res.Success { + t.Fatal("waitUntil notVisible passed although the screen was never read") + } + if !strings.Contains(res.Message, "Could not check") { + t.Errorf("message should say the check could not be made: %s", res.Message) + } + + server2 := unreadableScreenServer(t, 1) + defer server2.Close() + d2 := createTestDriver(server2) + if res := d2.waitUntil(&flow.WaitUntilStep{NotVisible: &sel, BaseStep: flow.BaseStep{TimeoutMs: 3000}}); !res.Success { + t.Fatalf("waitUntil notVisible should pass once the screen reads empty: %s", res.Message) + } +} + +func TestIsNotFound(t *testing.T) { + if !isNotFound(notFound("no elements match selector")) { + t.Error("a notFound error is not found") + } + if !isNotFound(fmt.Errorf("context deadline exceeded: %w", notFound("no elements match selector"))) { + t.Error("a wrapped notFound error is not found") + } + if isNotFound(nil) { + t.Error("nil is not a lookup that found nothing") + } + if isNotFound(errors.New("connection reset by peer")) { + t.Error("a read error is not a lookup that found nothing") + } +}