Merge pull request #22706 from tkopczynski/20784-builder-intg2unit-tests

Reimplement more builder integration tests as unit tests

builder/dockerfile/evaluator_test.go

@@ -3,6 +3,7 @@ package dockerfile
 import (
 	"io/ioutil"
 	"os"
+	"path/filepath"
 	"strings"
 	"testing"
 
@@ -16,6 +17,7 @@ import (
 
 type dispatchTestCase struct {
 	name, dockerfile, expectedError string
+	files                           map[string]string
 }
 
 func init() {
@@ -34,21 +36,97 @@ func initDispatchTestCases() []dispatchTestCase {
 			name:          "ONBUILD forbidden FROM",
 			dockerfile:    "ONBUILD FROM scratch",
 			expectedError: "FROM isn't allowed as an ONBUILD trigger",
+			files:         nil,
 		},
 		{
 			name:          "ONBUILD forbidden MAINTAINER",
 			dockerfile:    "ONBUILD MAINTAINER docker.io",
 			expectedError: "MAINTAINER isn't allowed as an ONBUILD trigger",
+			files:         nil,
 		},
 		{
 			name:          "ARG two arguments",
 			dockerfile:    "ARG foo bar",
 			expectedError: "ARG requires exactly one argument definition",
+			files:         nil,
 		},
 		{
 			name:          "MAINTAINER unknown flag",
 			dockerfile:    "MAINTAINER --boo joe@example.com",
 			expectedError: "Unknown flag: boo",
+			files:         nil,
+		},
+		{
+			name:          "ADD multiple files to file",
+			dockerfile:    "ADD file1.txt file2.txt test",
+			expectedError: "When using ADD with more than one source file, the destination must be a directory and end with a /",
+			files:         map[string]string{"file1.txt": "test1", "file2.txt": "test2"},
+		},
+		{
+			name:          "JSON ADD multiple files to file",
+			dockerfile:    `ADD ["file1.txt", "file2.txt", "test"]`,
+			expectedError: "When using ADD with more than one source file, the destination must be a directory and end with a /",
+			files:         map[string]string{"file1.txt": "test1", "file2.txt": "test2"},
+		},
+		{
+			name:          "Wiildcard ADD multiple files to file",
+			dockerfile:    "ADD file*.txt test",
+			expectedError: "When using ADD with more than one source file, the destination must be a directory and end with a /",
+			files:         map[string]string{"file1.txt": "test1", "file2.txt": "test2"},
+		},
+		{
+			name:          "Wiildcard JSON ADD multiple files to file",
+			dockerfile:    `ADD ["file*.txt", "test"]`,
+			expectedError: "When using ADD with more than one source file, the destination must be a directory and end with a /",
+			files:         map[string]string{"file1.txt": "test1", "file2.txt": "test2"},
+		},
+		{
+			name:          "COPY multiple files to file",
+			dockerfile:    "COPY file1.txt file2.txt test",
+			expectedError: "When using COPY with more than one source file, the destination must be a directory and end with a /",
+			files:         map[string]string{"file1.txt": "test1", "file2.txt": "test2"},
+		},
+		{
+			name:          "JSON COPY multiple files to file",
+			dockerfile:    `COPY ["file1.txt", "file2.txt", "test"]`,
+			expectedError: "When using COPY with more than one source file, the destination must be a directory and end with a /",
+			files:         map[string]string{"file1.txt": "test1", "file2.txt": "test2"},
+		},
+		{
+			name:          "ADD multiple files to file with whitespace",
+			dockerfile:    `ADD [ "test file1.txt", "test file2.txt", "test" ]`,
+			expectedError: "When using ADD with more than one source file, the destination must be a directory and end with a /",
+			files:         map[string]string{"test file1.txt": "test1", "test file2.txt": "test2"},
+		},
+		{
+			name:          "COPY multiple files to file with whitespace",
+			dockerfile:    `COPY [ "test file1.txt", "test file2.txt", "test" ]`,
+			expectedError: "When using COPY with more than one source file, the destination must be a directory and end with a /",
+			files:         map[string]string{"test file1.txt": "test1", "test file2.txt": "test2"},
+		},
+		{
+			name:          "COPY wildcard no files",
+			dockerfile:    `COPY file*.txt /tmp/`,
+			expectedError: "No source files were specified",
+			files:         nil,
+		},
+		{
+			name:          "COPY url",
+			dockerfile:    `COPY https://index.docker.io/robots.txt /`,
+			expectedError: "Source can't be a URL for COPY",
+			files:         nil,
+		},
+		{
+			name:          "Chaining ONBUILD",
+			dockerfile:    `ONBUILD ONBUILD RUN touch foobar`,
+			expectedError: "Chaining ONBUILD via `ONBUILD ONBUILD` isn't allowed",
+			files:         nil,
+		},
+		{
+			name:          "Invalid instruction",
+			dockerfile:    `foo bar`,
+			expectedError: "Unknown instruction: FOO",
+			files:         nil,
 		}}
 
 	return dispatchTestCases
@@ -66,6 +144,10 @@ func executeTestCase(t *testing.T, testCase dispatchTestCase) {
 	contextDir, cleanup := createTestTempDir(t, "", "builder-dockerfile-test")
 	defer cleanup()
 
+	for filename, content := range testCase.files {
+		createTestTempFile(t, contextDir, filename, content, 0777)
+	}
+
 	tarStream, err := archive.Tar(contextDir, archive.Uncompressed)
 
 	if err != nil {
@@ -132,3 +214,16 @@ func createTestTempDir(t *testing.T, dir, prefix string) (string, func()) {
 		}
 	}
 }
+
+// createTestTempFile creates a temporary file within dir with specific contents and permissions.
+// When an error occurs, it terminates the test
+func createTestTempFile(t *testing.T, dir, filename, contents string, perm os.FileMode) string {
+	filePath := filepath.Join(dir, filename)
+	err := ioutil.WriteFile(filePath, []byte(contents), perm)
+
+	if err != nil {
+		t.Fatalf("Error when creating %s file: %s", filename, err)
+	}
+
+	return filePath
+}

builder/dockerfile/internals.go

@@ -618,25 +618,8 @@ func (b *Builder) readDockerfile() error {
 		}
 	}
 
-	f, err := b.context.Open(b.options.Dockerfile)
+	err := b.parseDockerfile()
-	if err != nil {
+
-		if os.IsNotExist(err) {
-			return fmt.Errorf("Cannot locate specified Dockerfile: %s", b.options.Dockerfile)
-		}
-		return err
-	}
-	if f, ok := f.(*os.File); ok {
-		// ignoring error because Open already succeeded
-		fi, err := f.Stat()
-		if err != nil {
-			return fmt.Errorf("Unexpected error reading Dockerfile: %v", err)
-		}
-		if fi.Size() == 0 {
-			return fmt.Errorf("The Dockerfile (%s) cannot be empty", b.options.Dockerfile)
-		}
-	}
-	b.dockerfile, err = parser.Parse(f)
-	f.Close()
 	if err != nil {
 		return err
 	}
@@ -655,6 +638,33 @@ func (b *Builder) readDockerfile() error {
 	return nil
 }
 
+func (b *Builder) parseDockerfile() error {
+	f, err := b.context.Open(b.options.Dockerfile)
+	if err != nil {
+		if os.IsNotExist(err) {
+			return fmt.Errorf("Cannot locate specified Dockerfile: %s", b.options.Dockerfile)
+		}
+		return err
+	}
+	defer f.Close()
+	if f, ok := f.(*os.File); ok {
+		// ignoring error because Open already succeeded
+		fi, err := f.Stat()
+		if err != nil {
+			return fmt.Errorf("Unexpected error reading Dockerfile: %v", err)
+		}
+		if fi.Size() == 0 {
+			return fmt.Errorf("The Dockerfile (%s) cannot be empty", b.options.Dockerfile)
+		}
+	}
+	b.dockerfile, err = parser.Parse(f)
+	if err != nil {
+		return err
+	}
+
+	return nil
+}
+
 // determine if build arg is part of built-in args or user
 // defined args in Dockerfile at any point in time.
 func (b *Builder) isBuildArgAllowed(arg string) bool {

builder/dockerfile/internals_test.go

@@ -0,0 +1,55 @@
+package dockerfile
+
+import (
+	"strings"
+	"testing"
+
+	"github.com/docker/docker/builder"
+	"github.com/docker/docker/pkg/archive"
+	"github.com/docker/engine-api/types"
+)
+
+func TestEmptyDockerfile(t *testing.T) {
+	contextDir, cleanup := createTestTempDir(t, "", "builder-dockerfile-test")
+	defer cleanup()
+
+	createTestTempFile(t, contextDir, builder.DefaultDockerfileName, "", 0777)
+
+	tarStream, err := archive.Tar(contextDir, archive.Uncompressed)
+
+	if err != nil {
+		t.Fatalf("Error when creating tar stream: %s", err)
+	}
+
+	defer func() {
+		if err = tarStream.Close(); err != nil {
+			t.Fatalf("Error when closing tar stream: %s", err)
+		}
+	}()
+
+	context, err := builder.MakeTarSumContext(tarStream)
+
+	if err != nil {
+		t.Fatalf("Error when creating tar context: %s", err)
+	}
+
+	defer func() {
+		if err = context.Close(); err != nil {
+			t.Fatalf("Error when closing tar context: %s", err)
+		}
+	}()
+
+	options := &types.ImageBuildOptions{}
+
+	b := &Builder{options: options, context: context}
+
+	err = b.readDockerfile()
+
+	if err == nil {
+		t.Fatalf("No error when executing test for empty Dockerfile")
+	}
+
+	if !strings.Contains(err.Error(), "The Dockerfile (Dockerfile) cannot be empty") {
+		t.Fatalf("Wrong error message. Should be \"%s\". Got \"%s\"", "The Dockerfile (Dockerfile) cannot be empty", err.Error())
+	}
+}

integration-cli/docker_cli_build_test.go

@@ -862,138 +862,6 @@ RUN [ $(ls -l / | grep new_dir | awk '{print $3":"$4}') = 'root:root' ]`, true);
 	}
 }
 
-func (s *DockerSuite) TestBuildAddMultipleFilesToFile(c *check.C) {
-	name := "testaddmultiplefilestofile"
-
-	ctx, err := fakeContext(`FROM `+minimalBaseImage()+`
-	ADD file1.txt file2.txt test
-	`,
-		map[string]string{
-			"file1.txt": "test1",
-			"file2.txt": "test1",
-		})
-	if err != nil {
-		c.Fatal(err)
-	}
-	defer ctx.Close()
-
-	expected := "When using ADD with more than one source file, the destination must be a directory and end with a /"
-	if _, err := buildImageFromContext(name, ctx, true); err == nil || !strings.Contains(err.Error(), expected) {
-		c.Fatalf("Wrong error: (should contain %q) got:\n%v", expected, err)
-	}
-
-}
-
-func (s *DockerSuite) TestBuildJSONAddMultipleFilesToFile(c *check.C) {
-	name := "testjsonaddmultiplefilestofile"
-
-	ctx, err := fakeContext(`FROM `+minimalBaseImage()+`
-	ADD ["file1.txt", "file2.txt", "test"]
-	`,
-		map[string]string{
-			"file1.txt": "test1",
-			"file2.txt": "test1",
-		})
-	if err != nil {
-		c.Fatal(err)
-	}
-	defer ctx.Close()
-
-	expected := "When using ADD with more than one source file, the destination must be a directory and end with a /"
-	if _, err := buildImageFromContext(name, ctx, true); err == nil || !strings.Contains(err.Error(), expected) {
-		c.Fatalf("Wrong error: (should contain %q) got:\n%v", expected, err)
-	}
-
-}
-
-func (s *DockerSuite) TestBuildAddMultipleFilesToFileWild(c *check.C) {
-	name := "testaddmultiplefilestofilewild"
-
-	ctx, err := fakeContext(`FROM `+minimalBaseImage()+`
-	ADD file*.txt test
-	`,
-		map[string]string{
-			"file1.txt": "test1",
-			"file2.txt": "test1",
-		})
-	if err != nil {
-		c.Fatal(err)
-	}
-	defer ctx.Close()
-
-	expected := "When using ADD with more than one source file, the destination must be a directory and end with a /"
-	if _, err := buildImageFromContext(name, ctx, true); err == nil || !strings.Contains(err.Error(), expected) {
-		c.Fatalf("Wrong error: (should contain %q) got:\n%v", expected, err)
-	}
-
-}
-
-func (s *DockerSuite) TestBuildJSONAddMultipleFilesToFileWild(c *check.C) {
-	name := "testjsonaddmultiplefilestofilewild"
-
-	ctx, err := fakeContext(`FROM `+minimalBaseImage()+`
-	ADD ["file*.txt", "test"]
-	`,
-		map[string]string{
-			"file1.txt": "test1",
-			"file2.txt": "test1",
-		})
-	if err != nil {
-		c.Fatal(err)
-	}
-	defer ctx.Close()
-
-	expected := "When using ADD with more than one source file, the destination must be a directory and end with a /"
-	if _, err := buildImageFromContext(name, ctx, true); err == nil || !strings.Contains(err.Error(), expected) {
-		c.Fatalf("Wrong error: (should contain %q) got:\n%v", expected, err)
-	}
-
-}
-
-func (s *DockerSuite) TestBuildCopyMultipleFilesToFile(c *check.C) {
-	name := "testcopymultiplefilestofile"
-
-	ctx, err := fakeContext(`FROM `+minimalBaseImage()+`
-	COPY file1.txt file2.txt test
-	`,
-		map[string]string{
-			"file1.txt": "test1",
-			"file2.txt": "test1",
-		})
-	if err != nil {
-		c.Fatal(err)
-	}
-	defer ctx.Close()
-
-	expected := "When using COPY with more than one source file, the destination must be a directory and end with a /"
-	if _, err := buildImageFromContext(name, ctx, true); err == nil || !strings.Contains(err.Error(), expected) {
-		c.Fatalf("Wrong error: (should contain %q) got:\n%v", expected, err)
-	}
-
-}
-
-func (s *DockerSuite) TestBuildJSONCopyMultipleFilesToFile(c *check.C) {
-	name := "testjsoncopymultiplefilestofile"
-
-	ctx, err := fakeContext(`FROM `+minimalBaseImage()+`
-	COPY ["file1.txt", "file2.txt", "test"]
-	`,
-		map[string]string{
-			"file1.txt": "test1",
-			"file2.txt": "test1",
-		})
-	if err != nil {
-		c.Fatal(err)
-	}
-	defer ctx.Close()
-
-	expected := "When using COPY with more than one source file, the destination must be a directory and end with a /"
-	if _, err := buildImageFromContext(name, ctx, true); err == nil || !strings.Contains(err.Error(), expected) {
-		c.Fatalf("Wrong error: (should contain %q) got:\n%v", expected, err)
-	}
-
-}
-
 func (s *DockerSuite) TestBuildAddFileWithWhitespace(c *check.C) {
 	testRequires(c, DaemonIsLinux) // Not currently passing on Windows
 	name := "testaddfilewithwhitespace"
@@ -1066,48 +934,6 @@ RUN [ $(cat "/test dir/test_file6") = 'test6' ]`,
 	}
 }
 
