From 96094cc07e82f9f0b52b24db6e7e0b283a4daed0 Mon Sep 17 00:00:00 2001 From: Paul Scott <408401+icio@users.noreply.github.com> Date: Thu, 7 Sep 2023 17:18:26 +0100 Subject: [PATCH] cmd/testwrapper: exit code 1 when go build fails (#9276) Fixes #9275 Fixes #8586 Fixes tailscale/corp#13115 Signed-off-by: Paul Scott --- cmd/testwrapper/testwrapper.go | 60 ++++++-- cmd/testwrapper/testwrapper_test.go | 218 ++++++++++++++++++++++++++++ 2 files changed, 262 insertions(+), 16 deletions(-) create mode 100644 cmd/testwrapper/testwrapper_test.go diff --git a/cmd/testwrapper/testwrapper.go b/cmd/testwrapper/testwrapper.go index fa825856f..9f5d13bbd 100644 --- a/cmd/testwrapper/testwrapper.go +++ b/cmd/testwrapper/testwrapper.go @@ -8,6 +8,7 @@ package main import ( + "bufio" "bytes" "context" "encoding/json" @@ -59,9 +60,10 @@ var debug = os.Getenv("TS_TESTWRAPPER_DEBUG") != "" // runTests runs the tests in pt and sends the results on ch. It sends a // testAttempt for each test and a final testAttempt per pkg with pkgFinished -// set to true. +// set to true. Package build errors will not emit a testAttempt (as no valid +// JSON is produced) but the [os/exec.ExitError] will be returned. // It calls close(ch) when it's done. -func runTests(ctx context.Context, attempt int, pt *packageTests, otherArgs []string, ch chan<- *testAttempt) { +func runTests(ctx context.Context, attempt int, pt *packageTests, otherArgs []string, ch chan<- *testAttempt) error { defer close(ch) args := []string{"test", "-json", pt.pattern} args = append(args, otherArgs...) @@ -87,17 +89,17 @@ func runTests(ctx context.Context, attempt int, pt *packageTests, otherArgs []st log.Printf("error starting test: %v", err) os.Exit(1) } - done := make(chan struct{}) + cmdErr := make(chan error, 1) go func() { - defer close(done) - cmd.Wait() + defer close(cmdErr) + cmdErr <- cmd.Wait() }() - jd := json.NewDecoder(r) + s := bufio.NewScanner(r) resultMap := make(map[string]map[string]*testAttempt) // pkg -> test -> testAttempt - for { + for s.Scan() { var goOutput goTestOutput - if err := jd.Decode(&goOutput); err != nil { + if err := json.Unmarshal(s.Bytes(), &goOutput); err != nil { if errors.Is(err, io.EOF) || errors.Is(err, os.ErrClosed) { break } @@ -107,7 +109,7 @@ func runTests(ctx context.Context, attempt int, pt *packageTests, otherArgs []st // The build error will be printed to stderr. // See: https://github.com/golang/go/issues/35169 if _, ok := err.(*json.SyntaxError); ok { - jd = json.NewDecoder(r) + fmt.Println(s.Text()) continue } panic(err) @@ -162,7 +164,10 @@ func runTests(ctx context.Context, attempt int, pt *packageTests, otherArgs []st } } } - <-done + if err := s.Err(); err != nil { + return err + } + return <-cmdErr } func main() { @@ -244,12 +249,24 @@ func main() { fmt.Printf("\n\nAttempt #%d: Retrying flaky tests:\n\n", thisRun.attempt) } - failed := false toRetry := make(map[string][]string) // pkg -> tests to retry for _, pt := range thisRun.tests { ch := make(chan *testAttempt) - go runTests(ctx, thisRun.attempt, pt, otherArgs, ch) + runErr := make(chan error, 1) + go func() { + defer close(runErr) + runErr <- runTests(ctx, thisRun.attempt, pt, otherArgs, ch) + }() + + var failed bool for tr := range ch { + // Go assigns the package name "command-line-arguments" when you + // `go test FILE` rather than `go test PKG`. It's more + // convenient for us to to specify files in tests, so fix tr.pkg + // so that subsequent testwrapper attempts run correctly. + if tr.pkg == "command-line-arguments" { + tr.pkg = pattern + } if tr.pkgFinished { if tr.outcome == "fail" && len(toRetry[tr.pkg]) == 0 { // If a package fails and we don't have any tests to @@ -272,10 +289,21 @@ func main() { failed = true } } - } - if failed { - fmt.Println("\n\nNot retrying flaky tests because non-flaky tests failed.") - os.Exit(1) + if failed { + fmt.Println("\n\nNot retrying flaky tests because non-flaky tests failed.") + os.Exit(1) + } + + // If there's nothing to retry and no non-retryable tests have + // failed then we've probably hit a build error. + if err := <-runErr; len(toRetry) == 0 && err != nil { + var exit *exec.ExitError + if errors.As(err, &exit) { + if code := exit.ExitCode(); code > -1 { + os.Exit(exit.ExitCode()) + } + } + } } if len(toRetry) == 0 { continue diff --git a/cmd/testwrapper/testwrapper_test.go b/cmd/testwrapper/testwrapper_test.go new file mode 100644 index 000000000..d7dbccd09 --- /dev/null +++ b/cmd/testwrapper/testwrapper_test.go @@ -0,0 +1,218 @@ +// Copyright (c) Tailscale Inc & AUTHORS +// SPDX-License-Identifier: BSD-3-Clause + +package main_test + +import ( + "bytes" + "errors" + "fmt" + "os" + "os/exec" + "path/filepath" + "sync" + "testing" +) + +var ( + buildPath string + buildErr error + buildOnce sync.Once +) + +func cmdTestwrapper(t *testing.T, args ...string) *exec.Cmd { + buildOnce.Do(func() { + buildPath, buildErr = buildTestWrapper() + }) + if buildErr != nil { + t.Fatalf("building testwrapper: %s", buildErr) + } + return exec.Command(buildPath, args...) +} + +func buildTestWrapper() (string, error) { + dir, err := os.MkdirTemp("", "testwrapper") + if err != nil { + return "", fmt.Errorf("making temp dir: %w", err) + } + _, err = exec.Command("go", "build", "-o", dir, ".").Output() + if err != nil { + return "", fmt.Errorf("go build: %w", err) + } + return filepath.Join(dir, "testwrapper"), nil +} + +func TestRetry(t *testing.T) { + t.Parallel() + + testfile := filepath.Join(t.TempDir(), "retry_test.go") + code := []byte(`package retry_test + +import ( + "os" + "testing" + "tailscale.com/cmd/testwrapper/flakytest" +) + +func TestOK(t *testing.T) {} + +func TestFlakeRun(t *testing.T) { + flakytest.Mark(t, "https://github.com/tailscale/tailscale/issues/0") // random issue + e := os.Getenv(flakytest.FlakeAttemptEnv) + if e == "" { + t.Skip("not running in testwrapper") + } + if e == "1" { + t.Fatal("First run in testwrapper, failing so that test is retried. This is expected.") + } +} +`) + if err := os.WriteFile(testfile, code, 0o644); err != nil { + t.Fatalf("writing package: %s", err) + } + + out, err := cmdTestwrapper(t, "-v", testfile).CombinedOutput() + if err != nil { + t.Fatalf("go run . %s: %s with output:\n%s", testfile, err, out) + } + + want := []byte("ok\t" + testfile + " [attempt=2]") + if !bytes.Contains(out, want) { + t.Fatalf("wanted output containing %q but got:\n%s", want, out) + } + + if okRuns := bytes.Count(out, []byte("=== RUN TestOK")); okRuns != 1 { + t.Fatalf("expected TestOK to be run once but was run %d times in output:\n%s", okRuns, out) + } + if flakeRuns := bytes.Count(out, []byte("=== RUN TestFlakeRun")); flakeRuns != 2 { + t.Fatalf("expected TestFlakeRun to be run twice but was run %d times in output:\n%s", flakeRuns, out) + } + + if testing.Verbose() { + t.Logf("success - output:\n%s", out) + } +} + +func TestNoRetry(t *testing.T) { + t.Parallel() + + testfile := filepath.Join(t.TempDir(), "noretry_test.go") + code := []byte(`package noretry_test + +import ( + "testing" + "tailscale.com/cmd/testwrapper/flakytest" +) + +func TestFlakeRun(t *testing.T) { + flakytest.Mark(t, "https://github.com/tailscale/tailscale/issues/0") // random issue + t.Error("shouldn't be retried") +} + +func TestAlwaysError(t *testing.T) { + t.Error("error") +} +`) + if err := os.WriteFile(testfile, code, 0o644); err != nil { + t.Fatalf("writing package: %s", err) + } + + out, err := cmdTestwrapper(t, "-v", testfile).Output() + if err == nil { + t.Fatalf("go run . %s: expected error but it succeeded with output:\n%s", testfile, out) + } + if code, ok := errExitCode(err); ok && code != 1 { + t.Fatalf("expected exit code 1 but got %d", code) + } + + want := []byte("Not retrying flaky tests because non-flaky tests failed.") + if !bytes.Contains(out, want) { + t.Fatalf("wanted output containing %q but got:\n%s", want, out) + } + + if flakeRuns := bytes.Count(out, []byte("=== RUN TestFlakeRun")); flakeRuns != 1 { + t.Fatalf("expected TestFlakeRun to be run once but was run %d times in output:\n%s", flakeRuns, out) + } + + if testing.Verbose() { + t.Logf("success - output:\n%s", out) + } +} + +func TestBuildError(t *testing.T) { + t.Parallel() + + // Construct our broken package. + testfile := filepath.Join(t.TempDir(), "builderror_test.go") + code := []byte("package builderror_test\n\nderp") + err := os.WriteFile(testfile, code, 0o644) + if err != nil { + t.Fatalf("writing package: %s", err) + } + + buildErr := []byte("builderror_test.go:3:1: expected declaration, found derp\nFAIL command-line-arguments [setup failed]") + + // Confirm `go test` exits with code 1. + goOut, err := exec.Command("go", "test", testfile).CombinedOutput() + if code, ok := errExitCode(err); !ok || code != 1 { + t.Fatalf("go test %s: expected error with exit code 0 but got: %v", testfile, err) + } + if !bytes.Contains(goOut, buildErr) { + t.Fatalf("go test %s: expected build error containing %q but got:\n%s", testfile, buildErr, goOut) + } + + // Confirm `testwrapper` exits with code 1. + twOut, err := cmdTestwrapper(t, testfile).CombinedOutput() + if code, ok := errExitCode(err); !ok || code != 1 { + t.Fatalf("testwrapper %s: expected error with exit code 0 but got: %v", testfile, err) + } + if !bytes.Contains(twOut, buildErr) { + t.Fatalf("testwrapper %s: expected build error containing %q but got:\n%s", testfile, buildErr, twOut) + } + + if testing.Verbose() { + t.Logf("success - output:\n%s", twOut) + } +} + +func TestTimeout(t *testing.T) { + t.Parallel() + + // Construct our broken package. + testfile := filepath.Join(t.TempDir(), "timeout_test.go") + code := []byte(`package noretry_test + +import ( + "testing" + "time" +) + +func TestTimeout(t *testing.T) { + time.Sleep(500 * time.Millisecond) +} +`) + err := os.WriteFile(testfile, code, 0o644) + if err != nil { + t.Fatalf("writing package: %s", err) + } + + out, err := cmdTestwrapper(t, testfile, "-timeout=20ms").CombinedOutput() + if code, ok := errExitCode(err); !ok || code != 1 { + t.Fatalf("testwrapper %s: expected error with exit code 0 but got: %v; output was:\n%s", testfile, err, out) + } + if want := "panic: test timed out after 20ms"; !bytes.Contains(out, []byte(want)) { + t.Fatalf("testwrapper %s: expected build error containing %q but got:\n%s", testfile, buildErr, out) + } + + if testing.Verbose() { + t.Logf("success - output:\n%s", out) + } +} + +func errExitCode(err error) (int, bool) { + var exit *exec.ExitError + if errors.As(err, &exit) { + return exit.ExitCode(), true + } + return 0, false +}