Merge pull request #1826 from buildkite/test_tidyups_4

Tidy up t.Error/t.Fatal (part 4)

bootstrap/integration/artifact_integration_test.go

@@ -99,7 +99,7 @@ func TestArtifactsUploadAfterCommandHookFails(t *testing.T) {
 		AndExitWith(0)
 
 	if err := tester.Run(t, "BUILDKITE_ARTIFACT_PATHS=llamas.txt"); err == nil {
-		t.Fatal("tester.Run(BUILDKITE_ARTIFACT_PATHS=llamas.txt) = nil, want non-nil error")
+		t.Fatalf("tester.Run(BUILDKITE_ARTIFACT_PATHS=llamas.txt) = %v, want non-nil error", err)
 	}
 
 	tester.CheckMocks(t)

bootstrap/integration/checkout_integration_test.go

@@ -646,8 +646,8 @@ func TestCheckoutDoesNotRetryOnHookFailure(t *testing.T) {
 		}
 	})
 
-	if err = tester.Run(t); err == nil {
-		t.Fatal("Expected the bootstrap to fail")
+	if err := tester.Run(t); err == nil {
+		t.Fatalf("tester.Run(t) = %v, want non-nil error", err)
 	}
 
 	tester.CheckMocks(t)

bootstrap/shell/lookpath.go

@@ -38,7 +38,7 @@ func LookPath(file string, path string, fileExtensions string) (string, error) {
 		if err == nil {
 			return file, nil
 		}
-		return "", &exec.Error{file, err}
+		return "", &exec.Error{Name: file, Err: err}
 	}
 	for _, dir := range filepath.SplitList(path) {
 		if dir == "" {
@@ -50,5 +50,5 @@ func LookPath(file string, path string, fileExtensions string) (string, error) {
 			return path, nil
 		}
 	}
-	return "", &exec.Error{file, exec.ErrNotFound}
+	return "", &exec.Error{Name: file, Err: exec.ErrNotFound}
 }

hook/scriptwrapper_test.go

@@ -48,12 +48,12 @@ func TestRunningHookDetectsChangedEnvironment(t *testing.T) {
 	sh := shell.NewTestShell(t)
 
 	if err := sh.RunScript(ctx, wrapper.Path(), nil); err != nil {
-		t.Fatal(err)
+		t.Fatalf("sh.RunScript(ctx, %q, nil) = %v", wrapper.Path(), err)
 	}
 
 	changes, err := wrapper.Changes()
 	if err != nil {
-		t.Fatal(err)
+		t.Fatalf("wrapper.Changes() error = %v", err)
 	}
 
 	// Windows’ batch 'SET >' normalises environment variables case so we apply
@@ -181,7 +181,7 @@ func TestRunningHookDetectsChangedWorkingDirectory(t *testing.T) {
 
 	tempDir, err := os.MkdirTemp("", "hookwrapperdir")
 	if err != nil {
-		t.Fatal(err)
+		t.Fatalf(`os.MkdirTemp("", "hookwrapperdir") error = %v`, err)
 	}
 	defer os.RemoveAll(tempDir)
 
@@ -209,39 +209,40 @@ func TestRunningHookDetectsChangedWorkingDirectory(t *testing.T) {
 
 	sh := shell.NewTestShell(t)
 	if err := sh.Chdir(tempDir); err != nil {
-		t.Fatal(err)
+		t.Fatalf("sh.Chdir(%q) = %v", tempDir, err)
 	}
 
 	if err := sh.RunScript(ctx, wrapper.Path(), nil); err != nil {
-		t.Fatal(err)
+		t.Fatalf("sh.RunScript(ctx, %q, nil) = %v", wrapper.Path(), err)
 	}
 
 	changes, err := wrapper.Changes()
 	if err != nil {
-		t.Fatal(err)
+		t.Fatalf("wrapper.Changes() error = %v", err)
 	}
 
-	expected, err := filepath.EvalSymlinks(filepath.Join(tempDir, "mysubdir"))
+	wantChangesDir, err := filepath.EvalSymlinks(filepath.Join(tempDir, "mysubdir"))
 	if err != nil {
-		t.Fatal(err)
+		t.Fatalf("filepath.EvalSymlinks(mysubdir) error = %v", err)
 	}
 
 	afterWd, err := changes.GetAfterWd()
 	if err != nil {
-		t.Fatal(err)
+		t.Fatalf("changes.GetAfterWd() error = %v", err)
 	}
 
 	changesDir, err := filepath.EvalSymlinks(afterWd)
 	if err != nil {
-		t.Fatal(err)
+		t.Fatalf("filepath.EvalSymlinks(%q) error = %v", afterWd, err)
 	}
 
-	if changesDir != expected {
-		t.Fatalf("Expected working dir of %q, got %q", expected, changesDir)
+	if changesDir != wantChangesDir {
+		t.Fatalf("changesDir = %q, want %q", changesDir, wantChangesDir)
 	}
 
-	err = agent.CheckAndClose(t)
-	require.NoError(t, err)
+	if err := agent.CheckAndClose(t); err != nil {
+		t.Errorf("agent.CheckAndClose(t) = %v", err)
+	}
 }
 
 func newTestScriptWrapper(t *testing.T, script []string) *ScriptWrapper {

process/prefixer_test.go

@@ -7,24 +7,38 @@ import (
 	"testing"
 
 	"github.com/buildkite/agent/v3/process"
-	"github.com/stretchr/testify/assert"
+	"github.com/google/go-cmp/cmp"
 )
 
 func TestPrefixer(t *testing.T) {
-	for _, tc := range []struct {
-		input, expected string
+	tests := []struct {
+		input, want string
 	}{
-		{"alpacas\nllamas\n", "#1: alpacas\n#2: llamas\n#3: "},
-		{"blah\x1b[Kbler\x1bgh", "#1: blah\x1b[K#2: bler\x1bgh"},
-		{"blah\x1b[2Kblergh", "#1: blah\x1b[2K#2: blergh"},
-		{"blah\x1b[1B\x1b[1A\x1b[2Kblergh", "#1: blah\x1b[1B\x1b[1A\x1b[2K#2: blergh"},
-	} {
+		{
+			input: "alpacas\nllamas\n",
+			want:  "#1: alpacas\n#2: llamas\n#3: ",
+		},
+		{
+			input: "blah\x1b[Kbler\x1bgh",
+			want:  "#1: blah\x1b[K#2: bler\x1bgh",
+		},
+		{
+			input: "blah\x1b[2Kblergh",
+			want:  "#1: blah\x1b[2K#2: blergh",
+		},
+		{
+			input: "blah\x1b[1B\x1b[1A\x1b[2Kblergh",
+			want:  "#1: blah\x1b[1B\x1b[1A\x1b[2K#2: blergh",
+		},
+	}
+
+	for _, tc := range tests {
 		tc := tc
-		t.Run("", func(tt *testing.T) {
-			tt.Parallel()
+		t.Run(tc.input, func(t *testing.T) {
+			t.Parallel()
 
 			var lineCounter int32
-			var out = &bytes.Buffer{}
+			out := &bytes.Buffer{}
 
 			pw := process.NewPrefixer(out, func() string {
 				lineNumber := atomic.AddInt32(&lineCounter, 1)
@@ -33,14 +47,16 @@ func TestPrefixer(t *testing.T) {
 
 			n, err := pw.Write([]byte(tc.input))
 			if err != nil {
-				t.Fatal(err)
+				t.Fatalf("pw.Write([]byte(%q)) error = %v", tc.input, err)
 			}
 
-			if expected := len(tc.input); n != expected {
-				tt.Fatalf("Short write: %d vs expected %d", n, expected)
+			if got, want := n, len(tc.input); got != want {
+				t.Errorf("pw.Write([]byte(%q)) length = %d, want %d", tc.input, got, want)
 			}
 
-			assert.Equal(tt, tc.expected, out.String())
+			if diff := cmp.Diff(out.String(), tc.want); diff != "" {
+				t.Errorf("prefixer output diff (-got +want):\n%s", diff)
+			}
 		})
 	}
 }

process/process_test.go

@@ -32,15 +32,15 @@ func TestProcessOutput(t *testing.T) {
 
 	// wait for the process to finish
 	if err := p.Run(); err != nil {
-		t.Fatal(err)
+		t.Fatalf("p.Run() = %v", err)
 	}
 
-	if s := stdout.String(); s != `llamas1llamas2` {
-		t.Fatalf("Bad stdout, %q", s)
+	if got, want := stdout.String(), "llamas1llamas2"; got != want {
+		t.Errorf("stdout.String() = %q, want %q", got, want)
 	}
 
-	if s := stderr.String(); s != `alpacas1alpacas2` {
-		t.Fatalf("Bad stderr, %q", s)
+	if got, want := stderr.String(), "alpacas1alpacas2"; got != want {
+		t.Errorf("stderr.String() = %q, want %q", got, want)
 	}
 
 	assertProcessDoesntExist(t, p)
@@ -62,11 +62,11 @@ func TestProcessOutputPTY(t *testing.T) {
 
 	// wait for the process to finish
 	if err := p.Run(); err != nil {
-		t.Fatal(err)
+		t.Fatalf("p.Run() = %v", err)
 	}
 
-	if s := stdout.String(); s != `llamas1alpacas1llamas2alpacas2` {
-		t.Fatalf("Bad stdout, %q", s)
+	if got, want := stdout.String(), "llamas1alpacas1llamas2alpacas2"; got != want {
+		t.Errorf("stdout.String() = %q, want %q", got, want)
 	}
 
 	assertProcessDoesntExist(t, p)
@@ -83,10 +83,10 @@ func TestProcessInput(t *testing.T) {
 	})
 	// wait for the process to finish
 	if err := p.Run(); err != nil {
-		t.Fatal(err)
+		t.Fatalf("p.Run() = %v", err)
 	}
-	if expected, actual := "Hello World", stdout.String(); expected != actual {
-		t.Errorf("stdout expected %q, got %q", expected, actual)
+	if got, want := stdout.String(), "Hello World"; got != want {
+		t.Errorf("stdout.String() = %q, want %q", got, want)
 	}
 	assertProcessDoesntExist(t, p)
 }
@@ -114,18 +114,17 @@ func TestProcessRunsAndSignalsStartedAndStopped(t *testing.T) {
 
 	// wait for the process to finish
 	if err := p.Run(); err != nil {
-		t.Fatal(err)
+		t.Fatalf("p.Run() = %v", err)
 	}
 
 	// wait for our go routine to finish
 	wg.Wait()
 
-	if startedVal := atomic.LoadInt32(&started); startedVal != 1 {
-		t.Fatalf("Expected started to be 1, got %d", startedVal)
+	if got, want := atomic.LoadInt32(&started), int32(1); got != want {
+		t.Errorf("started = %d, want %d", got, want)
 	}
-
-	if doneVal := atomic.LoadInt32(&done); doneVal != 1 {
-		t.Fatalf("Expected done to be 1, got %d", doneVal)
+	if got, want := atomic.LoadInt32(&done), int32(1); got != want {
+		t.Errorf("done = %d, want %d", got, want)
 	}
 
 	assertProcessDoesntExist(t, p)
@@ -149,12 +148,12 @@ func TestProcessTerminatesWhenContextDoes(t *testing.T) {
 	}()
 
 	if err := p.Run(); err != nil {
-		t.Fatal(err)
+		t.Fatalf("p.Run() = %v", err)
 	}
 
-	if runtime.GOOS != `windows` {
-		if !p.WaitStatus().Signaled() {
-			t.Fatalf("Expected signaled")
+	if runtime.GOOS != "windows" {
+		if got, want := p.WaitStatus().Signaled(), true; got != want {
+			t.Fatalf("p.WaitStatus().Signaled() = %t, want %t", got, want)
 		}
 	}
 
@@ -167,12 +166,12 @@ func TestProcessInterrupts(t *testing.T) {
 		t.Skip("Works in windows, but not in docker")
 	}
 
-	b := &bytes.Buffer{}
+	stdout := &bytes.Buffer{}
 
 	p := process.New(logger.Discard, process.Config{
 		Path:   os.Args[0],
 		Env:    []string{"TEST_MAIN=tester-signal"},
-		Stdout: b,
+		Stdout: stdout,
 	})
 
 	var wg sync.WaitGroup
@@ -185,21 +184,19 @@ func TestProcessInterrupts(t *testing.T) {
 		// give the signal handler some time to install
 		time.Sleep(time.Millisecond * 50)
 
-		err := p.Interrupt()
-		if err != nil {
-			t.Error(err)
+		if err := p.Interrupt(); err != nil {
+			t.Errorf("p.Interrupt() = %v", err)
 		}
 	}()
 
 	if err := p.Run(); err != nil {
-		t.Fatal(err)
+		t.Fatalf("p.Run() = %v", err)
 	}
 
 	wg.Wait()
 
-	output := b.String()
-	if output != `SIG terminated` {
-		t.Fatalf("Bad output: %q", output)
+	if got, want := stdout.String(), "SIG terminated"; got != want {
+		t.Errorf("stdout.String() = %q, want %q", got, want)
 	}
 
 	assertProcessDoesntExist(t, p)
@@ -210,12 +207,12 @@ func TestProcessInterruptsWithCustomSignal(t *testing.T) {
 		t.Skip("Works in windows, but not in docker")
 	}
 
-	b := &bytes.Buffer{}
+	stdout := &bytes.Buffer{}
 
 	p := process.New(logger.Discard, process.Config{
 		Path:            os.Args[0],
 		Env:             []string{"TEST_MAIN=tester-signal"},
-		Stdout:          b,
+		Stdout:          stdout,
 		InterruptSignal: process.SIGINT,
 	})
 
@@ -229,21 +226,19 @@ func TestProcessInterruptsWithCustomSignal(t *testing.T) {
 		// give the signal handler some time to install
 		time.Sleep(time.Millisecond * 50)
 
-		err := p.Interrupt()
-		if err != nil {
-			t.Error(err)
+		if err := p.Interrupt(); err != nil {
+			t.Errorf("p.Interrupt() = %v", err)
 		}
 	}()
 
 	if err := p.Run(); err != nil {
-		t.Fatal(err)
+		t.Fatalf("p.Run() = %v", err)
 	}
 
 	wg.Wait()
 
-	output := b.String()
-	if output != `SIG interrupt` {
-		t.Fatalf("Bad output: %q", output)
+	if got, want := stdout.String(), "SIG interrupt"; got != want {
+		t.Errorf("stdout.String() = %q, want %q", got, want)
 	}
 
 	assertProcessDoesntExist(t, p)
@@ -261,7 +256,7 @@ func TestProcessSetsProcessGroupID(t *testing.T) {
 	})
 
 	if err := p.Run(); err != nil {
-		t.Fatal(err)
+		t.Fatalf("p.Run() = %v", err)
 	}
 
 	assertProcessDoesntExist(t, p)
@@ -274,8 +269,7 @@ func assertProcessDoesntExist(t *testing.T, p *process.Process) {
 	if err != nil {
 		return
 	}
-	signalErr := proc.Signal(syscall.Signal(0))
-	if signalErr == nil {
+	if err := proc.Signal(syscall.Signal(0)); err == nil {
 		t.Fatalf("Process %d exists and is running", p.Pid())
 	}
 }
@@ -287,7 +281,7 @@ func BenchmarkProcess(b *testing.B) {
 			Env:  []string{"TEST_MAIN=output"},
 		})
 		if err := proc.Run(); err != nil {
-			b.Fatal(err)
+			b.Fatalf("p.Run() = %v", err)
 		}
 	}
 }