cmd/testwrapper: exit code 1 when go build fails (#9276)

Fixes #9275
Fixes #8586
Fixes tailscale/corp#13115

Signed-off-by: Paul Scott <paul@tailscale.com>

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,11 +289,22 @@ func main() {
 					failed = true
 				}
 			}
-		}
 			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
 		}

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
+}