Skip to content

Commit ca9636a

Browse files
thaJeztahthaJeztah
committed
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 #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]> (cherry picked from commit ab23024) Signed-off-by: Sebastiaan van Stijn <[email protected]>
1 parent a2a0fb7 commit ca9636a

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

87 files changed

+677
-373
lines changed

‎cli/command/builder/prune_test.go

+3
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@ package builder
33
import (
44
"context"
55
"errors"
6+
"io"
67
"testing"
78

89
"github.com/docker/cli/internal/test"
@@ -19,5 +20,7 @@ func TestBuilderPromptTermination(t *testing.T) {
1920
},
2021
})
2122
cmd := NewPruneCommand(cli)
23+
cmd.SetOut(io.Discard)
24+
cmd.SetErr(io.Discard)
2225
test.TerminatePrompt(ctx, t, cmd, cli)
2326
}

‎cli/command/checkpoint/create_test.go

+1
Original file line numberDiff line numberDiff line change
@@ -42,6 +42,7 @@ func TestCheckpointCreateErrors(t *testing.T) {
4242
cmd := newCreateCommand(cli)
4343
cmd.SetArgs(tc.args)
4444
cmd.SetOut(io.Discard)
45+
cmd.SetErr(io.Discard)
4546
assert.ErrorContains(t, cmd.Execute(), tc.expectedError)
4647
}
4748
}

‎cli/command/checkpoint/list_test.go

+1
Original file line numberDiff line numberDiff line change
@@ -42,6 +42,7 @@ func TestCheckpointListErrors(t *testing.T) {
4242
cmd := newListCommand(cli)
4343
cmd.SetArgs(tc.args)
4444
cmd.SetOut(io.Discard)
45+
cmd.SetErr(io.Discard)
4546
assert.ErrorContains(t, cmd.Execute(), tc.expectedError)
4647
}
4748
}

‎cli/command/checkpoint/remove_test.go

+1
Original file line numberDiff line numberDiff line change
@@ -41,6 +41,7 @@ func TestCheckpointRemoveErrors(t *testing.T) {
4141
cmd := newRemoveCommand(cli)
4242
cmd.SetArgs(tc.args)
4343
cmd.SetOut(io.Discard)
44+
cmd.SetErr(io.Discard)
4445
assert.ErrorContains(t, cmd.Execute(), tc.expectedError)
4546
}
4647
}

‎cli/command/config/create_test.go

+12-8
Original file line numberDiff line numberDiff line change
@@ -43,14 +43,18 @@ func TestConfigCreateErrors(t *testing.T) {
4343
},
4444
}
4545
for _, tc := range testCases {
46-
cmd := newConfigCreateCommand(
47-
test.NewFakeCli(&fakeClient{
48-
configCreateFunc: tc.configCreateFunc,
49-
}),
50-
)
51-
cmd.SetArgs(tc.args)
52-
cmd.SetOut(io.Discard)
53-
assert.ErrorContains(t, cmd.Execute(), tc.expectedError)
46+
tc := tc
47+
t.Run(tc.expectedError, func(t *testing.T) {
48+
cmd := newConfigCreateCommand(
49+
test.NewFakeCli(&fakeClient{
50+
configCreateFunc: tc.configCreateFunc,
51+
}),
52+
)
53+
cmd.SetArgs(tc.args)
54+
cmd.SetOut(io.Discard)
55+
cmd.SetErr(io.Discard)
56+
assert.ErrorContains(t, cmd.Execute(), tc.expectedError)
57+
})
5458
}
5559
}
5660

‎cli/command/config/inspect_test.go

+1
Original file line numberDiff line numberDiff line change
@@ -61,6 +61,7 @@ func TestConfigInspectErrors(t *testing.T) {
6161
assert.Check(t, cmd.Flags().Set(key, value))
6262
}
6363
cmd.SetOut(io.Discard)
64+
cmd.SetErr(io.Discard)
6465
assert.ErrorContains(t, cmd.Execute(), tc.expectedError)
6566
}
6667
}

‎cli/command/config/ls_test.go

+1
Original file line numberDiff line numberDiff line change
@@ -42,6 +42,7 @@ func TestConfigListErrors(t *testing.T) {
4242
)
4343
cmd.SetArgs(tc.args)
4444
cmd.SetOut(io.Discard)
45+
cmd.SetErr(io.Discard)
4546
assert.ErrorContains(t, cmd.Execute(), tc.expectedError)
4647
}
4748
}

‎cli/command/config/remove_test.go

