From a8d721b72a14d49cfe6189c20a10c04b086dd2d5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jiri=20Dan=C4=9Bk?= Date: Tue, 17 Dec 2024 08:21:18 +0100 Subject: [PATCH 1/2] RHOAIENG-17634: ref(odh-nbc/tests): create Gomega custom matcher for comparing CRs in kubeflow notebooks tests This is the initial refactor to use a new custom matcher. The matcher has no new features compared to the old way. I will do the meaningful-diffs part as a next PR (under the same Jira number). --- .../controllers/matchers_test.go | 52 +++++++++++++++++++ .../controllers/notebook_controller_test.go | 47 +++++++---------- 2 files changed, 71 insertions(+), 28 deletions(-) create mode 100644 components/odh-notebook-controller/controllers/matchers_test.go diff --git a/components/odh-notebook-controller/controllers/matchers_test.go b/components/odh-notebook-controller/controllers/matchers_test.go new file mode 100644 index 00000000000..1d44f2b689f --- /dev/null +++ b/components/odh-notebook-controller/controllers/matchers_test.go @@ -0,0 +1,52 @@ +package controllers + +import ( + "fmt" + "github.com/google/go-cmp/cmp" + "github.com/onsi/gomega/types" +) + +// See https://onsi.github.io/gomega/#adding-your-own-matchers + +// BeMatchingK8sResource is a custom Gomega matcher that compares using `comparator` function and reports differences using +// [cmp.Diff]. It attempts to minimize the diff (TODO(jdanek): not yet implemented) to only include those entries that cause `comparator` to fail. +// +// Use this to replace assertions such as +// > Expect(CompareNotebookRoutes(*route, expectedRoute)).Should(BeTrueBecause(cmp.Diff(*route, expectedRoute))) +// with +// > Expect(*route).To(BeMatchingK8sResource(expectedRoute, CompareNotebookRoutes)) +// +// NOTE: The diff minimization functionality (TODO(jdanek): not yet implemented) is best-effort. It is designed to never under-approximate, but over-approximation is possible. +// NOTE2: Using [gcustom.MakeMatcher] is not possible because it does not conveniently allow running [cmp.Diff] at the time of failure message generation. +func BeMatchingK8sResource[T any](expected T, comparator func(T, T) bool) types.GomegaMatcher { + return &beMatchingK8sResource[T]{ + expected: expected, + comparator: comparator, + } +} + +type beMatchingK8sResource[T any] struct { + expected T + comparator func(r1 T, r2 T) bool +} + +var _ types.GomegaMatcher = &beMatchingK8sResource[any]{} + +func (m *beMatchingK8sResource[T]) Match(actual interface{}) (success bool, err error) { + actualT, ok := actual.(T) + if !ok { + return false, fmt.Errorf("BeMatchingK8sResource matcher expects two objects of the same type") + } + + return m.comparator(m.expected, actualT), nil +} + +func (m *beMatchingK8sResource[T]) FailureMessage(actual interface{}) (message string) { + diff := cmp.Diff(actual, m.expected) + return fmt.Sprintf("Expected\n\t%#v\nto compare identical to\n\t%#v\nbut it differs in\n%s", actual, m.expected, diff) +} + +func (m *beMatchingK8sResource[T]) NegatedFailureMessage(actual interface{}) (message string) { + diff := cmp.Diff(actual, m.expected) + return fmt.Sprintf("Expected\n\t%#v\nto not compare identical to\n\t%#v\nit differs in\n%s", actual, m.expected, diff) +} diff --git a/components/odh-notebook-controller/controllers/notebook_controller_test.go b/components/odh-notebook-controller/controllers/notebook_controller_test.go index 2908b7fae9b..10ece7619f6 100644 --- a/components/odh-notebook-controller/controllers/notebook_controller_test.go +++ b/components/odh-notebook-controller/controllers/notebook_controller_test.go @@ -24,7 +24,6 @@ import ( "time" "github.com/go-logr/logr" - "github.com/google/go-cmp/cmp" . "github.com/onsi/ginkgo" . "github.com/onsi/gomega" @@ -100,7 +99,7 @@ var _ = Describe("The Openshift Notebook controller", func() { key := types.NamespacedName{Name: Name, Namespace: Namespace} return cli.Get(ctx, key, route) }, duration, interval).Should(Succeed()) - Expect(CompareNotebookRoutes(*route, expectedRoute)).Should(BeTrueBecause(cmp.Diff(*route, expectedRoute))) + Expect(*route).To(BeMatchingK8sResource(expectedRoute, CompareNotebookRoutes)) }) It("Should reconcile the Route when modified", func() { @@ -118,7 +117,7 @@ var _ = Describe("The Openshift Notebook controller", func() { } return route.Spec.To.Name, nil }, duration, interval).Should(Equal(Name)) - Expect(CompareNotebookRoutes(*route, expectedRoute)).Should(BeTrueBecause(cmp.Diff(*route, expectedRoute))) + Expect(*route).To(BeMatchingK8sResource(expectedRoute, CompareNotebookRoutes)) }) It("Should recreate the Route when deleted", func() { @@ -131,7 +130,7 @@ var _ = Describe("The Openshift Notebook controller", func() { key := types.NamespacedName{Name: Name, Namespace: Namespace} return cli.Get(ctx, key, route) }, duration, interval).Should(Succeed()) - Expect(CompareNotebookRoutes(*route, expectedRoute)).Should(BeTrueBecause(cmp.Diff(*route, expectedRoute))) + Expect(*route).To(BeMatchingK8sResource(expectedRoute, CompareNotebookRoutes)) }) It("Should delete the Openshift Route", func() { @@ -490,15 +489,14 @@ var _ = Describe("The Openshift Notebook controller", func() { key := types.NamespacedName{Name: Name + "-ctrl-np", Namespace: Namespace} return cli.Get(ctx, key, notebookNetworkPolicy) }, duration, interval).Should(Succeed()) - Expect(CompareNotebookNetworkPolicies(*notebookNetworkPolicy, expectedNotebookNetworkPolicy)).Should(BeTrueBecause(cmp.Diff(*notebookNetworkPolicy, expectedNotebookNetworkPolicy))) + Expect(*notebookNetworkPolicy).To(BeMatchingK8sResource(expectedNotebookNetworkPolicy, CompareNotebookNetworkPolicies)) By("By checking that the controller has created Network policy to allow all requests on OAuth port") Eventually(func() error { key := types.NamespacedName{Name: Name + "-oauth-np", Namespace: Namespace} return cli.Get(ctx, key, notebookOAuthNetworkPolicy) }, duration, interval).Should(Succeed()) - Expect(CompareNotebookNetworkPolicies(*notebookOAuthNetworkPolicy, expectedNotebookOAuthNetworkPolicy)). - To(BeTrueBecause(cmp.Diff(*notebookOAuthNetworkPolicy, expectedNotebookOAuthNetworkPolicy))) + Expect(*notebookOAuthNetworkPolicy).To(BeMatchingK8sResource(expectedNotebookOAuthNetworkPolicy, CompareNotebookNetworkPolicies)) }) It("Should reconcile the Network policies when modified", func() { @@ -516,8 +514,7 @@ var _ = Describe("The Openshift Notebook controller", func() { } return string(notebookNetworkPolicy.Spec.PolicyTypes[0]), nil }, duration, interval).Should(Equal("Ingress")) - Expect(CompareNotebookNetworkPolicies(*notebookNetworkPolicy, expectedNotebookNetworkPolicy)).Should( - BeTrueBecause(cmp.Diff(*notebookNetworkPolicy, expectedNotebookNetworkPolicy))) + Expect(*notebookNetworkPolicy).To(BeMatchingK8sResource(expectedNotebookNetworkPolicy, CompareNotebookNetworkPolicies)) }) It("Should recreate the Network Policy when deleted", func() { @@ -530,8 +527,7 @@ var _ = Describe("The Openshift Notebook controller", func() { key := types.NamespacedName{Name: Name + "-oauth-np", Namespace: Namespace} return cli.Get(ctx, key, notebookOAuthNetworkPolicy) }, duration, interval).Should(Succeed()) - Expect(CompareNotebookNetworkPolicies(*notebookOAuthNetworkPolicy, expectedNotebookOAuthNetworkPolicy)).Should( - BeTrueBecause(cmp.Diff(*notebookOAuthNetworkPolicy, expectedNotebookOAuthNetworkPolicy))) + Expect(*notebookOAuthNetworkPolicy).To(BeMatchingK8sResource(expectedNotebookOAuthNetworkPolicy, CompareNotebookNetworkPolicies)) }) It("Should delete the Network Policies", func() { @@ -664,20 +660,17 @@ var _ = Describe("The Openshift Notebook controller", func() { time.Sleep(interval) By("By checking that the webhook has injected the sidecar container") - Expect(CompareNotebooks(*notebook, expectedNotebook)).Should(BeTrueBecause(cmp.Diff(*notebook, expectedNotebook))) + Expect(*notebook).To(BeMatchingK8sResource(expectedNotebook, CompareNotebooks)) }) It("Should remove the reconciliation lock annotation", func() { By("By checking that the annotation lock annotation is not present") delete(expectedNotebook.Annotations, culler.STOP_ANNOTATION) - Eventually(func() bool { + Eventually(func() (nbv1.Notebook, error) { key := types.NamespacedName{Name: Name, Namespace: Namespace} err := cli.Get(ctx, key, notebook) - if err != nil { - return false - } - return CompareNotebooks(*notebook, expectedNotebook) - }, duration, interval).Should(BeTrueBecause(cmp.Diff(*notebook, expectedNotebook))) + return *notebook, err + }, duration, interval).Should(BeMatchingK8sResource(expectedNotebook, CompareNotebooks)) }) It("Should reconcile the Notebook when modified", func() { @@ -693,7 +686,7 @@ var _ = Describe("The Openshift Notebook controller", func() { key := types.NamespacedName{Name: Name, Namespace: Namespace} return cli.Get(ctx, key, notebook) }, duration, interval).Should(Succeed()) - Expect(CompareNotebooks(*notebook, expectedNotebook)).Should(BeTrueBecause(cmp.Diff(*notebook, expectedNotebook))) + Expect(*notebook).To(BeMatchingK8sResource(expectedNotebook, CompareNotebooks)) }) serviceAccount := &corev1.ServiceAccount{} @@ -705,8 +698,7 @@ var _ = Describe("The Openshift Notebook controller", func() { key := types.NamespacedName{Name: Name, Namespace: Namespace} return cli.Get(ctx, key, serviceAccount) }, duration, interval).Should(Succeed()) - Expect(CompareNotebookServiceAccounts(*serviceAccount, expectedServiceAccount)).Should( - BeTrueBecause(cmp.Diff(*serviceAccount, expectedServiceAccount))) + Expect(*serviceAccount).To(BeMatchingK8sResource(expectedServiceAccount, CompareNotebookServiceAccounts)) }) It("Should recreate the Service Account when deleted", func() { @@ -719,8 +711,7 @@ var _ = Describe("The Openshift Notebook controller", func() { key := types.NamespacedName{Name: Name, Namespace: Namespace} return cli.Get(ctx, key, serviceAccount) }, duration, interval).Should(Succeed()) - Expect(CompareNotebookServiceAccounts(*serviceAccount, expectedServiceAccount)).Should( - BeTrueBecause(cmp.Diff(*serviceAccount, expectedServiceAccount))) + Expect(*serviceAccount).To(BeMatchingK8sResource(expectedServiceAccount, CompareNotebookServiceAccounts)) }) service := &corev1.Service{} @@ -751,7 +742,7 @@ var _ = Describe("The Openshift Notebook controller", func() { key := types.NamespacedName{Name: Name + "-tls", Namespace: Namespace} return cli.Get(ctx, key, service) }, duration, interval).Should(Succeed()) - Expect(CompareNotebookServices(*service, expectedService)).Should(BeTrueBecause(cmp.Diff(*service, expectedService))) + Expect(*service).To(BeMatchingK8sResource(expectedService, CompareNotebookServices)) }) It("Should recreate the Service when deleted", func() { @@ -764,7 +755,7 @@ var _ = Describe("The Openshift Notebook controller", func() { key := types.NamespacedName{Name: Name + "-tls", Namespace: Namespace} return cli.Get(ctx, key, service) }, duration, interval).Should(Succeed()) - Expect(CompareNotebookServices(*service, expectedService)).Should(BeTrueBecause(cmp.Diff(*service, expectedService))) + Expect(*service).To(BeMatchingK8sResource(expectedService, CompareNotebookServices)) }) secret := &corev1.Secret{} @@ -827,7 +818,7 @@ var _ = Describe("The Openshift Notebook controller", func() { key := types.NamespacedName{Name: Name, Namespace: Namespace} return cli.Get(ctx, key, route) }, duration, interval).Should(Succeed()) - Expect(CompareNotebookRoutes(*route, expectedRoute)).Should(BeTrueBecause(cmp.Diff(*route, expectedRoute))) + Expect(*route).To(BeMatchingK8sResource(expectedRoute, CompareNotebookRoutes)) }) It("Should recreate the Route when deleted", func() { @@ -840,7 +831,7 @@ var _ = Describe("The Openshift Notebook controller", func() { key := types.NamespacedName{Name: Name, Namespace: Namespace} return cli.Get(ctx, key, route) }, duration, interval).Should(Succeed()) - Expect(CompareNotebookRoutes(*route, expectedRoute)).Should(BeTrueBecause(cmp.Diff(*route, expectedRoute))) + Expect(*route).To(BeMatchingK8sResource(expectedRoute, CompareNotebookRoutes)) }) It("Should reconcile the Route when modified", func() { @@ -858,7 +849,7 @@ var _ = Describe("The Openshift Notebook controller", func() { } return route.Spec.To.Name, nil }, duration, interval).Should(Equal(Name + "-tls")) - Expect(CompareNotebookRoutes(*route, expectedRoute)).Should(BeTrueBecause(cmp.Diff(*route, expectedRoute))) + Expect(*route).To(BeMatchingK8sResource(expectedRoute, CompareNotebookRoutes)) }) It("Should delete the OAuth proxy objects", func() { From 6c9317c84aaa5cf52170b8ac197773d2c39e2889 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jiri=20Dan=C4=9Bk?= Date: Fri, 10 Jan 2025 16:20:45 +0100 Subject: [PATCH 2/2] RHOAIENG-17634: chore(odh-nbc/tests): implement diff minimization for k8s objects diffs This is a followup to the refactoring before. The minimal diffs are computed by taking the `actual` value, making a deep copy of it, applying the differences between `actual` and `expected` to the deep copy, and calling the comparison function repeatedly. --- .../controllers/matchers_test.go | 143 ++++++++++++++++-- 1 file changed, 132 insertions(+), 11 deletions(-) diff --git a/components/odh-notebook-controller/controllers/matchers_test.go b/components/odh-notebook-controller/controllers/matchers_test.go index 1d44f2b689f..5626fc37ed9 100644 --- a/components/odh-notebook-controller/controllers/matchers_test.go +++ b/components/odh-notebook-controller/controllers/matchers_test.go @@ -2,37 +2,50 @@ package controllers import ( "fmt" + "log" + "reflect" + "testing" + "github.com/google/go-cmp/cmp" "github.com/onsi/gomega/types" + + routev1 "github.com/openshift/api/route/v1" + "k8s.io/apimachinery/pkg/runtime" ) // See https://onsi.github.io/gomega/#adding-your-own-matchers // BeMatchingK8sResource is a custom Gomega matcher that compares using `comparator` function and reports differences using -// [cmp.Diff]. It attempts to minimize the diff (TODO(jdanek): not yet implemented) to only include those entries that cause `comparator` to fail. +// [cmp.Diff]. It attempts to minimize the diff to only include those entries that cause `comparator` to fail. // // Use this to replace assertions such as // > Expect(CompareNotebookRoutes(*route, expectedRoute)).Should(BeTrueBecause(cmp.Diff(*route, expectedRoute))) // with // > Expect(*route).To(BeMatchingK8sResource(expectedRoute, CompareNotebookRoutes)) // -// NOTE: The diff minimization functionality (TODO(jdanek): not yet implemented) is best-effort. It is designed to never under-approximate, but over-approximation is possible. +// NOTE: The diff minimization functionality is best-effort. It is designed to never under-approximate, but over-approximation is possible. // NOTE2: Using [gcustom.MakeMatcher] is not possible because it does not conveniently allow running [cmp.Diff] at the time of failure message generation. -func BeMatchingK8sResource[T any](expected T, comparator func(T, T) bool) types.GomegaMatcher { - return &beMatchingK8sResource[T]{ +func BeMatchingK8sResource[T any, PT interface { + deepCopyer[T] + *T +}](expected T, comparator func(T, T) bool) types.GomegaMatcher { + return &beMatchingK8sResource[T, PT]{ expected: expected, comparator: comparator, } } -type beMatchingK8sResource[T any] struct { +type beMatchingK8sResource[T any, PT interface { + *T + deepCopyer[T] +}] struct { expected T comparator func(r1 T, r2 T) bool } -var _ types.GomegaMatcher = &beMatchingK8sResource[any]{} +var _ types.GomegaMatcher = &beMatchingK8sResource[routev1.Route, *routev1.Route]{} -func (m *beMatchingK8sResource[T]) Match(actual interface{}) (success bool, err error) { +func (m *beMatchingK8sResource[T, PT]) Match(actual interface{}) (success bool, err error) { actualT, ok := actual.(T) if !ok { return false, fmt.Errorf("BeMatchingK8sResource matcher expects two objects of the same type") @@ -41,12 +54,120 @@ func (m *beMatchingK8sResource[T]) Match(actual interface{}) (success bool, err return m.comparator(m.expected, actualT), nil } -func (m *beMatchingK8sResource[T]) FailureMessage(actual interface{}) (message string) { - diff := cmp.Diff(actual, m.expected) +func (m *beMatchingK8sResource[T, PT]) FailureMessage(actual interface{}) (message string) { + //m.expected + diff := m.computeMinimizedDiff(actual.(T)) return fmt.Sprintf("Expected\n\t%#v\nto compare identical to\n\t%#v\nbut it differs in\n%s", actual, m.expected, diff) } -func (m *beMatchingK8sResource[T]) NegatedFailureMessage(actual interface{}) (message string) { - diff := cmp.Diff(actual, m.expected) +func (m *beMatchingK8sResource[T, PT]) NegatedFailureMessage(actual interface{}) (message string) { + diff := m.computeMinimizedDiff(actual.(T)) return fmt.Sprintf("Expected\n\t%#v\nto not compare identical to\n\t%#v\nit differs in\n%s", actual, m.expected, diff) } + +// diffReporter is the basis of a custom [cmp.Reporter] that records differences detected during comparison. +// See example_reporter_test.go in the go-cmp package where this comes from. +type diffReporter struct { + path cmp.Path + + // handler that allows us to work with the currently stored [path] + // called whenever we are visiting leaf values that are unequal + OnDiff func(path cmp.Path) +} + +func (r *diffReporter) PushStep(ps cmp.PathStep) { + // NOTE (from docs): The PathStep itself is only valid until the step is popped. + r.path = append(r.path, ps) +} +func (r *diffReporter) Report(rs cmp.Result) { + if rs.Equal() { + return + } + fmt.Println("Report", r.path, "Endreport") + r.OnDiff(r.path) +} +func (r *diffReporter) PopStep() { + r.path = r.path[:len(r.path)-1] +} + +// deepCopyer reflects the deep copy methods that Kubernetes types all have. +// This is used to create deep copies of the compared structs for the purposes of +// isolating what specific differences cause them to not be equal. +type deepCopyer[T any] interface { + DeepCopyInto(out *T) + DeepCopy() *T + DeepCopyObject() runtime.Object +} + +func (m *beMatchingK8sResource[T, PT]) computeMinimizedDiff(actual T) (message string) { + debug := false + + accumulator := PT(&actual).DeepCopy() + reporter := diffReporter{OnDiff: func(diffPath cmp.Path) { + fmt.Println(diffPath) + clone := PT(&actual).DeepCopy() + + // reflect.ValueOf(&v).Elem() is used to get a modifiable value + current := reflect.ValueOf(clone).Elem() + pointer := reflect.ValueOf(accumulator).Elem() + + // the root of the path is operation-less, skip path[0] + for i := 1; i < len(diffPath); i++ { + switch step := diffPath[i].(type) { + case cmp.StructField: + current = current.Field(step.Index()) + pointer = pointer.Field(step.Index()) + case cmp.SliceIndex: + current = current.Index(step.Key()) + pointer = pointer.Index(step.Key()) + case cmp.MapIndex: + current = current.MapIndex(step.Key()) + pointer = pointer.MapIndex(step.Key()) + case cmp.Indirect: + current = reflect.Indirect(current) + pointer = reflect.Indirect(pointer) + case cmp.TypeAssertion: + // this is imo a noop, we don't care about checking type assertions + case cmp.Transform: + panic("unhandled situation: we can't go deeper and have to end the traversal here") + default: + panic("unreachable") + } + } + vx, vy := diffPath.Last().Values() + current.Set(vx) + + if debug { + log.Printf("%#v:\n\t-: %+v\n\t+: %+v\n", diffPath, vx, vy) + } + + // Check that our comparator function cares about this field + if !m.comparator(actual, *clone) { + // If yes, store that in the accumulator object + pointer.Set(vx) + } + }} + cmp.Equal(m.expected, actual, cmp.Reporter(&reporter)) + + return cmp.Diff(actual, *accumulator) +} + +// TODO: make it assert something +// todo: don't use actual production cmp funcs +func TestDiffingRoute(t *testing.T) { + someRoute := routev1.Route{ + Spec: routev1.RouteSpec{ + Host: "", + Path: "", + }, + } + someOtherRoute := routev1.Route{ + Spec: routev1.RouteSpec{ + Host: "xxxx", + Path: "yy", + }, + } + cmp := BeMatchingK8sResource(someRoute, CompareNotebookRoutes) + cmp.Match(someOtherRoute) + cmp.FailureMessage(someOtherRoute) +}