Skip to content

Commit

Permalink
Merge pull request #502 from jiridanek/jiridanek-patch-2
Browse files Browse the repository at this point in the history
RHOAIENG-17634: ref(odh-nbc/tests): create Gomega custom matcher for comparing CRs in kubeflow notebooks tests
  • Loading branch information
openshift-merge-bot[bot] authored Jan 22, 2025
2 parents 9152d6b + a8d721b commit 9f7342b
Show file tree
Hide file tree
Showing 2 changed files with 71 additions and 28 deletions.
52 changes: 52 additions & 0 deletions components/odh-notebook-controller/controllers/matchers_test.go
Original file line number Diff line number Diff line change
@@ -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)
}
Original file line number Diff line number Diff line change
Expand Up @@ -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"

Expand Down Expand Up @@ -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() {
Expand All @@ -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() {
Expand All @@ -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() {
Expand Down Expand Up @@ -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() {
Expand All @@ -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() {
Expand All @@ -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() {
Expand Down Expand Up @@ -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() {
Expand All @@ -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{}
Expand All @@ -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() {
Expand All @@ -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{}
Expand Down Expand Up @@ -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() {
Expand All @@ -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{}
Expand Down Expand Up @@ -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() {
Expand All @@ -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() {
Expand All @@ -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() {
Expand Down

0 comments on commit 9f7342b

Please sign in to comment.