Merge pull request #54 from go-test/fix-issue-45

Fix issue 45: don't try to call Error method on unexported field

deep.go

@@ -27,7 +27,9 @@ var (
 	LogErrors = false
 
 	// CompareUnexportedFields causes unexported struct fields, like s in
-	// T{s int}, to be compared when true.
+	// T{s int}, to be compared when true. This does not work for comparing
+	// error or Time types on unexported fields because methods on unexported
+	// fields cannot be called.
 	CompareUnexportedFields = false
 
 	// NilSlicesAreEmpty causes a nil slice to be equal to an empty slice.
@@ -137,18 +139,23 @@ func (c *cmp) equals(a, b reflect.Value, level int) {
 	bElem := bKind == reflect.Ptr || bKind == reflect.Interface
 
 	// If both types implement the error interface, compare the error strings.
-	// This must be done before dereferencing because the interface is on a
-	// pointer receiver. Re https://github.com/go-test/deep/issues/31, a/b might
-	// be primitive kinds; see TestErrorPrimitiveKind.
-	if aType.Implements(errorType) && bType.Implements(errorType) {
-		if (!aElem || !a.IsNil()) && (!bElem || !b.IsNil()) {
-			aString := a.MethodByName("Error").Call(nil)[0].String()
-			bString := b.MethodByName("Error").Call(nil)[0].String()
-			if aString != bString {
-				c.saveDiff(aString, bString)
-				return
-			}
+	// This must be done before dereferencing because errors.New() returns a
+	// pointer to a struct that implements the interface:
+	//   func (e *errorString) Error() string {
+	// And we check CanInterface as a hack to make sure the underlying method
+	// is callable because https://github.com/golang/go/issues/32438
+	// Issues:
+	//   https://github.com/go-test/deep/issues/31
+	//   https://github.com/go-test/deep/issues/45
+	if (aType.Implements(errorType) && bType.Implements(errorType)) &&
+		((!aElem || !a.IsNil()) && (!bElem || !b.IsNil())) &&
+		(a.CanInterface() && b.CanInterface()) {
+		aString := a.MethodByName("Error").Call(nil)[0].String()
+		bString := b.MethodByName("Error").Call(nil)[0].String()
+		if aString != bString {
+			c.saveDiff(aString, bString)
 		}
+		return
 	}
 
 	// Dereference pointers and interface{}

deep_test.go

@@ -1304,21 +1304,7 @@ func TestError(t *testing.T) {
 
 func TestErrorWithOtherFields(t *testing.T) {
 	a := errors.New("it broke")
-	b := errors.New("it broke")
-
-	diff := deep.Equal(a, b)
-	if len(diff) != 0 {
-		t.Fatalf("expected zero diffs, got %d: %s", len(diff), diff)
-	}
-
-	b = errors.New("it fell apart")
-	diff = deep.Equal(a, b)
-	if len(diff) != 1 {
-		t.Fatalf("expected 1 diff, got %d: %s", len(diff), diff)
-	}
-	if diff[0] != "it broke != it fell apart" {
-		t.Errorf("got '%s', expected 'it broke != it fell apart'", diff[0])
-	}
+	b := errors.New("it fell apart")
 
 	// Both errors set
 	type tWithError struct {
@@ -1333,7 +1319,7 @@ func TestErrorWithOtherFields(t *testing.T) {
 		Error: b,
 		Other: "ok",
 	}
-	diff = deep.Equal(t1, t2)
+	diff := deep.Equal(t1, t2)
 	if len(diff) != 1 {
 		t.Fatalf("expected 1 diff, got %d: %s", len(diff), diff)
 	}
@@ -1409,6 +1395,25 @@ func TestErrorPrimitiveKind(t *testing.T) {
 	if len(diff) != 0 {
 		t.Fatalf("expected zero diffs, got %d: %s", len(diff), diff)
 	}
+
+	err2 = "def"
+	diff = deep.Equal(err1, err2)
+	if len(diff) != 1 {
+		t.Fatalf("expected 1 diff, got %d: %s", len(diff), diff)
+	}
+}
+
+func TestErrorUnexported(t *testing.T) {
+	// https://github.com/go-test/deep/issues/45
+	type foo struct {
+		bar error
+	}
+	defaultCompareUnexportedFields := deep.CompareUnexportedFields
+	deep.CompareUnexportedFields = true
+	defer func() { deep.CompareUnexportedFields = defaultCompareUnexportedFields }()
+	e1 := foo{bar: fmt.Errorf("error")}
+	e2 := foo{bar: fmt.Errorf("error")}
+	deep.Equal(e1, e2)
 }
 
 func TestNil(t *testing.T) {