test spring-cleaning

This makes a quick pass through our tests;

Discard output/err
----------------------------------------------

Many tests were testing for error-conditions, but didn't discard output.
This produced a lot of noise when running the tests, and made it hard
to discover if there were actual failures, or if the output was expected.
For example:

    === RUN   TestConfigCreateErrors
    Error: "create" requires exactly 2 arguments.
    See 'create --help'.

    Usage:  create [OPTIONS] CONFIG file|- [flags]

    Create a config from a file or STDIN
    Error: "create" requires exactly 2 arguments.
    See 'create --help'.

    Usage:  create [OPTIONS] CONFIG file|- [flags]

    Create a config from a file or STDIN
    Error: error creating config
    --- PASS: TestConfigCreateErrors (0.00s)

And after discarding output:

    === RUN   TestConfigCreateErrors
    --- PASS: TestConfigCreateErrors (0.00s)

Use sub-tests where possible
----------------------------------------------

Some tests were already set-up to use test-tables, and even had a usable
name (or in some cases "error" to check for). Change them to actual sub-
tests. Same test as above, but now with sub-tests and output discarded:

    === RUN   TestConfigCreateErrors
    === RUN   TestConfigCreateErrors/requires_exactly_2_arguments
    === RUN   TestConfigCreateErrors/requires_exactly_2_arguments#01
    === RUN   TestConfigCreateErrors/error_creating_config
    --- PASS: TestConfigCreateErrors (0.00s)
        --- PASS: TestConfigCreateErrors/requires_exactly_2_arguments (0.00s)
        --- PASS: TestConfigCreateErrors/requires_exactly_2_arguments#01 (0.00s)
        --- PASS: TestConfigCreateErrors/error_creating_config (0.00s)
    PASS

It's not perfect in all cases (in the above, there's duplicate "expected"
errors, but Go conveniently adds "#1" for the duplicate). There's probably
also various tests I missed that could still use the same changes applied;
we can improve these in follow-ups.

Set cmd.Args to prevent test-failures
----------------------------------------------

When running tests from my IDE, it compiles the tests before running,
then executes the compiled binary to run the tests. Cobra doesn't like
that, because in that situation `os.Args` is taken as argument for the
command that's executed. The command that's tested now sees the test-
flags as arguments (`-test.v -test.run ..`), which causes various tests
to fail ("Command XYZ does not accept arguments").

    # compile the tests:
    go test -c -o foo.test

    # execute the test:
    ./foo.test -test.v -test.run TestFoo
    === RUN   TestFoo
    Error: "foo" accepts no arguments.

The Cobra maintainers ran into the same situation, and for their own
use have added a special case to ignore `os.Args` in these cases;
https://github.com/spf13/cobra/blob/v1.8.1/command.go#L1078-L1083

    args := c.args

    // Workaround FAIL with "go test -v" or "cobra.test -test.v", see docker#155
    if c.args == nil && filepath.Base(os.Args[0]) != "cobra.test" {
        args = os.Args[1:]
    }

Unfortunately, that exception is too specific (only checks for `cobra.test`),
so doesn't automatically fix the issue for other test-binaries. They did
provide a `cmd.SetArgs()` utility for this purpose
https://github.com/spf13/cobra/blob/v1.8.1/command.go#L276-L280

    // SetArgs sets arguments for the command. It is set to os.Args[1:] by default, if desired, can be overridden
    // particularly useful when testing.
    func (c *Command) SetArgs(a []string) {
        c.args = a
    }

And the fix is to explicitly set the command's args to an empty slice to
prevent Cobra from falling back to using `os.Args[1:]` as arguments.

    cmd := newSomeThingCommand()
    cmd.SetArgs([]string{})

Some tests already take this issue into account, and I updated some tests
for this, but there's likely many other ones that can use the same treatment.

Perhaps the Cobra maintainers would accept a contribution to make their
condition less specific and to look for binaries ending with a `.test`
suffix (which is what compiled binaries usually are named as).

Signed-off-by: Sebastiaan van Stijn <[email protected]>

cli/command/builder/prune_test.go

@@ -3,6 +3,7 @@ package builder
 import (
 	"context"
 	"errors"
+	"io"
 	"testing"
 
 	"github.com/docker/cli/internal/test"
@@ -19,5 +20,7 @@ func TestBuilderPromptTermination(t *testing.T) {
 		},
 	})
 	cmd := NewPruneCommand(cli)
