From 08b2d318b9ce3904bf6c8432edfcedaf352f123e Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Wed, 6 Jul 2022 08:52:46 +0200 Subject: testhelper: Add new `Unsetenv()` helper With Go 1.17 a new `t.Setenv()` helper was introduced that automatically restores the previous environment variable and that verifies that the current test is not labelled as parallel. We're about to migrate all callers to use it instead of `testhelper.ModifyEnvironment()`. One part that `testhelper.ModifyEnvironment()` does though is to unset an environment variable in case the given value is the empty string. And unfortunately, Go didn't introduce a `t.Unsetenv()` helper at the same time. Implement a new function `testhelper.Unsetenv()` that behaves the same as `t.Setenv()`, except that it unsets the environment variable. --- internal/testhelper/testhelper.go | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/internal/testhelper/testhelper.go b/internal/testhelper/testhelper.go index dacf805b3..c5af5b254 100644 --- a/internal/testhelper/testhelper.go +++ b/internal/testhelper/testhelper.go @@ -284,6 +284,21 @@ 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() + + // 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. // It returns paths to the file with the certificate and its private key. func GenerateCerts(t *testing.T) (string, string) { -- cgit v1.2.3 From e34689502c46f6693006e259ed94e96ec3fa09b4 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Wed, 6 Jul 2022 09:29:40 +0200 Subject: env: Rewrite tests to match modern best practices The tests for our environment helpers aren't quite matching our modern best practices. Furthermore, they're missing tests for the case where environment variables are not set at all. Refactor these tests to adapt. While at it, convert them to use the new `t.Setenv()` and `testhelper.Unsetenv()` helper functions. --- internal/helper/env/env_test.go | 310 +++++++++++++++++++++++++--------------- 1 file changed, 198 insertions(+), 112 deletions(-) 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) + } +} -- cgit v1.2.3 From bf214c40cdea0f390e885298d7cf74056c68c6f2 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Wed, 6 Jul 2022 09:05:17 +0200 Subject: global: Rewrite callers of `testhelper.ModifyEnvironment()` Rewrite callers of `testhelper.ModifyEnviroment()`, which is about to be removed in favor of either `t.Setenv()` or `testhelper.Unsetenv()`. --- .golangci.yml | 5 ++--- client/dial_test.go | 6 ++++-- cmd/gitaly-ssh/auth_test.go | 3 +-- internal/backup/backup_test.go | 3 ++- internal/command/command_test.go | 4 ++-- internal/git/command_factory_test.go | 12 ++++++------ internal/git/execution_environment_test.go | 20 ++++++++++---------- internal/gitaly/server/server_factory_test.go | 2 +- .../gitaly/service/repository/create_fork_test.go | 3 +-- internal/praefect/checks_test.go | 2 +- internal/supervisor/supervisor_test.go | 2 +- 11 files changed, 31 insertions(+), 31 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/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() -- cgit v1.2.3 From 6d3c36e60a3923c4c7d755caa4f454c19eb3de24 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Wed, 6 Jul 2022 09:48:33 +0200 Subject: testhelper: Remove `testhelper.ModifyEnvironment()` helper Remove the `testhelper.ModifyEnvironment()` helper. One should instead either use `t.Setenv()` or `testhelper.Unsetenv()`. --- internal/testhelper/testhelper.go | 17 ----------------- 1 file changed, 17 deletions(-) diff --git a/internal/testhelper/testhelper.go b/internal/testhelper/testhelper.go index c5af5b254..e917a7b79 100644 --- a/internal/testhelper/testhelper.go +++ b/internal/testhelper/testhelper.go @@ -267,23 +267,6 @@ 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) { - 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) - } -} - // Unsetenv unsets an environment variable. The variable will be restored after the test has // finished. func Unsetenv(t testing.TB, key string) { -- cgit v1.2.3