Merge pull request #931 from dnephin/refactor-build-unit-tests

Refactor build tests to re-use more code and not require root

cli/command/image/build_linux_test.go

@@ -1,55 +0,0 @@
-//+build linux
-
-package image
-
-import (
-	"bytes"
-	"io"
-	"io/ioutil"
-	"syscall"
-	"testing"
-
-	"github.com/docker/cli/internal/test"
-	"github.com/docker/docker/api/types"
-	"github.com/docker/docker/pkg/archive"
-	"github.com/gotestyourself/gotestyourself/assert"
-	is "github.com/gotestyourself/gotestyourself/assert/cmp"
-	"github.com/gotestyourself/gotestyourself/fs"
-	"golang.org/x/net/context"
-)
-
-func TestRunBuildResetsUidAndGidInContext(t *testing.T) {
-	dest := fs.NewDir(t, "test-build-context-dest")
-	defer dest.Remove()
-
-	fakeImageBuild := func(_ context.Context, context io.Reader, options types.ImageBuildOptions) (types.ImageBuildResponse, error) {
-		assert.NilError(t, archive.Untar(context, dest.Path(), nil))
-
-		body := new(bytes.Buffer)
-		return types.ImageBuildResponse{Body: ioutil.NopCloser(body)}, nil
-	}
-	cli := test.NewFakeCli(&fakeClient{imageBuildFunc: fakeImageBuild})
-
-	dir := fs.NewDir(t, "test-build-context",
-		fs.WithFile("foo", "some content", fs.AsUser(65534, 65534)),
-		fs.WithFile("Dockerfile", `
-			FROM alpine:3.6
-			COPY foo bar /
-		`),
-	)
-	defer dir.Remove()
-
-	options := newBuildOptions()
-	options.context = dir.Path()
-	options.untrusted = true
-
-	err := runBuild(cli, options)
-	assert.NilError(t, err)
-
-	files, err := ioutil.ReadDir(dest.Path())
-	assert.NilError(t, err)
-	for _, fileInfo := range files {
-		assert.Check(t, is.Equal(uint32(0), fileInfo.Sys().(*syscall.Stat_t).Uid))
-		assert.Check(t, is.Equal(uint32(0), fileInfo.Sys().(*syscall.Stat_t).Gid))
-	}
-}

cli/command/image/build_test.go