-func (s *DockerSuite) TestBuildAddMultipleFilesToFileWithWhitespace(c *check.C) {
-	name := "testaddmultiplefilestofilewithwhitespace"
-	ctx, err := fakeContext(`FROM busybox
-	ADD [ "test file1", "test file2", "test" ]
-    `,
-		map[string]string{
-			"test file1": "test1",
-			"test file2": "test2",
-		})
-	if err != nil {
-		c.Fatal(err)
-	}
-	defer ctx.Close()
-
-	expected := "When using ADD with more than one source file, the destination must be a directory and end with a /"
-	if _, err := buildImageFromContext(name, ctx, true); err == nil || !strings.Contains(err.Error(), expected) {
-		c.Fatalf("Wrong error: (should contain %q) got:\n%v", expected, err)
-	}
-
-}
-
-func (s *DockerSuite) TestBuildCopyMultipleFilesToFileWithWhitespace(c *check.C) {
-	name := "testcopymultiplefilestofilewithwhitespace"
-	ctx, err := fakeContext(`FROM busybox
-	COPY [ "test file1", "test file2", "test" ]
-        `,
-		map[string]string{
-			"test file1": "test1",
-			"test file2": "test2",
-		})
-	if err != nil {
-		c.Fatal(err)
-	}
-	defer ctx.Close()
-
-	expected := "When using COPY with more than one source file, the destination must be a directory and end with a /"
-	if _, err := buildImageFromContext(name, ctx, true); err == nil || !strings.Contains(err.Error(), expected) {
-		c.Fatalf("Wrong error: (should contain %q) got:\n%v", expected, err)
-	}
-
-}
-
 func (s *DockerSuite) TestBuildCopyWildcard(c *check.C) {
 	testRequires(c, DaemonIsLinux) // Windows doesn't have httpserver image yet
 	name := "testcopywildcard"
@@ -1159,26 +985,6 @@ func (s *DockerSuite) TestBuildCopyWildcard(c *check.C) {
 
 }
 
-func (s *DockerSuite) TestBuildCopyWildcardNoFind(c *check.C) {
-	name := "testcopywildcardnofind"
-	ctx, err := fakeContext(`FROM busybox
-	COPY file*.txt /tmp/
-	`, nil)
-	if err != nil {
-		c.Fatal(err)
-	}
-	defer ctx.Close()
-
-	_, err = buildImageFromContext(name, ctx, true)
-	if err == nil {
-		c.Fatal("should have failed to find a file")
-	}
-	if !strings.Contains(err.Error(), "No source files were specified") {
-		c.Fatalf("Wrong error %v, must be about no source files", err)
-	}
-
-}
-
 func (s *DockerSuite) TestBuildCopyWildcardInName(c *check.C) {
 	name := "testcopywildcardinname"
 	ctx, err := fakeContext(`FROM busybox
@@ -1580,17 +1386,6 @@ COPY . /`,
 	}
 }
 
-func (s *DockerSuite) TestBuildCopyDisallowRemote(c *check.C) {
-	name := "testcopydisallowremote"
-
-	_, out, err := buildImageWithOut(name, `FROM `+minimalBaseImage()+`
-COPY https://index.docker.io/robots.txt /`,
-		true)
-	if err == nil || !strings.Contains(out, "Source can't be a URL for COPY") {
-		c.Fatalf("Error should be about disallowed remote source, got err: %s, out: %q", err, out)
-	}
-}
-
 func (s *DockerSuite) TestBuildAddBadLinks(c *check.C) {
 	testRequires(c, DaemonIsLinux) // Not currently working on Windows
 
@@ -3289,18 +3084,6 @@ func (s *DockerSuite) TestBuildFails(c *check.C) {
 	}
 }
 
-func (s *DockerSuite) TestBuildFailsDockerfileEmpty(c *check.C) {
-	name := "testbuildfails"
-	_, err := buildImage(name, ``, true)
-	if err != nil {
-		if !strings.Contains(err.Error(), "The Dockerfile (Dockerfile) cannot be empty") {
-			c.Fatalf("Wrong error %v, must be about empty Dockerfile", err)
-		}
-	} else {
-		c.Fatal("Error must not be nil")
-	}
-}
-
 func (s *DockerSuite) TestBuildOnBuild(c *check.C) {
 	name := "testbuildonbuild"
 	_, err := buildImage(name,
@@ -3319,21 +3102,6 @@ func (s *DockerSuite) TestBuildOnBuild(c *check.C) {
 	}
 }
 
-func (s *DockerSuite) TestBuildOnBuildForbiddenChained(c *check.C) {
-	name := "testbuildonbuildforbiddenchained"
-	_, err := buildImage(name,
-		`FROM busybox
-		ONBUILD ONBUILD RUN touch foobar`,
-		true)
-	if err != nil {
-		if !strings.Contains(err.Error(), "Chaining ONBUILD via `ONBUILD ONBUILD` isn't allowed") {
-			c.Fatalf("Wrong error %v, must be about chaining ONBUILD", err)
-		}
-	} else {
-		c.Fatal("Error must not be nil")
-	}
-}
-
 // gh #2446
 func (s *DockerSuite) TestBuildAddToSymlinkDest(c *check.C) {
 	testRequires(c, DaemonIsLinux)
@@ -4564,16 +4332,6 @@ func (s *DockerSuite) TestBuildCmdJSONNoShDashC(c *check.C) {
 
 }
 
-func (s *DockerSuite) TestBuildErrorInvalidInstruction(c *check.C) {
-	name := "testbuildignoreinvalidinstruction"
-
-	out, _, err := buildImageWithOut(name, "FROM busybox\nfoo bar", true)
-	if err == nil {
-		c.Fatalf("Should have failed: %s", out)
-	}
-
-}
-
 func (s *DockerSuite) TestBuildEntrypointInheritance(c *check.C) {
 
 	if _, err := buildImage("parent", `