command: Modernize tests to follow current code style

Modernize tests of the command package to follow our current code style.

7 files changed, 188 insertions, 188 deletions

diff --git a/internal/command/command_test.go b/internal/command/command_test.go
index 7611e10c6..0c48a18f9 100644
--- a/internal/command/command_test.go
+++ b/internal/command/command_test.go
@@ -6,7 +6,7 @@ import (
 	"fmt"
 	"io"
 	"os/exec"
-	"regexp"
+	"path/filepath"
 	"runtime"
 	"strings"
 	"testing"
@@ -22,27 +22,26 @@ import (
 	"gitlab.com/gitlab-org/gitaly/v15/internal/testhelper"
 )
 
-func TestMain(m *testing.M) {
-	testhelper.Run(m)
-}
+func TestNew_environment(t *testing.T) {
+	t.Parallel()
 
-func TestNewCommandExtraEnv(t *testing.T) {
 	ctx := testhelper.Context(t)
 
 	extraVar := "FOOBAR=123456"
-	buff := &bytes.Buffer{}
-	cmd, err := New(ctx, exec.Command("/usr/bin/env"), nil, buff, nil, extraVar)
+
+	var buf bytes.Buffer
+	cmd, err := New(ctx, exec.Command("/usr/bin/env"), nil, &buf, nil, extraVar)
 
 	require.NoError(t, err)
 	require.NoError(t, cmd.Wait())
 
-	require.Contains(t, strings.Split(buff.String(), "\n"), extraVar)
+	require.Contains(t, strings.Split(buf.String(), "\n"), extraVar)
 }
 
-func TestNewCommandExportedEnv(t *testing.T) {
+func TestNew_exportedEnvironment(t *testing.T) {
 	ctx := testhelper.Context(t)
 
-	testCases := []struct {
+	for _, tc := range []struct {
 		key   string
 		value string
 	}{
@@ -110,9 +109,7 @@ func TestNewCommandExportedEnv(t *testing.T) {
 			key:   "NO_PROXY",
 			value: "https://excluded:5000",
 		},
-	}
-
-	for _, tc := range testCases {
+	} {
 		t.Run(tc.key, func(t *testing.T) {
 			if tc.key == "LD_LIBRARY_PATH" && runtime.GOOS == "darwin" {
 				t.Skip("System Integrity Protection prevents using dynamic linker (dyld) environment variables on macOS. https://apple.co/2XDH4iC")
@@ -120,50 +117,41 @@ func TestNewCommandExportedEnv(t *testing.T) {
 
 			testhelper.ModifyEnvironment(t, tc.key, tc.value)
 
-			buff := &bytes.Buffer{}
-			cmd, err := New(ctx, exec.Command("/usr/bin/env"), nil, buff, nil)
+			var buf bytes.Buffer
+			cmd, err := New(ctx, exec.Command("/usr/bin/env"), nil, &buf, nil)
 			require.NoError(t, err)
 			require.NoError(t, cmd.Wait())
 
 			expectedEnv := fmt.Sprintf("%s=%s", tc.key, tc.value)
-			require.Contains(t, strings.Split(buff.String(), "\n"), expectedEnv)
+			require.Contains(t, strings.Split(buf.String(), "\n"), expectedEnv)
 		})
 	}
 }
 
-func TestNewCommandUnexportedEnv(t *testing.T) {
+func TestNew_unexportedEnv(t *testing.T) {
 	ctx := testhelper.Context(t)
 
 	unexportedEnvKey, unexportedEnvVal := "GITALY_UNEXPORTED_ENV", "foobar"
 	testhelper.ModifyEnvironment(t, unexportedEnvKey, unexportedEnvVal)
 
-	buff := &bytes.Buffer{}
-	cmd, err := New(ctx, exec.Command("/usr/bin/env"), nil, buff, nil)
-
+	var buf bytes.Buffer
+	cmd, err := New(ctx, exec.Command("/usr/bin/env"), nil, &buf, nil)
 	require.NoError(t, err)
 	require.NoError(t, cmd.Wait())
 
-	require.NotContains(t, strings.Split(buff.String(), "\n"), fmt.Sprintf("%s=%s", unexportedEnvKey, unexportedEnvVal))
+	require.NotContains(t, strings.Split(buf.String(), "\n"), fmt.Sprintf("%s=%s", unexportedEnvKey, unexportedEnvVal))
 }
 
-func TestRejectEmptyContextDone(t *testing.T) {
-	defer func() {
-		p := recover()
-		if p == nil {
-			t.Error("expected panic, got none")
-			return
-		}
-
-		if _, ok := p.(contextWithoutDonePanic); !ok {
-			panic(p)
-		}
-	}()
+func TestNew_rejectContextWithoutDone(t *testing.T) {
+	t.Parallel()
 
-	_, err := New(testhelper.ContextWithoutCancel(), exec.Command("true"), nil, nil, nil)
-	require.NoError(t, err)
+	require.PanicsWithValue(t, contextWithoutDonePanic("command spawned with context without Done() channel"), func() {
+		_, err := New(testhelper.ContextWithoutCancel(), exec.Command("true"), nil, nil, nil)
+		require.NoError(t, err)
+	})
 }
 
-func TestNewCommandTimeout(t *testing.T) {
+func TestNew_spawnTimeout(t *testing.T) {
 	ctx := testhelper.Context(t)
 
 	defer func(ch chan struct{}, t time.Duration) {
@@ -177,7 +165,6 @@ func TestNewCommandTimeout(t *testing.T) {
 	spawnTimeout := 200 * time.Millisecond
 	spawnConfig.Timeout = spawnTimeout
 
-	testDeadline := time.After(1 * time.Second)
 	tick := time.After(spawnTimeout / 2)
 
 	errCh := make(chan error)
@@ -186,27 +173,22 @@ func TestNewCommandTimeout(t *testing.T) {
 		errCh <- err
 	}()
 
-	var err error
-	timePassed := false
-
-wait:
-	for {
-		select {
-		case err = <-errCh:
-			break wait
-		case <-tick:
-			timePassed = true
-		case <-testDeadline:
-			t.Fatal("test timed out")
-		}
+	select {
+	case <-errCh:
+		require.FailNow(t, "expected spawning to be delayed")
+	case <-tick:
+		// This is the happy case: we expect spawning of the command to be delayed by up to
+		// 200ms until it finally times out.
 	}
 
-	require.True(t, timePassed, "time must have passed")
-	require.Error(t, err)
-	require.Contains(t, err.Error(), "process spawn timed out after")
+	// And after some time we expect that spawning of the command fails due to the configured
+	// timeout.
+	require.Equal(t, fmt.Errorf("process spawn timed out after 200ms"), <-errCh)
 }
 
-func TestCommand_Wait_interrupts_after_context_cancellation(t *testing.T) {
+func TestCommand_Wait_contextCancellationKillsCommand(t *testing.T) {
+	t.Parallel()
+
 	ctx, cancel := context.WithCancel(testhelper.Context(t))
 
 	cmd, err := New(ctx, exec.CommandContext(ctx, "sleep", "1h"), nil, nil, nil)
@@ -222,153 +204,201 @@ func TestCommand_Wait_interrupts_after_context_cancellation(t *testing.T) {
 	require.Equal(t, -1, s)
 }
 
-func TestNewCommandWithSetupStdin(t *testing.T) {
+func TestNew_setupStdin(t *testing.T) {
+	t.Parallel()
+
 	ctx := testhelper.Context(t)
 
-	value := "Test value"
-	output := bytes.NewBuffer(nil)
+	stdin := "Test value"
 
-	cmd, err := New(ctx, exec.Command("cat"), SetupStdin, nil, nil)
+	var buf bytes.Buffer
+	cmd, err := New(ctx, exec.Command("cat"), SetupStdin, &buf, nil)
 	require.NoError(t, err)
 
-	_, err = fmt.Fprintf(cmd, "%s", value)
+	_, err = fmt.Fprintf(cmd, "%s", stdin)
 	require.NoError(t, err)
 
-	// The output of the `cat` subprocess should exactly match its input
-	_, err = io.CopyN(output, cmd, int64(len(value)))
+	require.NoError(t, cmd.Wait())
+	require.Equal(t, stdin, buf.String())
+}
+
+func TestCommand_read(t *testing.T) {
+	t.Parallel()
+
+	ctx := testhelper.Context(t)
+
+	cmd, err := New(ctx, exec.Command("echo", "test value"), nil, nil, nil)
 	require.NoError(t, err)
-	require.Equal(t, value, output.String())
+
+	output, err := io.ReadAll(cmd)
+	require.NoError(t, err)
+	require.Equal(t, "test value\n", string(output))
 
 	require.NoError(t, cmd.Wait())
 }
 
-func TestNewCommandNullInArg(t *testing.T) {
+func TestNew_nulByteInArgument(t *testing.T) {
+	t.Parallel()
+
 	ctx := testhelper.Context(t)
 
-	_, err := New(ctx, exec.Command("sh", "-c", "hello\x00world"), nil, nil, nil)
-	require.Error(t, err)
-	require.EqualError(t, err, `detected null byte in command argument "hello\x00world"`)
+	cmd, err := New(ctx, exec.Command("sh", "-c", "hello\x00world"), nil, nil, nil)
+	require.Equal(t, fmt.Errorf("detected null byte in command argument %q", "hello\x00world"), err)
+	require.Nil(t, cmd)
 }
 
-func TestNewNonExistent(t *testing.T) {
+func TestNew_missingBinary(t *testing.T) {
+	t.Parallel()
+
 	ctx := testhelper.Context(t)
 
 	cmd, err := New(ctx, exec.Command("command-non-existent"), nil, nil, nil)
+	require.Equal(t, fmt.Errorf("GitCommand: start [command-non-existent]: exec: \"command-non-existent\": executable file not found in $PATH"), err)
 	require.Nil(t, cmd)
-	require.Error(t, err)
 }
 
-func TestCommandStdErr(t *testing.T) {
-	ctx := testhelper.Context(t)
-
-	var stdout, stderr bytes.Buffer
-	expectedMessage := `hello world\nhello world\nhello world\nhello world\nhello world\n`
+func TestCommand_stderrLogging(t *testing.T) {
+	t.Parallel()
 
-	logger := logrus.New()
-	logger.SetOutput(&stderr)
+	binaryPath := testhelper.WriteExecutable(t, filepath.Join(testhelper.TempDir(t), "script"), []byte(`#!/bin/bash
+		for i in {1..5}
+		do
+			echo 'hello world' 1>&2
+		done
+		exit 1
+	`))
 
+	logger, hook := test.NewNullLogger()
+	ctx := testhelper.Context(t)
 	ctx = ctxlogrus.ToContext(ctx, logrus.NewEntry(logger))
 
-	cmd, err := New(ctx, exec.Command("./testdata/stderr_script.sh"), nil, &stdout, nil)
+	var stdout bytes.Buffer
+	cmd, err := New(ctx, exec.Command(binaryPath), nil, &stdout, nil)
 	require.NoError(t, err)
-	require.Error(t, cmd.Wait())
 
-	assert.Empty(t, stdout.Bytes())
-	require.Equal(t, expectedMessage, extractLastMessage(stderr.String()))
+	require.EqualError(t, cmd.Wait(), "exit status 1")
+	require.Empty(t, stdout.Bytes())
+	require.Equal(t, strings.Repeat("hello world\n", 5), hook.LastEntry().Message)
 }
 
-func TestCommandStdErrLargeOutput(t *testing.T) {
-	ctx := testhelper.Context(t)
-
-	var stdout, stderr bytes.Buffer
+func TestCommand_stderrLoggingTruncation(t *testing.T) {
+	t.Parallel()
 
-	logger := logrus.New()
-	logger.SetOutput(&stderr)
+	binaryPath := testhelper.WriteExecutable(t, filepath.Join(testhelper.TempDir(t), "script"), []byte(`#!/bin/bash
+		for i in {1..1000}
+		do
+			printf '%06d zzzzzzzzzz\n' $i >&2
+		done
+		exit 1
+	`))
 
+	logger, hook := test.NewNullLogger()
+	ctx := testhelper.Context(t)
 	ctx = ctxlogrus.ToContext(ctx, logrus.NewEntry(logger))
 
-	cmd, err := New(ctx, exec.Command("./testdata/stderr_many_lines.sh"), nil, &stdout, nil)
+	var stdout bytes.Buffer
+	cmd, err := New(ctx, exec.Command(binaryPath), nil, &stdout, nil)
 	require.NoError(t, err)
-	require.Error(t, cmd.Wait())
 
-	assert.Empty(t, stdout.Bytes())
-	msg := strings.ReplaceAll(extractLastMessage(stderr.String()), "\\n", "\n")
-	require.LessOrEqual(t, len(msg), maxStderrBytes)
+	require.Error(t, cmd.Wait())
+	require.Empty(t, stdout.Bytes())
+	require.Len(t, hook.LastEntry().Message, maxStderrBytes)
 }
 
-func TestCommandStdErrBinaryNullBytes(t *testing.T) {
-	ctx := testhelper.Context(t)
-
-	var stdout, stderr bytes.Buffer
+func TestCommand_stderrLoggingWithNulBytes(t *testing.T) {
+	t.Parallel()
 
-	logger := logrus.New()
-	logger.SetOutput(&stderr)
+	binaryPath := testhelper.WriteExecutable(t, filepath.Join(testhelper.TempDir(t), "script"), []byte(`#!/bin/bash
+		dd if=/dev/zero bs=1000 count=1000 status=none >&2
+		exit 1
+	`))
 
+	logger, hook := test.NewNullLogger()
+	ctx := testhelper.Context(t)
 	ctx = ctxlogrus.ToContext(ctx, logrus.NewEntry(logger))
 
-	cmd, err := New(ctx, exec.Command("./testdata/stderr_binary_null.sh"), nil, &stdout, nil)
+	var stdout bytes.Buffer
+	cmd, err := New(ctx, exec.Command(binaryPath), nil, &stdout, nil)
 	require.NoError(t, err)
-	require.Error(t, cmd.Wait())
 
-	assert.Empty(t, stdout.Bytes())
-	msg := strings.SplitN(extractLastMessage(stderr.String()), "\\n", 2)[0]
-	require.Equal(t, strings.Repeat("\\x00", maxStderrLineLength), msg)
+	require.Error(t, cmd.Wait())
+	require.Empty(t, stdout.Bytes())
+	require.Equal(t, strings.Repeat("\x00", maxStderrLineLength), hook.LastEntry().Message)
 }
 
-func TestCommandStdErrLongLine(t *testing.T) {
-	ctx := testhelper.Context(t)
-
-	var stdout, stderr bytes.Buffer
+func TestCommand_stderrLoggingLongLine(t *testing.T) {
+	t.Parallel()
 
-	logger := logrus.New()
-	logger.SetOutput(&stderr)
+	binaryPath := testhelper.WriteExecutable(t, filepath.Join(testhelper.TempDir(t), "script"), []byte(`#!/bin/bash
+		printf 'a%.0s' {1..8192} >&2
+		printf '\n' >&2
+		printf 'b%.0s' {1..8192} >&2
+		exit 1
+	`))
 
+	logger, hook := test.NewNullLogger()
+	ctx := testhelper.Context(t)
 	ctx = ctxlogrus.ToContext(ctx, logrus.NewEntry(logger))
 
-	cmd, err := New(ctx, exec.Command("./testdata/stderr_repeat_a.sh"), nil, &stdout, nil)
+	var stdout bytes.Buffer
+	cmd, err := New(ctx, exec.Command(binaryPath), nil, &stdout, nil)
 	require.NoError(t, err)
-	require.Error(t, cmd.Wait())
 
-	assert.Empty(t, stdout.Bytes())
-	require.Contains(t, stderr.String(), fmt.Sprintf("%s\\n%s", strings.Repeat("a", maxStderrLineLength), strings.Repeat("b", maxStderrLineLength)))
+	require.Error(t, cmd.Wait())
+	require.Empty(t, stdout.Bytes())
+	require.Equal(t,
+		strings.Join([]string{
+			strings.Repeat("a", maxStderrLineLength),
+			strings.Repeat("b", maxStderrLineLength),
+		}, "\n"),
+		hook.LastEntry().Message,
+	)
 }
 
-func TestCommandStdErrMaxBytes(t *testing.T) {
-	ctx := testhelper.Context(t)
-
-	var stdout, stderr bytes.Buffer
-
-	logger := logrus.New()
-	logger.SetOutput(&stderr)
+func TestCommand_stderrLoggingMaxBytes(t *testing.T) {
+	t.Parallel()
+
+	binaryPath := testhelper.WriteExecutable(t, filepath.Join(testhelper.TempDir(t), "script"), []byte(`#!/bin/bash
+		# This script is used to test that a command writes at most maxBytes to stderr. It
+		# simulates the edge case where the logwriter has already written MaxStderrBytes-1
+		# (9999) bytes
+
+		# This edge case happens when 9999 bytes are written. To simulate this,
+		# stderr_max_bytes_edge_case has 4 lines of the following format:
+		#
+		# line1: 3333 bytes long
+		# line2: 3331 bytes
+		# line3: 3331 bytes
+		# line4: 1 byte
+		#
+		# The first 3 lines sum up to 9999 bytes written, since we write a 2-byte escaped
+		# "\n" for each \n we see. The 4th line can be any data.
+
+		printf 'a%.0s' {1..3333} >&2
+		printf '\n' >&2
+		printf 'a%.0s' {1..3331} >&2
+		printf '\n' >&2
+		printf 'a%.0s' {1..3331} >&2
+		printf '\na\n' >&2
+		exit 1
+	`))
 
+	logger, hook := test.NewNullLogger()
+	ctx := testhelper.Context(t)
 	ctx = ctxlogrus.ToContext(ctx, logrus.NewEntry(logger))
 
-	cmd, err := New(ctx, exec.Command("./testdata/stderr_max_bytes_edge_case.sh"), nil, &stdout, nil)
+	var stdout bytes.Buffer
+	cmd, err := New(ctx, exec.Command(binaryPath), nil, &stdout, nil)
 	require.NoError(t, err)
 	require.Error(t, cmd.Wait())
 
-	assert.Empty(t, stdout.Bytes())
-	message := extractLastMessage(stderr.String())
-	require.Equal(t, maxStderrBytes, len(strings.ReplaceAll(message, "\\n", "\n")))
-}
-
-var logMsgRegex = regexp.MustCompile(`msg="(.+?)"`)
-
-func extractLastMessage(logMessage string) string {
-	subMatchesAll := logMsgRegex.FindAllStringSubmatch(logMessage, -1)
-	if len(subMatchesAll) < 1 {
-		return ""
-	}
-
-	subMatches := subMatchesAll[len(subMatchesAll)-1]
-	if len(subMatches) != 2 {
-		return ""
-	}
-
-	return subMatches[1]
+	require.Empty(t, stdout.Bytes())
+	require.Len(t, hook.LastEntry().Message, maxStderrBytes)
 }
 
 func TestCommand_logMessage(t *testing.T) {
+	t.Parallel()
+
 	logger, hook := test.NewNullLogger()
 	logger.SetLevel(logrus.DebugLevel)
 
@@ -387,14 +417,19 @@ func TestCommand_logMessage(t *testing.T) {
 	assert.Equal(t, cgroupPath, logEntry.Data["command.cgroup_path"])
 }
 
-func TestNewCommandSpawnTokenMetrics(t *testing.T) {
-	spawnTokenAcquiringSeconds.Reset()
+func TestNew_commandSpawnTokenMetrics(t *testing.T) {
+	defer func(old func(time.Time) float64) {
+		getSpawnTokenAcquiringSeconds = old
+	}(getSpawnTokenAcquiringSeconds)
 
-	ctx := testhelper.Context(t)
 	getSpawnTokenAcquiringSeconds = func(t time.Time) float64 {
 		return 1
 	}
 
+	spawnTokenAcquiringSeconds.Reset()
+
+	ctx := testhelper.Context(t)
+
 	tags := grpcmwtags.NewTags()
 	tags.Set("grpc.request.fullMethod", "/test.Service/TestRPC")
 	ctx = grpcmwtags.SetInContext(ctx, tags)
@@ -408,7 +443,7 @@ func TestNewCommandSpawnTokenMetrics(t *testing.T) {
 # TYPE gitaly_command_spawn_token_acquiring_seconds_total counter
 gitaly_command_spawn_token_acquiring_seconds_total{cmd="echo",grpc_method="TestRPC",grpc_service="test.Service"} 1
 `
-	assert.NoError(
+	require.NoError(
 		t,
 		testutil.CollectAndCompare(
 			spawnTokenAcquiringSeconds,
diff --git a/internal/command/testdata/stderr_binary_null.sh b/internal/command/testdata/stderr_binary_null.sh
deleted file mode 100755
index e8e1d0baa..000000000
--- a/internal/command/testdata/stderr_binary_null.sh
+++ /dev/null
@@ -1,4 +0,0 @@
-#!/bin/bash
-
-dd if=/dev/zero bs=1000 count=1000 >&2
-exit 1
diff --git a/internal/command/testdata/stderr_many_lines.sh b/internal/command/testdata/stderr_many_lines.sh
deleted file mode 100755
index e8b51b646..000000000
--- a/internal/command/testdata/stderr_many_lines.sh
+++ /dev/null
@@ -1,9 +0,0 @@
-#!/bin/bash
-
-let x=0
-while [ $x -lt 100010 ]
-do
-	let x=x+1
-	printf '%06d zzzzzzzzzz\n' $x >&2
-done
-exit 1
diff --git a/internal/command/testdata/stderr_max_bytes_edge_case.sh b/internal/command/testdata/stderr_max_bytes_edge_case.sh
deleted file mode 100755
index 6e1454966..000000000
--- a/internal/command/testdata/stderr_max_bytes_edge_case.sh
+++ /dev/null
@@ -1,20 +0,0 @@
-#!/bin/bash
-
-# This script is used to test that a command writes at most maxBytes to stderr. It simulates the
-# edge case where the logwriter has already written MaxStderrBytes-1 (9999) bytes
-
-# This edge case happens when 9999 bytes are written. To simulate this, stderr_max_bytes_edge_case has 4 lines of the following format:
-# line1: 3333 bytes long
-# line2: 3331 bytes
-# line3: 3331 bytes
-# line4: 1 byte
-# The first 3 lines sum up to 9999 bytes written, since we write a 2-byte escaped `\n` for each \n we see.
-# The 4th line can be any data.
-
-printf 'a%.0s' {1..3333} >&2
-printf '\n' >&2
-printf 'a%.0s' {1..3331} >&2
-printf '\n' >&2
-printf 'a%.0s' {1..3331} >&2
-printf '\na\n' >&2
-exit 1
diff --git a/internal/command/testdata/stderr_repeat_a.sh b/internal/command/testdata/stderr_repeat_a.sh
deleted file mode 100755
index 8463929ae..000000000
--- a/internal/command/testdata/stderr_repeat_a.sh
+++ /dev/null
@@ -1,6 +0,0 @@
-#!/bin/bash
-
-printf 'a%.0s' {1..8192} >&2
-printf '\n' >&2
-printf 'b%.0s' {1..8192} >&2
-exit 1
diff --git a/internal/command/testdata/stderr_script.sh b/internal/command/testdata/stderr_script.sh
deleted file mode 100755
index 57abd97d0..000000000
--- a/internal/command/testdata/stderr_script.sh
+++ /dev/null
@@ -1,7 +0,0 @@
-#!/bin/bash
-
-for i in {1..5}
-do
-	echo 'hello world' 1>&2
-done
-exit 1
diff --git a/internal/command/testhelper_test.go b/internal/command/testhelper_test.go
new file mode 100644
index 000000000..e0398c2bd
--- /dev/null
+++ b/internal/command/testhelper_test.go
@@ -0,0 +1,11 @@
+package command
+
+import (
+	"testing"
+
+	"gitlab.com/gitlab-org/gitaly/v15/internal/testhelper"
+)
+
+func TestMain(m *testing.M) {
+	testhelper.Run(m)
+}