Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add unit tests for package cli/command/secret #32289

Merged

Conversation

adshmh
Copy link
Contributor

@adshmh adshmh commented Apr 1, 2017

This PR adds unit tests for all commands under cli/command/secret package (create, list, inspect, and remove). Part of the work on #31217

Signed-off-by: Arash Deshmeh adeshmeh@ca.ibm.com

- What I did
Added unit tests for all the commands in package cli/command/secret

- How I did it
Added unit tests and supporting code (builders + test data files in testdata directory) for all the commands in cli/command/secret package (create, ls, inspect, and remove)

- How to verify it
Run the unit tests.

  • There are no unit tests for the cli/command/secret package in current version (no test files)

  • With this PR:
    $ make test-unit
    ...
    ok github.com/docker/docker/cli/command/secret 0.069s coverage: 93.2% of statements
    ...

- Description for the changelog

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

@adshmh adshmh changed the title added unit tests for package cli/command/secret add unit tests for package cli/command/secret Apr 1, 2017
@dnephin dnephin added the status/failing-ci Indicates that the PR in its current state fails the test suite label Apr 3, 2017
Copy link
Member

@dnephin dnephin left a comment

Choose a reason for hiding this comment

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

Thanks! this looks good, but there are some missing comments causing CI to fail

@adshmh adshmh force-pushed the add-unit-tests-for-cli-command-secret-package branch from 4e6c738 to 1771f02 Compare April 3, 2017 21:05
@adshmh
Copy link
Contributor Author

adshmh commented Apr 3, 2017

Thank you for the review. I fixed the comments that were not written correctly and updated the PR.

@thaJeztah
Copy link
Member

Looks like tests are failing @adshmh

21:11:03 --- FAIL: TestSecretListWithConfigFormat (0.00s)
21:11:03 	assert.go:107: ls_test.go:121: Expected 'foo
21:11:03 		bar label1=label-foo,label2=label-bar
21:11:03 		' got 'foo
21:11:03 		bar label1=label-foo,label2=label-bar
21:11:03 		'%!(EXTRA string=foo 
21:11:03 		bar label2=label-bar,label1=label-foo
21:11:03 		, string=foo 
21:11:03 		bar label2=label-bar,label1=label-foo
21:11:03 		)
21:11:03 Error: error removing secret: foo
21:11:03 Usage:
21:11:03   rm SECRET [SECRET...] [flags]
21:11:03 
21:11:03 Aliases:
21:11:03   rm, remove
21:11:03 
21:11:03 
21:11:03 FAIL
21:11:03 coverage: 93.2% of statements
21:11:03 FAIL	github.com/docker/docker/cli/command/secret	0.020s

@adshmh adshmh force-pushed the add-unit-tests-for-cli-command-secret-package branch 2 times, most recently from a45258a to 7c4770b Compare April 4, 2017 04:24
@adshmh
Copy link
Contributor Author

adshmh commented Apr 4, 2017

Thank you for the notification. I think it was due to using multiple labels for test objects (Secret) and the order they appeared in the output (interestingly, all unit-tests passed on my test machine all the time).
It is fixed now.

Copy link
Member

@vdemeester vdemeester left a comment

Choose a reason for hiding this comment

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

Few comments but LGTM 👼 🐮

}{
{
args: []string{"too_few"},
expectedError: "\"create\" requires exactly 2 argument(s)",
Copy link
Member

Choose a reason for hiding this comment

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

It's a nit but I think we could even just use "requires exactly 2 argument(s)" here 👼

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for the review. The PR is now updated (removed references to the command name from the tests expected errors).

expectedError: "\"create\" requires exactly 2 argument(s)",
},
{args: []string{"too", "many", "arguments"},
expectedError: "\"create\" requires exactly 2 argument(s)",
Copy link
Member

Choose a reason for hiding this comment

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

Same here then 😛

}{
{
args: []string{},
expectedError: "\"rm\" requires at least 1 argument(s).",
Copy link
Member

Choose a reason for hiding this comment

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

Same here 👼

Signed-off-by: Arash Deshmeh <adeshmeh@ca.ibm.com>
@adshmh adshmh force-pushed the add-unit-tests-for-cli-command-secret-package branch from 7c4770b to 243a8e6 Compare April 4, 2017 19:35
@dnephin dnephin removed the status/failing-ci Indicates that the PR in its current state fails the test suite label Apr 6, 2017
Copy link
Member

@dnephin dnephin left a comment

Choose a reason for hiding this comment

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

LGTM

Thanks!

@vdemeester vdemeester merged commit 8b9d54c into moby:master Apr 7, 2017
@GordonTheTurtle GordonTheTurtle added this to the 17.05.0 milestone Apr 7, 2017
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.

None yet

6 participants