Use testing.T.Setenv instead of os.Setenv in tests

... 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č <mitr@redhat.com>

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)
+	t.Setenv("GNUPGHOME", testGPGHomeDirectory)
-	defer os.Unsetenv("GNUPGHOME")
 
 	// Signing a directory: reference, which does not have a DockerReference(), fails.
 	tempDir := t.TempDir()

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
+	t.Setenv("KUBECONFIG", fixtureKubeConfigPath)
-	// 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)
 }
 
 func TestClientConfigLoadingRules(t *testing.T) {

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
+	t.Setenv("HOME", homeDir)
-	// run in parallel unless they opt in by calling t.Parallel().  So don’t do that.
+	t.Setenv("XDG_DATA_HOME", xdgDataHome)
-	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)
 
 	// 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,14 +72,16 @@ func TestBlobInfoCacheDir(t *testing.T) {
 		{xdgDataHome, homeDir, filepath.Join(xdgDataHome, "containers", "cache")},       // both
 		{"", "", ""}, // neither
 	} {
-		if c.xdgDH != "" {
+		t.Run(fmt.Sprintf("unprivileged %d", caseIndex), func(t *testing.T) {
-			os.Setenv("XDG_DATA_HOME", c.xdgDH)
+			// Always use t.Setenv() to ensure the environment variable is restored to the original value after the test.
-		} else {
+			// 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")
 			}
-		if c.home != "" {
+			t.Setenv("HOME", c.home)
-			os.Setenv("HOME", c.home)
+			if c.home == "" {
-		} else {
 				os.Unsetenv("HOME")
 			}
 			for _, sys := range []*types.SystemContext{nil, {}} {
@@ -110,6 +93,7 @@ func TestBlobInfoCacheDir(t *testing.T) {
 					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
+	// 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.
-	// run in parallel unless they opt in by calling t.Parallel().  So don’t do that.
+	t.Setenv("HOME", "")
-	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.Unsetenv("HOME")
+	t.Setenv("XDG_DATA_HOME", "")
 	os.Unsetenv("XDG_DATA_HOME")
 	c = DefaultCache(nil)
 	assert.IsType(t, memory.New(), c)

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
+	for caseIndex, c := range []struct {
-	// 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 {
 		sys          *types.SystemContext
 		os           string
 		xrd          string
@@ -61,9 +50,12 @@ func TestGetPathToAuth(t *testing.T) {
 		{nil, linux, tmpDir + "/thisdoesnotexist", "", false},
 		{nil, darwin, tmpDir + "/thisdoesnotexist", darwinDefault, false},
 	} {
-		if c.xrd != "" {
+		t.Run(fmt.Sprintf("%d", caseIndex), func(t *testing.T) {
-			os.Setenv("XDG_RUNTIME_DIR", c.xrd)
+			// Always use t.Setenv() to ensure XDG_RUNTIME_DIR is restored to the original value after the test.
-		} else {
+			// 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)
@@ -74,27 +66,22 @@ func TestGetPathToAuth(t *testing.T) {
 				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
+	t.Setenv("XDG_RUNTIME_DIR", tmpXDGRuntimeDir)
-	os.Setenv("XDG_RUNTIME_DIR", tmpXDGRuntimeDir)
-	defer os.Setenv("XDG_RUNTIME_DIR", origXDG)
 
 	// 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
+	t.Setenv("XDG_RUNTIME_DIR", tmpXDGRuntimeDir)
-	os.Setenv("XDG_RUNTIME_DIR", tmpXDGRuntimeDir)
-	defer os.Setenv("XDG_RUNTIME_DIR", origXDG)
 
 	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"))
+	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)
-	os.Unsetenv("DOCKER_CONFIG")
+	})
 
 	for _, data := range [][]struct {
 		writeKey    string

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")
+	t.Setenv("GNUPGHOME", testGPGHomeDirectory)
-	defer os.Setenv("GNUPGHOME", origGNUPGHOME)
-	os.Setenv("GNUPGHOME", testGPGHomeDirectory)
 	mech, err = newGPGSigningMechanismInDirectory("")
 	require.NoError(t, err)
 	defer mech.Close()