+2
Original file line numberDiff line numberDiff line change
@@ -37,6 +37,7 @@ func TestConfigRemoveErrors(t *testing.T) {
3737
)
3838
cmd.SetArgs(tc.args)
3939
cmd.SetOut(io.Discard)
40+
cmd.SetErr(io.Discard)
4041
assert.ErrorContains(t, cmd.Execute(), tc.expectedError)
4142
}
4243
}
@@ -74,6 +75,7 @@ func TestConfigRemoveContinueAfterError(t *testing.T) {
7475
cmd := newConfigRemoveCommand(cli)
7576
cmd.SetArgs(names)
7677
cmd.SetOut(io.Discard)
78+
cmd.SetErr(io.Discard)
7779
assert.Error(t, cmd.Execute(), "error removing config: foo")
7880
assert.Check(t, is.DeepEqual(names, removedConfigs))
7981
}

‎cli/command/container/attach_test.go

+8-4
Original file line numberDiff line numberDiff line change
@@ -69,10 +69,14 @@ func TestNewAttachCommandErrors(t *testing.T) {
6969
},
7070
}
7171
for _, tc := range testCases {
72-
cmd := NewAttachCommand(test.NewFakeCli(&fakeClient{inspectFunc: tc.containerInspectFunc}))
73-
cmd.SetOut(io.Discard)
74-
cmd.SetArgs(tc.args)
75-
assert.ErrorContains(t, cmd.Execute(), tc.expectedError)
72+
tc := tc
73+
t.Run(tc.name, func(t *testing.T) {
74+
cmd := NewAttachCommand(test.NewFakeCli(&fakeClient{inspectFunc: tc.containerInspectFunc}))
75+
cmd.SetOut(io.Discard)
76+
cmd.SetErr(io.Discard)
77+
cmd.SetArgs(tc.args)
78+
assert.ErrorContains(t, cmd.Execute(), tc.expectedError)
79+
})
7680
}
7781
}
7882

‎cli/command/container/cp_test.go

+8-7
Original file line numberDiff line numberDiff line change
@@ -178,13 +178,14 @@ func TestSplitCpArg(t *testing.T) {
178178
expectedContainer: "container",
179179
},
180180
}
181-
for _, testcase := range testcases {
182-
t.Run(testcase.doc, func(t *testing.T) {
183-
skip.If(t, testcase.os != "" && testcase.os != runtime.GOOS)
184-
185-
ctr, path := splitCpArg(testcase.path)
186-
assert.Check(t, is.Equal(testcase.expectedContainer, ctr))
187-
assert.Check(t, is.Equal(testcase.expectedPath, path))
181+
for _, tc := range testcases {
182+
tc := tc
183+
t.Run(tc.doc, func(t *testing.T) {
184+
skip.If(t, tc.os == "windows" && runtime.GOOS != "windows" || tc.os == "linux" && runtime.GOOS == "windows")
185+
186+
ctr, path := splitCpArg(tc.path)
187+
assert.Check(t, is.Equal(tc.expectedContainer, ctr))
188+
assert.Check(t, is.Equal(tc.expectedPath, path))
188189
})
189190
}
190191
}

‎cli/command/container/create_test.go

+1
Original file line numberDiff line numberDiff line change
@@ -236,6 +236,7 @@ func TestNewCreateCommandWithContentTrustErrors(t *testing.T) {
236236
fakeCLI.SetNotaryClient(tc.notaryFunc)
237237
cmd := NewCreateCommand(fakeCLI)
238238
cmd.SetOut(io.Discard)
239+
cmd.SetErr(io.Discard)
239240
cmd.SetArgs(tc.args)
240241
err := cmd.Execute()
241242
assert.ErrorContains(t, err, tc.expectedError)

‎cli/command/container/export_test.go

+1
Original file line numberDiff line numberDiff line change
@@ -39,6 +39,7 @@ func TestContainerExportOutputToIrregularFile(t *testing.T) {
3939
})
4040
cmd := NewExportCommand(cli)
4141
cmd.SetOut(io.Discard)
42+
cmd.SetErr(io.Discard)
4243
cmd.SetArgs([]string{"-o", "/dev/random", "container"})
4344

4445
err := cmd.Execute()

‎cli/command/container/list_test.go

+1
Original file line numberDiff line numberDiff line change
@@ -163,6 +163,7 @@ func TestContainerListErrors(t *testing.T) {
163163
assert.Check(t, cmd.Flags().Set(key, value))
164164
}
165165
cmd.SetOut(io.Discard)
166+
cmd.SetErr(io.Discard)
166167
assert.ErrorContains(t, cmd.Execute(), tc.expectedError)
167168
}
168169
}

‎cli/command/container/prune_test.go

+3
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@ package container
22

33
import (
44
"context"
5+
"io"
56
"testing"
67

78
"github.com/docker/cli/internal/test"
@@ -20,5 +21,7 @@ func TestContainerPrunePromptTermination(t *testing.T) {
2021
},
2122
})
2223
cmd := NewPruneCommand(cli)
24+
cmd.SetOut(io.Discard)
25+
cmd.SetErr(io.Discard)
2326
test.TerminatePrompt(ctx, t, cmd, cli)
2427
}

