Skip to content

Add unit tests to cli/command/volume package#31124

Merged
thaJeztah merged 1 commit intomoby:masterfrom
vdemeester:volume-unit-tests
Mar 1, 2017
Merged

Add unit tests to cli/command/volume package#31124
thaJeztah merged 1 commit intomoby:masterfrom
vdemeester:volume-unit-tests

Conversation

@vdemeester
Copy link
Copy Markdown
Member

Another cli unit test PR, this time, on volumes 👼

ok      github.com/docker/docker/cli/command/volume     0.026s  coverage: 96.1% of statements

/cc @aaronlehmann @thaJeztah @cpuguy83 @dnephin @icecrime

🐸

Signed-off-by: Vincent Demeester vincent@sbr.pm

@AkihiroSuda
Copy link
Copy Markdown
Member

00:10:04.558 Errors from golint:
00:10:04.558 cli/internal/test/builders/doc.go:1:1: package comment should be of the form "Package builders ..."
00:10:04.558 cli/internal/test/cli.go:68:1: exported method FakeCli.ConfigFile should have comment or be unexported

@dnephin
Copy link
Copy Markdown
Member

dnephin commented Feb 21, 2017

The test failure is related to this change

@vdemeester
Copy link
Copy Markdown
Member Author

@dnephin yep I need to update 😛

Signed-off-by: Vincent Demeester <vincent@sbr.pm>
"golang.org/x/net/context"
)

type fakeClient struct {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice.. this reminds me of -> #30454

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know 😝

}

func runCreate(dockerCli *command.DockerCli, opts createOptions) error {
func runCreate(dockerCli command.Cli, opts createOptions) error {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Curious about removing the pointer reference here?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

command.DockerCli is a struct, command.Cli is an interface. I don't want a pointer on the interface, I just want something that satisfy my interface (and *command.DockerCli is one of them)

volumeInspectFunc func(volumeID string) (types.Volume, error)
expectedError string
}{
{
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think initialising this table value with nil args/volumeInspectFunc would be easier to read over time.

}

func TestVolumeInspectWithoutFormat(t *testing.T) {
testCases := []struct {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since the anonymous struct is re-used several times perhaps you could refactor it to its own named struct type (unexported)

if opts.name != "" {
fmt.Fprint(dockerCli.Err(), "Conflicting options: either specify --name or provide positional arg, not both\n")
return cli.StatusError{StatusCode: 1}
return fmt.Errorf("Conflicting options: either specify --name or provide positional arg, not both\n")
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just confirmed with @vdemeester that the exit code is still 1 here

Copy link
Copy Markdown
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM <3

@thaJeztah thaJeztah merged commit 822abee into moby:master Mar 1, 2017
@GordonTheTurtle GordonTheTurtle added this to the 17.04.0 milestone Mar 1, 2017
@vdemeester vdemeester deleted the volume-unit-tests branch March 1, 2017 13:14
dnephin pushed a commit to dnephin/docker that referenced this pull request Apr 17, 2017
Add unit tests to cli/command/volume package
@vdemeester vdemeester mentioned this pull request May 7, 2017
20 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants