Import TestBuildIidFileSquash from moby to cli by vdemeester · Pull Request #990 · docker/cli

vdemeester · 2018-04-09T13:56:33Z

It's a cli only feature so the test belongs to the cli.

See https://github.com/moby/moby/pull/36746/files#diff-64d1c3a97fd8b1abf170680883e81aaaL6241

It also make test-e2e run against experimental and non-experimental daemon.

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

cpuguy83 · 2018-04-09T14:22:02Z

e2e/compose-env.yaml

      image: 'docker:${TEST_ENGINE_VERSION:-edge-dind}'
      privileged: true
-      command: ['--insecure-registry=registry:5000']
+      command: ['--insecure-registry=registry:5000', '--experimental']


Doesn't this change the whole test suite?

@cpuguy83 yes it does. I think it's fine right now, but we way want to run e2e test against experimental and non-experimental job, but then the changes would be heavier.

The test would also need to ensure that experimental is enabled.
I think it may be prudent to support both modes in e2e and then skip tests that require experimental on non-experimental daemons rather than changing the suite for this test.

thaJeztah

test LGTM, but the --experimental change may need to be discussed

silvin-lubecki

LGTM

thaJeztah · 2018-04-26T14:08:59Z

ping @cpuguy83 this good to go, or want changes w.r.t. the experimental option?

cpuguy83 · 2018-04-26T14:15:10Z

I'm more worried about changing to only testing experimental than I am about having this test.

thaJeztah · 2018-04-26T16:36:15Z

ok, let's hold off merging to see if we can find a solution for that first

vdemeester · 2018-05-17T14:37:33Z

@cpuguy83 @thaJeztah should I go for "skip tests that require experimental on non-experimental" and in a follow-up update the e2e run to support being run in experimental or not ?

thaJeztah · 2018-05-17T14:54:52Z

SGTM for now (if we do the reverse as well, i.e. skip regular tests if experimental is enabled), we could just run them after each other?

vdemeester · 2018-05-17T14:57:14Z

we could just run them after each other?

Ah right

vdemeester · 2018-05-22T09:52:29Z

Updated

# […]
--- SKIP: TestBuildIidFileSquash (0.01s)
	build_test.go:106: !info.ExperimentalBuild: running against a non-experimental daemon
# […]

vdemeester · 2018-05-22T10:32:53Z

And now with an additionnal commit :

Make test-e2e run against experimental and non-experimental daemon

- `make test-e2e` runs the e2e tests twice : once against on
  non-experimental daemon (as before), once against an experimental
  daemon.
- adds `test-e2e-experimental` and `test-e2e-non-experimental` target
  to run tests for the specified cases

thaJeztah · 2018-05-22T10:38:50Z

docker.Makefile


 .PHONY: test-e2e
-test-e2e: binary
+test-e2e: test-e2e-non-experimental test-e2e-experimental


Would be nice to have a make help in this makefile as well (definitely not for this PR though 😄)

thaJeztah · 2018-05-22T10:39:42Z

dockerfiles/Dockerfile.test-e2e-env

 WORKDIR /work
 COPY    scripts/test/e2e scripts/test/e2e
 COPY    e2e/compose-env.yaml e2e/compose-env.yaml
+COPY    e2e/compose-env.yaml.experimental e2e/compose-env.yaml.experimental


compose-env-experimental.yaml

I used that to be able to do the file="${file}:${file}.experimental" hack 😛

thaJeztah · 2018-05-22T10:40:17Z

e2e/compose-env.yaml.experimental

@@ -0,0 +1,6 @@
+version: '2.1'


See above:

s/compose-env.yaml.experimental/compose-env-experimental.yaml/

thaJeztah · 2018-05-22T10:41:26Z

e2e/compose-env.yaml.experimental

+
+services:
+  engine:
+      command: ['--insecure-registry=registry:5000', '--experimental']


Should this be with double-quotes (JSON notation)?

["--insecure-registry=registry:5000", "--experimental"]

vdemeester · 2018-06-18T12:10:25Z

@thaJeztah this one should be ready, it does two e2e runs, one with experimental, the other without.

ijc · 2018-06-18T12:32:02Z

scripts/test/e2e/run

+    local file=./e2e/compose-env.yaml
+
+    DOCKERD_EXPERIMENTAL="${DOCKERD_EXPERIMENTAL:-}"
+    test -n "${DOCKERD_EXPERIMENTAL}" && file="${file}:./e2e/compose-env.experimental.yaml"


The calling code uses DOCKERD_EXPERIMENTAL=0, which also passes the test above -- not what you wanted I think?

🤦‍♂️

ijc · 2018-06-18T12:33:16Z

scripts/test/e2e/wrapper

 # Setup, run and teardown e2e test suite in containers.
 set -eu -o pipefail

+echo "exp $DOCKERD_EXPERIMENTAL"


Is this leftover debug? If not then due to set -u above if DOCKERD_EXPERIMENTAL is not set it'll error out I think.

ijc

The test itself LGTM. I had some minor comments on the scaffolding.

vdemeester · 2018-06-19T09:52:35Z

@thaJeztah @ijc updated 😅

ijc

Test itself still looks A-OK but one query and one very minor nit on the infrastructure.

ijc · 2018-06-19T10:03:36Z

scripts/test/e2e/run

+    local file=./e2e/compose-env.yaml
+
+    DOCKERD_EXPERIMENTAL="${DOCKERD_EXPERIMENTAL:-}"
+    test "${DOCKERD_EXPERIMENTAL}" -eq "1" && file="${file}:./e2e/compose-env.experimental.yaml"