+	cmd.SetOut(io.Discard)
+	cmd.SetErr(io.Discard)
 	test.TerminatePrompt(ctx, t, cmd, cli)
 }

cli/command/checkpoint/create_test.go

@@ -42,6 +42,7 @@ func TestCheckpointCreateErrors(t *testing.T) {
 		cmd := newCreateCommand(cli)
 		cmd.SetArgs(tc.args)
 		cmd.SetOut(io.Discard)
+		cmd.SetErr(io.Discard)
 		assert.ErrorContains(t, cmd.Execute(), tc.expectedError)
 	}
 }

cli/command/checkpoint/list_test.go

@@ -42,6 +42,7 @@ func TestCheckpointListErrors(t *testing.T) {
 		cmd := newListCommand(cli)
 		cmd.SetArgs(tc.args)
 		cmd.SetOut(io.Discard)
+		cmd.SetErr(io.Discard)
 		assert.ErrorContains(t, cmd.Execute(), tc.expectedError)
 	}
 }

cli/command/checkpoint/remove_test.go

@@ -41,6 +41,7 @@ func TestCheckpointRemoveErrors(t *testing.T) {
 		cmd := newRemoveCommand(cli)
 		cmd.SetArgs(tc.args)
 		cmd.SetOut(io.Discard)
+		cmd.SetErr(io.Discard)
 		assert.ErrorContains(t, cmd.Execute(), tc.expectedError)
 	}
 }

cli/command/config/create_test.go

@@ -43,14 +43,18 @@ func TestConfigCreateErrors(t *testing.T) {
 		},
 	}
 	for _, tc := range testCases {
-		cmd := newConfigCreateCommand(
-			test.NewFakeCli(&fakeClient{
-				configCreateFunc: tc.configCreateFunc,
-			}),
-		)
-		cmd.SetArgs(tc.args)
-		cmd.SetOut(io.Discard)
-		assert.ErrorContains(t, cmd.Execute(), tc.expectedError)
+		tc := tc
+		t.Run(tc.expectedError, func(t *testing.T) {
+			cmd := newConfigCreateCommand(
+				test.NewFakeCli(&fakeClient{
+					configCreateFunc: tc.configCreateFunc,
+				}),
+			)
+			cmd.SetArgs(tc.args)
+			cmd.SetOut(io.Discard)
+			cmd.SetErr(io.Discard)
+			assert.ErrorContains(t, cmd.Execute(), tc.expectedError)
+		})
 	}
 }
 

cli/command/config/inspect_test.go

@@ -61,6 +61,7 @@ func TestConfigInspectErrors(t *testing.T) {
 			assert.Check(t, cmd.Flags().Set(key, value))
 		}
 		cmd.SetOut(io.Discard)
+		cmd.SetErr(io.Discard)
 		assert.ErrorContains(t, cmd.Execute(), tc.expectedError)
 	}
 }

cli/command/config/ls_test.go

@@ -42,6 +42,7 @@ func TestConfigListErrors(t *testing.T) {
 		)
 		cmd.SetArgs(tc.args)
 		cmd.SetOut(io.Discard)
+		cmd.SetErr(io.Discard)
 		assert.ErrorContains(t, cmd.Execute(), tc.expectedError)
 	}
 }

cli/command/config/remove_test.go

@@ -37,6 +37,7 @@ func TestConfigRemoveErrors(t *testing.T) {
 		)
 		cmd.SetArgs(tc.args)
 		cmd.SetOut(io.Discard)
+		cmd.SetErr(io.Discard)
 		assert.ErrorContains(t, cmd.Execute(), tc.expectedError)
 	}
 }
@@ -74,6 +75,7 @@ func TestConfigRemoveContinueAfterError(t *testing.T) {
 	cmd := newConfigRemoveCommand(cli)
 	cmd.SetArgs(names)
 	cmd.SetOut(io.Discard)
+	cmd.SetErr(io.Discard)
 	assert.Error(t, cmd.Execute(), "error removing config: foo")
 	assert.Check(t, is.DeepEqual(names, removedConfigs))
 }

cli/command/container/attach_test.go

