Unit tests for cli/command/image package by icapurro · Pull Request #32248 · moby/moby

icapurro · 2017-03-31T00:36:56Z

This PR adds unit tests for the cli/command/image package. Part of the work on #31217

Signed-off-by: Ignacio Capurro [email protected]

- What I did
Added unit tests for the following commands in package cli/command/image

To accomplish the 100% package coverage the following two commands need to be tested:

build.go (issue link goes here)
tag.go (issue link goes here)

I will the issues to work on them since this PR was getting really big and those two commands in particular will need some code refactor to accomplish a good coverage.

- How I did it
I followed @vdemeester previous test structure in other cli commands. Used gold files, builders created under cli/internal/test/builders and added new interfaces for some structs to be stubbed.

- How to verify it
Run the unit tests or check the Jenkins build.

Before this PR

ok  	github.com/docker/docker/cli/command/image	0.086s	coverage: 0.0% of statements

After this PR

ok  	github.com/docker/docker/cli/command/image	0.346s	coverage: 45.4% of statements

- Description for the changelog

Added unit test coverage to some cli image commands.

vdemeester · 2017-03-31T10:09:05Z

cli/command/image/tag_test.go

Should contain NewTag instead of Leave (same for below 😝)

Thanks! You can figure out from where I'm copying the test structure now 😄

vdemeester

Big PR ! Nice, thanks for putting this work @icapurro. Few comments from me though 😉,

The less production code is changed when writing tests (afterwards), the better.

We shouldn't modify OutStream or InStream for writing tests (and this cli/command/stream.go, etc.. shouldn't be changed — FakeClican). Why not do a refactoring after if we feel like it, but it shouldn't change a think for the tests — if it does, we coupled our test heavily on the implementation.

On the same note, cli/compose/schema/bindata.go has no reason to change in this PR, don't know why it did, but this change shouldn't be there.

vdemeester · 2017-04-03T10:14:43Z

cli/command/cli.go

I'm pretty sure this is not required and complexify the code 😓

vdemeester · 2017-04-03T10:15:27Z

cli/command/cli.go

Same here, the only change I would see here is the addition of CredentialsStore on the Cli interface.

I did this because I need to stub the method IsTerminal(), and I don't see any other way to do it if the Cli interface only accepts the specific OutStream struct. Do you see any other alternative?

I think you can avoid this change by adding this to cli/command/in_test.go

func NewStubInStream(isTerminal bool) *InStream { return &InStream{ isTerminal: isTerminal, in: ioutil.NopCloser(strings.NewReader("")), } }

and using that stream in the FakeCli.

You can probably leave the existing NewFakeCli() as is, and add a new NewFakeCliWithStreams() that takes all the streams, not just stdout.

@dnephin I did what you propose, but I can't find a way to import a function from a *_test file in package cli/command into another *_test file in package cli/command/image. From what I've been reading it seems not possible, but maybe I'm missing something.

A solution could be to define that function in cli/command/in.go but we will end up with a function intended 100% for testing included in the production binary. 😿

Ah, you're right. I guess I was mistaken.

A function like this (with a better name) might still be ok in in.go, another option would be to create a setter for isTerminal.

Could go in the same package tree as the builders, i.e. cli/internal/test (to make sure it can't be imported outside of cli package 👼 )

vdemeester · 2017-04-03T10:22:00Z

cli/command/image/client_test.go

Hum I used the struct here client.Client, not sure if it does a different though 😛

It shouldn't, I'll change it.

vdemeester · 2017-04-03T10:25:28Z

cli/command/image/history_test.go

I don't think we need golden usage here, a assert.Error(t, cmd.Execute(), tc.expectedError) (with the corresponding expectedError field in the anonymous struct) feels simpler.

It would be more consistent with other cli tests too.

assert.Error does a "contains", so we only need to verify that it contains "requires exactly 1 argument(s)" or …

Ok, I just feel that I need to generalise this as much as possible. But I didn't know assert.Error does a contains , I'll change it.

vdemeester · 2017-04-03T10:28:19Z

cli/command/image/history_test.go

I tend to prefer if tc.outputRegex == "", because it's more readable for a human (and tests are meant to be read by humans). It's a nit though 👼

Ok, let me follow the same style then.

vdemeester · 2017-04-03T12:41:40Z

cli/command/image/trust.go

vdemeester · 2017-04-03T12:41:44Z

cli/command/image/trust.go

vdemeester · 2017-04-03T12:46:27Z

cli/internal/test/cli.go

If we don't introduce OutputStreamer (and I really think we shouldn't), this is not required.

