From a56436cb15f54dae0f348f30df54d480f080ed8c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Wed, 8 Jun 2022 16:41:49 +0200 Subject: [PATCH] Use testing.T.Setenv instead of os.Setenv in tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ... to simplify and benefit from Go 1.17. In some cases, wrap tests in testing.T.Run() to decrease the scope, or to make the relationship between the test and the cleanup clearer. In some cases it's still a bit awkward because there is no testing.T.Unsetenv, but still worth it. Signed-off-by: Miloslav Trmač --- copy/sign_test.go | 4 +- openshift/openshift-copies_test.go | 15 +---- pkg/blobinfocache/default_test.go | 88 ++++++++++-------------------- pkg/docker/config/config_test.go | 74 ++++++++++--------------- signature/mechanism_test.go | 4 +- 5 files changed, 60 insertions(+), 125 deletions(-) diff --git a/copy/sign_test.go b/copy/sign_test.go index 5c93d25f..5fa2df32 100644 --- a/copy/sign_test.go +++ b/copy/sign_test.go @@ -3,7 +3,6 @@ package copy import ( "context" "io" - "os" "testing" "github.com/containers/image/v5/directory" @@ -36,8 +35,7 @@ func TestCreateSignature(t *testing.T) { t.Skipf("Signing not supported: %v", err) } - os.Setenv("GNUPGHOME", testGPGHomeDirectory) - defer os.Unsetenv("GNUPGHOME") + t.Setenv("GNUPGHOME", testGPGHomeDirectory) // Signing a directory: reference, which does not have a DockerReference(), fails. tempDir := t.TempDir() diff --git a/openshift/openshift-copies_test.go b/openshift/openshift-copies_test.go index f4fabb8e..8346db1a 100644 --- a/openshift/openshift-copies_test.go +++ b/openshift/openshift-copies_test.go @@ -1,7 +1,6 @@ package openshift import ( - "os" "testing" "github.com/stretchr/testify/assert" @@ -13,20 +12,10 @@ const fixtureKubeConfigPath = "testdata/admin.kubeconfig" // These are only smoke tests based on the skopeo integration test cluster. Error handling, non-trivial configuration merging, // and any other situations are not currently covered. -// Set up KUBECONFIG to point at the fixture, and return a handler to clean it up. +// Set up KUBECONFIG to point at the fixture. // Callers MUST NOT call testing.T.Parallel(). func setupKubeConfigForSerialTest(t *testing.T) { - // Environment is per-process, so this looks very unsafe; actually it seems fine because tests are not - // run in parallel unless they opt in by calling t.Parallel(). So don’t do that. - oldKC, hasKC := os.LookupEnv("KUBECONFIG") - t.Cleanup(func() { - if hasKC { - os.Setenv("KUBECONFIG", oldKC) - } else { - os.Unsetenv("KUBECONFIG") - } - }) - os.Setenv("KUBECONFIG", fixtureKubeConfigPath) + t.Setenv("KUBECONFIG", fixtureKubeConfigPath) } func TestClientConfigLoadingRules(t *testing.T) { diff --git a/pkg/blobinfocache/default_test.go b/pkg/blobinfocache/default_test.go index bc3ca8eb..83a3fea9 100644 --- a/pkg/blobinfocache/default_test.go +++ b/pkg/blobinfocache/default_test.go @@ -1,6 +1,7 @@ package blobinfocache import ( + "fmt" "os" "path/filepath" "testing" @@ -20,28 +21,8 @@ func TestBlobInfoCacheDir(t *testing.T) { const homeDir = "/fake/home/directory" const xdgDataHome = "/fake/home/directory/XDG" - // Environment is per-process, so this looks very unsafe; actually it seems fine because tests are not - // run in parallel unless they opt in by calling t.Parallel(). So don’t do that. - oldXRD, hasXRD := os.LookupEnv("XDG_RUNTIME_DIR") - defer func() { - if hasXRD { - os.Setenv("XDG_RUNTIME_DIR", oldXRD) - } else { - os.Unsetenv("XDG_RUNTIME_DIR") - } - }() - // FIXME: This should be a shared helper in internal/testing - oldHome, hasHome := os.LookupEnv("HOME") - defer func() { - if hasHome { - os.Setenv("HOME", oldHome) - } else { - os.Unsetenv("HOME") - } - }() - - os.Setenv("HOME", homeDir) - os.Setenv("XDG_DATA_HOME", xdgDataHome) + t.Setenv("HOME", homeDir) + t.Setenv("XDG_DATA_HOME", xdgDataHome) // The default paths and explicit overrides for _, c := range []struct { @@ -83,7 +64,7 @@ func TestBlobInfoCacheDir(t *testing.T) { } // Paths used by unprivileged users - for _, c := range []struct { + for caseIndex, c := range []struct { xdgDH, home, expected string }{ {"", homeDir, filepath.Join(homeDir, ".local", "share", "containers", "cache")}, // HOME only @@ -91,25 +72,28 @@ func TestBlobInfoCacheDir(t *testing.T) { {xdgDataHome, homeDir, filepath.Join(xdgDataHome, "containers", "cache")}, // both {"", "", ""}, // neither } { - if c.xdgDH != "" { - os.Setenv("XDG_DATA_HOME", c.xdgDH) - } else { - os.Unsetenv("XDG_DATA_HOME") - } - if c.home != "" { - os.Setenv("HOME", c.home) - } else { - os.Unsetenv("HOME") - } - for _, sys := range []*types.SystemContext{nil, {}} { - path, err := blobInfoCacheDir(sys, 1) - if c.expected != "" { - require.NoError(t, err) - assert.Equal(t, c.expected, path) - } else { - assert.Error(t, err) + t.Run(fmt.Sprintf("unprivileged %d", caseIndex), func(t *testing.T) { + // Always use t.Setenv() to ensure the environment variable is restored to the original value after the test. + // Then, in cases where the test needs the variable unset (not just set to empty), use a raw os.Unsetenv() + // to override the situation. (Sadly there isn’t a t.Unsetenv() as of Go 1.17.) + t.Setenv("XDG_DATA_HOME", c.xdgDH) + if c.xdgDH == "" { + os.Unsetenv("XDG_DATA_HOME") } - } + t.Setenv("HOME", c.home) + if c.home == "" { + os.Unsetenv("HOME") + } + for _, sys := range []*types.SystemContext{nil, {}} { + path, err := blobInfoCacheDir(sys, 1) + if c.expected != "" { + require.NoError(t, err) + assert.Equal(t, c.expected, path) + } else { + assert.Error(t, err) + } + } + }) } } @@ -123,26 +107,10 @@ func TestDefaultCache(t *testing.T) { assert.Equal(t, boltdb.New(filepath.Join(normalDir, blobInfoCacheFilename)), c) // Error running blobInfoCacheDir: - // Environment is per-process, so this looks very unsafe; actually it seems fine because tests are not - // run in parallel unless they opt in by calling t.Parallel(). So don’t do that. - oldXRD, hasXRD := os.LookupEnv("XDG_RUNTIME_DIR") - defer func() { - if hasXRD { - os.Setenv("XDG_RUNTIME_DIR", oldXRD) - } else { - os.Unsetenv("XDG_RUNTIME_DIR") - } - }() - // FIXME: This should be a shared helper in internal/testing - oldHome, hasHome := os.LookupEnv("HOME") - defer func() { - if hasHome { - os.Setenv("HOME", oldHome) - } else { - os.Unsetenv("HOME") - } - }() + // Use t.Setenv() just as a way to set up cleanup to original values; then os.Unsetenv() to test a situation where the values are not set. + t.Setenv("HOME", "") os.Unsetenv("HOME") + t.Setenv("XDG_DATA_HOME", "") os.Unsetenv("XDG_DATA_HOME") c = DefaultCache(nil) assert.IsType(t, memory.New(), c) diff --git a/pkg/docker/config/config_test.go b/pkg/docker/config/config_test.go index 5287454b..4277bba8 100644 --- a/pkg/docker/config/config_test.go +++ b/pkg/docker/config/config_test.go @@ -25,18 +25,7 @@ func TestGetPathToAuth(t *testing.T) { tmpDir := t.TempDir() - // Environment is per-process, so this looks very unsafe; actually it seems fine because tests are not - // run in parallel unless they opt in by calling t.Parallel(). So don’t do that. - oldXRD, hasXRD := os.LookupEnv("XDG_RUNTIME_DIR") - defer func() { - if hasXRD { - os.Setenv("XDG_RUNTIME_DIR", oldXRD) - } else { - os.Unsetenv("XDG_RUNTIME_DIR") - } - }() - - for _, c := range []struct { + for caseIndex, c := range []struct { sys *types.SystemContext os string xrd string @@ -61,40 +50,38 @@ func TestGetPathToAuth(t *testing.T) { {nil, linux, tmpDir + "/thisdoesnotexist", "", false}, {nil, darwin, tmpDir + "/thisdoesnotexist", darwinDefault, false}, } { - if c.xrd != "" { - os.Setenv("XDG_RUNTIME_DIR", c.xrd) - } else { - os.Unsetenv("XDG_RUNTIME_DIR") - } - res, lf, err := getPathToAuthWithOS(c.sys, c.os) - if c.expected == "" { - assert.Error(t, err) - } else { - require.NoError(t, err) - assert.Equal(t, c.expected, res) - assert.Equal(t, c.legacyFormat, lf) - } + t.Run(fmt.Sprintf("%d", caseIndex), func(t *testing.T) { + // Always use t.Setenv() to ensure XDG_RUNTIME_DIR is restored to the original value after the test. + // Then, in cases where the test needs XDG_RUNTIME_DIR unset (not just set to empty), use a raw os.Unsetenv() + // to override the situation. (Sadly there isn’t a t.Unsetenv() as of Go 1.17.) + t.Setenv("XDG_RUNTIME_DIR", c.xrd) + if c.xrd == "" { + os.Unsetenv("XDG_RUNTIME_DIR") + } + res, lf, err := getPathToAuthWithOS(c.sys, c.os) + if c.expected == "" { + assert.Error(t, err) + } else { + require.NoError(t, err) + assert.Equal(t, c.expected, res) + assert.Equal(t, c.legacyFormat, lf) + } + }) } } func TestGetAuth(t *testing.T) { - origXDG := os.Getenv("XDG_RUNTIME_DIR") tmpXDGRuntimeDir := t.TempDir() t.Logf("using temporary XDG_RUNTIME_DIR directory: %q", tmpXDGRuntimeDir) - // override XDG_RUNTIME_DIR - os.Setenv("XDG_RUNTIME_DIR", tmpXDGRuntimeDir) - defer os.Setenv("XDG_RUNTIME_DIR", origXDG) + t.Setenv("XDG_RUNTIME_DIR", tmpXDGRuntimeDir) // override PATH for executing credHelper curtDir, err := os.Getwd() require.NoError(t, err) origPath := os.Getenv("PATH") newPath := fmt.Sprintf("%s:%s", filepath.Join(curtDir, "testdata"), origPath) - os.Setenv("PATH", newPath) + t.Setenv("PATH", newPath) t.Logf("using PATH: %q", newPath) - defer func() { - os.Setenv("PATH", origPath) - }() tmpHomeDir := t.TempDir() t.Logf("using temporary home directory: %q", tmpHomeDir) @@ -405,12 +392,9 @@ func TestGetAuthPreferNewConfig(t *testing.T) { } func TestGetAuthFailsOnBadInput(t *testing.T) { - origXDG := os.Getenv("XDG_RUNTIME_DIR") tmpXDGRuntimeDir := t.TempDir() t.Logf("using temporary XDG_RUNTIME_DIR directory: %q", tmpXDGRuntimeDir) - // override XDG_RUNTIME_DIR - os.Setenv("XDG_RUNTIME_DIR", tmpXDGRuntimeDir) - defer os.Setenv("XDG_RUNTIME_DIR", origXDG) + t.Setenv("XDG_RUNTIME_DIR", tmpXDGRuntimeDir) tmpHomeDir := t.TempDir() t.Logf("using temporary home directory: %q", tmpHomeDir) @@ -466,11 +450,8 @@ func TestGetAllCredentials(t *testing.T) { require.NoError(t, err) origPath := os.Getenv("PATH") newPath := fmt.Sprintf("%s:%s", filepath.Join(path, "testdata"), origPath) - os.Setenv("PATH", newPath) + t.Setenv("PATH", newPath) t.Logf("using PATH: %q", newPath) - defer func() { - os.Setenv("PATH", origPath) - }() err = os.Chmod(filepath.Join(path, "testdata", "docker-credential-helper-registry"), os.ModePerm) require.NoError(t, err) sys := types.SystemContext{ @@ -480,11 +461,12 @@ func TestGetAllCredentials(t *testing.T) { } // Make sure that we can handle no-creds-found errors. - os.Setenv("DOCKER_CONFIG", filepath.Join(path, "testdata")) - authConfigs, err := GetAllCredentials(nil) - require.NoError(t, err) - require.Empty(t, authConfigs) - os.Unsetenv("DOCKER_CONFIG") + t.Run("no credentials found", func(t *testing.T) { + t.Setenv("DOCKER_CONFIG", filepath.Join(path, "testdata")) + authConfigs, err := GetAllCredentials(nil) + require.NoError(t, err) + require.Empty(t, authConfigs) + }) for _, data := range [][]struct { writeKey string diff --git a/signature/mechanism_test.go b/signature/mechanism_test.go index 95f24038..6c2b3393 100644 --- a/signature/mechanism_test.go +++ b/signature/mechanism_test.go @@ -97,9 +97,7 @@ func TestNewGPGSigningMechanismInDirectory(t *testing.T) { } // If we use the default directory mechanism, GNUPGHOME is respected. - origGNUPGHOME := os.Getenv("GNUPGHOME") - defer os.Setenv("GNUPGHOME", origGNUPGHOME) - os.Setenv("GNUPGHOME", testGPGHomeDirectory) + t.Setenv("GNUPGHOME", testGPGHomeDirectory) mech, err = newGPGSigningMechanismInDirectory("") require.NoError(t, err) defer mech.Close()