Merge branch 'pks-testhelper-unsetenv' into 'master'

testhelper: Replace `testhelper.ModifyEnvironment()`

See merge request gitlab-org/gitaly!4685

13 files changed, 240 insertions, 156 deletions

diff --git a/.golangci.yml b/.golangci.yml
index db6ed2d7e..31c722364 100644
--- a/.golangci.yml
+++ b/.golangci.yml
@@ -59,9 +59,8 @@ linters-settings:
       - ^context.Background$
       - ^context.TODO$
       # Tests should not set the bare environment functions, but instead use
-      # `testhelper.ModifyEnvironment()`. This function has sanity checks to
-      # verify we don't use `t.Parallel()` when setting envvars by using the
-      # `t.Setenv()` helper.
+      # `t.Setenv()` or `testhelper.Unsetenv()`. These functions have sanity
+      # checks to verify we don't use `t.Parallel()` when setting envvars.
       - ^os.Setenv$
       - ^os.Unsetenv$
   stylecheck:
diff --git a/client/dial_test.go b/client/dial_test.go
index f958fc13c..f488a3ede 100644
--- a/client/dial_test.go
+++ b/client/dial_test.go
@@ -128,8 +128,9 @@ func TestDial(t *testing.T) {
 			}
 
 			if tt.envSSLCertFile != "" {
-				testhelper.ModifyEnvironment(t, gitalyx509.SSLCertFile, tt.envSSLCertFile)
+				t.Setenv(gitalyx509.SSLCertFile, tt.envSSLCertFile)
 			}
+
 			ctx := testhelper.Context(t)
 
 			conn, err := Dial(tt.rawAddress, tt.dialOpts)
