Merge pull request #32434 from smarterclayton/diff_improvements

Automatic merge from submit-queue

ObjectReflectDiff needs a few more checks for nil

pkg/util/diff/diff.go

@@ -85,7 +85,7 @@ func ObjectReflectDiff(a, b interface{}) string {
 	}
 	diffs := objectReflectDiff(field.NewPath("object"), vA, vB)
 	if len(diffs) == 0 {
-		return ""
+		return "<no diffs>"
 	}
 	out := []string{""}
 	for _, d := range diffs {
@@ -136,12 +136,16 @@ func objectReflectDiff(path *field.Path, a, b reflect.Value) []diff {
 		for i := 0; i < a.Type().NumField(); i++ {
 			if !public(a.Type().Field(i).Name) {
 				if reflect.DeepEqual(a.Interface(), b.Interface()) {
-					return nil
+					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...)
+			} else {
+				if !reflect.DeepEqual(a.Field(i).Interface(), b.Field(i).Interface()) {
+					changes = append(changes, diff{path: path, a: a.Field(i).Interface(), b: b.Field(i).Interface()})
+				}
 			}
 		}
 		return changes
@@ -163,14 +167,17 @@ func objectReflectDiff(path *field.Path, a, b reflect.Value) []diff {
 		}
 		return nil
 	case reflect.Slice:
-		if reflect.DeepEqual(a, b) {
-			return nil
-		}
 		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
+		}
 		for i := 0; i < l; i++ {
 			if !reflect.DeepEqual(a.Index(i), b.Index(i)) {
 				return objectReflectDiff(path.Index(i), a.Index(i), b.Index(i))
@@ -183,9 +190,12 @@ func objectReflectDiff(path *field.Path, a, b reflect.Value) []diff {
 		for i := l; i < lB; i++ {
 			diffs = append(diffs, diff{path: path.Index(i), a: nil, b: b.Index(i)})
 		}
+		if len(diffs) == 0 {
+			diffs = append(diffs, diff{path: path, a: a, b: b})
+		}
 		return diffs
 	case reflect.Map:
-		if reflect.DeepEqual(a, b) {
+		if reflect.DeepEqual(a.Interface(), b.Interface()) {
 			return nil
 		}
 		aKeys := make(map[interface{}]interface{})
@@ -207,6 +217,9 @@ func objectReflectDiff(path *field.Path, a, b reflect.Value) []diff {
 		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:

pkg/util/diff/diff_test.go

@@ -21,7 +21,28 @@ import (
 )
 
 func TestObjectReflectDiff(t *testing.T) {
-	expect := `
+	type struct1 struct{ A []int }
+
+	testCases := map[string]struct {
+		a, b interface{}
+		out  string
+	}{
+		"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: <nil>
@@ -30,10 +51,38 @@ object[test]:
   b: 2
 object[third]:
   a: <nil>
-  b: 3`
-	a := map[string]int{"test": 1, "other": 2}
-	b := map[string]int{"test": 2, "third": 3}
-	if actual := ObjectReflectDiff(a, b); actual != expect {
-		t.Errorf("unexpected output: %s", actual)
+  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: <nil>
+  b: 2`,
+		},
+		"detect slice changes 3": {a: struct1{A: []int{1}}, b: struct1{A: []int{}}, out: `
+object.A[0]:
+  a: 1
+  b: <nil>`,
+		},
+		"detect nil vs empty slices": {a: struct1{A: nil}, b: struct1{A: []int{}}, out: `
+object.A:
+  a: []int(nil)
+  b: []int{}`,
+		},
+	}
+	for name, test := range testCases {
+		expect := test.out
+		if len(expect) == 0 {
+			expect = "<no diffs>"
+		}
+		if actual := ObjectReflectDiff(test.a, test.b); actual != expect {
+			t.Errorf("%s: unexpected output: %s", name, actual)
+		}
 	}
 }