‎cli/command/container/rm_test.go

+1
Original file line numberDiff line numberDiff line change
@@ -45,6 +45,7 @@ func TestRemoveForce(t *testing.T) {
4545
})
4646
cmd := NewRmCommand(cli)
4747
cmd.SetOut(io.Discard)
48+
cmd.SetErr(io.Discard)
4849
cmd.SetArgs(tc.args)
4950

5051
err := cmd.Execute()

‎cli/command/container/run_test.go

+21-17
Original file line numberDiff line numberDiff line change
@@ -127,23 +127,27 @@ func TestRunCommandWithContentTrustErrors(t *testing.T) {
127127
},
128128
}
129129
for _, tc := range testCases {
130-
fakeCLI := test.NewFakeCli(&fakeClient{
131-
createContainerFunc: func(config *container.Config,
132-
hostConfig *container.HostConfig,
133-
networkingConfig *network.NetworkingConfig,
134-
platform *specs.Platform,
135-
containerName string,
136-
) (container.CreateResponse, error) {
137-
return container.CreateResponse{}, errors.New("shouldn't try to pull image")
138-
},
139-
}, test.EnableContentTrust)
140-
fakeCLI.SetNotaryClient(tc.notaryFunc)
141-
cmd := NewRunCommand(fakeCLI)
142-
cmd.SetArgs(tc.args)
143-
cmd.SetOut(io.Discard)
144-
err := cmd.Execute()
145-
assert.Assert(t, err != nil)
146-
assert.Assert(t, is.Contains(fakeCLI.ErrBuffer().String(), tc.expectedError))
130+
tc := tc
131+
t.Run(tc.name, func(t *testing.T) {
132+
fakeCLI := test.NewFakeCli(&fakeClient{
133+
createContainerFunc: func(config *container.Config,
134+
hostConfig *container.HostConfig,
135+
networkingConfig *network.NetworkingConfig,
136+
platform *specs.Platform,
137+
containerName string,
138+
) (container.CreateResponse, error) {
139+
return container.CreateResponse{}, errors.New("shouldn't try to pull image")
140+
},
141+
}, test.EnableContentTrust)
142+
fakeCLI.SetNotaryClient(tc.notaryFunc)
143+
cmd := NewRunCommand(fakeCLI)
144+
cmd.SetArgs(tc.args)
145+
cmd.SetOut(io.Discard)
146+
cmd.SetErr(io.Discard)
147+
err := cmd.Execute()
148+
assert.Assert(t, err != nil)
149+
assert.Assert(t, is.Contains(fakeCLI.ErrBuffer().String(), tc.expectedError))
150+
})
147151
}
148152
}
149153

‎cli/command/image/build_test.go

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

‎cli/command/image/history_test.go

+8-4
Original file line numberDiff line numberDiff line change
@@ -35,10 +35,14 @@ func TestNewHistoryCommandErrors(t *testing.T) {
3535
},
3636
}
3737
for _, tc := range testCases {
38-
cmd := NewHistoryCommand(test.NewFakeCli(&fakeClient{imageHistoryFunc: tc.imageHistoryFunc}))
39-
cmd.SetOut(io.Discard)
40-
cmd.SetArgs(tc.args)
41-
assert.ErrorContains(t, cmd.Execute(), tc.expectedError)
38+
tc := tc
39+
t.Run(tc.name, func(t *testing.T) {
40+
cmd := NewHistoryCommand(test.NewFakeCli(&fakeClient{imageHistoryFunc: tc.imageHistoryFunc}))
41+
cmd.SetOut(io.Discard)
42+
cmd.SetErr(io.Discard)
43+
cmd.SetArgs(tc.args)
44+
assert.ErrorContains(t, cmd.Execute(), tc.expectedError)
45+
})
4246
}
4347
}
4448

‎cli/command/image/import_test.go