@@ -3,6 +3,7 @@ package image
 import (
 	"archive/tar"
 	"bytes"
+	"compress/gzip"
 	"io"
 	"io/ioutil"
 	"os"
@@ -14,30 +15,21 @@ import (
 	"github.com/docker/cli/internal/test"
 	"github.com/docker/docker/api/types"
 	"github.com/docker/docker/pkg/archive"
+	"github.com/google/go-cmp/cmp"
 	"github.com/gotestyourself/gotestyourself/assert"
-	is "github.com/gotestyourself/gotestyourself/assert/cmp"
 	"github.com/gotestyourself/gotestyourself/fs"
+	"github.com/gotestyourself/gotestyourself/skip"
 	"golang.org/x/net/context"
 )
 
 func TestRunBuildDockerfileFromStdinWithCompress(t *testing.T) {
-	dest, err := ioutil.TempDir("", "test-build-compress-dest")
-	assert.NilError(t, err)
-	defer os.RemoveAll(dest)
-
-	var dockerfileName string
-	fakeImageBuild := func(_ context.Context, context io.Reader, options types.ImageBuildOptions) (types.ImageBuildResponse, error) {
 	buffer := new(bytes.Buffer)
+	fakeBuild := newFakeBuild()
+	fakeImageBuild := func(ctx context.Context, context io.Reader, options types.ImageBuildOptions) (types.ImageBuildResponse, error) {
 		tee := io.TeeReader(context, buffer)
-
+		gzipReader, err := gzip.NewReader(tee)
-		assert.NilError(t, archive.Untar(tee, dest, nil))
+		assert.NilError(t, err)
-		dockerfileName = options.Dockerfile
+		return fakeBuild.build(ctx, gzipReader, options)
-
-		header := buffer.Bytes()[:10]
-		assert.Check(t, is.Equal(archive.Gzip, archive.DetectCompression(header)))
-
-		body := new(bytes.Buffer)
-		return types.ImageBuildResponse{Body: ioutil.NopCloser(body)}, nil
 	}
 
 	cli := test.NewFakeCli(&fakeClient{imageBuildFunc: fakeImageBuild})
@@ -47,35 +39,57 @@ func TestRunBuildDockerfileFromStdinWithCompress(t *testing.T) {
 	`)
 	cli.SetIn(command.NewInStream(ioutil.NopCloser(dockerfile)))
 
-	dir, err := ioutil.TempDir("", "test-build-compress")
+	dir := fs.NewDir(t, t.Name(),
-	assert.NilError(t, err)
+		fs.WithFile("foo", "some content"))
-	defer os.RemoveAll(dir)
+	defer dir.Remove()
-
-	ioutil.WriteFile(filepath.Join(dir, "foo"), []byte("some content"), 0644)
 
 	options := newBuildOptions()
 	options.compress = true
 	options.dockerfileName = "-"
-	options.context = dir
+	options.context = dir.Path()
 	options.untrusted = true
+	assert.NilError(t, runBuild(cli, options))
 
-	err = runBuild(cli, options)
+	expected := []string{fakeBuild.options.Dockerfile, ".dockerignore", "foo"}
-	assert.NilError(t, err)
+	assert.DeepEqual(t, expected, fakeBuild.filenames(t))
 
-	files, err := ioutil.ReadDir(dest)
+	header := buffer.Bytes()[:10]
-	assert.NilError(t, err)
+	assert.Equal(t, archive.Gzip, archive.DetectCompression(header))
-	actual := []string{}
+}
-	for _, fileInfo := range files {
+
-		actual = append(actual, fileInfo.Name())
+func TestRunBuildResetsUidAndGidInContext(t *testing.T) {
+	skip.If(t, os.Getuid() != 0, "root is required to chown files")
+	fakeBuild := newFakeBuild()
+	cli := test.NewFakeCli(&fakeClient{imageBuildFunc: fakeBuild.build})
+
+	dir := fs.NewDir(t, "test-build-context",
+		fs.WithFile("foo", "some content", fs.AsUser(65534, 65534)),
+		fs.WithFile("Dockerfile", `
+			FROM alpine:3.6
+			COPY foo bar /
+		`),
+	)
+	defer dir.Remove()
+
+	options := newBuildOptions()
+	options.context = dir.Path()
+	options.untrusted = true
+	assert.NilError(t, runBuild(cli, options))
+
+	headers := fakeBuild.headers(t)
+	expected := []*tar.Header{
+		{Name: "Dockerfile"},
+		{Name: "foo"},
 	}
-	sort.Strings(actual)
+	var cmpTarHeaderNameAndOwner = cmp.Comparer(func(x, y tar.Header) bool {
-	assert.Check(t, is.DeepEqual([]string{dockerfileName, ".dockerignore", "foo"}, actual))
+		return x.Name == y.Name && x.Uid == y.Uid && x.Gid == y.Gid
+	})
+	assert.DeepEqual(t, expected, headers, cmpTarHeaderNameAndOwner)
 }
 
 func TestRunBuildDockerfileOutsideContext(t *testing.T) {
 	dir := fs.NewDir(t, t.Name(),
-		fs.WithFile("data", "data file"),
+		fs.WithFile("data", "data file"))
-	)
 	defer dir.Remove()
 
 	// Dockerfile outside of build-context
@@ -87,40 +101,17 @@ COPY data /data
 	)
 	defer df.Remove()
 
-	dest, err := ioutil.TempDir("", t.Name())
+	fakeBuild := newFakeBuild()
-	assert.NilError(t, err)
+	cli := test.NewFakeCli(&fakeClient{imageBuildFunc: fakeBuild.build})
-	defer os.RemoveAll(dest)
-
-	var dockerfileName string
-	fakeImageBuild := func(_ context.Context, context io.Reader, options types.ImageBuildOptions) (types.ImageBuildResponse, error) {
-		buffer := new(bytes.Buffer)
-		tee := io.TeeReader(context, buffer)
-
-		assert.NilError(t, archive.Untar(tee, dest, nil))
-		dockerfileName = options.Dockerfile
-
-		body := new(bytes.Buffer)
-		return types.ImageBuildResponse{Body: ioutil.NopCloser(body)}, nil
-	}
-
-	cli := test.NewFakeCli(&fakeClient{imageBuildFunc: fakeImageBuild})
 
 	options := newBuildOptions()
 	options.context = dir.Path()
 	options.dockerfileName = df.Path()
 	options.untrusted = true
+	assert.NilError(t, runBuild(cli, options))
 
-	err = runBuild(cli, options)
+	expected := []string{fakeBuild.options.Dockerfile, ".dockerignore", "data"}
-	assert.NilError(t, err)
+	assert.DeepEqual(t, expected, fakeBuild.filenames(t))
-
-	files, err := ioutil.ReadDir(dest)
-	assert.NilError(t, err)
-	var actual []string
-	for _, fileInfo := range files {
-		actual = append(actual, fileInfo.Name())
-	}
-	sort.Strings(actual)
-	assert.Check(t, is.DeepEqual([]string{dockerfileName, ".dockerignore", "data"}, actual))
 }
 
 // TestRunBuildFromLocalGitHubDirNonExistingRepo tests that build contexts
@@ -172,28 +163,54 @@ RUN echo hello world
 		fs.WithSymlink("context-link", "context"))
 	defer tmpDir.Remove()
 
-	files := []string{}
+	fakeBuild := newFakeBuild()
-	fakeImageBuild := func(_ context.Context, context io.Reader, options types.ImageBuildOptions) (types.ImageBuildResponse, error) {
+	cli := test.NewFakeCli(&fakeClient{imageBuildFunc: fakeBuild.build})
-		tarReader := tar.NewReader(context)
-		for {
-			hdr, err := tarReader.Next()
-			switch err {
-			case io.EOF:
-				body := new(bytes.Buffer)
-				return types.ImageBuildResponse{Body: ioutil.NopCloser(body)}, nil
-			case nil:
-				files = append(files, hdr.Name)
-			default:
-				return types.ImageBuildResponse{}, err
-			}
-		}
-	}
-
-	cli := test.NewFakeCli(&fakeClient{imageBuildFunc: fakeImageBuild})
 	options := newBuildOptions()
 	options.context = tmpDir.Join("context-link")
 	options.untrusted = true
 	assert.NilError(t, runBuild(cli, options))
 
-	assert.DeepEqual(t, files, []string{"Dockerfile"})
+	assert.DeepEqual(t, fakeBuild.filenames(t), []string{"Dockerfile"})
+}
+
+type fakeBuild struct {
+	context *tar.Reader
+	options types.ImageBuildOptions
+}
+
+func newFakeBuild() *fakeBuild {
+	return &fakeBuild{}
+}
+
+func (f *fakeBuild) build(_ context.Context, context io.Reader, options types.ImageBuildOptions) (types.ImageBuildResponse, error) {
+	f.context = tar.NewReader(context)
+	f.options = options
+	body := new(bytes.Buffer)
+	return types.ImageBuildResponse{Body: ioutil.NopCloser(body)}, nil
+}
+
+func (f *fakeBuild) headers(t *testing.T) []*tar.Header {
+	t.Helper()
+	headers := []*tar.Header{}
+	for {
+		hdr, err := f.context.Next()
+		switch err {
+		case io.EOF:
+			return headers
+		case nil:
+			headers = append(headers, hdr)
+		default:
+			assert.NilError(t, err)
+		}
+	}
+}
+
+func (f *fakeBuild) filenames(t *testing.T) []string {
+	t.Helper()
+	names := []string{}
+	for _, header := range f.headers(t) {
+		names = append(names, header.Name)
+	}
+	sort.Strings(names)
+	return names
 }

cli/command/image/history_test.go

@@ -10,6 +10,7 @@ import (
 	"github.com/docker/docker/api/types/image"
 	"github.com/gotestyourself/gotestyourself/assert"
 	"github.com/gotestyourself/gotestyourself/golden"
+	"github.com/gotestyourself/gotestyourself/skip"
 	"github.com/pkg/errors"
 )
 
@@ -42,7 +43,13 @@ func TestNewHistoryCommandErrors(t *testing.T) {
 	}
 }
 
+func notUTCTimezone() bool {
+	now := time.Now()
+	return now != now.UTC()
+}
+
 func TestNewHistoryCommandSuccess(t *testing.T) {
+	skip.If(t, notUTCTimezone, "expected output requires UTC timezone")
 	testCases := []struct {
 		name             string
 		args             []string

cli/command/manifest/client_test.go

@@ -10,7 +10,6 @@ import (
 )
 
 type fakeRegistryClient struct {
-	client.RegistryClient
 	getManifestFunc     func(ctx context.Context, ref reference.Named) (manifesttypes.ImageManifest, error)
 	getManifestListFunc func(ctx context.Context, ref reference.Named) ([]manifesttypes.ImageManifest, error)
 	mountBlobFunc       func(ctx context.Context, source reference.Canonical, target reference.Named) error
@@ -44,3 +43,5 @@ func (c *fakeRegistryClient) PutManifest(ctx context.Context, ref reference.Name
 	}
 	return digest.Digest(""), nil
 }
+
+var _ client.RegistryClient = &fakeRegistryClient{}

cli/command/manifest/push_test.go

@@ -12,9 +12,7 @@ import (
 	"golang.org/x/net/context"
 )
 
-func newFakeRegistryClient(t *testing.T) *fakeRegistryClient {
+func newFakeRegistryClient() *fakeRegistryClient {
-	assert.NilError(t, nil)
-
 	return &fakeRegistryClient{
 		getManifestFunc: func(_ context.Context, _ reference.Named) (manifesttypes.ImageManifest, error) {
 			return manifesttypes.ImageManifest{}, errors.New("")
@@ -49,12 +47,11 @@ func TestManifestPushErrors(t *testing.T) {
 	}
 }
 
-// store a one-image manifest list and puah it
 func TestManifestPush(t *testing.T) {
 	store, sCleanup := newTempManifestStore(t)
 	defer sCleanup()
 
-	registry := newFakeRegistryClient(t)
+	registry := newFakeRegistryClient()
 
 	cli := test.NewFakeCli(nil)
 	cli.SetManifestStore(store)