Handle some TODOs in tests

Use more gotestyourself for `env.Patch`, and `icmd.RunCommand`

Signed-off-by: Vincent Demeester <vincent@sbr.pm>

cli/command/cli_test.go

@@ -14,6 +14,7 @@ import (
 	"github.com/docker/docker/client"
 	"github.com/gotestyourself/gotestyourself/assert"
 	is "github.com/gotestyourself/gotestyourself/assert/cmp"
+	"github.com/gotestyourself/gotestyourself/env"
 	"github.com/gotestyourself/gotestyourself/fs"
 	"github.com/pkg/errors"
 	"golang.org/x/net/context"
@@ -44,7 +45,7 @@ func TestNewAPIClientFromFlags(t *testing.T) {
 
 func TestNewAPIClientFromFlagsWithAPIVersionFromEnv(t *testing.T) {
 	customVersion := "v3.3.3"
-	defer patchEnvVariable(t, "DOCKER_API_VERSION", customVersion)()
+	defer env.Patch(t, "DOCKER_API_VERSION", customVersion)()
 
 	opts := &flags.CommonOptions{}
 	configFile := &configfile.ConfigFile{}
@@ -53,19 +54,6 @@ func TestNewAPIClientFromFlagsWithAPIVersionFromEnv(t *testing.T) {
 	assert.Check(t, is.Equal(customVersion, apiclient.ClientVersion()))
 }
 
-// TODO: use gotestyourself/env.Patch
-func patchEnvVariable(t *testing.T, key, value string) func() {
-	oldValue, ok := os.LookupEnv(key)
-	assert.NilError(t, os.Setenv(key, value))
-	return func() {
-		if !ok {
-			assert.NilError(t, os.Unsetenv(key))
-			return
-		}
-		assert.NilError(t, os.Setenv(key, oldValue))
-	}
-}
-
 type fakeClient struct {
 	client.Client
 	pingFunc   func() (types.Ping, error)
@@ -260,7 +248,7 @@ func TestOrchestratorSwitch(t *testing.T) {
 				version: defaultVersion,
 			}
 			if testcase.envOrchestrator != "" {
-				defer patchEnvVariable(t, "DOCKER_ORCHESTRATOR", testcase.envOrchestrator)()
+				defer env.Patch(t, "DOCKER_ORCHESTRATOR", testcase.envOrchestrator)()
 			}
 
 			cli := &DockerCli{client: apiclient, err: os.Stderr}

e2e/container/attach_test.go

@@ -1,6 +1,7 @@
 package container
 
 import (
+	"fmt"
 	"strings"
 	"testing"
 
@@ -19,8 +20,8 @@ func TestAttachExitCode(t *testing.T) {
 }
 
 func runBackgroundContainsWithExitCode(t *testing.T, exitcode int) string {
-	result := icmd.RunCmd(shell(t,
+	result := icmd.RunCommand("docker", "run", "-d", "-i", "--rm", fixtures.AlpineImage,
-		"docker run -d -i --rm %s sh -c 'read; exit %d'", fixtures.AlpineImage, exitcode))
+		"sh", "-c", fmt.Sprintf("read; exit %d", exitcode))
 	result.Assert(t, icmd.Success)
 	return strings.TrimSpace(result.Stdout())
 }

e2e/container/kill_test.go

@@ -32,17 +32,14 @@ func TestKillContainer(t *testing.T) {
 }
 
 func runBackgroundTop(t *testing.T) string {
-	result := icmd.RunCmd(shell(t,
+	result := icmd.RunCommand("docker", "run", "-d", fixtures.AlpineImage, "top")
-		"docker run -d %s top", fixtures.AlpineImage))
 	result.Assert(t, icmd.Success)
 	return strings.TrimSpace(result.Stdout())
 }
 
 func containerStatus(t *testing.T, containerID, status string) func(poll.LogT) poll.Result {
 	return func(poll.LogT) poll.Result {
-		result := icmd.RunCmd(
+		result := icmd.RunCommand("docker", "inspect", "-f", "{{ .State.Status }}", containerID)
-			shell(t, "docker inspect -f '{{ .State.Status }}' %s", containerID),
-		)
 		result.Assert(t, icmd.Success)
 		actual := strings.TrimSpace(result.Stdout())
 		if actual == status {

e2e/container/run_test.go

@@ -5,7 +5,6 @@ import (
 	"testing"
 
 	"github.com/docker/cli/e2e/internal/fixtures"
-	shlex "github.com/flynn-archive/go-shlex"
 	"github.com/gotestyourself/gotestyourself/assert"
 	is "github.com/gotestyourself/gotestyourself/assert/cmp"
 	"github.com/gotestyourself/gotestyourself/golden"
@@ -17,8 +16,8 @@ const registryPrefix = "registry:5000"
 func TestRunAttachedFromRemoteImageAndRemove(t *testing.T) {
 	image := createRemoteImage(t)
 
-	result := icmd.RunCmd(shell(t,
+	result := icmd.RunCommand("docker", "run", "--rm", image,
-		"docker run --rm %s echo this is output", image))
+		"echo", "this", "is", "output")
 
 	result.Assert(t, icmd.Success)
 	assert.Check(t, is.Equal("this is output\n", result.Stdout()))
@@ -54,10 +53,3 @@ func createRemoteImage(t *testing.T) string {
 	icmd.RunCommand("docker", "rmi", image).Assert(t, icmd.Success)
 	return image
 }
-
-// TODO: move to gotestyourself
-func shell(t *testing.T, format string, args ...interface{}) icmd.Cmd {
-	cmd, err := shlex.Split(fmt.Sprintf(format, args...))
-	assert.NilError(t, err)
-	return icmd.Cmd{Command: cmd}
-}

e2e/stack/remove_test.go

@@ -1,13 +1,10 @@
 package stack
 
 import (
-	"fmt"
 	"strings"
 	"testing"
 
 	"github.com/docker/cli/internal/test/environment"
-	shlex "github.com/flynn-archive/go-shlex"
-	"github.com/gotestyourself/gotestyourself/assert"
 	"github.com/gotestyourself/gotestyourself/golden"
 	"github.com/gotestyourself/gotestyourself/icmd"
 	"github.com/gotestyourself/gotestyourself/poll"
@@ -20,7 +17,7 @@ func TestRemove(t *testing.T) {
 	deployFullStack(t, stackname)
 	defer cleanupFullStack(t, stackname)
 
-	result := icmd.RunCmd(shell(t, "docker stack rm %s", stackname))
+	result := icmd.RunCommand("docker", "stack", "rm", stackname)
 
 	result.Assert(t, icmd.Expected{Err: icmd.None})
 	golden.Assert(t, result.Stdout(), "stack-remove-success.golden")
@@ -28,8 +25,8 @@ func TestRemove(t *testing.T) {
 
 func deployFullStack(t *testing.T, stackname string) {
 	// TODO: this stack should have full options not minimal options
-	result := icmd.RunCmd(shell(t,
+	result := icmd.RunCommand("docker", "stack", "deploy",
-		"docker stack deploy --compose-file=./testdata/full-stack.yml %s", stackname))
+		"--compose-file=./testdata/full-stack.yml", stackname)
 	result.Assert(t, icmd.Success)
 
 	poll.WaitOn(t, taskCount(stackname, 2), pollSettings)
@@ -66,10 +63,3 @@ func taskCount(stackname string, expected int) func(t poll.LogT) poll.Result {
 func lines(out string) int {
 	return len(strings.Split(strings.TrimSpace(out), "\n"))
 }
-
-// TODO: move to gotestyourself
-func shell(t *testing.T, format string, args ...interface{}) icmd.Cmd {
-	cmd, err := shlex.Split(fmt.Sprintf(format, args...))
-	assert.NilError(t, err)
-	return icmd.Cmd{Command: cmd}
-}