Skip to content

Commit

Permalink
Use testing.T.Setenv instead of os.Setenv in tests
Browse files Browse the repository at this point in the history
... 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>
  • Loading branch information
mtrmac committed Jun 14, 2022
1 parent 225404e commit 121b400
Show file tree
Hide file tree
Showing 5 changed files with 54 additions and 125 deletions.
4 changes: 1 addition & 3 deletions copy/sign_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@ package copy
import (
"context"
"io"
"os"
"testing"

"github.com/containers/image/v5/directory"
Expand Down Expand Up @@ -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()
Expand Down
15 changes: 2 additions & 13 deletions openshift/openshift-copies_test.go
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
package openshift

import (
"os"
"testing"

"github.com/stretchr/testify/assert"
Expand All @@ -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) {
Expand Down
85 changes: 25 additions & 60 deletions pkg/blobinfocache/default_test.go
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package blobinfocache

import (
"fmt"
"os"
"path/filepath"
"testing"
Expand All @@ -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 {
Expand Down Expand Up @@ -83,33 +64,33 @@ 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
{xdgDataHome, "", filepath.Join(xdgDataHome, "containers", "cache")}, // XDG_DATA_HOME only
{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) {
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)
}
}
})
}
}

Expand All @@ -123,26 +104,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)
Expand Down
71 changes: 25 additions & 46 deletions pkg/docker/config/config_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -61,40 +50,35 @@ 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) {
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)
Expand Down Expand Up @@ -405,12 +389,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)
Expand Down Expand Up @@ -466,11 +447,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{
Expand All @@ -480,11 +458,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
Expand Down
4 changes: 1 addition & 3 deletions signature/mechanism_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand Down

0 comments on commit 121b400

Please sign in to comment.