Skip to content

Commit

Permalink
Disambiguate reporter output (#216)
Browse files Browse the repository at this point in the history
The reporter tries to aggresively elide data that is not interesting
to the user. However, doing so many result in an output that does not
visually indicate the difference between semantically different objects.
This CL modifies the reporter to try increasingly verbose presets until
two different objects are formatted differently.

This CL includes a custom implementation of reflect.Type.String that
can print the type with fully qualified names to disambiguate types
that happen to have the same base package name.

Fixes #194
  • Loading branch information
dsnet authored Jun 12, 2020
1 parent f1780cf commit 44914b3
Show file tree
Hide file tree
Showing 10 changed files with 597 additions and 44 deletions.
135 changes: 114 additions & 21 deletions cmp/compare_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,8 @@ import (

pb "github.com/google/go-cmp/cmp/internal/testprotos"
ts "github.com/google/go-cmp/cmp/internal/teststructs"
foo1 "github.com/google/go-cmp/cmp/internal/teststructs/foo1"
foo2 "github.com/google/go-cmp/cmp/internal/teststructs/foo2"
)

func init() {
Expand Down Expand Up @@ -101,7 +103,12 @@ func mustFormatGolden(path string, in []struct{ Name, Data string }) {

var now = time.Date(2009, time.November, 10, 23, 00, 00, 00, time.UTC)

func intPtr(n int) *int { return &n }
func newInt(n int) *int { return &n }

type Stringer string

func newStringer(s string) fmt.Stringer { return (*Stringer)(&s) }
func (s Stringer) String() string { return string(s) }

type test struct {
label string // Test name
Expand Down Expand Up @@ -149,6 +156,7 @@ func TestDiff(t *testing.T) {
}()

// TODO: Require every test case to provide a reason.
// TODO: Forbid any test cases with the same name.
if tt.wantPanic == "" {
if gotPanic != "" {
t.Fatalf("unexpected panic message: %s\nreason: %v", gotPanic, tt.reason)
Expand Down Expand Up @@ -295,26 +303,26 @@ func comparerTests() []test {
wantPanic: "cannot handle unexported field",
}, {
label: label,
x: &struct{ A *int }{intPtr(4)},
y: &struct{ A *int }{intPtr(4)},
x: &struct{ A *int }{newInt(4)},
y: &struct{ A *int }{newInt(4)},
wantEqual: true,
}, {
label: label,
x: &struct{ A *int }{intPtr(4)},
y: &struct{ A *int }{intPtr(5)},
x: &struct{ A *int }{newInt(4)},
y: &struct{ A *int }{newInt(5)},
wantEqual: false,
}, {
label: label,
x: &struct{ A *int }{intPtr(4)},
y: &struct{ A *int }{intPtr(5)},
x: &struct{ A *int }{newInt(4)},
y: &struct{ A *int }{newInt(5)},
opts: []cmp.Option{
cmp.Comparer(func(x, y int) bool { return true }),
},
wantEqual: true,
}, {
label: label,
x: &struct{ A *int }{intPtr(4)},
y: &struct{ A *int }{intPtr(5)},
x: &struct{ A *int }{newInt(4)},
y: &struct{ A *int }{newInt(5)},
opts: []cmp.Option{
cmp.Comparer(func(x, y *int) bool { return x != nil && y != nil }),
},
Expand Down Expand Up @@ -555,15 +563,6 @@ func comparerTests() []test {
new(int): "world",
},
wantEqual: false,
}, {
label: label,
x: intPtr(0),
y: intPtr(0),
opts: []cmp.Option{
cmp.Comparer(func(x, y *int) bool { return x == y }),
},
// TODO: This diff output is unhelpful and should show the address.
wantEqual: false,
}, {
label: label,
x: [2][]int{
Expand Down Expand Up @@ -822,6 +821,100 @@ func reporterTests() []test {
)

return []test{{
label: label + "/AmbiguousType",
x: foo1.Bar{},
y: foo2.Bar{},
wantEqual: false,
reason: "reporter should display the qualified type name to disambiguate between the two values",
}, {
label: label + "/AmbiguousPointer",
x: newInt(0),
y: newInt(0),
opts: []cmp.Option{
cmp.Comparer(func(x, y *int) bool { return x == y }),
},
wantEqual: false,
reason: "reporter should display the address to disambiguate between the two values",
}, {
label: label + "/AmbiguousPointerStruct",
x: struct{ I *int }{newInt(0)},
y: struct{ I *int }{newInt(0)},
opts: []cmp.Option{
cmp.Comparer(func(x, y *int) bool { return x == y }),
},
wantEqual: false,
reason: "reporter should display the address to disambiguate between the two struct fields",
}, {
label: label + "/AmbiguousPointerSlice",
x: []*int{newInt(0)},
y: []*int{newInt(0)},
opts: []cmp.Option{
cmp.Comparer(func(x, y *int) bool { return x == y }),
},
wantEqual: false,
reason: "reporter should display the address to disambiguate between the two slice elements",
}, {
label: label + "/AmbiguousPointerMap",
x: map[string]*int{"zero": newInt(0)},
y: map[string]*int{"zero": newInt(0)},
opts: []cmp.Option{
cmp.Comparer(func(x, y *int) bool { return x == y }),
},
wantEqual: false,
reason: "reporter should display the address to disambiguate between the two map values",
}, {
label: label + "/AmbiguousStringer",
x: Stringer("hello"),
y: newStringer("hello"),
wantEqual: false,
reason: "reporter should avoid calling String to disambiguate between the two values",
}, {
label: label + "/AmbiguousStringerStruct",
x: struct{ S fmt.Stringer }{Stringer("hello")},
y: struct{ S fmt.Stringer }{newStringer("hello")},
wantEqual: false,
reason: "reporter should avoid calling String to disambiguate between the two struct fields",
}, {
label: label + "/AmbiguousStringerSlice",
x: []fmt.Stringer{Stringer("hello")},
y: []fmt.Stringer{newStringer("hello")},
wantEqual: false,
reason: "reporter should avoid calling String to disambiguate between the two slice elements",
}, {
label: label + "/AmbiguousStringerMap",
x: map[string]fmt.Stringer{"zero": Stringer("hello")},
y: map[string]fmt.Stringer{"zero": newStringer("hello")},
wantEqual: false,
reason: "reporter should avoid calling String to disambiguate between the two map values",
}, {
label: label + "/AmbiguousSliceHeader",
x: make([]int, 0, 5),
y: make([]int, 0, 1000),
opts: []cmp.Option{
cmp.Comparer(func(x, y []int) bool { return cap(x) == cap(y) }),
},
wantEqual: false,
reason: "reporter should display the slice header to disambiguate between the two slice values",
}, {
label: label + "/AmbiguousStringerMapKey",
x: map[interface{}]string{
nil: "nil",
Stringer("hello"): "goodbye",
foo1.Bar{"fizz"}: "buzz",
},
y: map[interface{}]string{
newStringer("hello"): "goodbye",
foo2.Bar{"fizz"}: "buzz",
},
wantEqual: false,
reason: "reporter should avoid calling String to disambiguate between the two map keys",
}, {
label: label + "/NonAmbiguousStringerMapKey",
x: map[interface{}]string{Stringer("hello"): "goodbye"},
y: map[interface{}]string{newStringer("fizz"): "buzz"},
wantEqual: false,
reason: "reporter should call String as there is no ambiguity between the two map keys",
}, {
label: "/InvalidUTF8",
x: MyString("\xed\xa0\x80"),
wantEqual: false,
Expand Down Expand Up @@ -2146,7 +2239,7 @@ func project1Tests() []test {
Target: "corporation",
Immutable: &ts.GoatImmutable{
ID: "southbay",
State: (*pb.Goat_States)(intPtr(5)),
State: (*pb.Goat_States)(newInt(5)),
Started: now,
},
},
Expand Down Expand Up @@ -2174,7 +2267,7 @@ func project1Tests() []test {
Immutable: &ts.EagleImmutable{
ID: "eagleID",
Birthday: now,
MissingCall: (*pb.Eagle_MissingCalls)(intPtr(55)),
MissingCall: (*pb.Eagle_MissingCalls)(newInt(55)),
},
}
}
Expand Down Expand Up @@ -2219,7 +2312,7 @@ func project1Tests() []test {
x: func() ts.Eagle {
eg := createEagle()
eg.Dreamers[1].Animal[0].(ts.Goat).Immutable.ID = "southbay2"
eg.Dreamers[1].Animal[0].(ts.Goat).Immutable.State = (*pb.Goat_States)(intPtr(6))
eg.Dreamers[1].Animal[0].(ts.Goat).Immutable.State = (*pb.Goat_States)(newInt(6))
eg.Slaps[0].Immutable.MildSlap = false
return eg
}(),
Expand Down
10 changes: 10 additions & 0 deletions cmp/internal/teststructs/foo1/foo.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
// Copyright 2020, The Go Authors. All rights reserved.
// Use of this source code is governed by a BSD-style
// license that can be found in the LICENSE.md file.

// Package foo is deliberately named differently than the parent directory.
// It contain declarations that have ambiguity in their short names,
// relative to a different package also called foo.
package foo

type Bar struct{ S string }
10 changes: 10 additions & 0 deletions cmp/internal/teststructs/foo2/foo.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
// Copyright 2020, The Go Authors. All rights reserved.
// Use of this source code is governed by a BSD-style
// license that can be found in the LICENSE.md file.

// Package foo is deliberately named differently than the parent directory.
// It contain declarations that have ambiguity in their short names,
// relative to a different package also called foo.
package foo

type Bar struct{ S string }
157 changes: 157 additions & 0 deletions cmp/internal/value/name.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,157 @@
// Copyright 2020, The Go Authors. All rights reserved.
// Use of this source code is governed by a BSD-style
// license that can be found in the LICENSE.md file.

package value

import (
"reflect"
"strconv"
)

// TypeString is nearly identical to reflect.Type.String,
// but has an additional option to specify that full type names be used.
func TypeString(t reflect.Type, qualified bool) string {
return string(appendTypeName(nil, t, qualified, false))
}

func appendTypeName(b []byte, t reflect.Type, qualified, elideFunc bool) []byte {
// BUG: Go reflection provides no way to disambiguate two named types
// of the same name and within the same package,
// but declared within the namespace of different functions.

// Named type.
if t.Name() != "" {
if qualified && t.PkgPath() != "" {
b = append(b, '"')
b = append(b, t.PkgPath()...)
b = append(b, '"')
b = append(b, '.')
b = append(b, t.Name()...)
} else {
b = append(b, t.String()...)
}
return b
}

// Unnamed type.
switch k := t.Kind(); k {
case reflect.Bool, reflect.String, reflect.UnsafePointer,
reflect.Int, reflect.Int8, reflect.Int16, reflect.Int32, reflect.Int64,
reflect.Uint, reflect.Uint8, reflect.Uint16, reflect.Uint32, reflect.Uint64, reflect.Uintptr,
reflect.Float32, reflect.Float64, reflect.Complex64, reflect.Complex128:
b = append(b, k.String()...)
case reflect.Chan:
if t.ChanDir() == reflect.RecvDir {
b = append(b, "<-"...)
}
b = append(b, "chan"...)
if t.ChanDir() == reflect.SendDir {
b = append(b, "<-"...)
}
b = append(b, ' ')
b = appendTypeName(b, t.Elem(), qualified, false)
case reflect.Func:
if !elideFunc {
b = append(b, "func"...)
}
b = append(b, '(')
for i := 0; i < t.NumIn(); i++ {
if i > 0 {
b = append(b, ", "...)
}
if i == t.NumIn()-1 && t.IsVariadic() {
b = append(b, "..."...)
b = appendTypeName(b, t.In(i).Elem(), qualified, false)
} else {
b = appendTypeName(b, t.In(i), qualified, false)
}
}
b = append(b, ')')
switch t.NumOut() {
case 0:
// Do nothing
case 1:
b = append(b, ' ')
b = appendTypeName(b, t.Out(0), qualified, false)
default:
b = append(b, " ("...)
for i := 0; i < t.NumOut(); i++ {
if i > 0 {
b = append(b, ", "...)
}
b = appendTypeName(b, t.Out(i), qualified, false)
}
b = append(b, ')')
}
case reflect.Struct:
b = append(b, "struct{ "...)
for i := 0; i < t.NumField(); i++ {
if i > 0 {
b = append(b, "; "...)
}
sf := t.Field(i)
if !sf.Anonymous {
if qualified && sf.PkgPath != "" {
b = append(b, '"')
b = append(b, sf.PkgPath...)
b = append(b, '"')
b = append(b, '.')
}
b = append(b, sf.Name...)
b = append(b, ' ')
}
b = appendTypeName(b, sf.Type, qualified, false)
if sf.Tag != "" {
b = append(b, ' ')
b = strconv.AppendQuote(b, string(sf.Tag))
}
}
if b[len(b)-1] == ' ' {
b = b[:len(b)-1]
} else {
b = append(b, ' ')
}
b = append(b, '}')
case reflect.Slice, reflect.Array:
b = append(b, '[')
if k == reflect.Array {
b = strconv.AppendUint(b, uint64(t.Len()), 10)
}
b = append(b, ']')
b = appendTypeName(b, t.Elem(), qualified, false)
case reflect.Map:
b = append(b, "map["...)
b = appendTypeName(b, t.Key(), qualified, false)
b = append(b, ']')
b = appendTypeName(b, t.Elem(), qualified, false)
case reflect.Ptr:
b = append(b, '*')
b = appendTypeName(b, t.Elem(), qualified, false)
case reflect.Interface:
b = append(b, "interface{ "...)
for i := 0; i < t.NumMethod(); i++ {
if i > 0 {
b = append(b, "; "...)
}
m := t.Method(i)
if qualified && m.PkgPath != "" {
b = append(b, '"')
b = append(b, m.PkgPath...)
b = append(b, '"')
b = append(b, '.')
}
b = append(b, m.Name...)
b = appendTypeName(b, m.Type, qualified, true)
}
if b[len(b)-1] == ' ' {
b = b[:len(b)-1]
} else {
b = append(b, ' ')
}
b = append(b, '}')
default:
panic("invalid kind: " + k.String())
}
return b
}
Loading

0 comments on commit 44914b3

Please sign in to comment.