@@ -74,10 +74,14 @@ func TestNewAttachCommandErrors(t *testing.T) {
 		},
 	}
 	for _, tc := range testCases {
-		cmd := NewAttachCommand(test.NewFakeCli(&fakeClient{inspectFunc: tc.containerInspectFunc}))
-		cmd.SetOut(io.Discard)
-		cmd.SetArgs(tc.args)
-		assert.ErrorContains(t, cmd.Execute(), tc.expectedError)
+		tc := tc
+		t.Run(tc.name, func(t *testing.T) {
+			cmd := NewAttachCommand(test.NewFakeCli(&fakeClient{inspectFunc: tc.containerInspectFunc}))
+			cmd.SetOut(io.Discard)
+			cmd.SetErr(io.Discard)
+			cmd.SetArgs(tc.args)
+			assert.ErrorContains(t, cmd.Execute(), tc.expectedError)
+		})
 	}
 }
 

cli/command/container/cp_test.go

@@ -178,13 +178,14 @@ func TestSplitCpArg(t *testing.T) {
 			expectedContainer: "container",
 		},
 	}
-	for _, testcase := range testcases {
-		t.Run(testcase.doc, func(t *testing.T) {
-			skip.If(t, testcase.os != "" && testcase.os != runtime.GOOS)
-
-			ctr, path := splitCpArg(testcase.path)
-			assert.Check(t, is.Equal(testcase.expectedContainer, ctr))
-			assert.Check(t, is.Equal(testcase.expectedPath, path))
+	for _, tc := range testcases {
+		tc := tc
+		t.Run(tc.doc, func(t *testing.T) {
+			skip.If(t, tc.os == "windows" && runtime.GOOS != "windows" || tc.os == "linux" && runtime.GOOS == "windows")
+
+			ctr, path := splitCpArg(tc.path)
+			assert.Check(t, is.Equal(tc.expectedContainer, ctr))
+			assert.Check(t, is.Equal(tc.expectedPath, path))
 		})
 	}
 }

cli/command/container/create_test.go

@@ -236,6 +236,7 @@ func TestNewCreateCommandWithContentTrustErrors(t *testing.T) {
 		fakeCLI.SetNotaryClient(tc.notaryFunc)
 		cmd := NewCreateCommand(fakeCLI)
 		cmd.SetOut(io.Discard)
+		cmd.SetErr(io.Discard)
 		cmd.SetArgs(tc.args)
 		err := cmd.Execute()
 		assert.ErrorContains(t, err, tc.expectedError)

cli/command/container/export_test.go

@@ -39,6 +39,7 @@ func TestContainerExportOutputToIrregularFile(t *testing.T) {
 	})
 	cmd := NewExportCommand(cli)
 	cmd.SetOut(io.Discard)
+	cmd.SetErr(io.Discard)
 	cmd.SetArgs([]string{"-o", "/dev/random", "container"})
 
 	err := cmd.Execute()

cli/command/container/list_test.go

@@ -163,6 +163,7 @@ func TestContainerListErrors(t *testing.T) {
 			assert.Check(t, cmd.Flags().Set(key, value))
 		}
 		cmd.SetOut(io.Discard)
+		cmd.SetErr(io.Discard)
 		assert.ErrorContains(t, cmd.Execute(), tc.expectedError)
 	}
 }

cli/command/container/prune_test.go

@@ -2,6 +2,7 @@ package container
 
 import (
 	"context"
+	"io"
 	"testing"
 
 	"github.com/docker/cli/internal/test"
@@ -20,5 +21,7 @@ func TestContainerPrunePromptTermination(t *testing.T) {
 		},
 	})
 	cmd := NewPruneCommand(cli)
+	cmd.SetOut(io.Discard)
+	cmd.SetErr(io.Discard)
 	test.TerminatePrompt(ctx, t, cmd, cli)
 }

cli/command/container/rm_test.go

@@ -45,6 +45,7 @@ func TestRemoveForce(t *testing.T) {
 			})
 			cmd := NewRmCommand(cli)
 			cmd.SetOut(io.Discard)
+			cmd.SetErr(io.Discard)
 			cmd.SetArgs(tc.args)
 
 			err := cmd.Execute()

cli/command/container/run_test.go

