Skip to content

Commit

Permalink
Limit verbosity of reporter output (#215)
Browse files Browse the repository at this point in the history
A common complaint is that the reporter it prints out too much
irrelevant information, resulting in a low signal-to-noise ratio.
Improve this metric by imposing a verbosity limit. For nodes that
are equal, we set the verbosity level is a lower value than when
the nodes are inequal.

Other minor changes:
* Adjust heuristic for triple-quote usage to operate on more cases.
* Elide type more aggressively for equal nodes.
* Printing the address for a slice includes the length and capacity.
* The pointed-at value for map keys are printed.
  • Loading branch information
dsnet authored Jun 12, 2020
1 parent 0d296f9 commit f1780cf
Show file tree
Hide file tree
Showing 6 changed files with 287 additions and 197 deletions.
4 changes: 2 additions & 2 deletions cmp/compare_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -159,8 +159,8 @@ func TestDiff(t *testing.T) {
}
} else {
wantDiff := wantDiffs[t.Name()]
if gotDiff != wantDiff {
t.Fatalf("Diff:\ngot:\n%s\nwant:\n%s\nreason: %v", gotDiff, wantDiff, tt.reason)
if diff := cmp.Diff(wantDiff, gotDiff); diff != "" {
t.Fatalf("Diff:\ngot:\n%s\nwant:\n%s\ndiff: (-want +got)\n%s\nreason: %v", gotDiff, wantDiff, diff, tt.reason)
}
}
gotEqual := gotDiff == ""
Expand Down
2 changes: 1 addition & 1 deletion cmp/example_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ func ExampleDiff_testing() {
// SSID: "CoffeeShopWiFi",
// - IPAddress: s"192.168.0.2",
// + IPAddress: s"192.168.0.1",
// NetMask: net.IPMask{0xff, 0xff, 0x00, 0x00},
// NetMask: {0xff, 0xff, 0x00, 0x00},
// Clients: []cmp_test.Client{
// ... // 2 identical elements
// {Hostname: "macchiato", IPAddress: s"192.168.0.153", LastSeen: s"2009-11-10 23:39:43 +0000 UTC"},
Expand Down
46 changes: 41 additions & 5 deletions cmp/report_compare.go
Original file line number Diff line number Diff line change
Expand Up @@ -11,10 +11,6 @@ import (
"github.com/google/go-cmp/cmp/internal/value"
)

// TODO: Enforce limits?
// * Enforce maximum number of records to print per node?
// * Enforce maximum size in bytes allowed?
// * As a heuristic, use less verbosity for equal nodes than unequal nodes.
// TODO: Enforce unique outputs?
// * Avoid Stringer methods if it results in same output?
// * Print pointer address if outputs still equal?
Expand Down Expand Up @@ -71,10 +67,31 @@ func (opts formatOptions) WithTypeMode(t typeMode) formatOptions {
opts.TypeMode = t
return opts
}
func (opts formatOptions) WithVerbosity(level int) formatOptions {
opts.VerbosityLevel = level
opts.LimitVerbosity = true
return opts
}
func (opts formatOptions) verbosity() uint {
switch {
case opts.VerbosityLevel < 0:
return 0
case opts.VerbosityLevel > 16:
return 16 // some reasonable maximum to avoid shift overflow
default:
return uint(opts.VerbosityLevel)
}
}

// FormatDiff converts a valueNode tree into a textNode tree, where the later
// is a textual representation of the differences detected in the former.
func (opts formatOptions) FormatDiff(v *valueNode) textNode {
if opts.DiffMode == diffIdentical {
opts = opts.WithVerbosity(1)
} else {
opts = opts.WithVerbosity(3)
}

// Check whether we have specialized formatting for this node.
// This is not necessary, but helpful for producing more readable outputs.
if opts.CanFormatDiffSlice(v) {
Expand Down Expand Up @@ -124,6 +141,8 @@ func (opts formatOptions) FormatDiff(v *valueNode) textNode {
}
}

// TODO: Print cycle reference for pointers, maps, and elements of a slice.

// Descend into the child value node.
if v.TransformerName != "" {
out := opts.WithTypeMode(emitType).FormatDiff(v.Value)
Expand Down Expand Up @@ -162,12 +181,27 @@ func (opts formatOptions) formatDiffList(recs []reportRecord, k reflect.Kind) te
formatKey = formatMapKey
}

maxLen := -1
if opts.LimitVerbosity {
if opts.DiffMode == diffIdentical {
maxLen = ((1 << opts.verbosity()) >> 1) << 2 // 0, 4, 8, 16, 32, etc...
} else {
maxLen = (1 << opts.verbosity()) << 1 // 2, 4, 8, 16, 32, 64, etc...
}
opts.VerbosityLevel--
}

// Handle unification.
switch opts.DiffMode {
case diffIdentical, diffRemoved, diffInserted:
var list textList
var deferredEllipsis bool // Add final "..." to indicate records were dropped
for _, r := range recs {
if len(list) == maxLen {
deferredEllipsis = true
break
}

// Elide struct fields that are zero value.
if k == reflect.Struct {
var isZero bool
Expand Down Expand Up @@ -205,11 +239,12 @@ func (opts formatOptions) formatDiffList(recs []reportRecord, k reflect.Kind) te
}

// Handle differencing.
var numDiffs int
var list textList
groups := coalesceAdjacentRecords(name, recs)
maxGroup := diffStats{Name: name}
for i, ds := range groups {
if len(list) >= maxDiffElements {
if maxLen >= 0 && numDiffs >= maxLen {
maxGroup = maxGroup.Append(ds)
continue
}
Expand Down Expand Up @@ -273,6 +308,7 @@ func (opts formatOptions) formatDiffList(recs []reportRecord, k reflect.Kind) te
}
}
recs = recs[ds.NumDiff():]
numDiffs += ds.NumDiff()
}
if maxGroup.IsZero() {
assert(len(recs) == 0)
Expand Down
79 changes: 70 additions & 9 deletions cmp/report_reflect.go
Original file line number Diff line number Diff line change
Expand Up @@ -21,14 +21,23 @@ type formatValueOptions struct {
// methods like error.Error or fmt.Stringer.String.
AvoidStringer bool

// ShallowPointers controls whether to avoid descending into pointers.
// PrintShallowPointer controls whether to print the next pointer.
// Useful when printing map keys, where pointer comparison is performed
// on the pointer address rather than the pointed-at value.
ShallowPointers bool
PrintShallowPointer bool

// PrintAddresses controls whether to print the address of all pointers,
// slice elements, and maps.
PrintAddresses bool

// VerbosityLevel controls the amount of output to produce.
// A higher value produces more output. A value of zero or lower produces
// no output (represented using an ellipsis).
// If LimitVerbosity is false, then the level is treated as infinite.
VerbosityLevel int

// LimitVerbosity specifies that formatting should respect VerbosityLevel.
LimitVerbosity bool
}

// FormatType prints the type as if it were wrapping s.
Expand All @@ -45,6 +54,9 @@ func (opts formatOptions) FormatType(t reflect.Type, s textNode) textNode {
default:
return s
}
if opts.DiffMode == diffIdentical {
return s // elide type for identical nodes
}
case elideType:
return s
}
Expand Down Expand Up @@ -86,11 +98,22 @@ func (opts formatOptions) FormatValue(v reflect.Value, withinSlice bool, m visit
// Avoid calling Error or String methods on nil receivers since many
// implementations crash when doing so.
if (t.Kind() != reflect.Ptr && t.Kind() != reflect.Interface) || !v.IsNil() {
var prefix, strVal string
switch v := v.Interface().(type) {
case error:
return textLine("e" + formatString(v.Error()))
prefix, strVal = "e", v.Error()
case fmt.Stringer:
return textLine("s" + formatString(v.String()))
prefix, strVal = "s", v.String()
}
if prefix != "" {
maxLen := len(strVal)
if opts.LimitVerbosity {
maxLen = (1 << opts.verbosity()) << 5 // 32, 64, 128, 256, etc...
}
if len(strVal) > maxLen+len(textEllipsis) {
return textLine(prefix + formatString(strVal[:maxLen]) + string(textEllipsis))
}
return textLine(prefix + formatString(strVal))
}
}
}
Expand Down Expand Up @@ -123,17 +146,33 @@ func (opts formatOptions) FormatValue(v reflect.Value, withinSlice bool, m visit
case reflect.Complex64, reflect.Complex128:
return textLine(fmt.Sprint(v.Complex()))
case reflect.String:
maxLen := v.Len()
if opts.LimitVerbosity {
maxLen = (1 << opts.verbosity()) << 5 // 32, 64, 128, 256, etc...
}
if v.Len() > maxLen+len(textEllipsis) {
return textLine(formatString(v.String()[:maxLen]) + string(textEllipsis))
}
return textLine(formatString(v.String()))
case reflect.UnsafePointer, reflect.Chan, reflect.Func:
return textLine(formatPointer(v))
case reflect.Struct:
var list textList
v := makeAddressable(v) // needed for retrieveUnexportedField
maxLen := v.NumField()
if opts.LimitVerbosity {
maxLen = ((1 << opts.verbosity()) >> 1) << 2 // 0, 4, 8, 16, 32, etc...
opts.VerbosityLevel--
}
for i := 0; i < v.NumField(); i++ {
vv := v.Field(i)
if value.IsZero(vv) {
continue // Elide fields with zero values
}
if len(list) == maxLen {
list.AppendEllipsis(diffStats{})
break
}
sf := t.Field(i)
if supportExporters && !isExported(sf.Name) {
vv = retrieveUnexportedField(v, sf, true)
Expand All @@ -147,12 +186,21 @@ func (opts formatOptions) FormatValue(v reflect.Value, withinSlice bool, m visit
return textNil
}
if opts.PrintAddresses {
ptr = formatPointer(v)
ptr = fmt.Sprintf("⟪ptr:0x%x, len:%d, cap:%d⟫", pointerValue(v), v.Len(), v.Cap())
}
fallthrough
case reflect.Array:
maxLen := v.Len()
if opts.LimitVerbosity {
maxLen = ((1 << opts.verbosity()) >> 1) << 2 // 0, 4, 8, 16, 32, etc...
opts.VerbosityLevel--
}
var list textList
for i := 0; i < v.Len(); i++ {
if len(list) == maxLen {
list.AppendEllipsis(diffStats{})
break
}
vi := v.Index(i)
if vi.CanAddr() { // Check for cyclic elements
p := vi.Addr()
Expand All @@ -177,8 +225,17 @@ func (opts formatOptions) FormatValue(v reflect.Value, withinSlice bool, m visit
return textLine(formatPointer(v))
}

maxLen := v.Len()
if opts.LimitVerbosity {
maxLen = ((1 << opts.verbosity()) >> 1) << 2 // 0, 4, 8, 16, 32, etc...
opts.VerbosityLevel--
}
var list textList
for _, k := range value.SortKeys(v.MapKeys()) {
if len(list) == maxLen {
list.AppendEllipsis(diffStats{})
break
}
sk := formatMapKey(k)
sv := opts.WithTypeMode(elideType).FormatValue(v.MapIndex(k), false, m)
list = append(list, textRecord{Key: sk, Value: sv})
Expand All @@ -191,11 +248,12 @@ func (opts formatOptions) FormatValue(v reflect.Value, withinSlice bool, m visit
if v.IsNil() {
return textNil
}
if m.Visit(v) || opts.ShallowPointers {
if m.Visit(v) {
return textLine(formatPointer(v))
}
if opts.PrintAddresses {
if opts.PrintAddresses || opts.PrintShallowPointer {
ptr = formatPointer(v)
opts.PrintShallowPointer = false
}
skipType = true // Let the underlying value print the type instead
return textWrap{"&" + ptr, opts.FormatValue(v.Elem(), false, m), ""}
Expand All @@ -217,7 +275,7 @@ func (opts formatOptions) FormatValue(v reflect.Value, withinSlice bool, m visit
func formatMapKey(v reflect.Value) string {
var opts formatOptions
opts.TypeMode = elideType
opts.ShallowPointers = true
opts.PrintShallowPointer = true
s := opts.FormatValue(v, false, visitedPointers{}).String()
return strings.TrimSpace(s)
}
Expand Down Expand Up @@ -268,11 +326,14 @@ func formatHex(u uint64) string {

// formatPointer prints the address of the pointer.
func formatPointer(v reflect.Value) string {
return fmt.Sprintf("⟪0x%x⟫", pointerValue(v))
}
func pointerValue(v reflect.Value) uintptr {
p := v.Pointer()
if flags.Deterministic {
p = 0xdeadf00f // Only used for stable testing purposes
}
return fmt.Sprintf("⟪0x%x⟫", p)
return p
}

type visitedPointers map[value.Pointer]struct{}
Expand Down
38 changes: 24 additions & 14 deletions cmp/report_slices.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,10 +16,6 @@ import (
"github.com/google/go-cmp/cmp/internal/diff"
)

// maxDiffElements is the maximum number of difference elements to format
// before the remaining differences are coalesced together.
const maxDiffElements = 32

// CanFormatDiffSlice reports whether we support custom formatting for nodes
// that are slices of primitive kinds or strings.
func (opts formatOptions) CanFormatDiffSlice(v *valueNode) bool {
Expand Down Expand Up @@ -155,7 +151,8 @@ func (opts formatOptions) FormatDiffSlice(v *valueNode) textNode {
// + BAZ
// """
isTripleQuoted := true
prevDiffLines := map[string]bool{}
prevRemoveLines := map[string]bool{}
prevInsertLines := map[string]bool{}
var list2 textList
list2 = append(list2, textRecord{Value: textLine(`"""`), ElideComma: true})
for _, r := range list {
Expand All @@ -171,20 +168,24 @@ func (opts formatOptions) FormatDiffSlice(v *valueNode) textNode {
isPrintable := func(r rune) bool {
return unicode.IsPrint(r) || r == '\t' // specially treat tab as printable
}
isTripleQuoted = isTripleQuoted &&
!strings.HasPrefix(line, `"""`) &&
!strings.HasPrefix(line, "...") &&
strings.TrimFunc(line, isPrintable) == "" &&
(r.Diff == 0 || !prevDiffLines[normLine])
isTripleQuoted = !strings.HasPrefix(line, `"""`) && !strings.HasPrefix(line, "...") && strings.TrimFunc(line, isPrintable) == ""
switch r.Diff {
case diffRemoved:
isTripleQuoted = isTripleQuoted && !prevInsertLines[normLine]
prevRemoveLines[normLine] = true
case diffInserted:
isTripleQuoted = isTripleQuoted && !prevRemoveLines[normLine]
prevInsertLines[normLine] = true
}
if !isTripleQuoted {
break
}
r.Value = textLine(line)
r.ElideComma = true
prevDiffLines[normLine] = true
}
if r.Diff == 0 {
prevDiffLines = map[string]bool{} // start a new non-adjacent difference group
if !(r.Diff == diffRemoved || r.Diff == diffInserted) { // start a new non-adjacent difference group
prevRemoveLines = map[string]bool{}
prevInsertLines = map[string]bool{}
}
list2 = append(list2, r)
}
Expand Down Expand Up @@ -337,11 +338,18 @@ func (opts formatOptions) formatDiffSlice(
return n0 - v.Len()
}

var numDiffs int
maxLen := -1
if opts.LimitVerbosity {
maxLen = (1 << opts.verbosity()) << 2 // 4, 8, 16, 32, 64, etc...
opts.VerbosityLevel--
}

groups := coalesceAdjacentEdits(name, es)
groups = coalesceInterveningIdentical(groups, chunkSize/4)
maxGroup := diffStats{Name: name}
for i, ds := range groups {
if len(list) >= maxDiffElements {
if maxLen >= 0 && numDiffs >= maxLen {
maxGroup = maxGroup.Append(ds)
continue
}
Expand Down Expand Up @@ -374,10 +382,12 @@ func (opts formatOptions) formatDiffSlice(
}

// Print unequal.
len0 := len(list)
nx := appendChunks(vx.Slice(0, ds.NumIdentical+ds.NumRemoved+ds.NumModified), diffRemoved)
vx = vx.Slice(nx, vx.Len())
ny := appendChunks(vy.Slice(0, ds.NumIdentical+ds.NumInserted+ds.NumModified), diffInserted)
vy = vy.Slice(ny, vy.Len())
numDiffs += len(list) - len0
}
if maxGroup.IsZero() {
assert(vx.Len() == 0 && vy.Len() == 0)
Expand Down
Loading

0 comments on commit f1780cf

Please sign in to comment.