From 97bab44cae3857710bfb072d233e654784082ee3 Mon Sep 17 00:00:00 2001 From: Tim Hockin Date: Thu, 23 Mar 2023 11:10:32 -0700 Subject: [PATCH] Replace apimachinery diff.StringDiff with cmp I forced failures of most of these tests and verified readability --- pkg/api/testing/copy_test.go | 5 ++-- .../rbac/bootstrappolicy/policy_test.go | 4 +-- .../openapi/builder/builder_test.go | 15 +++------- .../k8s.io/apimachinery/pkg/util/diff/diff.go | 26 ++++------------- .../apimachinery/pkg/util/diff/diff_test.go | 29 ------------------- .../apiserver/pkg/endpoints/apiserver_test.go | 6 ---- .../k8s.io/kubectl/pkg/cmd/edit/edit_test.go | 6 ++-- .../k8s.io/kubectl/pkg/cmd/get/get_test.go | 4 +-- .../kubectl/pkg/explain/formatter_test.go | 4 +-- .../test/fixtures_test.go | 4 +-- 10 files changed, 23 insertions(+), 80 deletions(-) delete mode 100644 staging/src/k8s.io/apimachinery/pkg/util/diff/diff_test.go diff --git a/pkg/api/testing/copy_test.go b/pkg/api/testing/copy_test.go index 1169714e977..8f3acbda0e9 100644 --- a/pkg/api/testing/copy_test.go +++ b/pkg/api/testing/copy_test.go @@ -23,13 +23,12 @@ import ( "testing" "github.com/google/go-cmp/cmp" - "github.com/google/gofuzz" + fuzz "github.com/google/gofuzz" "k8s.io/apimachinery/pkg/api/apitesting/fuzzer" "k8s.io/apimachinery/pkg/api/apitesting/roundtrip" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/runtime/schema" - "k8s.io/apimachinery/pkg/util/diff" "k8s.io/kubernetes/pkg/api/legacyscheme" ) @@ -71,7 +70,7 @@ func doDeepCopyTest(t *testing.T, kind schema.GroupVersionKind, f *fuzz.Fuzzer) } if !bytes.Equal(prefuzzData.Bytes(), postfuzzData.Bytes()) { - t.Log(diff.StringDiff(prefuzzData.String(), postfuzzData.String())) + t.Log(cmp.Diff(prefuzzData.String(), postfuzzData.String())) t.Errorf("Fuzzing copy modified original of %#v", kind) return } diff --git a/plugin/pkg/auth/authorizer/rbac/bootstrappolicy/policy_test.go b/plugin/pkg/auth/authorizer/rbac/bootstrappolicy/policy_test.go index 836756e3c17..235b9f32e4e 100644 --- a/plugin/pkg/auth/authorizer/rbac/bootstrappolicy/policy_test.go +++ b/plugin/pkg/auth/authorizer/rbac/bootstrappolicy/policy_test.go @@ -22,13 +22,13 @@ import ( "reflect" "testing" + "github.com/google/go-cmp/cmp" "sigs.k8s.io/yaml" v1 "k8s.io/api/core/v1" rbacv1 "k8s.io/api/rbac/v1" "k8s.io/apimachinery/pkg/api/meta" "k8s.io/apimachinery/pkg/runtime" - "k8s.io/apimachinery/pkg/util/diff" "k8s.io/apimachinery/pkg/util/sets" "k8s.io/component-helpers/auth/rbac/validation" "k8s.io/kubernetes/pkg/api/legacyscheme" @@ -253,7 +253,7 @@ func testObjects(t *testing.T, list *api.List, fixtureFilename string) { t.Logf("Could not update data in %s: %v", filename, err) } } else { - t.Logf("Diff between bootstrap data and fixture data in %s:\n-------------\n%s", filename, diff.StringDiff(string(yamlData), string(expectedYAML))) + t.Logf("Diff between bootstrap data and fixture data in %s:\n-------------\n%s", filename, cmp.Diff(string(yamlData), string(expectedYAML))) t.Logf("If the change is expected, re-run with %s=true to update the fixtures", updateEnvVar) } } diff --git a/staging/src/k8s.io/apiextensions-apiserver/pkg/controller/openapi/builder/builder_test.go b/staging/src/k8s.io/apiextensions-apiserver/pkg/controller/openapi/builder/builder_test.go index ead09e2fadb..497df29c38d 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/pkg/controller/openapi/builder/builder_test.go +++ b/staging/src/k8s.io/apiextensions-apiserver/pkg/controller/openapi/builder/builder_test.go @@ -21,13 +21,13 @@ import ( "strings" "testing" + "github.com/google/go-cmp/cmp" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" apiextensionsinternal "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions" apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" structuralschema "k8s.io/apiextensions-apiserver/pkg/apiserver/schema" - "k8s.io/apimachinery/pkg/util/diff" "k8s.io/apimachinery/pkg/util/json" "k8s.io/apimachinery/pkg/util/sets" "k8s.io/apiserver/pkg/endpoints" @@ -438,7 +438,7 @@ func TestNewBuilder(t *testing.T) { gotListSchema := got.listSchema.Properties["items"].Items.Schema if !reflect.DeepEqual(&wantedItemsSchema, gotListSchema) { - t.Errorf("unexpected list schema: %s (want/got)", schemaDiff(&wantedItemsSchema, gotListSchema)) + t.Errorf("unexpected list schema:\n%s", schemaDiff(&wantedItemsSchema, gotListSchema)) } }) } @@ -560,15 +560,8 @@ func properties(p map[string]spec.Schema) sets.String { } func schemaDiff(a, b *spec.Schema) string { - as, err := json.Marshal(a) - if err != nil { - panic(err) - } - bs, err := json.Marshal(b) - if err != nil { - panic(err) - } - return diff.StringDiff(string(as), string(bs)) + // This option construct allows diffing all fields, even unexported ones. + return cmp.Diff(a, b, cmp.Exporter(func(reflect.Type) bool { return true })) } func TestBuildOpenAPIV2(t *testing.T) { 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 85066aea8e9..fc030184490 100644 --- a/staging/src/k8s.io/apimachinery/pkg/util/diff/diff.go +++ b/staging/src/k8s.io/apimachinery/pkg/util/diff/diff.go @@ -27,30 +27,16 @@ import ( "k8s.io/apimachinery/pkg/util/dump" ) -// StringDiff diffs a and b and returns a human readable diff. -func StringDiff(a, b string) string { - ba := []byte(a) - bb := []byte(b) - out := []byte{} - i := 0 - for ; i < len(ba) && i < len(bb); i++ { - if ba[i] != bb[i] { - break - } - out = append(out, ba[i]) - } - out = append(out, []byte("\n\nA: ")...) - out = append(out, ba[i:]...) - out = append(out, []byte("\n\nB: ")...) - out = append(out, bb[i:]...) - out = append(out, []byte("\n\n")...) - return string(out) -} - func legacyDiff(a, b interface{}) string { return cmp.Diff(a, b) } +// StringDiff diffs a and b and returns a human readable diff. +// DEPRECATED: use github.com/google/go-cmp/cmp.Diff +func StringDiff(a, b string) string { + return legacyDiff(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 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 deleted file mode 100644 index eb61a11d779..00000000000 --- a/staging/src/k8s.io/apimachinery/pkg/util/diff/diff_test.go +++ /dev/null @@ -1,29 +0,0 @@ -/* -Copyright 2016 The Kubernetes Authors. - -Licensed under the Apache License, Version 2.0 (the "License"); -you may not use this file except in compliance with the License. -You may obtain a copy of the License at - - http://www.apache.org/licenses/LICENSE-2.0 - -Unless required by applicable law or agreed to in writing, software -distributed under the License is distributed on an "AS IS" BASIS, -WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -See the License for the specific language governing permissions and -limitations under the License. -*/ - -package diff - -import ( - "testing" -) - -func TestStringDiff(t *testing.T) { - diff := StringDiff("aaabb", "aaacc") - expect := "aaa\n\nA: bb\n\nB: cc\n\n" - if diff != expect { - t.Errorf("diff returned %v", diff) - } -} diff --git a/staging/src/k8s.io/apiserver/pkg/endpoints/apiserver_test.go b/staging/src/k8s.io/apiserver/pkg/endpoints/apiserver_test.go index 11d8123754b..e08e6c2e7ba 100644 --- a/staging/src/k8s.io/apiserver/pkg/endpoints/apiserver_test.go +++ b/staging/src/k8s.io/apiserver/pkg/endpoints/apiserver_test.go @@ -2061,12 +2061,6 @@ func TestWatchTable(t *testing.T) { actual = append(actual, &event) } if !reflect.DeepEqual(test.expected, actual) { - for i := range test.expected { - if i >= len(actual) { - break - } - t.Logf("%s", diff.StringDiff(string(test.expected[i].Object.Raw), string(actual[i].Object.Raw))) - } t.Fatalf("unexpected: %s", diff.ObjectReflectDiff(test.expected, actual)) } }) diff --git a/staging/src/k8s.io/kubectl/pkg/cmd/edit/edit_test.go b/staging/src/k8s.io/kubectl/pkg/cmd/edit/edit_test.go index 42acf702407..c4746f4ebd6 100644 --- a/staging/src/k8s.io/kubectl/pkg/cmd/edit/edit_test.go +++ b/staging/src/k8s.io/kubectl/pkg/cmd/edit/edit_test.go @@ -27,12 +27,12 @@ import ( "strings" "testing" + "github.com/google/go-cmp/cmp" "github.com/spf13/cobra" yaml "gopkg.in/yaml.v2" "k8s.io/apimachinery/pkg/runtime/schema" - "k8s.io/apimachinery/pkg/util/diff" "k8s.io/apimachinery/pkg/util/sets" "k8s.io/cli-runtime/pkg/genericiooptions" "k8s.io/cli-runtime/pkg/resource" @@ -124,7 +124,7 @@ func TestEdit(t *testing.T) { // Convenience to allow recapturing the input and persisting it here os.WriteFile(inputFile, body, os.FileMode(0644)) } else { - t.Errorf("%s, step %d: diff in edit content:\n%s", name, i, diff.StringDiff(string(body), string(expectedInput))) + t.Errorf("%s, step %d: diff in edit content:\n%s", name, i, cmp.Diff(string(body), string(expectedInput))) t.Logf("If the change in input is expected, rerun tests with %s=true to update input fixtures", updateEnvVar) } } @@ -147,7 +147,7 @@ func TestEdit(t *testing.T) { // Convenience to allow recapturing the input and persisting it here os.WriteFile(inputFile, body, os.FileMode(0644)) } else { - t.Errorf("%s, step %d: diff in edit content:\n%s", name, i, diff.StringDiff(string(body), string(expectedInput))) + t.Errorf("%s, step %d: diff in edit content:\n%s", name, i, cmp.Diff(string(body), string(expectedInput))) t.Logf("If the change in input is expected, rerun tests with %s=true to update input fixtures", updateEnvVar) } } diff --git a/staging/src/k8s.io/kubectl/pkg/cmd/get/get_test.go b/staging/src/k8s.io/kubectl/pkg/cmd/get/get_test.go index c73bd15a3e0..0776b6740be 100644 --- a/staging/src/k8s.io/kubectl/pkg/cmd/get/get_test.go +++ b/staging/src/k8s.io/kubectl/pkg/cmd/get/get_test.go @@ -26,6 +26,7 @@ import ( "strings" "testing" + "github.com/google/go-cmp/cmp" appsv1 "k8s.io/api/apps/v1" autoscalingv1 "k8s.io/api/autoscaling/v1" batchv1 "k8s.io/api/batch/v1" @@ -36,7 +37,6 @@ import ( metav1beta1 "k8s.io/apimachinery/pkg/apis/meta/v1beta1" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/runtime/serializer/streaming" - "k8s.io/apimachinery/pkg/util/diff" "k8s.io/apimachinery/pkg/watch" "k8s.io/cli-runtime/pkg/genericiooptions" "k8s.io/cli-runtime/pkg/resource" @@ -1497,7 +1497,7 @@ func TestGetMultipleTypeObjectsAsList(t *testing.T) { } ` if e, a := expected, buf.String(); e != a { - t.Errorf("did not match: %v", diff.StringDiff(e, a)) + t.Errorf("did not match:\n%v", cmp.Diff(e, a)) } } diff --git a/staging/src/k8s.io/kubectl/pkg/explain/formatter_test.go b/staging/src/k8s.io/kubectl/pkg/explain/formatter_test.go index 41db9f55e60..1cf68a090af 100644 --- a/staging/src/k8s.io/kubectl/pkg/explain/formatter_test.go +++ b/staging/src/k8s.io/kubectl/pkg/explain/formatter_test.go @@ -20,7 +20,7 @@ import ( "bytes" "testing" - "k8s.io/apimachinery/pkg/util/diff" + "github.com/google/go-cmp/cmp" ) func TestFormatterWrite(t *testing.T) { @@ -113,7 +113,7 @@ fringilla velit. ` if buf.String() != want { - t.Errorf("Diff:\n%s", diff.StringDiff(buf.String(), want)) + t.Errorf("Diff:\n%s", cmp.Diff(buf.String(), want)) } } diff --git a/staging/src/k8s.io/pod-security-admission/test/fixtures_test.go b/staging/src/k8s.io/pod-security-admission/test/fixtures_test.go index 481436baec3..bd386b61183 100644 --- a/staging/src/k8s.io/pod-security-admission/test/fixtures_test.go +++ b/staging/src/k8s.io/pod-security-admission/test/fixtures_test.go @@ -24,9 +24,9 @@ import ( "strings" "testing" + "github.com/google/go-cmp/cmp" corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/runtime" - "k8s.io/apimachinery/pkg/util/diff" "k8s.io/apimachinery/pkg/util/sets" "k8s.io/client-go/kubernetes/scheme" "k8s.io/pod-security-admission/api" @@ -167,7 +167,7 @@ func testFixtureFile(t *testing.T, dir, name string, pod *corev1.Pod) string { t.Logf("Could not update data in %s: %v", filename, err) } } else { - t.Logf("Diff between generated data and fixture data in %s:\n-------------\n%s", filename, diff.StringDiff(string(yamlData), string(expectedYAML))) + t.Logf("Diff between generated data and fixture data in %s:\n-------------\n%s", filename, cmp.Diff(string(yamlData), string(expectedYAML))) t.Logf("If the change is expected, re-run with %s=true to update the fixtures", updateEnvVar) } }