vdemeester · 2017-04-03T12:46:57Z

cli/internal/test/in.go

Same as for OutStreamer, I think we shouldn't have to do that.

vdemeester · 2017-04-03T12:47:08Z

cli/internal/test/out.go

Neither that one.

vdemeester · 2017-04-03T12:53:47Z

Ah, and there is a build error on gofmt.

09:39:31 These files are not properly gofmt'd:
09:39:31  - cli/command/image/client_test.go
09:39:31  - cli/command/image/history_test.go
09:39:31  - cli/command/image/import_test.go
09:39:31  - cli/command/image/inspect_test.go
09:39:31  - cli/command/image/list_test.go
09:39:31  - cli/command/image/load_test.go
09:39:31 
09:39:31 Please reformat the above files using "gofmt -s -w" and commit the result.

icapurro · 2017-04-03T15:27:55Z

@vdemeester Thanks for the thorough review! I applied most of your comments but I still have question on the following:

~~The OutputStreamer interface. I need a way to stub the IsTerminal method from cli.Out()~~
A nicer approach to count method invocations in cli/command/image/inspect_test.go

icapurro · 2017-04-04T02:17:03Z

@dnephin I went in the direction of creating a setter SetIsTerminal(tty bool) in the CommonStream struct that is applied to both InStream and OutStream.

FYI: CommonStream was created in this PR to remove the duplicated code from InStream and OutStream.

icapurro · 2017-04-18T21:56:26Z

@dnephin @vdemeester love the moby project initiative, but I think the repo rename broke the PR update... I made the changes in my branch but they are not appearing here. Should I create a new PR?

dnephin · 2017-04-18T22:02:20Z

I think it's broken for all PRs. We're looking into it. Don't open a new PR just yet.

icapurro · 2017-04-22T05:00:46Z

@dnephin can we re run experimental and windowsRS1 please? thanks!

Signed-off-by: Ignacio Capurro <[email protected]>

icapurro · 2017-04-24T13:40:33Z

Should be done

dnephin

LGTM

lowenna · 2017-04-24T23:14:28Z

😢 This has broken docker run -it on Windows....

vdemeester · 2017-04-25T06:50:36Z

@jhowardmsft arg, really ? I'll have to try it out and see how to fix it then 😓

aaronlehmann · 2017-04-25T20:45:46Z

Sorry I didn't have a chance to review the final version.

With testify, the expected value comes before the actual value. So:

assert.Equal(t, ref, "image:local")

should be:

assert.Equal(t, "image:local", ref)

... and so on.

There are also some opportunities to use testify helpers like assert.Len and assert.True.

I can make these changes if that's helpful.

aaronlehmann · 2017-04-25T23:27:08Z

Opened #32829

thaJeztah · 2024-10-08T17:44:30Z

cli/command/image/save_test.go

+			imageSaveFunc: func(images []string) (io.ReadCloser, error) {
+				return ioutil.NopCloser(strings.NewReader("")), nil
+			},


Heh; 7 Years later, and I stumble on this line to discover that tc.imageSaveFunc was not wired up here, so this test didn't actually run the tests 😂

cli/command/image: fix TestNewSaveCommandSuccess to actually test docker/cli#5520

GordonTheTurtle added the status/0-triage label Mar 31, 2017

vdemeester self-requested a review March 31, 2017 10:07

vdemeester reviewed Mar 31, 2017

View reviewed changes

vdemeester mentioned this pull request Mar 31, 2017

[tests] cli unit tests #31217

Closed

19 tasks

vdemeester requested review from dnephin and vdemeester April 1, 2017 18:42

vdemeester added status/2-code-review and removed status/0-triage labels Apr 1, 2017

GordonTheTurtle added the dco/no Automatically set by a bot when one of the commits lacks proper signature label Apr 1, 2017

icapurro force-pushed the cli-image-tests branch from 74489ee to d4e6676 Compare April 1, 2017 19:56

GordonTheTurtle removed the dco/no Automatically set by a bot when one of the commits lacks proper signature label Apr 1, 2017

icapurro force-pushed the cli-image-tests branch from 525cfc7 to dd921f6 Compare April 3, 2017 02:43

