[test-integration] clean docker_cli_build_test.go #30058

Merged
merged 1 commit into from Jan 16, 2017

Projects

None yet

5 participants

@vdemeester
Member
vdemeester commented Jan 11, 2017 edited

Refactoring a bit some piece of docker_cli_build_test.go.

  • Introduce new buildImage commands (that are extensible and more readable) and deprecate old ones.
buildImageSuccessfully(c, "imageName", withDockerfile("FROM …"))
buildImageSuccessfully(c, "imageName", withBuildFlag("--rm=false"), withDockerfile("FROM …"))
buildImageSuccessfully(c, "imageName", withoutCache, withDockerfile("FROM …"))
// […]
buildImageNew("imageName", withContext(c,
    withFile("Dockerfile", "FROM …"),
    withFile("foo", "bar")
).Assert(c, icmd.Expected{
    ExitCode: 1,
    Err: "something on stderr",
})
  • Small cleanups, refactoring of tests, to make them more readable, more concise or use less compute power 😝

There is still ways to go : some tests need more care to make them more readable and I didn't touch to all of them. The file is way to big (>6000 loc) so.. it's a "first part/wip" but it should lower a little the loc and ease the split later on.

/cc @thaJeztah @icecrime @dnephin @tiborvass @cpuguy83 @duglin

🐸 🦁

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

integration-cli/docker_utils_test.go
+ return icmd.RunCmd(cmd)
+}
+
+func withContext(c *check.C, contextOperators ...func(*FakeContext) error) func(*icmd.Cmd) func() {
@cpuguy83
cpuguy83 Jan 11, 2017 Contributor

withBuildContext

+ return nil
+}
+
+func withFile(name, content string) func(*FakeContext) error {
@cpuguy83
cpuguy83 Jan 11, 2017 Contributor

All these functions are a bit weird in the main testing package. Maybe they need to be split out.

@vdemeester
vdemeester Jan 11, 2017 Member

yeah, wasn't sure yet where to put them, but I want them out of the main integration-cli package indeed 👼

@cpuguy83
Contributor

In general +1.
I do have a strong feeling though that a lot of these build tests can be moved to unit tests.

@vdemeester
Member

I do have a strong feeling though that a lot of these build tests can be moved to unit tests.

Definitely 👼 It's on my list 😝

@AkihiroSuda
Contributor

windowsRS1 failed

00:22:43.979 ----------------------------------------------------------------------
00:22:43.979 FAIL: docker_cli_build_test.go:122: DockerSuite.TestBuildEnvironmentReplacementWorkdir
00:22:43.979 
00:22:43.979 docker_cli_build_test.go:134:
00:22:43.980     c.Fatal("Workdir /workdir from environment not in Config.WorkingDir on image")
00:22:43.980 ... Error: Workdir /workdir from environment not in Config.WorkingDir on image
00:22:43.981 
00:22:59.676 
00:22:59.676 ----------------------------------------------------------------------
@dnephin

This is awesome!

I'm glad the icmd objects are getting more use.

@@ -3378,42 +2555,40 @@ func (s *DockerSuite) TestBuildDockerignoreTouchDockerfile(c *check.C) {
if id1 != id2 {
c.Fatalf("Didn't use the cache - 3")
}
-
}
func (s *DockerSuite) TestBuildDockerignoringWholeDir(c *check.C) {
@vdemeester
vdemeester Jan 11, 2017 Member

This test was really weird. The last 3 builds were wrong because a change in .dockerignore doesn't seem to invalidate the cache. If it did, this tests should have been red as .* ignores only dotfiles, * ignore all and ?, . ignore pretty much nothing.

/cc @duglin @tiborvass @LK4D4

integration-cli/docker_cli_build_test.go
- c.Fatal(err)
+ expected := `"/work"`
+ if daemonPlatform == "windows" {
+ expected = `"c:\work"`
@AkihiroSuda
AkihiroSuda Jan 12, 2017 edited Contributor

"c:\work" -> "C:\\work" (this should fix CI failure)

@vdemeester
vdemeester Jan 12, 2017 Member

Yep, I wasn't sure what it should be 😂

integration-cli/docker_cli_build_test.go
+ expected = `"c:\work"`
+ }
+ if res != expected {
+ c.Fatal("Workdir /workdir from environment not in Config.WorkingDir on image: %s", res)
@AkihiroSuda
AkihiroSuda Jan 12, 2017 Contributor

Fatalf

@@ -4920,50 +3903,23 @@ func (s *DockerSuite) TestBuildFromMixedcaseDockerfile(c *check.C) {
testRequires(c, UnixCli) // Dockerfile overwrites dockerfile on windows
testRequires(c, DaemonIsLinux)
- ctx, err := fakeContext(`FROM busybox
@vdemeester
vdemeester Jan 12, 2017 Member

This test was weird too… The way it was written, there was both Dockerfile and dockerfile in the build context, with the same content… thus it was not testing what it was supposed to test...

@vdemeester
Member

windowsRS1 is green (just didn't report correctly)

@vdemeester vdemeester Refactor docker_cli_build_test.go
Use `testutil/cmd` for `buildCommand`.

Signed-off-by: Vincent Demeester <vincent@sbr.pm>
c778f4b
@vdemeester
Member
@AkihiroSuda

LGTM and merging

@AkihiroSuda AkihiroSuda merged commit dcf1264 into docker:master Jan 16, 2017

4 checks passed

dco-signed All commits are signed
experimental Jenkins build Docker-PRs-experimental 29639 has succeeded
Details
janky Jenkins build Docker-PRs 38233 has succeeded
Details
windowsRS1 Jenkins build Docker-PRs-WoW-RS1 9286 has succeeded
Details
@GordonTheTurtle GordonTheTurtle added this to the 1.14.0 milestone Jan 16, 2017
@vdemeester vdemeester deleted the vdemeester:integration-build-cmd-clean branch Jan 16, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment