The logic in this function is confusing; let's start make it obvious where
the error that is returned is produced,
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
This confused me fore a bit, because I thought the test was checking for
an actual `context.Canceled` error (which is spelled "context canceled"
with a single "l". But then I found that this was a string that's printed
as part of a test-utility, just looking very similar but with the British
spelling ("cancelled").
Let's change this to a message that's unique for the test, also to make it
more grep'able.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Improve the output for these validation errors:
- Removes the short command description from the output. This information
does not provide much useful help, and distracts from the error message.
- Reduces punctuation, and
- Prefixes the error message with the binary / root-command name
(usually `docker:`) to be consistent with other similar errors.
- Adds an empty line between the error-message and the "call to action"
(`Run 'docker volume --help'...` in the example below). This helps
separating the error message and "usage" from the call-to-action.
Before this patch:
$ docker volume ls one two three
"docker volume ls" accepts no arguments.
See 'docker volume ls --help'.
Usage: docker volume ls [OPTIONS]
List volumes
$ docker volume create one two three
"docker volume create" requires at most 1 argument.
See 'docker volume create --help'.
Usage: docker volume create [OPTIONS] [VOLUME]
Create a volume
With this patch:
$ docker volume ls one two three
docker: 'docker volume ls' accepts no arguments
Usage: docker volume ls [OPTIONS]
Run 'docker volume ls --help' for more information
$ docker voludocker volume create one two three
docker: 'docker volume create' requires at most 1 argument
Usage: docker volume create [OPTIONS] [VOLUME]
SRun 'docker volume create --help' for more information
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Before this patch, output for invalid top-level and sub-commands differed.
For top-level commands, the CLI would print an error-message and a suggestion
to use `--help`. For missing *subcommands*, we would hit a different code-path,
and different output, which includes full "usage" / "help" output.
While it is a common convention to show usage output, and may have been
a nice gesture when docker was still young and only had a few commands
and options ("you did something wrong; here's an overview of what you
can use"), that's no longer the case, and many commands have a _very_
long output.
The result of this is that the error message, which is the relevant
information in this case - "You mis-typed something" - is lost in the
output, and hard to find (sometimes even requiring scrolling back).
The output is also confusing, because it _looks_ like something ran
successfully (most of the output is not about the error!).
Even further; the suggested resolution (try `--help` to see the correct
options) is rather redundant, because running teh command with `--help`
produces _exactly_ the same output as was just showh, baring the error
message. As a fun fact, due to the usage output being printed, the
output even contains not one, but _two_ "call to actions";
- `See 'docker volume --help'.` (under the erro message)
- `Run 'docker volume COMMAND --help' for more information on a command.`
(under the usage output)
In short; the output is too verbose, confusing, and doesn't provide
a good UX. Let's reduce the output produced so that the focus is on the
important information.
This patch:
- Changes the usage to the short-usage.
- Changes the error-message to mention the _full_ command instead of only
the command after `docker` (so `docker no-such-command` instead of
`no-such-command`).
- Prefixes the error message with the binary / root-command name
(usually `docker:`); this is something we can still decide on, but
it's a pattern we already use in some places. The motivation for this
is that `docker` commands can often produce output that's a combination
of output from the CLI itself, output from the daemon, and even output
from the container. The `docker:` prefix helps to distinguish where
the message originated from (the `docker` CLI in this case).
- Adds an empty line between the error-message and the "call to action"
(`Run 'docker volume --help'...` in the example below). This helps
separating the error message ("unkown flag") from the call-to-action.
Before this patch:
Unknown top-level command:
docker nosuchcommand foo
docker: 'nosuchcommand' is not a docker command.
See 'docker --help'
Unknown sub-command:
docker volume nosuchcommand foo
Usage: docker volume COMMAND
Manage volumes
Commands:
create Create a volume
inspect Display detailed information on one or more volumes
ls List volumes
prune Remove unused local volumes
rm Remove one or more volumes
update Update a volume (cluster volumes only)
Run 'docker volume COMMAND --help' for more information on a command.
After this patch:
Unknown top-level command:
docker nosuchcommand foo
docker: unknown command: docker nosuchcommand
Run 'docker --help' for more information
Unknown sub-command:
docker volume nosuchcommand foo
docker: unknown command: 'docker volume nosuchcommand'
Usage: docker volume COMMAND
Run 'docker volume --help' for more information
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
This error didn't do a great job at formatting. If a StatusError was
produced without a Status message, it would print a very non-informative
error, with information missing.
Let's update the output:
- If a status-message is provided; print just that (after all the
status code is something that can be found from the shell, e.g.
through `echo $?` in Bash).
- If no status-message is provided: print a message more similar to
Go's `exec.ExecError`, which uses `os.rocessState.String()` (see [1]).
Before this patch, an error without custom status would print:
Status: , Code: 2
After this patch:
exit status 2
In situations where a custom error-message is provided, the error-message
is print as-is, whereas before this patch, the message got combined with
the `Status:` and `Code:`, which resulted in some odd output.
Before this patch:
docker volume --no-such-flag
Status: unknown flag: --no-such-flag
See 'docker volume --help'.
Usage: docker volume COMMAND
Manage volumes
Commands:
create Create a volume
inspect Display detailed information on one or more volumes
ls List volumes
prune Remove unused local volumes
rm Remove one or more volumes
update Update a volume (cluster volumes only)
Run 'docker volume COMMAND --help' for more information on a command.
, Code: 125
With this patch, the error is shown as-is;
docker volume --no-such-flag
unknown flag: --no-such-flag
See 'docker volume --help'.
Usage: docker volume COMMAND
Manage volumes
Commands:
create Create a volume
inspect Display detailed information on one or more volumes
ls List volumes
prune Remove unused local volumes
rm Remove one or more volumes
update Update a volume (cluster volumes only)
Run 'docker volume COMMAND --help' for more information on a command.
While the exit-code is no longer printed, it's still properly handled;
echo $?
125
[1]: 82c14346d8/src/os/exec_posix.go (L107-L135)
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Trying to make the logic slightly clearer, and adding a custom
message for the skip,
Before this:
=== RUN TestSplitCpArg/absolute_path_with_drive
cp_test.go:184: tc.os == "windows" && runtime.GOOS != "windows" || tc.os == "linux" && runtime.GOOS == "windows"
After this:
=== RUN TestSplitCpArg/absolute_path_with_drive
cp_test.go:184: skipping windows test on non-windows platform
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Introduce a (non-exported) ipamOptions that collects all options for
creating a network.IPAM, so that this utility is more atomic (potentially
even could be moved to a separate package and exported).
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
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 "#01" 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>
Sebastiaan van Stijn
Sebastiaan van Stijn
Paweł Gronowski
Laura Brehm
David Karlsson
Dan Wallis
Alano Terblanche