From 04e83b515d35e9cc1f031f9a918dc4fb43d90d97 Mon Sep 17 00:00:00 2001 From: Sergio Andres Rodriguez Orama Date: Wed, 6 May 2026 15:24:22 -0400 Subject: [PATCH] Return struct CommandOutput rather than two strings. Follow up for comment: https://github.com/google/android-cuttlefish/pull/2498#discussion_r3171242393 Bug: b/507588992 --- e2etests/cvd/bugreport_tests/main_test.go | 2 +- e2etests/cvd/common/common.go | 46 +++++++++++-------- e2etests/cvd/cvd_powerwash_tests/main_test.go | 6 +-- e2etests/cvd/display_tests/main_test.go | 4 +- e2etests/cvd/env_tests/main_test.go | 2 +- .../cvd/graphics_detector_tests/main_test.go | 4 +- e2etests/cvd/logs_tests/main_test.go | 10 ++-- e2etests/cvd/media_tests/main_test.go | 4 +- e2etests/cvd/metrics_tests/main_test.go | 4 +- 9 files changed, 45 insertions(+), 37 deletions(-) diff --git a/e2etests/cvd/bugreport_tests/main_test.go b/e2etests/cvd/bugreport_tests/main_test.go index 5394d4ec2aa..a1d9c5d02c0 100644 --- a/e2etests/cvd/bugreport_tests/main_test.go +++ b/e2etests/cvd/bugreport_tests/main_test.go @@ -39,7 +39,7 @@ func TestTakeBugreport(t *testing.T) { t.Fatal(err) } - if _, _, err := c.RunCmd(c.TargetBin(), "host_bugreport", "--output=/tmp/host_bugreport.zip", "--include_adb_bugreport=true"); err != nil { + if _, err := c.RunCmd(c.TargetBin(), "host_bugreport", "--output=/tmp/host_bugreport.zip", "--include_adb_bugreport=true"); err != nil { t.Fatal(err) } diff --git a/e2etests/cvd/common/common.go b/e2etests/cvd/common/common.go index e3efe2c0e86..25938b22d80 100644 --- a/e2etests/cvd/common/common.go +++ b/e2etests/cvd/common/common.go @@ -79,7 +79,13 @@ type TestContext struct { usePodcvd bool } -func runCmdWithContextEnv(ctx context.Context, command []string, envvars map[string]string) (string, string, error) { +// Output of running a command. +type CommandOutput struct { + Stdout string + Stderr string +} + +func runCmdWithContextEnv(ctx context.Context, command []string, envvars map[string]string) (CommandOutput, error) { cmd := exec.CommandContext(ctx, command[0], command[1:]...) stdOutBuf := bytes.Buffer{} @@ -99,14 +105,14 @@ func runCmdWithContextEnv(ctx context.Context, command []string, envvars map[str log.Printf("Running `%s %s`\n", strings.Join(envvarPairs, " "), strings.Join(command, " ")) err := cmd.Run() if err != nil { - return "", "", fmt.Errorf("`%s` failed: %w", strings.Join(command, " "), err) + return CommandOutput{}, fmt.Errorf("`%s` failed: %w", strings.Join(command, " "), err) } - return stdOutBuf.String(), stdErrBuf.String(), nil + return CommandOutput{Stdout: stdOutBuf.String(), Stderr: stdErrBuf.String()}, nil } // Runs the given command with the given set of envvars overrided. -func (tc *TestContext) RunCmdWithEnv(command []string, envvars map[string]string) (string, string, error) { +func (tc *TestContext) RunCmdWithEnv(command []string, envvars map[string]string) (CommandOutput, error) { return runCmdWithContextEnv(tc.context, command, envvars) } @@ -119,14 +125,14 @@ func (tc *TestContext) RunAdbWaitForDevice() error { "adb", "wait-for-device", } - if _, _, err := tc.RunCmd(adbCommand...); err != nil { + if _, err := tc.RunCmd(adbCommand...); err != nil { return fmt.Errorf("timed out waiting for Cuttlefish device to connect to adb: %w", err) } return nil } // Runs the given command with the existing envvars. -func (tc *TestContext) RunCmd(args ...string) (string, string, error) { +func (tc *TestContext) RunCmd(args ...string) (CommandOutput, error) { command := []string{} command = append(command, args...) return tc.RunCmdWithEnv(command, map[string]string{}) @@ -176,7 +182,7 @@ func (tc *TestContext) CVDFetch(args FetchArgs) error { if credentialArg != "" { fetchCmd = append(fetchCmd, fmt.Sprintf("--credential_source=%s", credentialArg)) } - if _, _, err := tc.RunCmd(fetchCmd...); err != nil { + if _, err := tc.RunCmd(fetchCmd...); err != nil { log.Printf("Failed to fetch: %w", err) return err } @@ -204,7 +210,7 @@ func (tc *TestContext) CVDCreate(args CreateArgs) error { if len(args.Args) > 0 { createCmd = append(createCmd, args.Args...) } - if _, _, err := tc.RunCmdWithEnv(createCmd, tempdirEnv); err != nil { + if _, err := tc.RunCmdWithEnv(createCmd, tempdirEnv); err != nil { log.Printf("Failed to create instance(s): %w", err) return err } @@ -220,7 +226,7 @@ func (tc *TestContext) CVDStop() error { } stopCmd := []string{tc.TargetBin(), "stop"} - if _, _, err := tc.RunCmdWithEnv(stopCmd, tempdirEnv); err != nil { + if _, err := tc.RunCmdWithEnv(stopCmd, tempdirEnv); err != nil { log.Printf("Failed to stop instance(s): %w", err) return err } @@ -240,7 +246,7 @@ func (tc *TestContext) LaunchCVD(args CreateArgs) error { if len(args.Args) > 0 { createCmd = append(createCmd, args.Args...) } - if _, _, err := tc.RunCmdWithEnv(createCmd, tempdirEnv); err != nil { + if _, err := tc.RunCmdWithEnv(createCmd, tempdirEnv); err != nil { log.Printf("Failed to create instance(s): %w", err) return err } @@ -256,7 +262,7 @@ func (tc *TestContext) StopCVD() error { } stopCmd := []string{"bin/stop_cvd"} - if _, _, err := tc.RunCmdWithEnv(stopCmd, tempdirEnv); err != nil { + if _, err := tc.RunCmdWithEnv(stopCmd, tempdirEnv); err != nil { log.Printf("Failed to stop instance(s): %w", err) return err } @@ -271,7 +277,7 @@ func (tc *TestContext) CVDPowerwash() error { } createCmd := []string{tc.TargetBin(), "powerwash"} - if _, _, err := tc.RunCmdWithEnv(createCmd, tempdirEnv); err != nil { + if _, err := tc.RunCmdWithEnv(createCmd, tempdirEnv); err != nil { log.Printf("Failed to powerwash instance(s): %w", err) return err } @@ -306,7 +312,7 @@ func (tc *TestContext) CVDLoad(load LoadArgs) error { if credentialArg != "" { loadCmd = append(loadCmd, fmt.Sprintf("--credential_source=%s", credentialArg)) } - if _, _, err := tc.RunCmd(loadCmd...); err != nil { + if _, err := tc.RunCmd(loadCmd...); err != nil { log.Printf("Failed to perform `cvd load`: %w", err) return err } @@ -317,13 +323,13 @@ func (tc *TestContext) CVDLoad(load LoadArgs) error { } func (tc *TestContext) GetMetricsDir() (string, error) { - stdOut, _, err := tc.RunCmd("cvd", "fleet") + res, err := tc.RunCmd("cvd", "fleet") if err != nil { return "", fmt.Errorf("failed to run `cvd fleet`") } re := regexp.MustCompile(`"metrics_dir" : "(.*)",`) - matches := re.FindStringSubmatch(stdOut) + matches := re.FindStringSubmatch(res.Stdout) if len(matches) != 2 { return "", fmt.Errorf("failed to find metrics directory") } @@ -346,7 +352,7 @@ func (tc *TestContext) SetUp(t *testing.T) { log.Printf("Initializing %s test...", tc.t.Name()) log.Printf("Cleaning up any pre-existing instances...") - if _, _, err := tc.RunCmd(tc.TargetBin(), "reset", "-y"); err != nil { + if _, err := tc.RunCmd(tc.TargetBin(), "reset", "-y"); err != nil { log.Printf("Failed to cleanup any pre-existing instances: %w", err) } log.Printf("Finished cleaning up any pre-existing instances!") @@ -428,7 +434,7 @@ func (tc *TestContext) TearDown() { matches, err := filepath.Glob(path.Join(tc.tempdir, pattern)) if err == nil { for _, file := range matches { - _, _, err := tc.RunCmd("cp", "--dereference", file, testoutdir) + _, err := tc.RunCmd("cp", "--dereference", file, testoutdir) if err != nil { log.Printf("failed to copy %s to %s: %w", file, testoutdir, err) } @@ -447,7 +453,7 @@ func (tc *TestContext) TearDown() { err := os.MkdirAll(outinstancedir, os.ModePerm) if err == nil { logdir := path.Join(instancedir, "logs") - _, _, err := runCmdWithContextEnv(context.TODO(), []string{"cp", "-r", "--dereference", logdir, outinstancedir}, map[string]string{}) + _, err := runCmdWithContextEnv(context.TODO(), []string{"cp", "-r", "--dereference", logdir, outinstancedir}, map[string]string{}) if err != nil { log.Printf("failed to copy %s to %s: %w", logdir, outinstancedir, err) } @@ -463,7 +469,7 @@ func (tc *TestContext) TearDown() { outmetricsdir := path.Join(testoutdir, "metrics_files") err := os.MkdirAll(outmetricsdir, os.ModePerm) if err == nil { - _, _, err := runCmdWithContextEnv(context.TODO(), []string{"cp", "-r", "--dereference", metricsdir, outmetricsdir}, map[string]string{}) + _, err := runCmdWithContextEnv(context.TODO(), []string{"cp", "-r", "--dereference", metricsdir, outmetricsdir}, map[string]string{}) if err != nil { log.Printf("failed to copy %s to %s: %w", metricsdir, outmetricsdir, err) } @@ -588,7 +594,7 @@ func RunXts(t *testing.T, cuttlefishArgs FetchAndCreateArgs, xtsArgs XtsArgs) { "--log-level-display=INFO", } xtsCommand = append(xtsCommand, xtsArgs.XtsArgs...) - if _, _, err := tc.RunCmd(xtsCommand...); err != nil { + if _, err := tc.RunCmd(xtsCommand...); err != nil { t.Fatalf("failed to fully run XTS: %w", err) } log.Printf("Finished running XTS!") diff --git a/e2etests/cvd/cvd_powerwash_tests/main_test.go b/e2etests/cvd/cvd_powerwash_tests/main_test.go index c800b755698..e5161ab00ed 100644 --- a/e2etests/cvd/cvd_powerwash_tests/main_test.go +++ b/e2etests/cvd/cvd_powerwash_tests/main_test.go @@ -53,11 +53,11 @@ func TestCvdPowerwash(t *testing.T) { } const tmpFile = "/data/local/tmp/foo" - if _, _, err := c.RunCmd("adb", "shell", "touch", tmpFile); err != nil { + if _, err := c.RunCmd("adb", "shell", "touch", tmpFile); err != nil { t.Fatalf("failed to create %s: %w", tmpFile, err) } - if _, _, err := c.RunCmd("adb", "shell", "stat", tmpFile); err != nil { + if _, err := c.RunCmd("adb", "shell", "stat", tmpFile); err != nil { t.Fatal("failed to verify %s created: %w", err) } @@ -65,7 +65,7 @@ func TestCvdPowerwash(t *testing.T) { t.Fatal(err) } - if _, _, err := c.RunCmd("adb", "shell", "stat", tmpFile); err == nil { + if _, err := c.RunCmd("adb", "shell", "stat", tmpFile); err == nil { t.Fatal("failed to powerwash, %s still exists") } }) diff --git a/e2etests/cvd/display_tests/main_test.go b/e2etests/cvd/display_tests/main_test.go index f56d065b6e9..be15878fa4f 100644 --- a/e2etests/cvd/display_tests/main_test.go +++ b/e2etests/cvd/display_tests/main_test.go @@ -35,7 +35,7 @@ func addDisplay(c e2etests.TestContext, t *testing.T) { t.Fatal(err) } - if _, _, err := c.RunCmd(c.TargetBin(), "display", "add", "--display=width=500,height=500"); err != nil { + if _, err := c.RunCmd(c.TargetBin(), "display", "add", "--display=width=500,height=500"); err != nil { t.Fatal(err) } } @@ -55,7 +55,7 @@ func listDisplays(c e2etests.TestContext, t *testing.T) { t.Fatal(err) } - if _, _, err := c.RunCmd(c.TargetBin(), "display", "list"); err != nil { + if _, err := c.RunCmd(c.TargetBin(), "display", "list"); err != nil { t.Fatal(err) } } diff --git a/e2etests/cvd/env_tests/main_test.go b/e2etests/cvd/env_tests/main_test.go index aaf527b41b0..d304e256b90 100644 --- a/e2etests/cvd/env_tests/main_test.go +++ b/e2etests/cvd/env_tests/main_test.go @@ -36,7 +36,7 @@ func TestListEnvServices(t *testing.T) { t.Fatal(err) } - if _, _, err := c.RunCmd(c.TargetBin(), "env", "ls"); err != nil { + if _, err := c.RunCmd(c.TargetBin(), "env", "ls"); err != nil { t.Fatal(err) } } diff --git a/e2etests/cvd/graphics_detector_tests/main_test.go b/e2etests/cvd/graphics_detector_tests/main_test.go index e54a4f6d8f1..72aa11ea463 100644 --- a/e2etests/cvd/graphics_detector_tests/main_test.go +++ b/e2etests/cvd/graphics_detector_tests/main_test.go @@ -41,11 +41,11 @@ func TestLaunchingWithAutoEnablesGfxstream(t *testing.T) { t.Fatalf("failed to wait for Cuttlefish device to connect to adb: %w", err) } - output, _, err := c.RunCmd("adb", "shell", "getprop", "ro.hardware.egl") + res, err := c.RunCmd("adb", "shell", "getprop", "ro.hardware.egl") if err != nil { t.Fatalf("failed to get EGL sysprop: %w", err) } - output = strings.TrimSpace(output) + output := strings.TrimSpace(res.Stdout) if output != "emulation" { t.Errorf(`"ro.hardware.egl" was "%s"; expected "emulation"`, output) } diff --git a/e2etests/cvd/logs_tests/main_test.go b/e2etests/cvd/logs_tests/main_test.go index 4ad790f715f..bf0d22ce7dd 100644 --- a/e2etests/cvd/logs_tests/main_test.go +++ b/e2etests/cvd/logs_tests/main_test.go @@ -49,10 +49,11 @@ func TestPrintLogs(t *testing.T) { t.Fatal(err) } - stdOut, _, err := c.RunCmd(c.TargetBin(), "logs") + res, err := c.RunCmd(c.TargetBin(), "logs") if err != nil { t.Fatal(err) } + stdOut := res.Stdout listedNames := []string{} lines := strings.Split(stdOut, "\n") @@ -61,9 +62,9 @@ func TestPrintLogs(t *testing.T) { if len(fields) != 2 { t.Errorf("line %q does not match format ' '", line) } - _, stderr, err := c.RunCmd("stat", fields[1]) + res, err := c.RunCmd("stat", fields[1]) if err != nil { - t.Fatal(stderr) + t.Fatal(res.Stderr) } listedNames = append(listedNames, fields[0]) } @@ -74,10 +75,11 @@ func TestPrintLogs(t *testing.T) { } } - stdOut, _, err = c.RunCmd(c.TargetBin(), "logs", "--print", "launcher.log") + res, err = c.RunCmd(c.TargetBin(), "logs", "--print", "launcher.log") if err != nil { t.Fatal(err) } + stdOut = res.Stdout if len(stdOut) == 0 { t.Fatalf("empty launcher.log") } diff --git a/e2etests/cvd/media_tests/main_test.go b/e2etests/cvd/media_tests/main_test.go index 36837d11d85..1ba9d3a734a 100644 --- a/e2etests/cvd/media_tests/main_test.go +++ b/e2etests/cvd/media_tests/main_test.go @@ -52,11 +52,11 @@ func TestEmulatedCameraV4l2Compliance(t *testing.T) { t.Fatalf("failed to wait for Cuttlefish device to connect to adb: %w", err) } - if _, _, err := c.RunCmd("adb", "shell", "su", "0", "v4l2-ctl", "--list-devices"); err != nil { + if _, err := c.RunCmd("adb", "shell", "su", "0", "v4l2-ctl", "--list-devices"); err != nil { t.Fatalf("v4l2-ctl --list-devices failed: %w", err) } - if _, _, err := c.RunCmd("adb", "shell", "su", "0", "v4l2-compliance", "-d1", "-s"); err != nil { + if _, err := c.RunCmd("adb", "shell", "su", "0", "v4l2-compliance", "-d1", "-s"); err != nil { t.Fatalf("v4l2-compliance failed: %w", err) } }) diff --git a/e2etests/cvd/metrics_tests/main_test.go b/e2etests/cvd/metrics_tests/main_test.go index 46d03ec3452..59b6741a824 100644 --- a/e2etests/cvd/metrics_tests/main_test.go +++ b/e2etests/cvd/metrics_tests/main_test.go @@ -50,13 +50,13 @@ func TestMetrics(t *testing.T) { var metricsdir string err := func() error { - output, _, err := c.RunCmd(c.TargetBin(), "fleet") + res, err := c.RunCmd(c.TargetBin(), "fleet") if err != nil { return fmt.Errorf("failed to run `cvd fleet`") } re := regexp.MustCompile(`"metrics_dir" : "(.*)",`) - matches := re.FindStringSubmatch(output) + matches := re.FindStringSubmatch(res.Stdout) if len(matches) != 2 { return fmt.Errorf("failed to find metrics directory.") }