+10-4
Original file line numberDiff line numberDiff line change
@@ -36,6 +36,7 @@ func TestNewImportCommandErrors(t *testing.T) {
3636
for _, tc := range testCases {
3737
cmd := NewImportCommand(test.NewFakeCli(&fakeClient{imageImportFunc: tc.imageImportFunc}))
3838
cmd.SetOut(io.Discard)
39+
cmd.SetErr(io.Discard)
3940
cmd.SetArgs(tc.args)
4041
assert.ErrorContains(t, cmd.Execute(), tc.expectedError)
4142
}
@@ -44,6 +45,7 @@ func TestNewImportCommandErrors(t *testing.T) {
4445
func TestNewImportCommandInvalidFile(t *testing.T) {
4546
cmd := NewImportCommand(test.NewFakeCli(&fakeClient{}))
4647
cmd.SetOut(io.Discard)
48+
cmd.SetErr(io.Discard)
4749
cmd.SetArgs([]string{"testdata/import-command-success.unexistent-file"})
4850
assert.ErrorContains(t, cmd.Execute(), "testdata/import-command-success.unexistent-file")
4951
}
@@ -96,9 +98,13 @@ func TestNewImportCommandSuccess(t *testing.T) {
9698
},
9799
}
98100
for _, tc := range testCases {
99-
cmd := NewImportCommand(test.NewFakeCli(&fakeClient{imageImportFunc: tc.imageImportFunc}))
100-
cmd.SetOut(io.Discard)
101-
cmd.SetArgs(tc.args)
102-
assert.NilError(t, cmd.Execute())
101+
tc := tc
102+
t.Run(tc.name, func(t *testing.T) {
103+
cmd := NewImportCommand(test.NewFakeCli(&fakeClient{imageImportFunc: tc.imageImportFunc}))
104+
cmd.SetOut(io.Discard)
105+
cmd.SetErr(io.Discard)
106+
cmd.SetArgs(tc.args)
107+
assert.NilError(t, cmd.Execute())
108+
})
103109
}
104110
}

‎cli/command/image/inspect_test.go

+8-4
Original file line numberDiff line numberDiff line change
@@ -25,10 +25,14 @@ func TestNewInspectCommandErrors(t *testing.T) {
2525
},
2626
}
2727
for _, tc := range testCases {
28-
cmd := newInspectCommand(test.NewFakeCli(&fakeClient{}))
29-
cmd.SetOut(io.Discard)
30-
cmd.SetArgs(tc.args)
31-
assert.ErrorContains(t, cmd.Execute(), tc.expectedError)
28+
tc := tc
29+
t.Run(tc.name, func(t *testing.T) {
30+
cmd := newInspectCommand(test.NewFakeCli(&fakeClient{}))
31+
cmd.SetOut(io.Discard)
32+
cmd.SetErr(io.Discard)
33+
cmd.SetArgs(tc.args)
34+
assert.ErrorContains(t, cmd.Execute(), tc.expectedError)
35+
})
3236
}
3337
}
3438

‎cli/command/image/list_test.go

+20-12
Original file line numberDiff line numberDiff line change
@@ -35,10 +35,14 @@ func TestNewImagesCommandErrors(t *testing.T) {
3535
},
3636
}
3737
for _, tc := range testCases {
38-
cmd := NewImagesCommand(test.NewFakeCli(&fakeClient{imageListFunc: tc.imageListFunc}))
39-
cmd.SetOut(io.Discard)
40-
cmd.SetArgs(tc.args)
41-
assert.ErrorContains(t, cmd.Execute(), tc.expectedError)
38+
tc := tc
39+
t.Run(tc.name, func(t *testing.T) {
40+
cmd := NewImagesCommand(test.NewFakeCli(&fakeClient{imageListFunc: tc.imageListFunc}))
41+
cmd.SetOut(io.Discard)
42+
cmd.SetErr(io.Discard)
43+
cmd.SetArgs(tc.args)
44+
assert.ErrorContains(t, cmd.Execute(), tc.expectedError)
45+
})
4246
}
4347
}
4448

@@ -79,14 +83,18 @@ func TestNewImagesCommandSuccess(t *testing.T) {
7983
},
8084
}
8185
for _, tc := range testCases {
82-
cli := test.NewFakeCli(&fakeClient{imageListFunc: tc.imageListFunc})
83-
cli.SetConfigFile(&configfile.ConfigFile{ImagesFormat: tc.imageFormat})
84-
cmd := NewImagesCommand(cli)
85-
cmd.SetOut(io.Discard)
86-
cmd.SetArgs(tc.args)
87-
err := cmd.Execute()
88-
assert.NilError(t, err)
89-
golden.Assert(t, cli.OutBuffer().String(), fmt.Sprintf("list-command-success.%s.golden", tc.name))
86+
tc := tc
87+
t.Run(tc.name, func(t *testing.T) {
88+
cli := test.NewFakeCli(&fakeClient{imageListFunc: tc.imageListFunc})
89+
cli.SetConfigFile(&configfile.ConfigFile{ImagesFormat: tc.imageFormat})
90+
cmd := NewImagesCommand(cli)
91+
cmd.SetOut(io.Discard)
92+
cmd.SetErr(io.Discard)
93+
cmd.SetArgs(tc.args)
94+
err := cmd.Execute()
95+
assert.NilError(t, err)
96+
golden.Assert(t, cli.OutBuffer().String(), fmt.Sprintf("list-command-success.%s.golden", tc.name))
97+
})
9098
}
9199
}
92100

0 commit comments

Comments
 (0)