test spring-cleaning

[thaJeztah]

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).

- A picture of a cute animal (not mandatory but encouraged)

[thaJeztah]

Also logs / output before/after

after.log
before.log

[codecov-commenter]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 61.46%. Comparing base (3837aa6) to head (ab23024).
Report is 3 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #5224      +/-   ##
==========================================
+ Coverage   61.01%   61.46%   +0.45%     
==========================================
  Files         295      298       +3     
  Lines       20799    20807       +8     
==========================================
+ Hits        12691    12790      +99     
+ Misses       7193     7105      -88     
+ Partials      915      912       -3     

[thaJeztah]

💡 👉 As some of the sub-test changes introduce whitespace noise; the diff is easier to read when ignoring white-spaces; use this URL to see the diff with white-space suppressed; https://github.com/docker/cli/pull/5224/files?w=1 (i.e. add ?w=1 to the URL)

[vvoland]

nit: This could be a bit more readable with:

Suggested change

	skip.If(t, tc.os == "windows" && runtime.GOOS != "windows" || tc.os == "linux" && runtime.GOOS == "windows")
	windowsTestOnNonWindowsOs := tc.os == "windows" && runtime.GOOS != "windows"
	linuxTestOnWindows := tc.os == "linux" && runtime.GOOS == "windows"
	skip.If(t, windowsTestOnNonWindowsOs || linuxTestOnWindows)

[thaJeztah]

Ah, yeah, this one was a bit ugly. Maybe unix as value in the test, a IsUnix() utility; although I guess we could use os.PathSeparator;

tc.os == "unix" && os.PathSeparator != "/" || tc.os == "windows" && os.PathSeparator != `\`

I may do some follow-ups to this one; I'll have a look at part of those 👍