From 9cf6ed60b6d288270e556dbb87806d1f72cf0917 Mon Sep 17 00:00:00 2001 From: Daniel Nichter Date: Tue, 6 Dec 2022 13:21:38 -0500 Subject: [PATCH 1/2] Fix issue 45: don't try to call Error method on unexported field --- deep.go | 27 ++++++++++++++++----------- deep_test.go | 35 +++++++++++++++++++---------------- 2 files changed, 35 insertions(+), 27 deletions(-) diff --git a/deep.go b/deep.go index dbc89c0..254ee7d 100644 --- a/deep.go +++ b/deep.go @@ -137,18 +137,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{} diff --git a/deep_test.go b/deep_test.go index 2a03392..d2ae7ce 100644 --- a/deep_test.go +++ b/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,23 @@ 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 + } + deep.CompareUnexportedFields = true + e1 := foo{bar: fmt.Errorf("error")} + e2 := foo{bar: fmt.Errorf("error")} + deep.Equal(e1, e2) } func TestNil(t *testing.T) { From c81fca0b35ac2fd9450b3eec1cf793cc24eb51f5 Mon Sep 17 00:00:00 2001 From: Daniel Nichter Date: Tue, 6 Dec 2022 13:27:55 -0500 Subject: [PATCH 2/2] Update docs and restore pkg var in test to original --- deep.go | 4 +++- deep_test.go | 2 ++ 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/deep.go b/deep.go index 254ee7d..3e6636c 100644 --- a/deep.go +++ b/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. diff --git a/deep_test.go b/deep_test.go index d2ae7ce..98fba1f 100644 --- a/deep_test.go +++ b/deep_test.go @@ -1408,7 +1408,9 @@ func TestErrorUnexported(t *testing.T) { 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)