Skip to content

Commit ab23024

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 <github@gone.nl>
1 parent 3837aa6 commit ab23024

87 files changed

Lines changed: 677 additions & 373 deletions

Some content is hidden

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

‎cli/command/builder/prune_test.go‎

Lines changed: 3 additions & 0 deletions
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‎

Lines changed: 1 addition & 0 deletions
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‎

Lines changed: 1 addition & 0 deletions
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‎

Lines changed: 1 addition & 0 deletions
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‎

Lines changed: 12 additions & 8 deletions
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‎

Lines changed: 1 addition & 0 deletions
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‎

Lines changed: 1 addition & 0 deletions
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‎

Lines changed: 2 additions & 0 deletions
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‎

Lines changed: 8 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -74,10 +74,14 @@ func TestNewAttachCommandErrors(t *testing.T) {
7474
},
7575
}
7676
for _, tc := range testCases {
77-
cmd := NewAttachCommand(test.NewFakeCli(&fakeClient{inspectFunc: tc.containerInspectFunc}))
78-
cmd.SetOut(io.Discard)
79-
cmd.SetArgs(tc.args)
80-
assert.ErrorContains(t, cmd.Execute(), tc.expectedError)
77+
tc := tc
78+
t.Run(tc.name, func(t *testing.T) {
79+
cmd := NewAttachCommand(test.NewFakeCli(&fakeClient{inspectFunc: tc.containerInspectFunc}))
80+
cmd.SetOut(io.Discard)
81+
cmd.SetErr(io.Discard)
82+
cmd.SetArgs(tc.args)
83+
assert.ErrorContains(t, cmd.Execute(), tc.expectedError)
84+
})
8185
}
8286
}
8387

‎cli/command/container/cp_test.go‎

Lines changed: 8 additions & 7 deletions
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
}

0 commit comments

Comments
 (0)