@@ -127,23 +127,27 @@ func TestRunCommandWithContentTrustErrors(t *testing.T) {
 		},
 	}
 	for _, tc := range testCases {
-		fakeCLI := test.NewFakeCli(&fakeClient{
-			createContainerFunc: func(config *container.Config,
-				hostConfig *container.HostConfig,
-				networkingConfig *network.NetworkingConfig,
-				platform *specs.Platform,
-				containerName string,
-			) (container.CreateResponse, error) {
-				return container.CreateResponse{}, errors.New("shouldn't try to pull image")
-			},
-		}, test.EnableContentTrust)
-		fakeCLI.SetNotaryClient(tc.notaryFunc)
-		cmd := NewRunCommand(fakeCLI)
-		cmd.SetArgs(tc.args)
-		cmd.SetOut(io.Discard)
-		err := cmd.Execute()
-		assert.Assert(t, err != nil)
-		assert.Assert(t, is.Contains(fakeCLI.ErrBuffer().String(), tc.expectedError))
+		tc := tc
+		t.Run(tc.name, func(t *testing.T) {
+			fakeCLI := test.NewFakeCli(&fakeClient{
+				createContainerFunc: func(config *container.Config,
+					hostConfig *container.HostConfig,
+					networkingConfig *network.NetworkingConfig,
+					platform *specs.Platform,
+					containerName string,
+				) (container.CreateResponse, error) {
+					return container.CreateResponse{}, errors.New("shouldn't try to pull image")
+				},
+			}, test.EnableContentTrust)
+			fakeCLI.SetNotaryClient(tc.notaryFunc)
+			cmd := NewRunCommand(fakeCLI)
+			cmd.SetArgs(tc.args)
+			cmd.SetOut(io.Discard)
+			cmd.SetErr(io.Discard)
+			err := cmd.Execute()
+			assert.Assert(t, err != nil)
+			assert.Assert(t, is.Contains(fakeCLI.ErrBuffer().String(), tc.expectedError))
+		})
 	}
 }
 

cli/command/image/build_test.go

@@ -127,6 +127,7 @@ func TestRunBuildFromGitHubSpecialCase(t *testing.T) {
 	// Clone a small repo that exists so git doesn't prompt for credentials
 	cmd.SetArgs([]string{"github.com/docker/for-win"})
 	cmd.SetOut(io.Discard)
+	cmd.SetErr(io.Discard)
 	err := cmd.Execute()
 	assert.ErrorContains(t, err, "unable to prepare context")
 	assert.ErrorContains(t, err, "docker-build-git")

cli/command/image/history_test.go

@@ -35,10 +35,14 @@ func TestNewHistoryCommandErrors(t *testing.T) {
 		},
 	}
 	for _, tc := range testCases {
-		cmd := NewHistoryCommand(test.NewFakeCli(&fakeClient{imageHistoryFunc: tc.imageHistoryFunc}))
-		cmd.SetOut(io.Discard)
-		cmd.SetArgs(tc.args)
-		assert.ErrorContains(t, cmd.Execute(), tc.expectedError)
+		tc := tc
+		t.Run(tc.name, func(t *testing.T) {
+			cmd := NewHistoryCommand(test.NewFakeCli(&fakeClient{imageHistoryFunc: tc.imageHistoryFunc}))
+			cmd.SetOut(io.Discard)
+			cmd.SetErr(io.Discard)
+			cmd.SetArgs(tc.args)
+			assert.ErrorContains(t, cmd.Execute(), tc.expectedError)
+		})
 	}
 }
 

cli/command/image/import_test.go