icapurro changed the title ~~[wip] Tests for cli/command/image package~~ Tests for cli/command/image package Apr 3, 2017

icapurro changed the title ~~Tests for cli/command/image package~~ Unit tests for cli/command/image package Apr 3, 2017

vdemeester added the status/failing-ci Indicates that the PR in its current state fails the test suite label Apr 3, 2017

vdemeester suggested changes Apr 3, 2017

View reviewed changes

icapurro force-pushed the cli-image-tests branch from 1fdcdf7 to d85daad Compare April 4, 2017 02:14

vdemeester self-assigned this Apr 4, 2017

GordonTheTurtle added the dco/no Automatically set by a bot when one of the commits lacks proper signature label Apr 5, 2017

icapurro force-pushed the cli-image-tests branch from 6f83d07 to a9d4422 Compare April 5, 2017 00:01

GordonTheTurtle removed the dco/no Automatically set by a bot when one of the commits lacks proper signature label Apr 5, 2017

icapurro force-pushed the cli-image-tests branch from a9d4422 to 1805315 Compare April 5, 2017 04:06

dnephin added rebuild/z and removed status/failing-ci Indicates that the PR in its current state fails the test suite labels Apr 5, 2017

GordonTheTurtle removed the rebuild/z label Apr 5, 2017

icapurro force-pushed the cli-image-tests branch from 9e49b04 to 3aafdd6 Compare April 19, 2017 19:50

GordonTheTurtle added the dco/no Automatically set by a bot when one of the commits lacks proper signature label Apr 20, 2017

icapurro force-pushed the cli-image-tests branch from 3aafdd6 to 8e354a6 Compare April 21, 2017 14:39

GordonTheTurtle removed the dco/no Automatically set by a bot when one of the commits lacks proper signature label Apr 21, 2017

icapurro force-pushed the cli-image-tests branch 3 times, most recently from 461c5ea to c3ae796 Compare April 22, 2017 00:19

dnephin added rebuild/experimental labels Apr 22, 2017

GordonTheTurtle removed rebuild/experimental labels Apr 22, 2017

Unit tests for cli/commands/image (except build and tag)

b2551c6

Signed-off-by: Ignacio Capurro <[email protected]>

icapurro force-pushed the cli-image-tests branch from c3ae796 to b2551c6 Compare April 23, 2017 17:28

dnephin approved these changes Apr 24, 2017

View reviewed changes

vdemeester merged commit 663f0ba into moby:master Apr 24, 2017

lowenna mentioned this pull request Apr 24, 2017

Windows: docker run -it is broken on master #32799

Closed

vdemeester mentioned this pull request May 26, 2017

unit tests on commands docker/cli#37

Closed

20 tasks

thaJeztah mentioned this pull request Feb 24, 2023

formatter: Consider empty RepoTags and RepoDigests as dangling docker/cli#4046

Merged

vvoland mentioned this pull request Feb 24, 2023

test/cli: Use empty array as empty output of images/json docker/cli#4050

Merged

thaJeztah mentioned this pull request Apr 1, 2023

cli/streams: minor refactoring and docs touch-ups docker/cli#4153

Merged

thaJeztah mentioned this pull request Oct 8, 2024

cli/command/image: fix TestNewSaveCommandSuccess to actually test docker/cli#5520

Merged

thaJeztah reviewed Oct 8, 2024

View reviewed changes

Conversation

icapurro commented Mar 31, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vdemeester left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

icapurro Apr 4, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vdemeester commented Apr 3, 2017

Uh oh!

icapurro commented Apr 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

icapurro commented Apr 4, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

icapurro commented Apr 18, 2017

Uh oh!

dnephin commented Apr 18, 2017

Uh oh!

icapurro commented Apr 22, 2017

Uh oh!

icapurro commented Apr 24, 2017

Uh oh!

dnephin left a comment

Choose a reason for hiding this comment

Uh oh!

lowenna commented Apr 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vdemeester commented Apr 25, 2017

Uh oh!

aaronlehmann commented Apr 25, 2017

Uh oh!

aaronlehmann commented Apr 25, 2017

Uh oh!

Choose a reason for hiding this comment

icapurro commented Mar 31, 2017 •

edited

Loading

icapurro Apr 4, 2017 •

edited

Loading

icapurro commented Apr 3, 2017 •

edited

Loading

icapurro commented Apr 4, 2017 •

edited

Loading

lowenna commented Apr 24, 2017 •

edited

Loading