Unless you wanted to subsequently export it you could inline the ${DOCKERD_EXPERIMENTAL:-} into the usage in the test, but that's a very minor nit.

ijc · 2018-06-19T10:04:42Z

scripts/test/e2e/run

 function setup {
    local project=$1
-    COMPOSE_PROJECT_NAME=$1 COMPOSE_FILE=$2 docker-compose up --build -d >&2
+    local file=./e2e/compose-env.yaml


Previously $file would (logically) have been $2 but here you seem to have expanded/inlined it without changing the caller AFAICT.

thaJeztah

overall looks good; left a nit and a question

thaJeztah · 2018-06-25T09:25:43Z

e2e/image/build_test.go

+	FROM %s
+	ENV FOO FOO
+	ENV BAR BAR
+	RUN touch /foop`, fixtures.AlpineImage)),


Looks like this Dockerfile will only add a single layer; should we have something that actually has multiple layers before squashing?

(If there's some specific case we're testing for, we may need a comment to describe that case)

Oh good point. I took the test case from moby/moby but yeah we could add one more RUN to it 😉

The original issue was moby/moby#33822, which lead to moby/moby#33824.

Rereading that I think the number of layers was not the issue, just the wrong thing was being returned.

Ah, thanks!

@vdemeester @ijc do think it would be good to add that as a comment/godoc for the test, so that the original intent doesn't get lost?

thaJeztah · 2018-06-25T09:36:10Z

e2e/compose-env.experimental.yaml

+
+services:
+  engine:
+      command: ['--insecure-registry=registry:5000', '--experimental']


looks like my previous comment got collapsed; #990 (comment)

(nit); was wondering if we should prefer to use double-quotes here to be consistent with the Dockerfile format

Ah, I don't really care so I can switch to " 😉

It's a cli only feature so the test belongs to the cli. Signed-off-by: Vincent Demeester <vincent@sbr.pm>

- `make test-e2e` runs the e2e tests twice : once against on non-experimental daemon (as before), once against an experimental daemon. - adds `test-e2e-experimental` and `test-e2e-non-experimental` target to run tests for the specified cases Signed-off-by: Vincent Demeester <vincent@sbr.pm>

vdemeester · 2018-06-25T09:47:00Z

@thaJeztah updated 🎐

thaJeztah

LGTM

vdemeester requested review from cpuguy83 and thaJeztah April 9, 2018 13:56

GordonTheTurtle added the status/0-triage label Apr 9, 2018

vdemeester added status/2-code-review and removed status/0-triage labels Apr 9, 2018

cpuguy83 reviewed Apr 9, 2018

View reviewed changes

thaJeztah approved these changes Apr 9, 2018

View reviewed changes

silvin-lubecki approved these changes Apr 10, 2018

View reviewed changes

vdemeester added the area/testing label Apr 16, 2018

vdemeester force-pushed the e2e-iddfile-from-moby branch from 762ec3e to 3bea26e Compare May 22, 2018 09:52

vdemeester force-pushed the e2e-iddfile-from-moby branch from 3bea26e to eb32506 Compare May 22, 2018 09:56

thaJeztah requested changes May 22, 2018

View reviewed changes

vdemeester force-pushed the e2e-iddfile-from-moby branch 2 times, most recently from 796b0f6 to 507ee91 Compare May 22, 2018 12:45

vdemeester force-pushed the e2e-iddfile-from-moby branch from 507ee91 to 3ea53e7 Compare June 18, 2018 12:09

ijc reviewed Jun 18, 2018

View reviewed changes

vdemeester force-pushed the e2e-iddfile-from-moby branch 2 times, most recently from 2a528bb to dc3929f Compare June 19, 2018 09:44

ijc reviewed Jun 19, 2018

View reviewed changes

vdemeester force-pushed the e2e-iddfile-from-moby branch from dc3929f to f5fabf2 Compare June 19, 2018 12:16

thaJeztah requested changes Jun 25, 2018

View reviewed changes

vdemeester added 2 commits June 25, 2018 11:44

Import TestBuildIidFileSquash from moby to cli

0e83042

It's a cli only feature so the test belongs to the cli. Signed-off-by: Vincent Demeester <vincent@sbr.pm>

vdemeester force-pushed the e2e-iddfile-from-moby branch from f5fabf2 to a522a78 Compare June 25, 2018 09:46

thaJeztah approved these changes Jul 30, 2018

View reviewed changes

thaJeztah merged commit ed335ab into docker:master Jul 30, 2018

GordonTheTurtle added this to the 18.09.0 milestone Jul 30, 2018

vdemeester deleted the e2e-iddfile-from-moby branch July 30, 2018 09:37

Conversation

vdemeester commented Apr 9, 2018 • 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!

Choose a reason for hiding this comment

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

silvin-lubecki left a comment

Choose a reason for hiding this comment

Uh oh!

thaJeztah commented Apr 26, 2018

Uh oh!

cpuguy83 commented Apr 26, 2018

Uh oh!

thaJeztah commented Apr 26, 2018

Uh oh!

vdemeester commented May 17, 2018

Uh oh!

thaJeztah commented May 17, 2018

Uh oh!

vdemeester commented May 17, 2018

Uh oh!

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

Uh oh!

Uh oh!

vdemeester commented May 22, 2018

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 Jun 18, 2018

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!

ijc left a comment

Choose a reason for hiding this comment

Uh oh!

vdemeester commented Jun 19, 2018

Uh oh!

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

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vdemeester commented Jun 25, 2018

Uh oh!

vdemeester commented Apr 9, 2018 •

edited

Loading

vdemeester commented May 22, 2018 •

edited

Loading