Merge pull request #23652 from dnephin/better-bundle-load-errors

Improve the error messages for loading a bundle

api/client/bundlefile/bundlefile.go

@@ -4,8 +4,8 @@ package bundlefile
 
 import (
 	"encoding/json"
+	"fmt"
 	"io"
-	"os"
 )
 
 // Bundlefile stores the contents of a bundlefile
@@ -34,19 +34,28 @@ type Port struct {
 }
 
 // LoadFile loads a bundlefile from a path to the file
-func LoadFile(path string) (*Bundlefile, error) {
-	reader, err := os.Open(path)
-	if err != nil {
-		return nil, err
-	}
-
+func LoadFile(reader io.Reader) (*Bundlefile, error) {
 	bundlefile := &Bundlefile{}
 
-	if err := json.NewDecoder(reader).Decode(bundlefile); err != nil {
+	decoder := json.NewDecoder(reader)
+	if err := decoder.Decode(bundlefile); err != nil {
+		switch jsonErr := err.(type) {
+		case *json.SyntaxError:
+			return nil, fmt.Errorf(
+				"JSON syntax error at byte %v: %s",
+				jsonErr.Offset,
+				jsonErr.Error())
+		case *json.UnmarshalTypeError:
+			return nil, fmt.Errorf(
+				"Unexpected type at byte %v. Expected %s but received %s.",
+				jsonErr.Offset,
+				jsonErr.Type,
+				jsonErr.Value)
+		}
 		return nil, err
 	}
 
-	return bundlefile, err
+	return bundlefile, nil
 }
 
 // Print writes the contents of the bundlefile to the output writer

api/client/bundlefile/bundlefile_test.go

@@ -0,0 +1,79 @@
+// +build experimental
+
+package bundlefile
+
+import (
+	"bytes"
+	"strings"
+	"testing"
+
+	"github.com/docker/docker/pkg/testutil/assert"
+)
+
+func TestLoadFileV01Success(t *testing.T) {
+	reader := strings.NewReader(`{
+		"Version": "0.1",
+		"Services": {
+			"redis": {
+				"Image": "redis@sha256:4b24131101fa0117bcaa18ac37055fffd9176aa1a240392bb8ea85e0be50f2ce",
+				"Networks": ["default"]
+			},
+			"web": {
+				"Image": "dockercloud/hello-world@sha256:fe79a2cfbd17eefc344fb8419420808df95a1e22d93b7f621a7399fd1e9dca1d",
+				"Networks": ["default"],
+				"User": "web"
+			}
+		}
+	}`)
+
+	bundle, err := LoadFile(reader)
+	assert.NilError(t, err)
+	assert.Equal(t, bundle.Version, "0.1")
+	assert.Equal(t, len(bundle.Services), 2)
+}
+
+func TestLoadFileSyntaxError(t *testing.T) {
+	reader := strings.NewReader(`{
+		"Version": "0.1",
+		"Services": unquoted string
+	}`)
+
+	_, err := LoadFile(reader)
+	assert.Error(t, err, "syntax error at byte 37: invalid character 'u'")
+}
+
+func TestLoadFileTypeError(t *testing.T) {
+	reader := strings.NewReader(`{
+		"Version": "0.1",
+		"Services": {
+			"web": {
+				"Image": "redis",
+				"Networks": "none"
+			}
+		}
+	}`)
+
+	_, err := LoadFile(reader)
+	assert.Error(t, err, "Unexpected type at byte 94. Expected []string but received string")
+}
+
+func TestPrint(t *testing.T) {
+	var buffer bytes.Buffer
+	bundle := &Bundlefile{
+		Version: "0.1",
+		Services: map[string]Service{
+			"web": {
+				Image:   "image",
+				Command: []string{"echo", "something"},
+			},
+		},
+	}
+	assert.NilError(t, Print(&buffer, bundle))
+	output := buffer.String()
+	assert.Contains(t, output, "\"Image\": \"image\"")
+	assert.Contains(t, output,
+		`"Command": [
+                "echo",
+                "something"
+            ]`)
+}

api/client/service/opts_test.go

@@ -1,82 +1,60 @@
 package service
 
 import (
-	"strings"
 	"testing"
 	"time"
 
+	"github.com/docker/docker/pkg/testutil/assert"
 	"github.com/docker/engine-api/types/swarm"
 )
 
-func assertEqual(t *testing.T, actual, expected interface{}) {
-	if expected != actual {
-		t.Fatalf("Expected '%v' (%T) got '%v' (%T)", expected, expected, actual, actual)
-	}
-}
-
-func assertNilError(t *testing.T, err error) {
-	if err != nil {
-		t.Fatalf("Expected no error, got: %s", err.Error())
-	}
-}
-
-func assertError(t *testing.T, err error, contains string) {
-	if err == nil {
-		t.Fatalf("Expected an error, but error was nil")
-	}
-
-	if !strings.Contains(err.Error(), contains) {
-		t.Fatalf("Expected error to contain '%s', got '%s'", contains, err.Error())
-	}
-}
-
 func TestMemBytesString(t *testing.T) {
 	var mem memBytes = 1048576
-	assertEqual(t, mem.String(), "1 MiB")
+	assert.Equal(t, mem.String(), "1 MiB")
 }
 
 func TestMemBytesSetAndValue(t *testing.T) {
 	var mem memBytes
-	assertNilError(t, mem.Set("5kb"))
-	assertEqual(t, mem.Value(), int64(5120))
+	assert.NilError(t, mem.Set("5kb"))
+	assert.Equal(t, mem.Value(), int64(5120))
 }
 
 func TestNanoCPUsString(t *testing.T) {
 	var cpus nanoCPUs = 6100000000
-	assertEqual(t, cpus.String(), "6.100")
+	assert.Equal(t, cpus.String(), "6.100")
 }
 
 func TestNanoCPUsSetAndValue(t *testing.T) {
 	var cpus nanoCPUs
-	assertNilError(t, cpus.Set("0.35"))
-	assertEqual(t, cpus.Value(), int64(350000000))
+	assert.NilError(t, cpus.Set("0.35"))
+	assert.Equal(t, cpus.Value(), int64(350000000))
 }
 
 func TestDurationOptString(t *testing.T) {
 	dur := time.Duration(300 * 10e8)
 	duration := DurationOpt{value: &dur}
-	assertEqual(t, duration.String(), "5m0s")
+	assert.Equal(t, duration.String(), "5m0s")
 }
 
 func TestDurationOptSetAndValue(t *testing.T) {
 	var duration DurationOpt
-	assertNilError(t, duration.Set("300s"))
-	assertEqual(t, *duration.Value(), time.Duration(300*10e8))
+	assert.NilError(t, duration.Set("300s"))
+	assert.Equal(t, *duration.Value(), time.Duration(300*10e8))
 }
 
 func TestUint64OptString(t *testing.T) {
 	value := uint64(2345678)
 	opt := Uint64Opt{value: &value}
-	assertEqual(t, opt.String(), "2345678")
+	assert.Equal(t, opt.String(), "2345678")
 
 	opt = Uint64Opt{}
-	assertEqual(t, opt.String(), "none")
+	assert.Equal(t, opt.String(), "none")
 }
 
 func TestUint64OptSetAndValue(t *testing.T) {
 	var opt Uint64Opt
-	assertNilError(t, opt.Set("14445"))
-	assertEqual(t, *opt.Value(), uint64(14445))
+	assert.NilError(t, opt.Set("14445"))
+	assert.Equal(t, *opt.Value(), uint64(14445))
 }
 
 func TestMountOptString(t *testing.T) {
@@ -95,16 +73,16 @@ func TestMountOptString(t *testing.T) {
 		},
 	}
 	expected := "BIND /home/path /target, VOLUME foo /target/foo"
-	assertEqual(t, mount.String(), expected)
+	assert.Equal(t, mount.String(), expected)
 }
 
 func TestMountOptSetNoError(t *testing.T) {
 	var mount MountOpt
-	assertNilError(t, mount.Set("type=bind,target=/target,source=/foo"))
+	assert.NilError(t, mount.Set("type=bind,target=/target,source=/foo"))
 
 	mounts := mount.Value()
-	assertEqual(t, len(mounts), 1)
-	assertEqual(t, mounts[0], swarm.Mount{
+	assert.Equal(t, len(mounts), 1)
+	assert.Equal(t, mounts[0], swarm.Mount{
 		Type:   swarm.MountType("BIND"),
 		Source: "/foo",
 		Target: "/target",
@@ -113,25 +91,25 @@ func TestMountOptSetNoError(t *testing.T) {
 
 func TestMountOptSetErrorNoType(t *testing.T) {
 	var mount MountOpt
-	assertError(t, mount.Set("target=/target,source=/foo"), "type is required")
+	assert.Error(t, mount.Set("target=/target,source=/foo"), "type is required")
 }
 
 func TestMountOptSetErrorNoTarget(t *testing.T) {
 	var mount MountOpt
-	assertError(t, mount.Set("type=VOLUME,source=/foo"), "target is required")
+	assert.Error(t, mount.Set("type=VOLUME,source=/foo"), "target is required")
 }
 
 func TestMountOptSetErrorInvalidKey(t *testing.T) {
 	var mount MountOpt
-	assertError(t, mount.Set("type=VOLUME,bogus=foo"), "unexpected key 'bogus'")
+	assert.Error(t, mount.Set("type=VOLUME,bogus=foo"), "unexpected key 'bogus'")
 }
 
 func TestMountOptSetErrorInvalidField(t *testing.T) {
 	var mount MountOpt
-	assertError(t, mount.Set("type=VOLUME,bogus"), "invalid field 'bogus'")
+	assert.Error(t, mount.Set("type=VOLUME,bogus"), "invalid field 'bogus'")
 }
 
 func TestMountOptSetErrorInvalidWritable(t *testing.T) {
 	var mount MountOpt
-	assertError(t, mount.Set("type=VOLUME,writable=yes"), "invalid value for writable: yes")
+	assert.Error(t, mount.Set("type=VOLUME,writable=yes"), "invalid value for writable: yes")
 }

api/client/stack/opts.go

@@ -31,7 +31,12 @@ func loadBundlefile(stderr io.Writer, namespace string, path string) (*bundlefil
 	}
 
 	fmt.Fprintf(stderr, "Loading bundle from %s\n", path)
-	bundle, err := bundlefile.LoadFile(path)
+	reader, err := os.Open(path)
+	if err != nil {
+		return nil, err
+	}
+
+	bundle, err := bundlefile.LoadFile(reader)
 	if err != nil {
 		return nil, fmt.Errorf("Error reading %s: %v\n", path, err)
 	}

pkg/testutil/assert/assert.go

@@ -0,0 +1,47 @@
+// Package assert contains functions for making assertions in unit tests
+package assert
+
+import (
+	"strings"
+)
+
+// TestingT is an interface which defines the methods of testing.T that are
+// required by this package
+type TestingT interface {
+	Fatalf(string, ...interface{})
+}
+
+// Equal compare the actual value to the expected value and fails the test if
+// they are not equal.
+func Equal(t TestingT, actual, expected interface{}) {
+	if expected != actual {
+		t.Fatalf("Expected '%v' (%T) got '%v' (%T)", expected, expected, actual, actual)
+	}
+}
+
+// NilError asserts that the error is nil, otherwise it fails the test.
+func NilError(t TestingT, err error) {
+	if err != nil {
+		t.Fatalf("Expected no error, got: %s", err.Error())
+	}
+}
+
+// Error asserts that error is not nil, and contains the expected text,
+// otherwise it fails the test.
+func Error(t TestingT, err error, contains string) {
+	if err == nil {
+		t.Fatalf("Expected an error, but error was nil")
+	}
+
+	if !strings.Contains(err.Error(), contains) {
+		t.Fatalf("Expected error to contain '%s', got '%s'", contains, err.Error())
+	}
+}
+
+// Contains asserts that the string contains a substring, otherwise it fails the
+// test.
+func Contains(t TestingT, actual, contains string) {
+	if !strings.Contains(actual, contains) {
+		t.Fatalf("Expected '%s' to contain '%s'", actual, contains)
+	}
+}

pkg/testutil/pkg.go

@@ -0,0 +1 @@
+package testutil