From 5a57da9aba3ca7dff4966c63db84e7fd953d6f36 Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Mon, 5 Jun 2017 12:04:38 -0400 Subject: [PATCH] Add tests for exec and cleanup existing tests. Improve test coverage Reduce cyclo to 16 Signed-off-by: Daniel Nephin --- cli/command/container/client_test.go | 136 ++++++++++++++++++ cli/command/container/exec.go | 24 ++-- cli/command/container/exec_test.go | 171 +++++++++++++---------- cli/command/container/stats_helpers.go | 1 + cli/command/formatter/custom_test.go | 7 +- cli/command/formatter/image_test.go | 24 +++- cli/command/service/progress/progress.go | 1 + cli/command/service/ps.go | 11 +- cli/compose/loader/loader.go | 1 - cli/compose/loader/volume_test.go | 10 ++ gometalinter.json | 2 +- 11 files changed, 295 insertions(+), 93 deletions(-) diff --git a/cli/command/container/client_test.go b/cli/command/container/client_test.go index 1e2d5d9cf876..caabe43880f9 100644 --- a/cli/command/container/client_test.go +++ b/cli/command/container/client_test.go @@ -1,7 +1,13 @@ package container import ( + "io" + "time" + "github.com/docker/docker/api/types" + "github.com/docker/docker/api/types/container" + "github.com/docker/docker/api/types/filters" + "github.com/docker/docker/api/types/network" "github.com/docker/docker/client" "golang.org/x/net/context" ) @@ -9,6 +15,7 @@ import ( type fakeClient struct { client.Client containerInspectFunc func(string) (types.ContainerJSON, error) + execInspectFunc func(ctx context.Context, execID string) (types.ContainerExecInspect, error) } func (cli *fakeClient) ContainerInspect(_ context.Context, containerID string) (types.ContainerJSON, error) { @@ -17,3 +24,132 @@ func (cli *fakeClient) ContainerInspect(_ context.Context, containerID string) ( } return types.ContainerJSON{}, nil } + +func (cli *fakeClient) ContainerAttach(ctx context.Context, container string, options types.ContainerAttachOptions) (types.HijackedResponse, error) { + return types.HijackedResponse{}, nil +} + +func (cli *fakeClient) ContainerCommit(ctx context.Context, container string, options types.ContainerCommitOptions) (types.IDResponse, error) { + return types.IDResponse{}, nil +} + +func (cli *fakeClient) ContainerCreate( + ctx context.Context, + config *container.Config, + hostConfig *container.HostConfig, + networkingConfig *network.NetworkingConfig, + containerName string, +) (container.ContainerCreateCreatedBody, error) { + return container.ContainerCreateCreatedBody{}, nil +} + +func (cli *fakeClient) ContainerDiff(ctx context.Context, container string) ([]container.ContainerChangeResponseItem, error) { + return nil, nil +} + +func (cli *fakeClient) ContainerExecAttach(ctx context.Context, execID string, config types.ExecConfig) (types.HijackedResponse, error) { + return types.HijackedResponse{}, nil +} + +func (cli *fakeClient) ContainerExecCreate(ctx context.Context, container string, config types.ExecConfig) (types.IDResponse, error) { + return types.IDResponse{}, nil +} + +func (cli *fakeClient) ContainerExecInspect(ctx context.Context, execID string) (types.ContainerExecInspect, error) { + if cli.execInspectFunc != nil { + return cli.execInspectFunc(ctx, execID) + } + return types.ContainerExecInspect{}, nil +} + +func (cli *fakeClient) ContainerExecResize(ctx context.Context, execID string, options types.ResizeOptions) error { + return nil +} + +func (cli *fakeClient) ContainerExecStart(ctx context.Context, execID string, config types.ExecStartCheck) error { + return nil +} + +func (cli *fakeClient) ContainerExport(ctx context.Context, container string) (io.ReadCloser, error) { + return nil, nil +} + +func (cli *fakeClient) ContainerInspectWithRaw(ctx context.Context, container string, getSize bool) (types.ContainerJSON, []byte, error) { + return types.ContainerJSON{}, nil, nil +} + +func (cli *fakeClient) ContainerKill(ctx context.Context, container, signal string) error { + return nil +} + +func (cli *fakeClient) ContainerList(ctx context.Context, options types.ContainerListOptions) ([]types.Container, error) { + return nil, nil +} + +func (cli *fakeClient) ContainerLogs(ctx context.Context, container string, options types.ContainerLogsOptions) (io.ReadCloser, error) { + return nil, nil +} + +func (cli *fakeClient) ContainerPause(ctx context.Context, container string) error { + return nil +} + +func (cli *fakeClient) ContainerRemove(ctx context.Context, container string, options types.ContainerRemoveOptions) error { + return nil +} + +func (cli *fakeClient) ContainerRename(ctx context.Context, container, newContainerName string) error { + return nil +} + +func (cli *fakeClient) ContainerResize(ctx context.Context, container string, options types.ResizeOptions) error { + return nil +} + +func (cli *fakeClient) ContainerRestart(ctx context.Context, container string, timeout *time.Duration) error { + return nil +} + +func (cli *fakeClient) ContainerStatPath(ctx context.Context, container, path string) (types.ContainerPathStat, error) { + return types.ContainerPathStat{}, nil +} + +func (cli *fakeClient) ContainerStats(ctx context.Context, container string, stream bool) (types.ContainerStats, error) { + return types.ContainerStats{}, nil +} + +func (cli *fakeClient) ContainerStart(ctx context.Context, container string, options types.ContainerStartOptions) error { + return nil +} + +func (cli *fakeClient) ContainerStop(ctx context.Context, container string, timeout *time.Duration) error { + return nil +} + +func (cli *fakeClient) ContainerTop(ctx context.Context, containerID string, arguments []string) (container.ContainerTopOKBody, error) { + return container.ContainerTopOKBody{}, nil +} + +func (cli *fakeClient) ContainerUnpause(ctx context.Context, container string) error { + return nil +} + +func (cli *fakeClient) ContainerUpdate(ctx context.Context, containerID string, updateConfig container.UpdateConfig) (container.ContainerUpdateOKBody, error) { + return container.ContainerUpdateOKBody{}, nil +} + +func (cli *fakeClient) ContainerWait(ctx context.Context, container string, condition container.WaitCondition) (<-chan container.ContainerWaitOKBody, <-chan error) { + return nil, nil +} + +func (cli *fakeClient) CopyFromContainer(ctx context.Context, container, srcPath string) (io.ReadCloser, types.ContainerPathStat, error) { + return nil, types.ContainerPathStat{}, nil +} + +func (cli *fakeClient) CopyToContainer(ctx context.Context, container, path string, content io.Reader, options types.CopyToContainerOptions) error { + return nil +} + +func (cli *fakeClient) ContainersPrune(ctx context.Context, pruneFilters filters.Args) (types.ContainersPruneReport, error) { + return types.ContainersPruneReport{}, nil +} diff --git a/cli/command/container/exec.go b/cli/command/container/exec.go index 449efbee46a4..115dc1d73fb8 100644 --- a/cli/command/container/exec.go +++ b/cli/command/container/exec.go @@ -7,6 +7,7 @@ import ( "github.com/Sirupsen/logrus" "github.com/docker/cli/cli" "github.com/docker/cli/cli/command" + "github.com/docker/cli/cli/config/configfile" "github.com/docker/cli/opts" "github.com/docker/docker/api/types" apiclient "github.com/docker/docker/client" @@ -63,19 +64,11 @@ func NewExecCommand(dockerCli command.Cli) *cobra.Command { } func runExec(dockerCli command.Cli, options *execOptions, container string, execCmd []string) error { - execConfig, err := parseExec(options, execCmd) - // just in case the ParseExec does not exit - if container == "" || err != nil { - return cli.StatusError{StatusCode: 1} + execConfig := parseExec(options, dockerCli.ConfigFile(), execCmd) + if container == "" { + return cli.StatusError{StatusCode: 1, Status: "No container id or name"} } - if options.detachKeys != "" { - dockerCli.ConfigFile().DetachKeys = options.detachKeys - } - - // Send client escape keys - execConfig.DetachKeys = dockerCli.ConfigFile().DetachKeys - ctx := context.Background() client := dockerCli.Client() @@ -187,7 +180,7 @@ func getExecExitStatus(ctx context.Context, client apiclient.ContainerAPIClient, // parseExec parses the specified args for the specified command and generates // an ExecConfig from it. -func parseExec(opts *execOptions, execCmd []string) (*types.ExecConfig, error) { +func parseExec(opts *execOptions, configFile *configfile.ConfigFile, execCmd []string) *types.ExecConfig { execConfig := &types.ExecConfig{ User: opts.user, Privileged: opts.privileged, @@ -209,5 +202,10 @@ func parseExec(opts *execOptions, execCmd []string) (*types.ExecConfig, error) { execConfig.Env = opts.env.GetAll() } - return execConfig, nil + if opts.detachKeys != "" { + execConfig.DetachKeys = opts.detachKeys + } else { + execConfig.DetachKeys = configFile.DetachKeys + } + return execConfig } diff --git a/cli/command/container/exec_test.go b/cli/command/container/exec_test.go index b2df1c2b0c75..c00e7b779ee3 100644 --- a/cli/command/container/exec_test.go +++ b/cli/command/container/exec_test.go @@ -5,33 +5,39 @@ import ( "io/ioutil" "testing" + "github.com/docker/cli/cli" "github.com/docker/cli/cli/config/configfile" "github.com/docker/cli/cli/internal/test" "github.com/docker/docker/api/types" "github.com/docker/docker/pkg/testutil" "github.com/pkg/errors" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "golang.org/x/net/context" ) -type arguments struct { - options execOptions - execCmd []string -} - func TestParseExec(t *testing.T) { - valids := map[*arguments]*types.ExecConfig{ + testcases := []struct { + options execOptions + execCmd []string + configFile configfile.ConfigFile + expected types.ExecConfig + }{ { execCmd: []string{"command"}, - }: { - Cmd: []string{"command"}, - AttachStdout: true, - AttachStderr: true, + expected: types.ExecConfig{ + Cmd: []string{"command"}, + AttachStdout: true, + AttachStderr: true, + }, }, { execCmd: []string{"command1", "command2"}, - }: { - Cmd: []string{"command1", "command2"}, - AttachStdout: true, - AttachStderr: true, + expected: types.ExecConfig{ + Cmd: []string{"command1", "command2"}, + AttachStdout: true, + AttachStderr: true, + }, }, { options: execOptions{ @@ -40,25 +46,24 @@ func TestParseExec(t *testing.T) { user: "uid", }, execCmd: []string{"command"}, - }: { - User: "uid", - AttachStdin: true, - AttachStdout: true, - AttachStderr: true, - Tty: true, - Cmd: []string{"command"}, + expected: types.ExecConfig{ + User: "uid", + AttachStdin: true, + AttachStdout: true, + AttachStderr: true, + Tty: true, + Cmd: []string{"command"}, + }, }, { options: execOptions{ detach: true, }, execCmd: []string{"command"}, - }: { - AttachStdin: false, - AttachStdout: false, - AttachStderr: false, - Detach: true, - Cmd: []string{"command"}, + expected: types.ExecConfig{ + Detach: true, + Cmd: []string{"command"}, + }, }, { options: execOptions{ @@ -67,58 +72,84 @@ func TestParseExec(t *testing.T) { detach: true, }, execCmd: []string{"command"}, - }: { - AttachStdin: false, - AttachStdout: false, - AttachStderr: false, - Detach: true, - Tty: true, - Cmd: []string{"command"}, + expected: types.ExecConfig{ + Detach: true, + Tty: true, + Cmd: []string{"command"}, + }, + }, + { + execCmd: []string{"command"}, + options: execOptions{detach: true}, + configFile: configfile.ConfigFile{DetachKeys: "de"}, + expected: types.ExecConfig{ + Cmd: []string{"command"}, + DetachKeys: "de", + Detach: true, + }, + }, + { + execCmd: []string{"command"}, + options: execOptions{detach: true, detachKeys: "ab"}, + configFile: configfile.ConfigFile{DetachKeys: "de"}, + expected: types.ExecConfig{ + Cmd: []string{"command"}, + DetachKeys: "ab", + Detach: true, + }, }, } - for valid, expectedExecConfig := range valids { - execConfig, err := parseExec(&valid.options, valid.execCmd) - if err != nil { - t.Fatal(err) - } - if !compareExecConfig(expectedExecConfig, execConfig) { - t.Fatalf("Expected [%v] for %v, got [%v]", expectedExecConfig, valid, execConfig) - } + for _, testcase := range testcases { + execConfig := parseExec(&testcase.options, &testcase.configFile, testcase.execCmd) + assert.Equal(t, testcase.expected, *execConfig) } } -func compareExecConfig(config1 *types.ExecConfig, config2 *types.ExecConfig) bool { - if config1.AttachStderr != config2.AttachStderr { - return false - } - if config1.AttachStdin != config2.AttachStdin { - return false - } - if config1.AttachStdout != config2.AttachStdout { - return false - } - if config1.Detach != config2.Detach { - return false - } - if config1.Privileged != config2.Privileged { - return false - } - if config1.Tty != config2.Tty { - return false - } - if config1.User != config2.User { - return false - } - if len(config1.Cmd) != len(config2.Cmd) { - return false +func TestRunExec(t *testing.T) { + client := &fakeClient{} + out := new(bytes.Buffer) + cli := test.NewFakeCli(client, out) + cli.SetConfigfile(&configfile.ConfigFile{}) + options := &execOptions{detach: true} + + err := runExec(cli, options, "cid", []string{"bash"}) + require.NoError(t, err) +} + +func TestGetExecExitStatus(t *testing.T) { + execID := "the exec id" + expecatedErr := errors.New("unexpected error") + + testcases := []struct { + inspectError error + exitCode int + expectedError error + }{ + { + inspectError: nil, + expectedError: nil, + }, + { + inspectError: expecatedErr, + expectedError: expecatedErr, + }, + { + exitCode: 15, + expectedError: cli.StatusError{StatusCode: 15}, + }, } - for index, value := range config1.Cmd { - if value != config2.Cmd[index] { - return false + + for _, testcase := range testcases { + client := &fakeClient{ + execInspectFunc: func(ctx context.Context, id string) (types.ContainerExecInspect, error) { + assert.Equal(t, execID, id) + return types.ContainerExecInspect{ExitCode: testcase.exitCode}, testcase.inspectError + }, } + err := getExecExitStatus(context.Background(), client, execID) + assert.Equal(t, testcase.expectedError, err) } - return true } func TestNewExecCommandErrors(t *testing.T) { diff --git a/cli/command/container/stats_helpers.go b/cli/command/container/stats_helpers.go index eb12cd0dec47..3b4bd66bba3f 100644 --- a/cli/command/container/stats_helpers.go +++ b/cli/command/container/stats_helpers.go @@ -52,6 +52,7 @@ func (s *stats) isKnownContainer(cid string) (int, bool) { return -1, false } +// nolint: gocyclo func collect(ctx context.Context, s *formatter.ContainerStats, cli client.APIClient, streamStats bool, waitFirst *sync.WaitGroup) { logrus.Debugf("collecting stats for %s", s.Container) var ( diff --git a/cli/command/formatter/custom_test.go b/cli/command/formatter/custom_test.go index da42039dca2d..a9f6ccdac9cb 100644 --- a/cli/command/formatter/custom_test.go +++ b/cli/command/formatter/custom_test.go @@ -1,9 +1,10 @@ package formatter import ( - "reflect" "strings" "testing" + + "github.com/stretchr/testify/assert" ) func compareMultipleValues(t *testing.T, value, expected string) { @@ -22,7 +23,5 @@ func compareMultipleValues(t *testing.T, value, expected string) { keyval := strings.Split(expected, "=") expMap[keyval[0]] = keyval[1] } - if !reflect.DeepEqual(expMap, entriesMap) { - t.Fatalf("Expected entries: %v, got: %v", expected, value) - } + assert.Equal(t, expMap, entriesMap) } diff --git a/cli/command/formatter/image_test.go b/cli/command/formatter/image_test.go index b3c4cc8094b9..ee22fe669556 100644 --- a/cli/command/formatter/image_test.go +++ b/cli/command/formatter/image_test.go @@ -55,6 +55,26 @@ func TestImageContext(t *testing.T) { i: types.ImageSummary{}, digest: "sha256:d149ab53f8718e987c3a3024bb8aa0e2caadf6c0328f1d9d850b2a2a67f2819a", }, "sha256:d149ab53f8718e987c3a3024bb8aa0e2caadf6c0328f1d9d850b2a2a67f2819a", ctx.Digest}, + { + imageContext{ + i: types.ImageSummary{Containers: 10}, + }, "10", ctx.Containers, + }, + { + imageContext{ + i: types.ImageSummary{VirtualSize: 10000}, + }, "10kB", ctx.VirtualSize, + }, + { + imageContext{ + i: types.ImageSummary{SharedSize: 10000}, + }, "10kB", ctx.SharedSize, + }, + { + imageContext{ + i: types.ImageSummary{SharedSize: 5000, VirtualSize: 20000}, + }, "15kB", ctx.UniqueSize, + }, } for _, c := range cases { @@ -62,8 +82,8 @@ func TestImageContext(t *testing.T) { v := c.call() if strings.Contains(v, ",") { compareMultipleValues(t, v, c.expValue) - } else if v != c.expValue { - t.Fatalf("Expected %s, was %s\n", c.expValue, v) + } else { + assert.Equal(t, c.expValue, v) } } } diff --git a/cli/command/service/progress/progress.go b/cli/command/service/progress/progress.go index d522fc08b44f..05dfa23cbeec 100644 --- a/cli/command/service/progress/progress.go +++ b/cli/command/service/progress/progress.go @@ -340,6 +340,7 @@ type globalProgressUpdater struct { done bool } +// nolint: gocyclo func (u *globalProgressUpdater) update(service swarm.Service, tasks []swarm.Task, activeNodes map[string]swarm.Node, rollback bool) (bool, error) { // If there are multiple tasks with the same node ID, favor the one // with the *lowest* desired state. This can happen in restart diff --git a/cli/command/service/ps.go b/cli/command/service/ps.go index 741f6b589f2b..38e63c6f8f98 100644 --- a/cli/command/service/ps.go +++ b/cli/command/service/ps.go @@ -57,6 +57,9 @@ func runPS(dockerCli command.Cli, options psOptions) error { if err != nil { return err } + if err := updateNodeFilter(ctx, client, filter); err != nil { + return err + } tasks, err := client.TaskList(ctx, types.TaskListOptions{Filters: filter}) if err != nil { @@ -135,16 +138,20 @@ loop: if serviceCount == 0 { return filter, nil, errors.New(strings.Join(notfound, "\n")) } + return filter, notfound, err +} + +func updateNodeFilter(ctx context.Context, client client.APIClient, filter filters.Args) error { if filter.Include("node") { nodeFilters := filter.Get("node") for _, nodeFilter := range nodeFilters { nodeReference, err := node.Reference(ctx, client, nodeFilter) if err != nil { - return filter, nil, err + return err } filter.Del("node", nodeFilter) filter.Add("node", nodeReference) } } - return filter, notfound, err + return nil } diff --git a/cli/compose/loader/loader.go b/cli/compose/loader/loader.go index 51cf43ee1526..09aaf95c8e25 100644 --- a/cli/compose/loader/loader.go +++ b/cli/compose/loader/loader.go @@ -566,7 +566,6 @@ func transformServiceVolumeConfig(data interface{}) (interface{}, error) { default: return data, errors.Errorf("invalid type %T for service volume", value) } - } func transformServiceNetworkMap(value interface{}) (interface{}, error) { diff --git a/cli/compose/loader/volume_test.go b/cli/compose/loader/volume_test.go index f1b90fe8eace..1e5b73a9c50b 100644 --- a/cli/compose/loader/volume_test.go +++ b/cli/compose/loader/volume_test.go @@ -200,3 +200,13 @@ func TestParseVolumeSplitCases(t *testing.T) { assert.Equal(t, expected, parsed.Source != "", msg) } } + +func TestParseVolumeInvalidEmptySpec(t *testing.T) { + _, err := ParseVolume("") + testutil.ErrorContains(t, err, "invalid empty volume spec") +} + +func TestParseVolumeInvalidSections(t *testing.T) { + _, err := ParseVolume("/foo::rw") + testutil.ErrorContains(t, err, "invalid spec") +} diff --git a/gometalinter.json b/gometalinter.json index a9530e8d1172..853d797687ea 100644 --- a/gometalinter.json +++ b/gometalinter.json @@ -22,6 +22,6 @@ "vet" ], - "Cyclo": 18, + "Cyclo": 16, "LineLength": 200 }