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()