From 1e9ce46f0a677c16493a8987ab0aceb3709a821e Mon Sep 17 00:00:00 2001 From: Mike Danese Date: Sat, 13 Apr 2019 09:46:54 -0700 Subject: [PATCH] migrate k8s.io/apimachinery/pkg/util/diff to cmp --- .../apimachinery/pkg/api/resource/quantity.go | 6 + .../k8s.io/apimachinery/pkg/util/diff/BUILD | 2 +- .../k8s.io/apimachinery/pkg/util/diff/diff.go | 242 ++---------------- .../apimachinery/pkg/util/diff/diff_test.go | 133 ---------- 4 files changed, 24 insertions(+), 359 deletions(-) diff --git a/staging/src/k8s.io/apimachinery/pkg/api/resource/quantity.go b/staging/src/k8s.io/apimachinery/pkg/api/resource/quantity.go index 54fda58064d..93a6c0c5004 100644 --- a/staging/src/k8s.io/apimachinery/pkg/api/resource/quantity.go +++ b/staging/src/k8s.io/apimachinery/pkg/api/resource/quantity.go @@ -584,6 +584,12 @@ func (q *Quantity) Neg() { q.d.Dec.Neg(q.d.Dec) } +// Equal checks equality of two Quantities. This is useful for testing with +// cmp.Equal. +func (q Quantity) Equal(v Quantity) bool { + return q.Cmp(v) == 0 +} + // int64QuantityExpectedBytes is the expected width in bytes of the canonical string representation // of most Quantity values. const int64QuantityExpectedBytes = 18 diff --git a/staging/src/k8s.io/apimachinery/pkg/util/diff/BUILD b/staging/src/k8s.io/apimachinery/pkg/util/diff/BUILD index 0089ba84821..006929eacbc 100644 --- a/staging/src/k8s.io/apimachinery/pkg/util/diff/BUILD +++ b/staging/src/k8s.io/apimachinery/pkg/util/diff/BUILD @@ -18,8 +18,8 @@ go_library( importmap = "k8s.io/kubernetes/vendor/k8s.io/apimachinery/pkg/util/diff", importpath = "k8s.io/apimachinery/pkg/util/diff", deps = [ - "//staging/src/k8s.io/apimachinery/pkg/util/validation/field:go_default_library", "//vendor/github.com/davecgh/go-spew/spew:go_default_library", + "//vendor/github.com/google/go-cmp/cmp:go_default_library", ], ) diff --git a/staging/src/k8s.io/apimachinery/pkg/util/diff/diff.go b/staging/src/k8s.io/apimachinery/pkg/util/diff/diff.go index 5ce92482999..a006b925a9e 100644 --- a/staging/src/k8s.io/apimachinery/pkg/util/diff/diff.go +++ b/staging/src/k8s.io/apimachinery/pkg/util/diff/diff.go @@ -18,16 +18,12 @@ package diff import ( "bytes" - "encoding/json" "fmt" - "reflect" - "sort" "strings" "text/tabwriter" "github.com/davecgh/go-spew/spew" - - "k8s.io/apimachinery/pkg/util/validation/field" + "github.com/google/go-cmp/cmp" ) // StringDiff diffs a and b and returns a human readable diff. @@ -50,233 +46,29 @@ func StringDiff(a, b string) string { return string(out) } -// ObjectDiff writes the two objects out as JSON and prints out the identical part of -// the objects followed by the remaining part of 'a' and finally the remaining part of 'b'. -// For debugging tests. +func legacyDiff(a, b interface{}) string { + return cmp.Diff(a, b) +} + +// ObjectDiff prints the diff of two go objects and fails if the objects +// contain unhandled unexported fields. +// DEPRECATED: use github.com/google/go-cmp/cmp.Diff func ObjectDiff(a, b interface{}) string { - ab, err := json.Marshal(a) - if err != nil { - panic(fmt.Sprintf("a: %v", err)) - } - bb, err := json.Marshal(b) - if err != nil { - panic(fmt.Sprintf("b: %v", err)) - } - return StringDiff(string(ab), string(bb)) + return legacyDiff(a, b) } -// ObjectGoPrintDiff is like ObjectDiff, but uses go-spew to print the objects, -// which shows absolutely everything by recursing into every single pointer -// (go's %#v formatters OTOH stop at a certain point). This is needed when you -// can't figure out why reflect.DeepEqual is returning false and nothing is -// showing you differences. This will. +// ObjectGoPrintDiff prints the diff of two go objects and fails if the objects +// contain unhandled unexported fields. +// DEPRECATED: use github.com/google/go-cmp/cmp.Diff func ObjectGoPrintDiff(a, b interface{}) string { - s := spew.ConfigState{DisableMethods: true} - return StringDiff( - s.Sprintf("%#v", a), - s.Sprintf("%#v", b), - ) + return legacyDiff(a, b) } -// ObjectReflectDiff returns a multi-line formatted diff between two objects -// of equal type. If an object with private fields is passed you will -// only see string comparison for those fields. Otherwise this presents the -// most human friendly diff of two structs of equal type in this package. +// ObjectReflectDiff prints the diff of two go objects and fails if the objects +// contain unhandled unexported fields. +// DEPRECATED: use github.com/google/go-cmp/cmp.Diff func ObjectReflectDiff(a, b interface{}) string { - if a == nil && b == nil { - return "" - } - if a == nil { - return fmt.Sprintf("a is nil and b is not-nil") - } - if b == nil { - return fmt.Sprintf("a is not-nil and b is nil") - } - vA, vB := reflect.ValueOf(a), reflect.ValueOf(b) - if vA.Type() != vB.Type() { - return fmt.Sprintf("type A %T and type B %T do not match", a, b) - } - diffs := objectReflectDiff(field.NewPath("object"), vA, vB) - if len(diffs) == 0 { - return "" - } - out := []string{""} - for _, d := range diffs { - elidedA, elidedB := limit(d.a, d.b, 80) - out = append(out, - fmt.Sprintf("%s:", d.path), - fmt.Sprintf(" a: %s", elidedA), - fmt.Sprintf(" b: %s", elidedB), - ) - } - return strings.Join(out, "\n") -} - -// limit: -// 1. stringifies aObj and bObj -// 2. elides identical prefixes if either is too long -// 3. elides remaining content from the end if either is too long -func limit(aObj, bObj interface{}, max int) (string, string) { - elidedPrefix := "" - elidedASuffix := "" - elidedBSuffix := "" - a, b := fmt.Sprintf("%#v", aObj), fmt.Sprintf("%#v", bObj) - - if aObj != nil && bObj != nil { - if aType, bType := fmt.Sprintf("%T", aObj), fmt.Sprintf("%T", bObj); aType != bType { - a = fmt.Sprintf("%s (%s)", a, aType) - b = fmt.Sprintf("%s (%s)", b, bType) - } - } - - for { - switch { - case len(a) > max && len(a) > 4 && len(b) > 4 && a[:4] == b[:4]: - // a is too long, b has data, and the first several characters are the same - elidedPrefix = "..." - a = a[2:] - b = b[2:] - - case len(b) > max && len(b) > 4 && len(a) > 4 && a[:4] == b[:4]: - // b is too long, a has data, and the first several characters are the same - elidedPrefix = "..." - a = a[2:] - b = b[2:] - - case len(a) > max: - a = a[:max] - elidedASuffix = "..." - - case len(b) > max: - b = b[:max] - elidedBSuffix = "..." - - default: - // both are short enough - return elidedPrefix + a + elidedASuffix, elidedPrefix + b + elidedBSuffix - } - } -} - -func public(s string) bool { - if len(s) == 0 { - return false - } - return s[:1] == strings.ToUpper(s[:1]) -} - -type diff struct { - path *field.Path - a, b interface{} -} - -type orderedDiffs []diff - -func (d orderedDiffs) Len() int { return len(d) } -func (d orderedDiffs) Swap(i, j int) { d[i], d[j] = d[j], d[i] } -func (d orderedDiffs) Less(i, j int) bool { - a, b := d[i].path.String(), d[j].path.String() - if a < b { - return true - } - return false -} - -func objectReflectDiff(path *field.Path, a, b reflect.Value) []diff { - switch a.Type().Kind() { - case reflect.Struct: - var changes []diff - for i := 0; i < a.Type().NumField(); i++ { - if !public(a.Type().Field(i).Name) { - if reflect.DeepEqual(a.Interface(), b.Interface()) { - continue - } - return []diff{{path: path, a: fmt.Sprintf("%#v", a), b: fmt.Sprintf("%#v", b)}} - } - if sub := objectReflectDiff(path.Child(a.Type().Field(i).Name), a.Field(i), b.Field(i)); len(sub) > 0 { - changes = append(changes, sub...) - } - } - return changes - case reflect.Ptr, reflect.Interface: - if a.IsNil() || b.IsNil() { - switch { - case a.IsNil() && b.IsNil(): - return nil - case a.IsNil(): - return []diff{{path: path, a: nil, b: b.Interface()}} - default: - return []diff{{path: path, a: a.Interface(), b: nil}} - } - } - return objectReflectDiff(path, a.Elem(), b.Elem()) - case reflect.Chan: - if !reflect.DeepEqual(a.Interface(), b.Interface()) { - return []diff{{path: path, a: a.Interface(), b: b.Interface()}} - } - return nil - case reflect.Slice: - lA, lB := a.Len(), b.Len() - l := lA - if lB < lA { - l = lB - } - if lA == lB && lA == 0 { - if a.IsNil() != b.IsNil() { - return []diff{{path: path, a: a.Interface(), b: b.Interface()}} - } - return nil - } - var diffs []diff - for i := 0; i < l; i++ { - if !reflect.DeepEqual(a.Index(i), b.Index(i)) { - diffs = append(diffs, objectReflectDiff(path.Index(i), a.Index(i), b.Index(i))...) - } - } - for i := l; i < lA; i++ { - diffs = append(diffs, diff{path: path.Index(i), a: a.Index(i), b: nil}) - } - for i := l; i < lB; i++ { - diffs = append(diffs, diff{path: path.Index(i), a: nil, b: b.Index(i)}) - } - return diffs - case reflect.Map: - if reflect.DeepEqual(a.Interface(), b.Interface()) { - return nil - } - aKeys := make(map[interface{}]interface{}) - for _, key := range a.MapKeys() { - aKeys[key.Interface()] = a.MapIndex(key).Interface() - } - var missing []diff - for _, key := range b.MapKeys() { - if _, ok := aKeys[key.Interface()]; ok { - delete(aKeys, key.Interface()) - if reflect.DeepEqual(a.MapIndex(key).Interface(), b.MapIndex(key).Interface()) { - continue - } - missing = append(missing, objectReflectDiff(path.Key(fmt.Sprintf("%s", key.Interface())), a.MapIndex(key), b.MapIndex(key))...) - continue - } - missing = append(missing, diff{path: path.Key(fmt.Sprintf("%s", key.Interface())), a: nil, b: b.MapIndex(key).Interface()}) - } - for key, value := range aKeys { - missing = append(missing, diff{path: path.Key(fmt.Sprintf("%s", key)), a: value, b: nil}) - } - if len(missing) == 0 { - missing = append(missing, diff{path: path, a: a.Interface(), b: b.Interface()}) - } - sort.Sort(orderedDiffs(missing)) - return missing - default: - if reflect.DeepEqual(a.Interface(), b.Interface()) { - return nil - } - if !a.CanInterface() { - return []diff{{path: path, a: fmt.Sprintf("%#v", a), b: fmt.Sprintf("%#v", b)}} - } - return []diff{{path: path, a: a.Interface(), b: b.Interface()}} - } + return legacyDiff(a, b) } // ObjectGoPrintSideBySide prints a and b as textual dumps side by side, diff --git a/staging/src/k8s.io/apimachinery/pkg/util/diff/diff_test.go b/staging/src/k8s.io/apimachinery/pkg/util/diff/diff_test.go index 6af43f415b7..eb61a11d779 100644 --- a/staging/src/k8s.io/apimachinery/pkg/util/diff/diff_test.go +++ b/staging/src/k8s.io/apimachinery/pkg/util/diff/diff_test.go @@ -20,92 +20,6 @@ import ( "testing" ) -func TestObjectReflectDiff(t *testing.T) { - type struct1 struct{ A []int } - - testCases := map[string]struct { - a, b interface{} - out string - }{ - "both nil": { - a: interface{}(nil), - b: interface{}(nil), - }, - "a nil": { - a: interface{}(nil), - b: "test", - out: "a is nil and b is not-nil", - }, - "b nil": { - a: "test", - b: interface{}(nil), - out: "a is not-nil and b is nil", - }, - "map": { - a: map[string]int{}, - b: map[string]int{}, - }, - "detect nil map": { - a: map[string]int(nil), - b: map[string]int{}, - out: ` -object: - a: map[string]int(nil) - b: map[string]int{}`, - }, - "detect map changes": { - a: map[string]int{"test": 1, "other": 2}, - b: map[string]int{"test": 2, "third": 3}, - out: ` -object[other]: - a: 2 - b: -object[test]: - a: 1 - b: 2 -object[third]: - a: - b: 3`, - }, - "nil slice": {a: struct1{A: nil}, b: struct1{A: nil}}, - "empty slice": {a: struct1{A: []int{}}, b: struct1{A: []int{}}}, - "detect slice changes 1": {a: struct1{A: []int{1}}, b: struct1{A: []int{2}}, out: ` -object.A[0]: - a: 1 - b: 2`, - }, - "detect slice changes 2": {a: struct1{A: []int{}}, b: struct1{A: []int{2}}, out: ` -object.A[0]: - a: - b: 2`, - }, - "detect slice changes 3": {a: struct1{A: []int{1}}, b: struct1{A: []int{}}, out: ` -object.A[0]: - a: 1 - b: `, - }, - "detect nil vs empty slices": {a: struct1{A: nil}, b: struct1{A: []int{}}, out: ` -object.A: - a: []int(nil) - b: []int{}`, - }, - "display type differences": {a: []interface{}{int64(1)}, b: []interface{}{uint64(1)}, out: ` -object[0]: - a: 1 (int64) - b: 0x1 (uint64)`, - }, - } - for name, test := range testCases { - expect := test.out - if len(expect) == 0 { - expect = "" - } - if actual := ObjectReflectDiff(test.a, test.b); actual != expect { - t.Errorf("%s: unexpected output: %s", name, actual) - } - } -} - func TestStringDiff(t *testing.T) { diff := StringDiff("aaabb", "aaacc") expect := "aaa\n\nA: bb\n\nB: cc\n\n" @@ -113,50 +27,3 @@ func TestStringDiff(t *testing.T) { t.Errorf("diff returned %v", diff) } } - -func TestLimit(t *testing.T) { - testcases := []struct { - a interface{} - b interface{} - expectA string - expectB string - }{ - { - a: `short a`, - b: `short b`, - expectA: `"short a"`, - expectB: `"short b"`, - }, - { - a: `short a`, - b: `long b needs truncating`, - expectA: `"short a"`, - expectB: `"long b ne...`, - }, - { - a: `long a needs truncating`, - b: `long b needs truncating`, - expectA: `...g a needs ...`, - expectB: `...g b needs ...`, - }, - { - a: `long common prefix with different stuff at the end of a`, - b: `long common prefix with different stuff at the end of b`, - expectA: `...end of a"`, - expectB: `...end of b"`, - }, - { - a: `long common prefix with different stuff at the end of a`, - b: `long common prefix with different stuff at the end of b which continues`, - expectA: `...of a"`, - expectB: `...of b which...`, - }, - } - - for _, tc := range testcases { - a, b := limit(tc.a, tc.b, 10) - if a != tc.expectA || b != tc.expectB { - t.Errorf("limit(%q, %q)\n\texpected: %s, %s\n\tgot: %s, %s", tc.a, tc.b, tc.expectA, tc.expectB, a, b) - } - } -}