@@ -216,8 +217,9 @@ func TestDialSidechannel(t *testing.T) {
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
 			if tt.envSSLCertFile != "" {
-				testhelper.ModifyEnvironment(t, gitalyx509.SSLCertFile, tt.envSSLCertFile)
+				t.Setenv(gitalyx509.SSLCertFile, tt.envSSLCertFile)
 			}
+
 			ctx := testhelper.Context(t)
 
 			conn, err := DialSidechannel(ctx, tt.rawAddress, registry, tt.dialOpts)
diff --git a/cmd/gitaly-ssh/auth_test.go b/cmd/gitaly-ssh/auth_test.go
index a8cf5a6a3..007522314 100644
--- a/cmd/gitaly-ssh/auth_test.go
+++ b/cmd/gitaly-ssh/auth_test.go
@@ -96,8 +96,7 @@ func TestConnectivity(t *testing.T) {
 			name: "tls",
 			addr: func(t *testing.T, cfg config.Cfg) (string, string) {
 				certFile, keyFile := testhelper.GenerateCerts(t)
-
-				testhelper.ModifyEnvironment(t, x509.SSLCertFile, certFile)
+				t.Setenv(x509.SSLCertFile, certFile)
 
 				cfg.TLSListenAddr = "localhost:0"
 				cfg.TLS = config.TLS{
diff --git a/internal/backup/backup_test.go b/internal/backup/backup_test.go
index 3bcaad4c8..0ae30291f 100644
--- a/internal/backup/backup_test.go
+++ b/internal/backup/backup_test.go
@@ -602,8 +602,9 @@ func TestResolveSink(t *testing.T) {
 	} {
 		t.Run(tc.desc, func(t *testing.T) {
 			for k, v := range tc.envs {
-				testhelper.ModifyEnvironment(t, k, v)
+				t.Setenv(k, v)
 			}
+
 			sink, err := ResolveSink(ctx, tc.path)
 			if tc.errMsg != "" {
 				require.EqualError(t, err, tc.errMsg)
diff --git a/internal/command/command_test.go b/internal/command/command_test.go
index 4e2f80248..98569183f 100644
--- a/internal/command/command_test.go
+++ b/internal/command/command_test.go
@@ -116,7 +116,7 @@ func TestNew_exportedEnvironment(t *testing.T) {
 				t.Skip("System Integrity Protection prevents using dynamic linker (dyld) environment variables on macOS. https://apple.co/2XDH4iC")
 			}
 
-			testhelper.ModifyEnvironment(t, tc.key, tc.value)
+			t.Setenv(tc.key, tc.value)
 
 			var buf bytes.Buffer
 			cmd, err := New(ctx, exec.Command("/usr/bin/env"), WithStdout(&buf))
@@ -133,7 +133,7 @@ func TestNew_unexportedEnv(t *testing.T) {
 	ctx := testhelper.Context(t)
 
 	unexportedEnvKey, unexportedEnvVal := "GITALY_UNEXPORTED_ENV", "foobar"
-	testhelper.ModifyEnvironment(t, unexportedEnvKey, unexportedEnvVal)
+	t.Setenv(unexportedEnvKey, unexportedEnvVal)
 
 	var buf bytes.Buffer
 	cmd, err := New(ctx, exec.Command("/usr/bin/env"), WithStdout(&buf))
diff --git a/internal/git/command_factory_test.go b/internal/git/command_factory_test.go
index ede45c745..5868e539c 100644
--- a/internal/git/command_factory_test.go
+++ b/internal/git/command_factory_test.go
@@ -33,7 +33,7 @@ func TestGitCommandProxy(t *testing.T) {
 	}))
 	defer ts.Close()
 
-	testhelper.ModifyEnvironment(t, "http_proxy", ts.URL)
+	t.Setenv("http_proxy", ts.URL)
 
 	ctx := testhelper.Context(t)
 
@@ -146,8 +146,8 @@ func TestExecCommandFactory_NewWithDir(t *testing.T) {
 }
 
 func TestCommandFactory_ExecutionEnvironment(t *testing.T) {
-	testhelper.ModifyEnvironment(t, "GITALY_TESTING_GIT_BINARY", "")
-	testhelper.ModifyEnvironment(t, "GITALY_TESTING_BUNDLED_GIT_PATH", "")
+	testhelper.Unsetenv(t, "GITALY_TESTING_GIT_BINARY")
+	testhelper.Unsetenv(t, "GITALY_TESTING_BUNDLED_GIT_PATH")
 
 	ctx := testhelper.Context(t)
 
@@ -198,7 +198,7 @@ func TestCommandFactory_ExecutionEnvironment(t *testing.T) {
 	})
 
 	t.Run("set using GITALY_TESTING_GIT_BINARY", func(t *testing.T) {
-		testhelper.ModifyEnvironment(t, "GITALY_TESTING_GIT_BINARY", "/path/to/env_git")
+		t.Setenv("GITALY_TESTING_GIT_BINARY", "/path/to/env_git")
 
 		assertExecEnv(t, config.Cfg{
 			Git: config.Git{
@@ -234,7 +234,7 @@ func TestCommandFactory_ExecutionEnvironment(t *testing.T) {
 		bundledGitExecutable := filepath.Join(bundledGitDir, "gitaly-git"+suffix)
 		bundledGitRemoteExecutable := filepath.Join(bundledGitDir, "gitaly-git-remote-http"+suffix)
 
-		testhelper.ModifyEnvironment(t, "GITALY_TESTING_BUNDLED_GIT_PATH", bundledGitDir)
+		t.Setenv("GITALY_TESTING_BUNDLED_GIT_PATH", bundledGitDir)
 
 		t.Run("missing bin_dir", func(t *testing.T) {
 			_, _, err := git.NewExecCommandFactory(config.Cfg{Git: config.Git{}}, git.WithSkipHooks())
@@ -321,7 +321,7 @@ func TestCommandFactory_ExecutionEnvironment(t *testing.T) {
 	})
 
 	t.Run("doesn't exist in the system", func(t *testing.T) {
-		testhelper.ModifyEnvironment(t, "PATH", "")
+		testhelper.Unsetenv(t, "PATH")
 
 		_, _, err := git.NewExecCommandFactory(config.Cfg{}, git.WithSkipHooks())
 		require.EqualError(t, err, "setting up Git execution environment: could not set up any Git execution environments")
diff --git a/internal/git/execution_environment_test.go b/internal/git/execution_environment_test.go
index d617227eb..a82910fcb 100644
--- a/internal/git/execution_environment_test.go
+++ b/internal/git/execution_environment_test.go
@@ -16,7 +16,7 @@ import (
 func TestDistributedGitEnvironmentConstructor(t *testing.T) {
 	constructor := git.DistributedGitEnvironmentConstructor{}
 
-	testhelper.ModifyEnvironment(t, "GITALY_TESTING_GIT_BINARY", "")
+	testhelper.Unsetenv(t, "GITALY_TESTING_GIT_BINARY")
 
 	t.Run("empty configuration fails", func(t *testing.T) {
 		_, err := constructor.Construct(config.Cfg{})
@@ -37,7 +37,7 @@ func TestDistributedGitEnvironmentConstructor(t *testing.T) {
 	})
 
 	t.Run("empty configuration with environment override", func(t *testing.T) {
-		testhelper.ModifyEnvironment(t, "GITALY_TESTING_GIT_BINARY", "/foo/bar")
+		t.Setenv("GITALY_TESTING_GIT_BINARY", "/foo/bar")
 
 		execEnv, err := constructor.Construct(config.Cfg{})
 		require.NoError(t, err)
@@ -48,7 +48,7 @@ func TestDistributedGitEnvironmentConstructor(t *testing.T) {
 	})
 
 	t.Run("configuration overrides environment variable", func(t *testing.T) {
-		testhelper.ModifyEnvironment(t, "GITALY_TESTING_GIT_BINARY", "envvar")
+		t.Setenv("GITALY_TESTING_GIT_BINARY", "envvar")
 
 		execEnv, err := constructor.Construct(config.Cfg{
 			Git: config.Git{
@@ -64,7 +64,7 @@ func TestDistributedGitEnvironmentConstructor(t *testing.T) {
 }
 
 func TestBundledGitEnvironmentConstructor(t *testing.T) {
-	testhelper.ModifyEnvironment(t, "GITALY_TESTING_BUNDLED_GIT_PATH", "")
+	testhelper.Unsetenv(t, "GITALY_TESTING_BUNDLED_GIT_PATH")
 
 	constructor := git.BundledGitEnvironmentConstructor{}
 
@@ -147,13 +147,13 @@ func TestBundledGitEnvironmentConstructor(t *testing.T) {
 	})
 
 	t.Run("bundled Git path without binary directory fails", func(t *testing.T) {
-		testhelper.ModifyEnvironment(t, "GITALY_TESTING_BUNDLED_GIT_PATH", "/does/not/exist")
+		t.Setenv("GITALY_TESTING_BUNDLED_GIT_PATH", "/does/not/exist")
 		_, err := constructor.Construct(config.Cfg{})
 		require.Equal(t, errors.New("cannot use bundled binaries without bin path being set"), err)
 	})
 
 	t.Run("nonexistent bundled Git path via environment fails", func(t *testing.T) {
-		testhelper.ModifyEnvironment(t, "GITALY_TESTING_BUNDLED_GIT_PATH", "/does/not/exist")
+		t.Setenv("GITALY_TESTING_BUNDLED_GIT_PATH", "/does/not/exist")
 		_, err := constructor.Construct(config.Cfg{
 			BinDir: testhelper.TempDir(t),
 		})
@@ -163,7 +163,7 @@ func TestBundledGitEnvironmentConstructor(t *testing.T) {
 
 	t.Run("incomplete bundled Git environment fails", func(t *testing.T) {
 		bundledGitPath := seedDirWithExecutables(t, "gitaly-git", "gitaly-git-remote-http")
-		testhelper.ModifyEnvironment(t, "GITALY_TESTING_BUNDLED_GIT_PATH", bundledGitPath)
+		t.Setenv("GITALY_TESTING_BUNDLED_GIT_PATH", bundledGitPath)
 
 		_, err := constructor.Construct(config.Cfg{
 			BinDir: testhelper.TempDir(t),
@@ -174,7 +174,7 @@ func TestBundledGitEnvironmentConstructor(t *testing.T) {
 
 	t.Run("complete bundled Git environment populates binary directory", func(t *testing.T) {
 		bundledGitPath := seedDirWithExecutables(t, "gitaly-git", "gitaly-git-remote-http", "gitaly-git-http-backend")
-		testhelper.ModifyEnvironment(t, "GITALY_TESTING_BUNDLED_GIT_PATH", bundledGitPath)
+		t.Setenv("GITALY_TESTING_BUNDLED_GIT_PATH", bundledGitPath)
 
 		execEnv, err := constructor.Construct(config.Cfg{
 			BinDir: testhelper.TempDir(t),
@@ -230,7 +230,7 @@ func TestFallbackGitEnvironmentConstructor(t *testing.T) {
 	constructor := git.FallbackGitEnvironmentConstructor{}
 
 	t.Run("failing lookup of executable causes failure", func(t *testing.T) {
-		testhelper.ModifyEnvironment(t, "PATH", "/does/not/exist")
+		t.Setenv("PATH", "/does/not/exist")
 
 		_, err := constructor.Construct(config.Cfg{})
 		require.Equal(t, fmt.Errorf("%w: no git executable found in PATH", git.ErrNotConfigured), err)
@@ -241,7 +241,7 @@ func TestFallbackGitEnvironmentConstructor(t *testing.T) {
 		gitPath := filepath.Join(tempDir, "git")
 		require.NoError(t, os.WriteFile(gitPath, nil, 0o755))
 
-		testhelper.ModifyEnvironment(t, "PATH", "/does/not/exist:"+tempDir)
+		t.Setenv("PATH", "/does/not/exist:"+tempDir)
 
 		execEnv, err := constructor.Construct(config.Cfg{})
 		require.NoError(t, err)
diff --git a/internal/gitaly/server/server_factory_test.go b/internal/gitaly/server/server_factory_test.go
index a0cfafd1f..8053b5a1e 100644
--- a/internal/gitaly/server/server_factory_test.go
+++ b/internal/gitaly/server/server_factory_test.go
@@ -148,7 +148,7 @@ func TestGitalyServerFactory(t *testing.T) {
 	})
 
 	t.Run("logging check", func(t *testing.T) {
-		testhelper.ModifyEnvironment(t, "GITALY_LOG_REQUEST_METHOD_ALLOW_PATTERN", ".")
+		t.Setenv("GITALY_LOG_REQUEST_METHOD_ALLOW_PATTERN", ".")
 
 		cfg := testcfg.Build(t)
 		logger, hook := test.NewNullLogger()
diff --git a/internal/gitaly/service/repository/create_fork_test.go b/internal/gitaly/service/repository/create_fork_test.go
index d4ccd2994..a25c83aa5 100644
--- a/internal/gitaly/service/repository/create_fork_test.go
+++ b/internal/gitaly/service/repository/create_fork_test.go
@@ -222,8 +222,7 @@ func TestCreateFork_targetExists(t *testing.T) {
 
 func injectCustomCATestCerts(t *testing.T) (*x509.CertPool, config.TLS) {
 	certFile, keyFile := testhelper.GenerateCerts(t)
-
-	testhelper.ModifyEnvironment(t, gitalyx509.SSLCertFile, certFile)
+	t.Setenv(gitalyx509.SSLCertFile, certFile)
 
 	caPEMBytes := testhelper.MustReadFile(t, certFile)
 	pool := x509.NewCertPool()
diff --git a/internal/helper/env/env_test.go b/internal/helper/env/env_test.go
index d38cf0362..5a83ebbe4 100644
--- a/internal/helper/env/env_test.go
+++ b/internal/helper/env/env_test.go
@@ -1,208 +1,288 @@
 package env_test
 
 import (
-	"errors"
 	"fmt"
 	"strconv"
 	"testing"
 	"time"
 
-	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/helper/env"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/testhelper"
 )
 
 func TestGetBool(t *testing.T) {
+	const envvar = "TEST_BOOL"
+
 	for _, tc := range []struct {
-		value         string
+		desc          string
+		setup         func(t *testing.T)
 		fallback      bool
-		expected      bool
-		expectedErrIs error
+		expectedValue bool
+		expectedErr   error
 	}{
 		{
-			value:    "true",
-			expected: true,
+			desc:          "explicitly true",
+			setup:         setupEnv(envvar, "true"),
+			expectedValue: true,
+		},
+		{
+			desc:          "explicitly false",
+			setup:         setupEnv(envvar, "false"),
+			expectedValue: false,
 		},
 		{
-			value:    "false",
-			expected: false,
+			desc:          "explicitly 1",
+			setup:         setupEnv(envvar, "1"),
+			expectedValue: true,
 		},
 		{
-			value:    "1",
-			expected: true,
+			desc:          "explicitly 0",
+			setup:         setupEnv(envvar, "0"),
+			expectedValue: false,
+		},
+		{
+			desc: "missing value",
+			setup: func(t *testing.T) {
+				testhelper.Unsetenv(t, envvar)
+			},
+			expectedValue: false,
 		},
 		{
-			value:    "0",
-			expected: false,
+			desc: "missing value with fallback",
+			setup: func(t *testing.T) {
+				testhelper.Unsetenv(t, envvar)
+			},
+			fallback:      true,
+			expectedValue: true,
 		},
 		{
-			value:    "",
-			expected: false,
+			desc:          "empty value",
+			setup:         setupEnv(envvar, ""),
+			expectedValue: false,
 		},
 		{
-			value:    "",
-			fallback: true,
-			expected: true,
+			desc:          "empty value with fallback",
+			setup:         setupEnv(envvar, ""),
+			fallback:      true,
+			expectedValue: true,
 		},
 		{
-			value:         "bad",
-			expected:      false,
-			expectedErrIs: strconv.ErrSyntax,
+			desc:          "invalid value",
+			setup:         setupEnv(envvar, "bad"),
+			expectedValue: false,
+			expectedErr: fmt.Errorf("get bool TEST_BOOL: %w", &strconv.NumError{
+				Func: "ParseBool",
+				Num:  "bad",
+				Err:  strconv.ErrSyntax,
+			}),
 		},
 		{
-			value:         "bad",
+			desc:          "invalid value with fallback",
+			setup:         setupEnv(envvar, "bad"),
 			fallback:      true,
-			expected:      true,
-			expectedErrIs: strconv.ErrSyntax,
+			expectedValue: true,
+			expectedErr: fmt.Errorf("get bool TEST_BOOL: %w", &strconv.NumError{
+				Func: "ParseBool",
+				Num:  "bad",
+				Err:  strconv.ErrSyntax,
+			}),
 		},
 	} {
-		t.Run(fmt.Sprintf("value=%s,fallback=%t", tc.value, tc.fallback), func(t *testing.T) {
-			testhelper.ModifyEnvironment(t, "TEST_BOOL", tc.value)
-
-			result, err := env.GetBool("TEST_BOOL", tc.fallback)
-
-			if tc.expectedErrIs != nil {
-				assert.Error(t, err)
-				assert.True(t, errors.Is(err, tc.expectedErrIs), err)
-			} else {
-				assert.NoError(t, err)
-			}
+		t.Run(tc.desc, func(t *testing.T) {
+			tc.setup(t)
 
-			assert.Equal(t, tc.expected, result)
+			value, err := env.GetBool(envvar, tc.fallback)
+			require.Equal(t, tc.expectedErr, err)
+			require.Equal(t, tc.expectedValue, value)
 		})
 	}
 }
 
 func TestGetInt(t *testing.T) {
+	const envvar = "TEST_INT"
+
 	for _, tc := range []struct {
-		value         string
+		desc          string
+		setup         func(t *testing.T)
 		fallback      int
-		expected      int
-		expectedErrIs error
+		expectedValue int
+		expectedErr   error
 	}{
 		{
-			value:    "3",
-			expected: 3,
+			desc:          "valid number",
+			setup:         setupEnv(envvar, "3"),
+			expectedValue: 3,
+		},
+		{
+			desc: "unset value",
+			setup: func(t *testing.T) {
+				testhelper.Unsetenv(t, envvar)
+			},
+			expectedValue: 0,
+		},
+		{
+			desc: "unset value with fallback",
+			setup: func(t *testing.T) {
+				testhelper.Unsetenv(t, envvar)
+			},
+			fallback:      3,
+			expectedValue: 3,
 		},
 		{
-			value:    "",
-			expected: 0,
+			desc:          "empty value",
+			setup:         setupEnv(envvar, ""),
+			expectedValue: 0,
 		},
 		{
-			value:    "",
-			fallback: 3,
-			expected: 3,
+			desc:          "empty value with fallback",
+			setup:         setupEnv(envvar, ""),
+			fallback:      3,
+			expectedValue: 3,
 		},
 		{
-			value:         "bad",
-			expected:      0,
-			expectedErrIs: strconv.ErrSyntax,
+			desc:          "invalid value",
+			setup:         setupEnv(envvar, "bad"),
+			expectedValue: 0,
+			expectedErr: fmt.Errorf("get int TEST_INT: %w", &strconv.NumError{
+				Func: "Atoi",
+				Num:  "bad",
+				Err:  strconv.ErrSyntax,
+			}),
 		},
 		{
-			value:         "bad",
+			desc:          "invalid value with fallback",
+			setup:         setupEnv(envvar, "bad"),
 			fallback:      3,
-			expected:      3,
-			expectedErrIs: strconv.ErrSyntax,
+			expectedValue: 3,
+			expectedErr: fmt.Errorf("get int TEST_INT: %w", &strconv.NumError{
+				Func: "Atoi",
+				Num:  "bad",
+				Err:  strconv.ErrSyntax,
+			}),
 		},
 	} {
-		t.Run(fmt.Sprintf("value=%s,fallback=%d", tc.value, tc.fallback), func(t *testing.T) {
-			testhelper.ModifyEnvironment(t, "TEST_INT", tc.value)
-
-			result, err := env.GetInt("TEST_INT", tc.fallback)
-
-			if tc.expectedErrIs != nil {
-				assert.Error(t, err)
-				assert.True(t, errors.Is(err, tc.expectedErrIs), err)
-			} else {
-				assert.NoError(t, err)
-			}
+		t.Run(tc.desc, func(t *testing.T) {
+			tc.setup(t)
 
-			assert.Equal(t, tc.expected, result)
+			value, err := env.GetInt(envvar, tc.fallback)
+			require.Equal(t, tc.expectedErr, err)
+			require.Equal(t, tc.expectedValue, value)
 		})
 	}
 }
 
 func TestGetDuration(t *testing.T) {
+	const envvar = "TEST_DURATION"
+
 	for _, tc := range []struct {
-		value       string
-		fallback    time.Duration
-		expected    time.Duration
-		expectedErr string
+		desc          string
+		setup         func(t *testing.T)
+		fallback      time.Duration
+		expectedValue time.Duration
+		expectedErr   error
 	}{
 		{
-			value:    "3m",
-			fallback: 0,
-			expected: 3 * time.Minute,
+			desc:          "valid duration",
+			setup:         setupEnv(envvar, "3m"),
+			fallback:      0,
+			expectedValue: 3 * time.Minute,
+		},
+		{
+			desc: "unset value",
+			setup: func(t *testing.T) {
+				testhelper.Unsetenv(t, envvar)
+			},
+			expectedValue: 0,
+		},
+		{
+			desc: "unset value with fallback",
+			setup: func(t *testing.T) {
+				testhelper.Unsetenv(t, envvar)
+			},
+			fallback:      3,
+			expectedValue: 3,
 		},
 		{
-			value:    "",
-			expected: 0,
+			desc:          "empty value",
+			setup:         setupEnv(envvar, ""),
+			expectedValue: 0,
 		},
 		{
-			value:    "",
-			fallback: 3,
-			expected: 3,
+			desc:          "empty value with fallback",
+			setup:         setupEnv(envvar, ""),
+			fallback:      3,
+			expectedValue: 3,
 		},
 		{
-			value:       "bad",
-			expected:    0,
-			expectedErr: `get duration TEST_DURATION: time: invalid duration "bad"`,
+			desc:          "invalid value",
+			setup:         setupEnv(envvar, "bad"),
+			expectedValue: 0,
+			expectedErr:   fmt.Errorf("get duration TEST_DURATION: %w", fmt.Errorf("time: invalid duration \"bad\"")),
 		},
 		{
-			value:       "bad",
-			fallback:    3,
-			expected:    3,
-			expectedErr: `get duration TEST_DURATION: time: invalid duration "bad"`,
+			desc:          "invalid value with fallback",
+			setup:         setupEnv(envvar, "bad"),
+			fallback:      3,
+			expectedValue: 3,
+			expectedErr:   fmt.Errorf("get duration TEST_DURATION: %w", fmt.Errorf("time: invalid duration \"bad\"")),
 		},
 	} {
-		t.Run(fmt.Sprintf("value=%s,fallback=%d", tc.value, tc.fallback), func(t *testing.T) {
-			testhelper.ModifyEnvironment(t, "TEST_DURATION", tc.value)
-
-			result, err := env.GetDuration("TEST_DURATION", tc.fallback)
-
-			if tc.expectedErr != "" {
-				assert.Error(t, err)
-				assert.EqualError(t, err, tc.expectedErr)
-			} else {
-				assert.NoError(t, err)
-			}
+		t.Run(tc.desc, func(t *testing.T) {
+			tc.setup(t)
 
-			assert.Equal(t, tc.expected, result)
+			value, err := env.GetDuration(envvar, tc.fallback)
+			require.Equal(t, tc.expectedErr, err)
+			require.Equal(t, tc.expectedValue, value)
 		})
 	}
 }
 
 func TestGetString(t *testing.T) {
+	const envvar = "TEST_STRING"
+
 	for _, tc := range []struct {
-		value    string
-		fallback string
-		expected string
+		desc          string
+		setup         func(t *testing.T)
+		fallback      string
+		expectedValue string
 	}{
 		{
-			value:    "Hello",
-			expected: "Hello",
+			desc:          "simple string",
+			setup:         setupEnv(envvar, "Hello"),
+			expectedValue: "Hello",
 		},
 		{
-			value:    "hello ",
-			expected: "hello",
+			desc:          "string with trailing whitespace",
+			setup:         setupEnv(envvar, "hello "),
+			expectedValue: "hello",
+		},
+		{
+			desc: "unset value",
+			setup: func(t *testing.T) {
+				testhelper.Unsetenv(t, envvar)
+			},
+			fallback:      "fallback value",
+			expectedValue: "fallback value",
 		},
 		{
-			fallback: "fallback value",
-			expected: "fallback value",
+			desc:          "empty value",
+			setup:         setupEnv(envvar, ""),
+			fallback:      "fallback value",
+			expectedValue: "fallback value",
 		},
 		{
-			value:    " ",
-			fallback: "fallback value",
-			expected: "",
+			desc:          "whitespace only",
+			setup:         setupEnv(envvar, " "),
+			fallback:      "fallback value",
+			expectedValue: "",
 		},
 	} {
-		t.Run(fmt.Sprintf("value=%s,fallback=%s", tc.value, tc.fallback), func(t *testing.T) {
-			testhelper.ModifyEnvironment(t, "TEST_STRING", tc.value)
-
-			result := env.GetString("TEST_STRING", tc.fallback)
+		t.Run(tc.desc, func(t *testing.T) {
+			tc.setup(t)
 
-			assert.Equal(t, tc.expected, result)
+			value := env.GetString(envvar, tc.fallback)
+			require.Equal(t, tc.expectedValue, value)
 		})
 	}
 }
@@ -262,3 +342,9 @@ func TestExtractKey(t *testing.T) {
 		})
 	}
 }
+
+func setupEnv(key, value string) func(t *testing.T) {
+	return func(t *testing.T) {
+		t.Setenv(key, value)
+	}
+}
diff --git a/internal/praefect/checks_test.go b/internal/praefect/checks_test.go
index cddaa7849..2c1c3c147 100644
--- a/internal/praefect/checks_test.go
+++ b/internal/praefect/checks_test.go
@@ -535,7 +535,7 @@ func TestNewClockSyncCheck(t *testing.T) {
 				return true, nil
 			},
 			setup: func(t *testing.T) {
-				testhelper.ModifyEnvironment(t, "NTP_HOST", "custom")
+				t.Setenv("NTP_HOST", "custom")
 			},
 		},
 	} {
diff --git a/internal/supervisor/supervisor_test.go b/internal/supervisor/supervisor_test.go
index f7bb2e990..0dd3d3658 100644
--- a/internal/supervisor/supervisor_test.go
+++ b/internal/supervisor/supervisor_test.go
@@ -179,7 +179,7 @@ func TestNewConfigFromEnv(t *testing.T) {
 	} {
 		t.Run(tc.desc, func(t *testing.T) {
 			for key, value := range tc.envvars {
-				testhelper.ModifyEnvironment(t, key, value)
+				t.Setenv(key, value)
 			}
 
 			config, err := NewConfigFromEnv()
diff --git a/internal/testhelper/testhelper.go b/internal/testhelper/testhelper.go
index dacf805b3..e917a7b79 100644
--- a/internal/testhelper/testhelper.go
+++ b/internal/testhelper/testhelper.go
@@ -267,21 +267,19 @@ func WriteExecutable(t testing.TB, path string, content []byte) string {
 	return path
 }
 
-// ModifyEnvironment will change an environment variable and revert it when the test completed.
-func ModifyEnvironment(t testing.TB, key string, value string) {
+// Unsetenv unsets an environment variable. The variable will be restored after the test has
+// finished.
+func Unsetenv(t testing.TB, key string) {
 	t.Helper()
 
-	oldValue, hasOldValue := os.LookupEnv(key)
-	if value == "" {
-		require.NoError(t, os.Unsetenv(key))
-		t.Cleanup(func() {
-			if hasOldValue {
-				require.NoError(t, os.Setenv(key, oldValue))
-			}
-		})
-	} else {
-		t.Setenv(key, value)
-	}
+	// We're first using `t.Setenv()` here due to two reasons: first, it will automitcally
+	// handle restoring the environment variable for us after the test has finished. And second,
+	// it performs a check whether we're running with `t.Parallel()`.
+	t.Setenv(key, "")
+
+	// And now we can unset the environment variable given that we know we're not running in a
+	// parallel test and where the cleanup function has been installed.
+	require.NoError(t, os.Unsetenv(key))
 }
 
 // GenerateCerts creates a certificate that can be used to establish TLS protected TCP connection.