From a5f86334aff26fe4b581d0ad809d13f560c3cac8 Mon Sep 17 00:00:00 2001 From: PJ Date: Sat, 15 Aug 2026 22:36:50 +0530 Subject: [PATCH 1/6] fix(android): say what the sdk lookup checked, not just to set ANDROID_HOME --- internal/android/android.go | 52 +++++++++++++-- internal/android/android_test.go | 110 +++++++++++++++++++++++++++++++ 2 files changed, 156 insertions(+), 6 deletions(-) diff --git a/internal/android/android.go b/internal/android/android.go index b8038a2..17470ac 100644 --- a/internal/android/android.go +++ b/internal/android/android.go @@ -247,7 +247,9 @@ func EnvWithAndroidPlatformTools(env []string) []string { // AdbBinary locates the adb binary via PATH or known Android SDK locations. func AdbBinary() (string, error) { return findAndroidTool("adb", "platform-tools") } -func emulatorBinary() (string, error) { return findAndroidTool("emulator", "emulator") } +// EmulatorBinary locates the emulator binary via PATH or known Android SDK +// locations. +func EmulatorBinary() (string, error) { return findAndroidTool("emulator", "emulator") } // findAndroidTool locates a binary from the Android SDK. It checks PATH, // then $ANDROID_HOME// and $ANDROID_SDK_ROOT//, @@ -264,7 +266,36 @@ func findAndroidTool(name, subdir string) (string, error) { } tried = append(tried, candidate) } - return "", fmt.Errorf("could not locate %q: not on PATH and not under any known Android SDK root (set $ANDROID_HOME to point at your SDK; tried %v)", name, tried) + return "", fmt.Errorf( + "%s not found: not on PATH, and not at [%s]; %s\nput %s on PATH, or point $ANDROID_HOME at an Android SDK that has %s/%s", + name, strings.Join(tried, ", "), sdkRootStatus(), name, subdir, name, + ) +} + +// sdkRootStatus reports what the SDK root variables hold, so a lookup failure +// says whether they were unset or pointed somewhere that is not an SDK instead +// of leaving the reader to work out which from a list of paths. +func sdkRootStatus() string { + var reported []string + for _, variable := range []string{"ANDROID_HOME", "ANDROID_SDK_ROOT"} { + value := os.Getenv(variable) + if value == "" { + continue + } + info, err := os.Stat(value) + switch { + case err != nil: + reported = append(reported, fmt.Sprintf("$%s=%s does not exist", variable, value)) + case !info.IsDir(): + reported = append(reported, fmt.Sprintf("$%s=%s is not a directory", variable, value)) + default: + reported = append(reported, fmt.Sprintf("$%s=%s", variable, value)) + } + } + if len(reported) == 0 { + return "$ANDROID_HOME and $ANDROID_SDK_ROOT are unset" + } + return strings.Join(reported, ", ") } func androidSDKCandidates() []string { @@ -283,11 +314,20 @@ func androidSDKCandidates() []string { addRoot(filepath.Join(home, "Library", "Android", "sdk")) addRoot(filepath.Join(home, "Android", "Sdk")) } - addRoot("/opt/homebrew/share/android-commandlinetools") - addRoot("/usr/local/share/android-commandlinetools") + for _, root := range standardSDKRoots { + addRoot(root) + } return roots } +// standardSDKRoots are the install locations checked after the environment and +// the home directory. A var so a resolution test can point it at a fixture +// instead of whatever SDK the host running the test happens to have. +var standardSDKRoots = []string{ + "/opt/homebrew/share/android-commandlinetools", + "/usr/local/share/android-commandlinetools", +} + func listAdbDevices(ctx context.Context) ([]string, error) { adb, err := AdbBinary() if err != nil { @@ -317,7 +357,7 @@ func parseAdbDevices(output string) []string { } func listAVDs(ctx context.Context) ([]string, error) { - emulator, err := emulatorBinary() + emulator, err := EmulatorBinary() if err != nil { return nil, err } @@ -382,7 +422,7 @@ func pickAVD(requested string, available []string) (string, error) { } func bootAVD(_ context.Context, name string) error { - emulator, err := emulatorBinary() + emulator, err := EmulatorBinary() if err != nil { return err } diff --git a/internal/android/android_test.go b/internal/android/android_test.go index 01a017e..2161cd3 100644 --- a/internal/android/android_test.go +++ b/internal/android/android_test.go @@ -1,6 +1,8 @@ package android import ( + "os" + "path/filepath" "reflect" "slices" "strings" @@ -136,6 +138,114 @@ func TestPickAVD_NoneAvailable(t *testing.T) { } } +// fakeSDK writes an SDK layout holding only the named tools ("emulator/emulator"), +// so a lookup test never resolves against the host's own SDK. +func fakeSDK(t *testing.T, tools ...string) string { + t.Helper() + root := t.TempDir() + for _, tool := range tools { + path := filepath.Join(root, filepath.FromSlash(tool)) + if err := os.MkdirAll(filepath.Dir(path), 0o755); err != nil { + t.Fatalf("mkdir %s: %v", filepath.Dir(path), err) + } + if err := os.WriteFile(path, nil, 0o755); err != nil { + t.Fatalf("write %s: %v", path, err) + } + } + return root +} + +// isolateSDKLookup closes every route to the host's own SDK: an empty PATH, no +// root variables, a home with nothing under it, and the standard install +// locations replaced by the given roots. It returns the fake home directory. +func isolateSDKLookup(t *testing.T, roots ...string) string { + t.Helper() + home := t.TempDir() + t.Setenv("PATH", t.TempDir()) + t.Setenv("ANDROID_HOME", "") + t.Setenv("ANDROID_SDK_ROOT", "") + t.Setenv("HOME", home) + original := standardSDKRoots + t.Cleanup(func() { standardSDKRoots = original }) + standardSDKRoots = roots + return home +} + +func TestAdbBinary_ResolvesUnderStandardSDKRootWithAndroidHomeUnset(t *testing.T) { + sdk := fakeSDK(t, "platform-tools/adb") + isolateSDKLookup(t, sdk) + + adb, err := AdbBinary() + if err != nil { + t.Fatalf("AdbBinary: %v", err) + } + if want := filepath.Join(sdk, "platform-tools", "adb"); adb != want { + t.Errorf("AdbBinary() = %q, want %q", adb, want) + } +} + +func TestEmulatorBinary_ResolvesUnderStandardSDKRootWithAndroidHomeUnset(t *testing.T) { + sdk := fakeSDK(t, "emulator/emulator") + isolateSDKLookup(t, sdk) + + emulator, err := EmulatorBinary() + if err != nil { + t.Fatalf("EmulatorBinary: %v", err) + } + if want := filepath.Join(sdk, "emulator", "emulator"); emulator != want { + t.Errorf("EmulatorBinary() = %q, want %q", emulator, want) + } +} + +func TestAdbBinary_UsesAndroidHome(t *testing.T) { + sdk := fakeSDK(t, "platform-tools/adb") + isolateSDKLookup(t) + t.Setenv("ANDROID_HOME", sdk) + + adb, err := AdbBinary() + if err != nil { + t.Fatalf("AdbBinary: %v", err) + } + if want := filepath.Join(sdk, "platform-tools", "adb"); adb != want { + t.Errorf("AdbBinary() = %q, want %q", adb, want) + } +} + +func TestAdbBinary_NoSDKAnywhereReportsWhereItLooked(t *testing.T) { + empty := t.TempDir() + home := isolateSDKLookup(t, empty) + + _, err := AdbBinary() + if err == nil { + t.Fatal("expected an error with no SDK anywhere") + } + for _, want := range []string{ + "adb not found: not on PATH", + filepath.Join(home, "Library", "Android", "sdk", "platform-tools", "adb"), + filepath.Join(empty, "platform-tools", "adb"), + "$ANDROID_HOME and $ANDROID_SDK_ROOT are unset", + "put adb on PATH, or point $ANDROID_HOME at an Android SDK that has platform-tools/adb", + } { + if !strings.Contains(err.Error(), want) { + t.Errorf("error %q missing %q", err, want) + } + } +} + +func TestAdbBinary_AndroidHomePointingNowhereIsNamed(t *testing.T) { + isolateSDKLookup(t, t.TempDir()) + missing := filepath.Join(t.TempDir(), "no-such-sdk") + t.Setenv("ANDROID_HOME", missing) + + _, err := AdbBinary() + if err == nil { + t.Fatal("expected an error when ANDROID_HOME points nowhere") + } + if want := "$ANDROID_HOME=" + missing + " does not exist"; !strings.Contains(err.Error(), want) { + t.Errorf("error %q must say %q rather than leaving it in the tried list", err, want) + } +} + func TestPathContains(t *testing.T) { path := "/usr/bin:/opt/tools:/usr/local/bin" if !pathContains(path, "/opt/tools") { From 22115a6c25aacd0fab313bc7242092fbbcfc3642 Mon Sep 17 00:00:00 2001 From: PJ Date: Sat, 15 Aug 2026 22:38:23 +0530 Subject: [PATCH 2/6] fix(doctor): resolve adb and emulator the way a run does --- cmd/sanderling/doctor.go | 31 ++++++++++---------------- cmd/sanderling/doctor_test.go | 41 ++++++++++++++++++++++++++++++++++- 2 files changed, 52 insertions(+), 20 deletions(-) diff --git a/cmd/sanderling/doctor.go b/cmd/sanderling/doctor.go index d642c3e..e206ce5 100644 --- a/cmd/sanderling/doctor.go +++ b/cmd/sanderling/doctor.go @@ -5,15 +5,14 @@ import ( "flag" "fmt" "io" - "os" "os/exec" - "path/filepath" "regexp" "strconv" "time" "github.com/chromedp/chromedp" + "github.com/priyanshujain/sanderling/internal/android" "github.com/priyanshujain/sanderling/internal/driver/ioscompanion" "github.com/priyanshujain/sanderling/internal/ios" "github.com/priyanshujain/sanderling/internal/sidecarassets" @@ -52,8 +51,8 @@ func webChecks() []doctorCheck { func androidChecks() []doctorCheck { return []doctorCheck{ - {Name: "adb on PATH", Run: checkExecutableOnPath("adb")}, - {Name: "emulator on PATH or under ANDROID_HOME", Run: checkEmulator}, + {Name: "adb on PATH or under the Android SDK", Run: checkAdb}, + {Name: "emulator on PATH or under the Android SDK", Run: checkEmulator}, {Name: "java 17+ on PATH", Run: checkJavaVersion}, {Name: "sidecar JAR is real (not placeholder)", Run: checkSidecarJAR}, } @@ -235,22 +234,16 @@ func checkExecutableOnPath(name string) func(context.Context) error { } } +// checkAdb and checkEmulator resolve through the same helpers a run uses, so a +// host the doctor passes is a host a run can drive. +func checkAdb(_ context.Context) error { + _, err := android.AdbBinary() + return err +} + func checkEmulator(_ context.Context) error { - if _, err := exec.LookPath("emulator"); err == nil { - return nil - } - androidHome := os.Getenv("ANDROID_HOME") - if androidHome == "" { - androidHome = os.Getenv("ANDROID_SDK_ROOT") - } - if androidHome == "" { - return fmt.Errorf("not on PATH and ANDROID_HOME is unset") - } - candidate := filepath.Join(androidHome, "emulator", "emulator") - if _, err := os.Stat(candidate); err != nil { - return fmt.Errorf("not found at %s", candidate) - } - return nil + _, err := android.EmulatorBinary() + return err } var javaVersionPattern = regexp.MustCompile(`(?:java|openjdk)[^"]*"(\d+)(?:\.(\d+))?`) diff --git a/cmd/sanderling/doctor_test.go b/cmd/sanderling/doctor_test.go index 3155180..489b498 100644 --- a/cmd/sanderling/doctor_test.go +++ b/cmd/sanderling/doctor_test.go @@ -6,6 +6,8 @@ import ( "errors" "flag" "io" + "os" + "path/filepath" "strings" "testing" @@ -110,6 +112,43 @@ func TestDoctorChecksFor_Android_IncludesADB(t *testing.T) { } } +// The SDK tools a run invokes are resolved through $ANDROID_HOME and the +// standard install locations, never PATH alone, so a doctor that turns away a +// host on a PATH lookup condemns a setup every run on it would drive fine. +func TestAndroidChecks_AcceptSDKToolsThatAreNotOnPath(t *testing.T) { + sdk := t.TempDir() + for _, tool := range []string{"platform-tools/adb", "emulator/emulator"} { + path := filepath.Join(sdk, filepath.FromSlash(tool)) + if err := os.MkdirAll(filepath.Dir(path), 0o755); err != nil { + t.Fatalf("mkdir %s: %v", filepath.Dir(path), err) + } + if err := os.WriteFile(path, nil, 0o755); err != nil { + t.Fatalf("write %s: %v", path, err) + } + } + t.Setenv("PATH", t.TempDir()) + t.Setenv("ANDROID_HOME", sdk) + t.Setenv("ANDROID_SDK_ROOT", "") + + var sdkChecks []doctorCheck + for _, check := range doctorChecksFor("android") { + if strings.Contains(check.Name, "adb") || strings.Contains(check.Name, "emulator") { + sdkChecks = append(sdkChecks, check) + } + } + if len(sdkChecks) != 2 { + t.Fatalf("expected the adb and emulator checks, got %d", len(sdkChecks)) + } + + var stdout bytes.Buffer + if err := runDoctorChecks(context.Background(), sdkChecks, &stdout); err != nil { + t.Fatalf("doctor: %v\n%s", err, stdout.String()) + } + if strings.Contains(stdout.String(), "FAIL") { + t.Errorf("doctor rejected an SDK it can resolve:\n%s", stdout.String()) + } +} + func TestDoctorChecksFor_iOS_IncludesXcrun(t *testing.T) { checks := doctorChecksFor("ios") found := false @@ -129,7 +168,7 @@ func TestDoctorChecksFor_All_IsUnion(t *testing.T) { for _, c := range all { names[c.Name]++ } - for _, name := range []string{"adb on PATH", "xcrun on PATH (ios simulator)", "headless chromium can launch"} { + for _, name := range []string{"adb on PATH or under the Android SDK", "xcrun on PATH (ios simulator)", "headless chromium can launch"} { if names[name] != 1 { t.Errorf("expected %q in 'all' exactly once, got %d", name, names[name]) } From 3af679102987accc22c1404133410f7fb506e922 Mon Sep 17 00:00:00 2001 From: PJ Date: Sat, 15 Aug 2026 22:38:34 +0530 Subject: [PATCH 3/6] docs(cli): the android doctor checks are not path-only --- docs/manual/cli.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/manual/cli.md b/docs/manual/cli.md index 04ab826..c5b5bf2 100644 --- a/docs/manual/cli.md +++ b/docs/manual/cli.md @@ -56,7 +56,7 @@ sanderling doctor [--platform web|android|ios|ios-device|all] | Platform | Checks | |---|---| | `web` | headless Chromium can launch (the bundled CDP surface boots a real browser). | -| `android` | `adb` on PATH; `emulator` on PATH or under `ANDROID_HOME`; Java 17+; embedded native sidecar JAR is real. | +| `android` | `adb` and `emulator` on PATH, or under `$ANDROID_HOME`, `$ANDROID_SDK_ROOT` or a standard SDK install location; Java 17+; embedded native sidecar JAR is real. | | `ios` | `xcrun` on PATH; `simctl` on PATH. The simulator path drives the native companion with no JVM. | | `ios-device` | the `ios` checks plus `devicectl`; the macOS `usbmuxd` socket; a connected, paired device; App Store Connect signing credentials present. | From f819dad7b65fad389601a15a764b0845624e32c1 Mon Sep 17 00:00:00 2001 From: PJ Date: Sat, 15 Aug 2026 22:39:38 +0530 Subject: [PATCH 4/6] fix(testrun): preflight resolves adb through the sdk, not just PATH --- internal/testrun/preflight.go | 10 ++++++++++ internal/testrun/preflight_test.go | 29 +++++++++++++++++++++++++++++ 2 files changed, 39 insertions(+) diff --git a/internal/testrun/preflight.go b/internal/testrun/preflight.go index a65c88b..e95caf8 100644 --- a/internal/testrun/preflight.go +++ b/internal/testrun/preflight.go @@ -4,6 +4,8 @@ import ( "context" "fmt" "os/exec" + + "github.com/priyanshujain/sanderling/internal/android" ) // Preflight runs platform-specific host checks before sidecar/driver setup. @@ -18,7 +20,15 @@ func Preflight(ctx context.Context, platform string) error { type preflightFunc func(name string) error +// preflightCheck resolves adb through the same helper every adb call in a run +// uses, so a host whose SDK is only reachable through $ANDROID_HOME or a +// standard install location is not turned away here and then driven fine by +// the rest of the pipeline. func preflightCheck(name string) error { + if name == "adb" { + _, err := android.AdbBinary() + return err + } if _, err := exec.LookPath(name); err != nil { return fmt.Errorf("%s not found on PATH: %w", name, err) } diff --git a/internal/testrun/preflight_test.go b/internal/testrun/preflight_test.go index e460d07..d2f32f1 100644 --- a/internal/testrun/preflight_test.go +++ b/internal/testrun/preflight_test.go @@ -3,6 +3,8 @@ package testrun import ( "context" "errors" + "os" + "path/filepath" "strings" "testing" ) @@ -49,6 +51,33 @@ func TestPreflight_AndroidNeedsAdbAndJava(t *testing.T) { } } +// Every adb call in an android run resolves through $ANDROID_HOME and the +// standard SDK locations, so a preflight that only looks at PATH turns away a +// host the run itself would drive. +func TestPreflight_AndroidAcceptsAdbUnderAndroidHome(t *testing.T) { + sdk := t.TempDir() + writeExecutable(t, filepath.Join(sdk, "platform-tools", "adb")) + pathDirectory := t.TempDir() + writeExecutable(t, filepath.Join(pathDirectory, "java")) + t.Setenv("PATH", pathDirectory) + t.Setenv("ANDROID_HOME", sdk) + t.Setenv("ANDROID_SDK_ROOT", "") + + if err := Preflight(context.Background(), "android"); err != nil { + t.Fatalf("Preflight with adb under $ANDROID_HOME: %v", err) + } +} + +func writeExecutable(t *testing.T, path string) { + t.Helper() + if err := os.MkdirAll(filepath.Dir(path), 0o755); err != nil { + t.Fatalf("mkdir %s: %v", filepath.Dir(path), err) + } + if err := os.WriteFile(path, nil, 0o755); err != nil { + t.Fatalf("write %s: %v", path, err) + } +} + func TestPreflight_iOSNeedsXcrun(t *testing.T) { check := func(name string) error { if name == "xcrun" { From 708a8a29292661f600b2077212bcfbb1a980ffd1 Mon Sep 17 00:00:00 2001 From: PJ Date: Sat, 15 Aug 2026 22:42:26 +0530 Subject: [PATCH 5/6] fix(testrun): report a sidecar that dies at startup as the exit it was --- internal/android/android.go | 6 +-- internal/testrun/driver.go | 95 ++++++++++++++++++++++++++------- internal/testrun/driver_test.go | 72 +++++++++++++++++++++++++ 3 files changed, 148 insertions(+), 25 deletions(-) diff --git a/internal/android/android.go b/internal/android/android.go index 17470ac..5983056 100644 --- a/internal/android/android.go +++ b/internal/android/android.go @@ -221,11 +221,7 @@ func navOverlayCommand(ctx context.Context, adb, serial, overlay string) *exec.C // EnvWithAndroidPlatformTools returns env with the directory containing adb // prepended to PATH, so child processes (the sidecar) can invoke adb even // when the user hasn't set up their shell PATH. -func EnvWithAndroidPlatformTools(env []string) []string { - adb, err := AdbBinary() - if err != nil { - return env - } +func EnvWithAndroidPlatformTools(env []string, adb string) []string { adbDir := filepath.Dir(adb) result := make([]string, 0, len(env)) found := false diff --git a/internal/testrun/driver.go b/internal/testrun/driver.go index a46a857..109fd53 100644 --- a/internal/testrun/driver.go +++ b/internal/testrun/driver.go @@ -148,10 +148,14 @@ func buildDriver(ctx context.Context, options Options, stdout io.Writer) (driver if options.Device != "" { sidecarArgs = append(sidecarArgs, "--serial", options.Device) } + adbPath, err := android.AdbBinary() + if err != nil { + return nil, nil, preflightFailure("android", err) + } sidecarCommand := exec.CommandContext(ctx, "java", sidecarArgs...) sidecarCommand.Stdout = stdout sidecarCommand.Stderr = stdout - sidecarCommand.Env = android.EnvWithAndroidPlatformTools(os.Environ()) + sidecarCommand.Env = android.EnvWithAndroidPlatformTools(os.Environ(), adbPath) // SIGTERM lets the sidecar's shutdown hook stop the iOS XCTest runner. // SIGKILL skips the hook and orphans an xcodebuild session that later // restarts its runner and hijacks the simulator mid-run. @@ -162,11 +166,19 @@ func buildDriver(ctx context.Context, options Options, stdout io.Writer) (driver if err := sidecarCommand.Start(); err != nil { return nil, nil, fmt.Errorf("spawn sidecar: %w", err) } - fmt.Fprintf(stdout, "sidecar pid=%d listening on 127.0.0.1:%d\n", sidecarCommand.Process.Pid, sidecarPort) + // The close after the send lets the cleanup path receive again once the + // startup path has already taken the exit status. + sidecarExited := make(chan error, 1) + go func() { + sidecarExited <- sidecarCommand.Wait() + close(sidecarExited) + }() + address := fmt.Sprintf("127.0.0.1:%d", sidecarPort) + fmt.Fprintf(stdout, "sidecar pid=%d listening on %s (adb: %s)\n", sidecarCommand.Process.Pid, address, adbPath) - driverClient, err := driverSidecar.Dial(fmt.Sprintf("127.0.0.1:%d", sidecarPort)) + driverClient, err := driverSidecar.Dial(address) if err != nil { - stopSidecar(sidecarCommand) + stopSidecar(sidecarCommand, sidecarExited) return nil, nil, fmt.Errorf("dial sidecar: %w", err) } driverClient.SetPlatform(options.Platform) @@ -175,22 +187,70 @@ func buildDriver(ctx context.Context, options Options, stdout io.Writer) (driver // (absorbing the XCUITest startup race) runs inside IosDriverBackend.init // in the sidecar - no additional sleep needed here. healthCtx, healthCancel := context.WithTimeout(ctx, sidecarStartupTimeout) - if err := driverClient.WaitForHealth(healthCtx, 250e6); err != nil { - healthCancel() - stopSidecar(sidecarCommand) - _ = driverClient.Close() - return nil, nil, fmt.Errorf("sidecar health check: %w", err) - } + healthErr := awaitSidecar(healthCtx, address, sidecarStartupTimeout, func(pollCtx context.Context) error { + return driverClient.WaitForHealth(pollCtx, 250e6) + }, sidecarExited) healthCancel() + if healthErr != nil { + stopSidecar(sidecarCommand, sidecarExited) + _ = driverClient.Close() + return nil, nil, healthErr + } fmt.Fprintln(stdout, "sidecar is healthy") cleanup := func() { _ = driverClient.Close() - stopSidecar(sidecarCommand) + stopSidecar(sidecarCommand, sidecarExited) } return driverClient, cleanup, nil } +// awaitSidecar waits for the sidecar to answer a health check, racing that +// against the process exiting so a sidecar that dies during startup is reported +// as the exit it was rather than as a deadline half a minute later. Neither +// failure knows why the sidecar was unhappy, so both name what to look at +// instead of picking a cause. +func awaitSidecar( + ctx context.Context, + address string, + timeout time.Duration, + health func(context.Context) error, + exited <-chan error, +) error { + healthy := make(chan error, 1) + go func() { healthy <- health(ctx) }() + select { + case exitErr := <-exited: + return sidecarExitedError(address, exitErr) + case err := <-healthy: + if err == nil { + return nil + } + select { + case exitErr := <-exited: + return sidecarExitedError(address, exitErr) + default: + return fmt.Errorf( + "sidecar did not answer a health check on %s within %s and is still running\n%s", + address, timeout, sidecarWhatToCheck, + ) + } + } +} + +const sidecarWhatToCheck = "check the sidecar output above, then `sanderling doctor --platform=android` (java 17+, adb, Android SDK)" + +func sidecarExitedError(address string, exitErr error) error { + status := "exit status 0" + if exitErr != nil { + status = exitErr.Error() + } + return fmt.Errorf( + "sidecar exited before it answered a health check on %s: %s\n%s", + address, status, sidecarWhatToCheck, + ) +} + // sidecarShutdownGrace bounds how long the sidecar gets to run its shutdown // hook (terminate the app, stop the XCTest runner) before being killed. const sidecarShutdownGrace = 15 * time.Second @@ -198,25 +258,20 @@ const sidecarShutdownGrace = 15 * time.Second // stopSidecar terminates the sidecar gracefully so its shutdown hook can stop // the device-side runner processes, escalating to SIGKILL when it does not // exit within the grace window. -func stopSidecar(sidecarCommand *exec.Cmd) { +func stopSidecar(sidecarCommand *exec.Cmd, exited <-chan error) { if sidecarCommand.Process == nil { return } if err := sidecarCommand.Process.Signal(syscall.SIGTERM); err != nil { _ = sidecarCommand.Process.Kill() - _ = sidecarCommand.Wait() + <-exited return } - done := make(chan struct{}) - go func() { - _ = sidecarCommand.Wait() - close(done) - }() select { - case <-done: + case <-exited: case <-time.After(sidecarShutdownGrace): _ = sidecarCommand.Process.Kill() - <-done + <-exited } } diff --git a/internal/testrun/driver_test.go b/internal/testrun/driver_test.go index 5e461b3..35b2a04 100644 --- a/internal/testrun/driver_test.go +++ b/internal/testrun/driver_test.go @@ -4,7 +4,9 @@ import ( "context" "errors" "io" + "strings" "testing" + "time" "github.com/priyanshujain/sanderling/internal/driver" "github.com/priyanshujain/sanderling/internal/driver/ioscompanion" @@ -65,6 +67,76 @@ func TestBuildDriverSurfacesDeviceConstructionError(t *testing.T) { } } +// A sidecar that dies during startup leaves the health poll with nothing to +// talk to, and reporting that as a deadline sends the reader after a gRPC +// timeout instead of the exit that already happened. +func TestAwaitSidecarReportsTheExitItSaw(t *testing.T) { + exited := make(chan error, 1) + exited <- errors.New("exit status 1") + close(exited) + + err := awaitSidecar( + context.Background(), + "127.0.0.1:54321", + 30*time.Second, + func(ctx context.Context) error { <-ctx.Done(); return ctx.Err() }, + exited, + ) + if err == nil { + t.Fatal("expected an error when the sidecar exits before it is healthy") + } + for _, want := range []string{ + "sidecar exited before it answered a health check on 127.0.0.1:54321: exit status 1", + "check the sidecar output above", + "sanderling doctor --platform=android", + } { + if !strings.Contains(err.Error(), want) { + t.Errorf("error %q missing %q", err, want) + } + } +} + +func TestAwaitSidecarTimeoutSaysOnlyWhatItObserved(t *testing.T) { + ctx, cancel := context.WithTimeout(context.Background(), 50*time.Millisecond) + defer cancel() + + err := awaitSidecar( + ctx, + "127.0.0.1:54321", + 50*time.Millisecond, + func(ctx context.Context) error { <-ctx.Done(); return ctx.Err() }, + make(chan error, 1), + ) + if err == nil { + t.Fatal("expected an error when the sidecar never answers") + } + for _, want := range []string{ + "sidecar did not answer a health check on 127.0.0.1:54321 within 50ms and is still running", + "check the sidecar output above", + "sanderling doctor --platform=android", + } { + if !strings.Contains(err.Error(), want) { + t.Errorf("error %q missing %q", err, want) + } + } + if strings.Contains(err.Error(), "context deadline exceeded") { + t.Errorf("error %q must not hand the reader a bare gRPC deadline", err) + } +} + +func TestAwaitSidecarHealthyReturnsNil(t *testing.T) { + err := awaitSidecar( + context.Background(), + "127.0.0.1:54321", + 30*time.Second, + func(context.Context) error { return nil }, + make(chan error, 1), + ) + if err != nil { + t.Fatalf("expected a healthy sidecar to pass, got %v", err) + } +} + // stubPreflight bypasses the host-readiness checks so routing tests exercise // driver construction on a Linux CI runner that lacks xcrun/java. func stubPreflight(t *testing.T) { From e61b39f2e387127492631c56a41e23f198ab5c5a Mon Sep 17 00:00:00 2001 From: PJ Date: Sat, 15 Aug 2026 22:46:47 +0530 Subject: [PATCH 6/6] test(testrun): cover the sidecar shutdown path after an early exit --- internal/testrun/driver.go | 20 +++++++----- internal/testrun/driver_test.go | 55 +++++++++++++++++++++++++++++++++ 2 files changed, 68 insertions(+), 7 deletions(-) diff --git a/internal/testrun/driver.go b/internal/testrun/driver.go index 109fd53..e11db44 100644 --- a/internal/testrun/driver.go +++ b/internal/testrun/driver.go @@ -166,13 +166,7 @@ func buildDriver(ctx context.Context, options Options, stdout io.Writer) (driver if err := sidecarCommand.Start(); err != nil { return nil, nil, fmt.Errorf("spawn sidecar: %w", err) } - // The close after the send lets the cleanup path receive again once the - // startup path has already taken the exit status. - sidecarExited := make(chan error, 1) - go func() { - sidecarExited <- sidecarCommand.Wait() - close(sidecarExited) - }() + sidecarExited := watchSidecar(sidecarCommand) address := fmt.Sprintf("127.0.0.1:%d", sidecarPort) fmt.Fprintf(stdout, "sidecar pid=%d listening on %s (adb: %s)\n", sidecarCommand.Process.Pid, address, adbPath) @@ -205,6 +199,18 @@ func buildDriver(ctx context.Context, options Options, stdout io.Writer) (driver return driverClient, cleanup, nil } +// watchSidecar reaps the sidecar and publishes its exit status. The channel is +// closed after the send so the shutdown path can still receive once the startup +// path has taken the status. +func watchSidecar(sidecarCommand *exec.Cmd) <-chan error { + exited := make(chan error, 1) + go func() { + exited <- sidecarCommand.Wait() + close(exited) + }() + return exited +} + // awaitSidecar waits for the sidecar to answer a health check, racing that // against the process exiting so a sidecar that dies during startup is reported // as the exit it was rather than as a deadline half a minute later. Neither diff --git a/internal/testrun/driver_test.go b/internal/testrun/driver_test.go index 35b2a04..44f63c2 100644 --- a/internal/testrun/driver_test.go +++ b/internal/testrun/driver_test.go @@ -4,6 +4,7 @@ import ( "context" "errors" "io" + "os/exec" "strings" "testing" "time" @@ -137,6 +138,60 @@ func TestAwaitSidecarHealthyReturnsNil(t *testing.T) { } } +func TestStopSidecarTerminatesARunningSidecar(t *testing.T) { + command := exec.Command("sleep", "60") + if err := command.Start(); err != nil { + t.Fatalf("start: %v", err) + } + exited := watchSidecar(command) + + stopped := make(chan struct{}) + go func() { + stopSidecar(command, exited) + close(stopped) + }() + select { + case <-stopped: + case <-time.After(10 * time.Second): + t.Fatal("stopSidecar never returned for a running sidecar") + } + if got := command.ProcessState.String(); got != "signal: terminated" { + t.Errorf("sidecar ended as %q, want the SIGTERM its shutdown hook needs", got) + } +} + +// The startup path takes the exit status to report it, so the shutdown path +// that follows must not sit waiting for a status nobody will send again. +func TestStopSidecarAfterTheStartupPathTookTheExitStatus(t *testing.T) { + command := exec.Command("sh", "-c", "exit 3") + if err := command.Start(); err != nil { + t.Fatalf("start: %v", err) + } + exited := watchSidecar(command) + + err := awaitSidecar( + context.Background(), + "127.0.0.1:54321", + 30*time.Second, + func(ctx context.Context) error { <-ctx.Done(); return ctx.Err() }, + exited, + ) + if err == nil || !strings.Contains(err.Error(), "exit status 3") { + t.Fatalf("expected the sidecar's real exit status, got %v", err) + } + + stopped := make(chan struct{}) + go func() { + stopSidecar(command, exited) + close(stopped) + }() + select { + case <-stopped: + case <-time.After(10 * time.Second): + t.Fatal("stopSidecar blocked on an exit status the startup path had already taken") + } +} + // stubPreflight bypasses the host-readiness checks so routing tests exercise // driver construction on a Linux CI runner that lacks xcrun/java. func stubPreflight(t *testing.T) {