Migrate volumes tests in integration-cli to api tests by yongtang · Pull Request #36292 · moby/moby

yongtang · 2018-02-12T23:12:32Z

This fix migrates volumes tests in integration-cli to api tests in integration/

Signed-off-by: Yong Tang yong.tang.github@outlook.com

vdemeester

LGTM 🦁

vdemeester · 2018-02-13T08:44:34Z

We could create an helper for that one (because I think it's gonna be use quite a lot)

container.Run(t, ctx, client, WithVolume("source:target"), WithVolume("another:one"))

@yongtang @dnephin wdyt ?

There are actually two formats. One is source:target which is added to c.HostConfig.Binds. Another is path (without :) which is added to c.Config.Volumes. Maybe one WithBind() and another one WithVolume()?

WithBind(src, target string) WithVolume(name string) SGTM

Updated. Thanks.

yongtang · 2018-02-13T17:18:06Z

Thanks @vdemeester @dnephin for the review. The PR has been updated. Please take a look.

dnephin

Thanks, a few comments

dnephin · 2018-02-13T18:00:54Z

Checking the list contains a specific name is a pretty weak assertion.

Can we use a label and filter so that we can use assert.Equal() ?
Can we compare the entire struct instead of just names?

Thanks @dnephin. I updated the PR so that now all elements (except for create time) is compared against.

dnephin · 2018-02-13T18:02:06Z

Does this need to create a container? Could we use the volume create API instead? It should be much faster and lets us test the volume system independently from containers.

Thanks @dnephin. I think it make sense to not create the container. I updated the PR. Also I merged TestVolumesCreate and TestVolumesList into TestVolumesCreateAndList.

I still see it creating a conatiner here. Is that change not pushed?

Looks like it's needed to get a "volume in use" error below

Likely the container doesn't have to be started though, only created (which would make the test faster to run and cleanup)

@thaJeztah @dnephin Thanks. The PR has been updated. Now the container is created but not started.

dnephin · 2018-02-13T18:02:39Z

Could this assert.Equal() the entire struct instead of just the mountpoint?

Done. It is now compared against the whole Volume struct.

dnephin · 2018-02-13T18:03:04Z

In this case I guess the container is necessary to test the "in use" case. It would be great if this could be done with a unit test instead, because it would be easier to set up that state without involving the container subsystem.

I guess it's ok like this.

dnephin · 2018-02-13T18:09:29Z

https://godoc.org/github.com/gotestyourself/gotestyourself/assert/cmp#DeepEqual should make this type of comparison a lot cleaner. I hope we can use it soon.

@dnephin Thanks. It seems that a vendor update is needed. I will take a look and create a follow up PR for that.

Don't worry about using it just yet. I have an automated migration that we can use to convert things all at once. I'll open a PR for it soon.

yongtang · 2018-02-13T22:41:44Z

@dnephin Thanks for the review. The PR has been updated. I will create a follow up PR to try to utilize DeepEqual. Please take a look.

vdemeester · 2018-02-14T07:17:38Z

@yongtang hum, janky failed

00:14:11 --- FAIL: TestVolumesCreateAndList (0.02s)
00:14:11 	assertions.go:226: 
                          
	Error Trace:	volume_test.go:45
00:14:11 		
	Error:      	Not equal: 
00:14:11 		
	            	expected: 2
00:14:11 		
	            	received: 1

yongtang · 2018-02-14T12:06:51Z

Think there might be some leftover from previous tests. I added additional logging to take a look.

yongtang · 2018-02-14T12:52:04Z

The build failure is caused by additional volume somevolume, seems to be left from docker-py test?

12:39:53 --- FAIL: TestVolumesCreateAndList (0.02s)
12:39:53 	volume_test.go:46: Volume[0]: {CreatedAt:2018-02-14T12:33:51Z Driver:local Labels:map[] Mountpoint:/var/lib/docker/volumes/somevolume/_data Name:somevolume Options:map[] Scope:local Status:map[] UsageData:<nil>}

yongtang · 2018-02-14T13:40:32Z

Opened a PR docker/docker-py#1909 in docker-py to remove somevolume during test teardown.

This fix updates docker-py: ``` -ENV DOCKER_PY_COMMIT 1d6b5b203222ba5df7dedfcd1ee061a452f99c8a +ENV DOCKER_PY_COMMIT 5e28dcaace5f7b70cbe44c313b7a3b288fa38916 ``` The updated docker-py includes docker/docker-py#1909 which is required to have moby#36292 pass the tests. Full diff is in docker/docker-py@1d6b5b2...5e28dca. Signed-off-by: Yong Tang <yong.tang.github@outlook.com>

yongtang · 2018-02-15T07:49:19Z

docker/docker-py#1909 has been merged. Created a PR #36318 to update the docker-py.

codecov · 2018-02-22T02:54:21Z

Codecov Report

❗ No coverage uploaded for pull request base (master@6004757). Click here to learn what that means.
The diff coverage is n/a.

@@            Coverage Diff            @@
##             master   #36292   +/-   ##
=========================================
  Coverage          ?   34.64%           
=========================================
  Files             ?      612           
  Lines             ?    45297           
  Branches          ?        0           
=========================================
  Hits              ?    15695           
  Misses            ?    27544           
  Partials          ?     2058

thaJeztah · 2018-02-22T10:41:37Z

Looks like there's some failures;

03:25:08 === RUN   TestVolumesCreateAndList
03:25:08 --- FAIL: TestVolumesCreateAndList (0.02s)
03:25:08 	volume_test.go:46: Volume[0]: {CreatedAt:2018-02-22T03:17:44Z Driver:local Labels:map[] Mountpoint:/var/lib/docker/volumes/fd3e13663271fafcf7366b3bf595c1dfd1c6aaa4e34c04997dfc23d332d72dac/_data Name:fd3e13663271fafcf7366b3bf595c1dfd1c6aaa4e34c04997dfc23d332d72dac Options:map[] Scope:local Status:map[] UsageData:<nil>}
03:25:08 	volume_test.go:46: Volume[1]: {CreatedAt:2018-02-22T03:17:42Z Driver:local Labels:map[] Mountpoint:/var/lib/docker/volumes/somevolume/_data Name:somevolume Options:map[] Scope:local Status:map[] UsageData:<nil>}
03:25:08 	volume_test.go:46: Volume[2]: {CreatedAt:2018-02-22T03:25:08Z Driver:local Labels:map[] Mountpoint:/var/lib/docker/volumes/test/_data Name:test Options:map[] Scope:local Status:map[] UsageData:<nil>}
03:25:08 	volume_test.go:46: Volume[3]: {CreatedAt:2018-02-22T03:17:42Z Driver:local Labels:map[] Mountpoint:/var/lib/docker/volumes/1ed15ecbfbe0c65bcfdbd1b19901fe0f1ec312a8f9269699e7e133e8a3ae298d/_data Name:1ed15ecbfbe0c65bcfdbd1b19901fe0f1ec312a8f9269699e7e133e8a3ae298d Options:map[] Scope:local Status:map[] UsageData:<nil>}
03:25:08 	volume_test.go:46: Volume[4]: {CreatedAt:2018-02-22T03:17:45Z Driver:local Labels:map[] Mountpoint:/var/lib/docker/volumes/dc5ef5e600853abc6b2ac97d8c68353e6b06ed4dd58553f79a315cfe10e92b60/_data Name:dc5ef5e600853abc6b2ac97d8c68353e6b06ed4dd58553f79a315cfe10e92b60 Options:map[] Scope:local Status:map[] UsageData:<nil>}
03:25:08 	volume_test.go:46: Volume[5]: {CreatedAt:2018-02-22T03:17:42Z Driver:local Labels:map[] Mountpoint:/var/lib/docker/volumes/f324b219c9599d0a33c5ff2cc546a459c07d0a4d138abed91c2cc0313a1c70b5/_data Name:f324b219c9599d0a33c5ff2cc546a459c07d0a4d138abed91c2cc0313a1c70b5 Options:map[] Scope:local Status:map[] UsageData:<nil>}
03:25:08 	assertions.go:226: 
                          
	Error Trace:	volume_test.go:49
03:25:08 		
	Error:      	Not equal: 
03:25:08 		
	            	expected: 6
03:25:08 		
	            	received: 1
03:25:08 	assertions.go:226: 
                          
	Error Trace:	volume_test.go:51
03:25:08 		
	Error:      	Not equal: 
03:25:08 		
	            	expected: types.Volume{CreatedAt:"2018-02-22T03:17:44Z", Driver:"local", Labels:map[string]string(nil), Mountpoint:"/var/lib/docker/volumes/fd3e13663271fafcf7366b3bf595c1dfd1c6aaa4e34c04997dfc23d332d72dac/_data", Name:"fd3e13663271fafcf7366b3bf595c1dfd1c6aaa4e34c04997dfc23d332d72dac", Options:map[string]string{}, Scope:"local", Status:map[string]interface {}(nil), UsageData:(*types.VolumeUsageData)(nil)}
03:25:08 		
	            	received: types.Volume{CreatedAt:"2018-02-22T03:25:08Z", Driver:"local", Labels:map[string]string(nil), Mountpoint:"/var/lib/docker/volumes/test/_data", Name:"test", Options:map[string]string{}, Scope:"local", Status:map[string]interface {}(nil), UsageData:(*types.VolumeUsageData)(nil)}
03:25:08 		
	            	
03:25:08 		
	            	Diff:
03:25:08 		
	            	--- Expected
03:25:08 		
	            	+++ Actual
03:25:08 		
	            	@@ -1,7 +1,7 @@
03:25:08 		
	            	 (types.Volume) {
03:25:08 		
	            	- CreatedAt: (string) (len=20) "2018-02-22T03:17:44Z",
03:25:08 		
	            	+ CreatedAt: (string) (len=20) "2018-02-22T03:25:08Z",
03:25:08 		
	            	  Driver: (string) (len=5) "local",
03:25:08 		
	            	  Labels: (map[string]string) <nil>,
03:25:08 		
	            	- Mountpoint: (string) (len=94) "/var/lib/docker/volumes/fd3e13663271fafcf7366b3bf595c1dfd1c6aaa4e34c04997dfc23d332d72dac/_data",
03:25:08 		
	            	- Name: (string) (len=64) "fd3e13663271fafcf7366b3bf595c1dfd1c6aaa4e34c04997dfc23d332d72dac",
03:25:08 		
	            	+ Mountpoint: (string) (len=34) "/var/lib/docker/volumes/test/_data",
03:25:08 		
	            	+ Name: (string) (len=4) "test",
03:25:08 		
	            	  Options: (map[string]string) {

Once addressed, can you also squash commits? I think it's clearer if the test "remove" and "move" are in a single commit

yongtang · 2018-02-22T20:44:02Z

@thaJeztah Thanks. I am still trying to figure out where the volumes comes from. Might be from docker-py but I am not so sure. May still need to play with the Jenkins build around. Will squash once I figure it out.

yongtang · 2018-02-23T15:26:59Z

Added a PR in docker-py docker/docker-py#1922

yongtang · 2018-02-25T23:49:09Z

@thaJeztah The PR has been updated. Please take a look.

thaJeztah

Left some suggestions 🤗

thaJeztah · 2018-02-26T11:10:03Z

Doc doesn't match the function? 😅

Thanks @thaJeztah. Done.

thaJeztah · 2018-02-26T11:19:07Z

I know this was discussed in some other PR's; should this be a unique name (to allow tests running in parallel?). Could use t.Name() as part of the volume-name to make it unique, or just a random name

thaJeztah · 2018-02-26T11:20:11Z

Same here with the name. In this case, I don't think we need a name at all (as we can reference the container by it's ID, correct?)

thaJeztah · 2018-02-26T11:20:35Z

Same here 🤗

yongtang · 2018-02-26T19:35:51Z

Thanks @thaJeztah for the review. The PR has been updated.

This fix migrates volumes tests in integration-cli to api tests in integration/ Signed-off-by: Yong Tang <yong.tang.github@outlook.com>

thaJeztah

LGTM, thanks!

yongtang requested review from dnephin and vdemeester as code owners February 12, 2018 23:12

GordonTheTurtle added the status/0-triage label Feb 12, 2018

vdemeester added status/2-code-review and removed status/0-triage labels Feb 13, 2018

vdemeester approved these changes Feb 13, 2018

View reviewed changes

yongtang force-pushed the 0212-2018-volumes-tests branch from fe70d54 to 3efbed4 Compare February 13, 2018 17:17

dnephin reviewed Feb 13, 2018

View reviewed changes

yongtang force-pushed the 0212-2018-volumes-tests branch from 3efbed4 to 489fe2f Compare February 13, 2018 22:33

yongtang added the rebuild/z label Feb 13, 2018

GordonTheTurtle removed the rebuild/z label Feb 13, 2018

yongtang mentioned this pull request Feb 13, 2018

Migrate several docker rm tests to api tests #36293

Merged

yongtang added the rebuild/janky label Feb 13, 2018

GordonTheTurtle removed the rebuild/janky label Feb 13, 2018

yongtang force-pushed the 0212-2018-volumes-tests branch 2 times, most recently from 19b79bf to 2559ad9 Compare February 14, 2018 12:06

yongtang force-pushed the 0212-2018-volumes-tests branch 2 times, most recently from edd0ae8 to bb4efdb Compare February 14, 2018 12:45

yongtang mentioned this pull request Feb 14, 2018

Clean up created volume from test_run_with_named_volume docker/docker-py#1909

Merged

yongtang added the rebuild/janky label Feb 15, 2018

GordonTheTurtle removed the rebuild/janky label Feb 15, 2018

yongtang mentioned this pull request Feb 15, 2018

Update docker-py to 5e28dcaace5f7b70cbe44c313b7a3b288fa38916 #36318

Merged

yongtang force-pushed the 0212-2018-volumes-tests branch from bfc676f to 2f33c0c Compare February 22, 2018 01:13

yongtang force-pushed the 0212-2018-volumes-tests branch 3 times, most recently from 9f0ee2f to 056156a Compare February 22, 2018 22:15

yongtang mentioned this pull request Feb 23, 2018

Cleanup containers during the tests docker/docker-py#1922

Merged

yongtang force-pushed the 0212-2018-volumes-tests branch from 056156a to af955d1 Compare February 25, 2018 19:02

yongtang added the rebuild/experimental label Feb 25, 2018

GordonTheTurtle removed the rebuild/experimental label Feb 25, 2018

thaJeztah requested changes Feb 26, 2018

View reviewed changes

yongtang force-pushed the 0212-2018-volumes-tests branch from af955d1 to be78e25 Compare February 26, 2018 19:31

GordonTheTurtle assigned vdemeester Feb 27, 2018

Migrate volumes tests in integration-cli to api tests

d896f87

This fix migrates volumes tests in integration-cli to api tests in integration/ Signed-off-by: Yong Tang <yong.tang.github@outlook.com>

yongtang force-pushed the 0212-2018-volumes-tests branch from be78e25 to d896f87 Compare February 27, 2018 05:18

vdemeester added the rebuild/z label Feb 27, 2018

GordonTheTurtle removed the rebuild/z label Feb 27, 2018

thaJeztah approved these changes Feb 27, 2018

View reviewed changes

thaJeztah added status/4-merge area/testing and removed status/2-code-review labels Feb 27, 2018

vdemeester merged commit 407e122 into moby:master Feb 27, 2018

yongtang deleted the 0212-2018-volumes-tests branch February 27, 2018 15:48

thaJeztah mentioned this pull request Mar 16, 2018

[18.03] ExportContainer: do not panic docker-archive/docker-ce#468

Merged

This was referenced Apr 10, 2018

[WIP] [18.03] backport test changes docker-archive/docker-ce#502

Closed

[18.03] backport updates to tests, and fix empty LogPath with non-blocking logging mode docker-archive/docker-ce#512

Merged

Conversation

yongtang commented Feb 12, 2018

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!

yongtang commented Feb 13, 2018

Uh oh!

dnephin 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!

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!

yongtang commented Feb 13, 2018

Uh oh!

vdemeester commented Feb 14, 2018

Uh oh!

yongtang commented Feb 14, 2018

Uh oh!

yongtang commented Feb 14, 2018

Uh oh!

yongtang commented Feb 14, 2018

Uh oh!

yongtang commented Feb 15, 2018

Uh oh!

codecov Bot commented Feb 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

thaJeztah commented Feb 22, 2018

Uh oh!

yongtang commented Feb 22, 2018

Uh oh!

yongtang commented Feb 23, 2018

Uh oh!

yongtang commented Feb 25, 2018

Uh oh!

thaJeztah 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!

codecov Bot commented Feb 22, 2018 •

edited

Loading