@@ -36,6 +36,7 @@ func TestNewImportCommandErrors(t *testing.T) {
 	for _, tc := range testCases {
 		cmd := NewImportCommand(test.NewFakeCli(&fakeClient{imageImportFunc: tc.imageImportFunc}))
 		cmd.SetOut(io.Discard)
+		cmd.SetErr(io.Discard)
 		cmd.SetArgs(tc.args)
 		assert.ErrorContains(t, cmd.Execute(), tc.expectedError)
 	}
@@ -44,6 +45,7 @@ func TestNewImportCommandErrors(t *testing.T) {
 func TestNewImportCommandInvalidFile(t *testing.T) {
 	cmd := NewImportCommand(test.NewFakeCli(&fakeClient{}))
 	cmd.SetOut(io.Discard)
+	cmd.SetErr(io.Discard)
 	cmd.SetArgs([]string{"testdata/import-command-success.unexistent-file"})
 	assert.ErrorContains(t, cmd.Execute(), "testdata/import-command-success.unexistent-file")
 }
@@ -96,9 +98,13 @@ func TestNewImportCommandSuccess(t *testing.T) {
 		},
 	}
 	for _, tc := range testCases {
-		cmd := NewImportCommand(test.NewFakeCli(&fakeClient{imageImportFunc: tc.imageImportFunc}))
-		cmd.SetOut(io.Discard)
-		cmd.SetArgs(tc.args)
-		assert.NilError(t, cmd.Execute())
+		tc := tc
+		t.Run(tc.name, func(t *testing.T) {
+			cmd := NewImportCommand(test.NewFakeCli(&fakeClient{imageImportFunc: tc.imageImportFunc}))
+			cmd.SetOut(io.Discard)
+			cmd.SetErr(io.Discard)
+			cmd.SetArgs(tc.args)
+			assert.NilError(t, cmd.Execute())
+		})
 	}
 }

cli/command/image/inspect_test.go

@@ -25,10 +25,14 @@ func TestNewInspectCommandErrors(t *testing.T) {
 		},
 	}
 	for _, tc := range testCases {
-		cmd := newInspectCommand(test.NewFakeCli(&fakeClient{}))
-		cmd.SetOut(io.Discard)
-		cmd.SetArgs(tc.args)
-		assert.ErrorContains(t, cmd.Execute(), tc.expectedError)
+		tc := tc
+		t.Run(tc.name, func(t *testing.T) {
+			cmd := newInspectCommand(test.NewFakeCli(&fakeClient{}))
+			cmd.SetOut(io.Discard)
+			cmd.SetErr(io.Discard)
+			cmd.SetArgs(tc.args)
+			assert.ErrorContains(t, cmd.Execute(), tc.expectedError)
+		})
 	}
 }
 

cli/command/image/list_test.go

@@ -35,10 +35,14 @@ func TestNewImagesCommandErrors(t *testing.T) {
 		},
 	}
 	for _, tc := range testCases {
-		cmd := NewImagesCommand(test.NewFakeCli(&fakeClient{imageListFunc: tc.imageListFunc}))
-		cmd.SetOut(io.Discard)
-		cmd.SetArgs(tc.args)
-		assert.ErrorContains(t, cmd.Execute(), tc.expectedError)
+		tc := tc
+		t.Run(tc.name, func(t *testing.T) {
+			cmd := NewImagesCommand(test.NewFakeCli(&fakeClient{imageListFunc: tc.imageListFunc}))
+			cmd.SetOut(io.Discard)
+			cmd.SetErr(io.Discard)
+			cmd.SetArgs(tc.args)
+			assert.ErrorContains(t, cmd.Execute(), tc.expectedError)
+		})
 	}
 }
 
@@ -79,14 +83,18 @@ func TestNewImagesCommandSuccess(t *testing.T) {
 		},
 	}
 	for _, tc := range testCases {
-		cli := test.NewFakeCli(&fakeClient{imageListFunc: tc.imageListFunc})
-		cli.SetConfigFile(&configfile.ConfigFile{ImagesFormat: tc.imageFormat})
-		cmd := NewImagesCommand(cli)
-		cmd.SetOut(io.Discard)
-		cmd.SetArgs(tc.args)
-		err := cmd.Execute()
-		assert.NilError(t, err)
-		golden.Assert(t, cli.OutBuffer().String(), fmt.Sprintf("list-command-success.%s.golden", tc.name))
+		tc := tc
+		t.Run(tc.name, func(t *testing.T) {
+			cli := test.NewFakeCli(&fakeClient{imageListFunc: tc.imageListFunc})
+			cli.SetConfigFile(&configfile.ConfigFile{ImagesFormat: tc.imageFormat})
+			cmd := NewImagesCommand(cli)
+			cmd.SetOut(io.Discard)
+			cmd.SetErr(io.Discard)
+			cmd.SetArgs(tc.args)
+			err := cmd.Execute()
+			assert.NilError(t, err)
+			golden.Assert(t, cli.OutBuffer().String(), fmt.Sprintf("list-command-success.%s.golden", tc.name))
+		})
 	}
 }