From d10a665c8b476227d997721eb3e0a03e3d6d91f1 Mon Sep 17 00:00:00 2001 From: Nahshon Unna-Tsameret Date: Wed, 10 Jan 2024 08:46:59 +0200 Subject: [PATCH 1/3] Enable ginkgolinter and fix findings ginkgolinter finds bugs and enforces standards of using the ginkgo and gomega packages. See more details here: https://github.com/nunnatsa/ginkgolinter This PR enables the ginkgolinter in the .golangci.yml, and fixes all the new finding from running golangci-lint. Note: all the finding were auto fixed by running the ginkgolinter cli with the `-fix` flag. Signed-off-by: Nahshon Unna-Tsameret --- .golangci.yml | 1 + handler/enqueue_annotation_test.go | 18 +-- handler/instrumented_enqueue_object_test.go | 22 +-- internal/utils/utils_test.go | 4 +- leader/leader_test.go | 52 +++---- proxy/proxy_test.go | 8 +- prune/prune_test.go | 156 ++++++++++---------- 7 files changed, 131 insertions(+), 130 deletions(-) diff --git a/.golangci.yml b/.golangci.yml index cda2f0c..749e483 100644 --- a/.golangci.yml +++ b/.golangci.yml @@ -7,6 +7,7 @@ linters: - asciicheck - bodyclose - errorlint + - ginkgolinter - gofmt - goimports - gosec diff --git a/handler/enqueue_annotation_test.go b/handler/enqueue_annotation_test.go index ba32bda..f36658a 100644 --- a/handler/enqueue_annotation_test.go +++ b/handler/enqueue_annotation_test.go @@ -57,7 +57,7 @@ var _ = Describe("EnqueueRequestForAnnotation", func() { podOwner.SetGroupVersionKind(schema.GroupVersionKind{Group: "", Kind: "Pod"}) err := SetOwnerAnnotations(podOwner, pod) - Expect(err).To(BeNil()) + Expect(err).ToNot(HaveOccurred()) instance = EnqueueRequestForAnnotation{ Type: schema.GroupKind{ Group: "", @@ -93,7 +93,7 @@ var _ = Describe("EnqueueRequestForAnnotation", func() { } err := SetOwnerAnnotations(podOwner, repl) - Expect(err).To(BeNil()) + Expect(err).ToNot(HaveOccurred()) evt := event.CreateEvent{ Object: repl, @@ -249,7 +249,7 @@ var _ = Describe("EnqueueRequestForAnnotation", func() { newPod.Namespace = pod.Namespace + "2" err := SetOwnerAnnotations(podOwner, pod) - Expect(err).To(BeNil()) + Expect(err).ToNot(HaveOccurred()) evt := event.UpdateEvent{ ObjectOld: pod, @@ -338,7 +338,7 @@ var _ = Describe("EnqueueRequestForAnnotation", func() { newPod.Namespace = pod.Namespace + "2" err := SetOwnerAnnotations(podOwner, pod) - Expect(err).To(BeNil()) + Expect(err).ToNot(HaveOccurred()) var podOwner2 = &corev1.Pod{ ObjectMeta: metav1.ObjectMeta{ @@ -349,7 +349,7 @@ var _ = Describe("EnqueueRequestForAnnotation", func() { podOwner2.SetGroupVersionKind(schema.GroupVersionKind{Group: "", Kind: "Pod"}) err = SetOwnerAnnotations(podOwner2, newPod) - Expect(err).To(BeNil()) + Expect(err).ToNot(HaveOccurred()) evt := event.UpdateEvent{ ObjectOld: pod, @@ -390,7 +390,7 @@ var _ = Describe("EnqueueRequestForAnnotation", func() { } err := SetOwnerAnnotations(podOwner, nd) - Expect(err).To(BeNil()) + Expect(err).ToNot(HaveOccurred()) expected := map[string]string{ "my-test-annotation": "should-keep", @@ -398,7 +398,7 @@ var _ = Describe("EnqueueRequestForAnnotation", func() { TypeAnnotation: schema.GroupKind{Group: "", Kind: "Pod"}.String(), } - Expect(len(nd.GetAnnotations())).To(Equal(3)) + Expect(nd.GetAnnotations()).To(HaveLen(3)) Expect(nd.GetAnnotations()).To(Equal(expected)) }) It("should return error when the owner Kind is not present", func() { @@ -410,7 +410,7 @@ var _ = Describe("EnqueueRequestForAnnotation", func() { podOwner.SetGroupVersionKind(schema.GroupVersionKind{Group: "Pod", Kind: ""}) err := SetOwnerAnnotations(podOwner, nd) - Expect(err).NotTo(BeNil()) + Expect(err).To(HaveOccurred()) }) It("should return an error when the owner Name is not set", func() { nd := &corev1.Node{ @@ -427,7 +427,7 @@ var _ = Describe("EnqueueRequestForAnnotation", func() { ownerNew.SetGroupVersionKind(schema.GroupVersionKind{Group: "Pod", Kind: ""}) err := SetOwnerAnnotations(ownerNew, nd) - Expect(err).NotTo(BeNil()) + Expect(err).To(HaveOccurred()) }) }) }) diff --git a/handler/instrumented_enqueue_object_test.go b/handler/instrumented_enqueue_object_test.go index 60911f4..dd69ebb 100644 --- a/handler/instrumented_enqueue_object_test.go +++ b/handler/instrumented_enqueue_object_test.go @@ -79,7 +79,7 @@ var _ = Describe("InstrumentedEnqueueRequestForObject", func() { // verify metrics gauges, err := registry.Gather() Expect(err).NotTo(HaveOccurred()) - Expect(len(gauges)).To(Equal(1)) + Expect(gauges).To(HaveLen(1)) assertMetrics(gauges[0], 1, []*corev1.Pod{pod}) }) }) @@ -114,7 +114,7 @@ var _ = Describe("InstrumentedEnqueueRequestForObject", func() { // verify metrics gauges, err := registry.Gather() Expect(err).NotTo(HaveOccurred()) - Expect(len(gauges)).To(Equal(0)) + Expect(gauges).To(BeEmpty()) }) }) Context("when a gauge does not exist", func() { @@ -139,7 +139,7 @@ var _ = Describe("InstrumentedEnqueueRequestForObject", func() { // verify metrics gauges, err := registry.Gather() Expect(err).NotTo(HaveOccurred()) - Expect(len(gauges)).To(Equal(0)) + Expect(gauges).To(BeEmpty()) }) }) @@ -174,7 +174,7 @@ var _ = Describe("InstrumentedEnqueueRequestForObject", func() { // verify metrics gauges, err := registry.Gather() Expect(err).NotTo(HaveOccurred()) - Expect(len(gauges)).To(Equal(1)) + Expect(gauges).To(HaveLen(1)) assertMetrics(gauges[0], 2, []*corev1.Pod{newpod, pod}) }) }) @@ -183,7 +183,7 @@ var _ = Describe("InstrumentedEnqueueRequestForObject", func() { It("should fill out map with values from given objects", func() { labelMap := getResourceLabels(pod) Expect(labelMap).ShouldNot(BeEmpty()) - Expect(len(labelMap)).To(Equal(5)) + Expect(labelMap).To(HaveLen(5)) Expect(labelMap["name"]).To(Equal(pod.GetObjectMeta().GetName())) Expect(labelMap["namespace"]).To(Equal(pod.GetObjectMeta().GetNamespace())) Expect(labelMap["group"]).To(Equal(pod.GetObjectKind().GroupVersionKind().Group)) @@ -201,22 +201,22 @@ func assertMetrics(gauge *dto.MetricFamily, count int, pods []*corev1.Pod) { v := "version" k := "kind" - Expect(len(gauge.Metric)).To(Equal(count)) + Expect(gauge.Metric).To(HaveLen(count)) for i := 0; i < count; i++ { Expect(*gauge.Metric[i].Gauge.Value).To(Equal(float64(pods[i].GetObjectMeta().GetCreationTimestamp().UTC().Unix()))) for _, l := range gauge.Metric[i].Label { switch l.Name { case &name: - Expect(l.Value).To(Equal(pods[i].GetObjectMeta().GetName())) + Expect(l.Value).To(HaveValue(Equal(pods[i].GetObjectMeta().GetName()))) case &namespace: - Expect(l.Value).To(Equal(pods[i].GetObjectMeta().GetNamespace())) + Expect(l.Value).To(HaveValue(Equal(pods[i].GetObjectMeta().GetNamespace()))) case &g: - Expect(l.Value).To(Equal(pods[i].GetObjectKind().GroupVersionKind().Group)) + Expect(l.Value).To(HaveValue(Equal(pods[i].GetObjectKind().GroupVersionKind().Group))) case &v: - Expect(l.Value).To(Equal(pods[i].GetObjectKind().GroupVersionKind().Version)) + Expect(l.Value).To(HaveValue(Equal(pods[i].GetObjectKind().GroupVersionKind().Version))) case &k: - Expect(l.Value).To(Equal(pods[i].GetObjectKind().GroupVersionKind().Kind)) + Expect(l.Value).To(HaveValue(Equal(pods[i].GetObjectKind().GroupVersionKind().Kind))) } } } diff --git a/internal/utils/utils_test.go b/internal/utils/utils_test.go index 1c7e11c..b3bb7a1 100644 --- a/internal/utils/utils_test.go +++ b/internal/utils/utils_test.go @@ -38,7 +38,7 @@ var _ = Describe("Helpers test", func() { // test namespace, err := GetOperatorNamespace() - Expect(err).Should(BeNil()) + Expect(err).ShouldNot(HaveOccurred()) Expect(namespace).To(Equal("testnamespace")) }) It("should trim whitespace from namespace", func() { @@ -48,7 +48,7 @@ var _ = Describe("Helpers test", func() { // test namespace, err := GetOperatorNamespace() - Expect(err).Should(BeNil()) + Expect(err).ShouldNot(HaveOccurred()) Expect(namespace).To(Equal("testnamespace")) }) }) diff --git a/leader/leader_test.go b/leader/leader_test.go index b1a1c2e..7d67a0a 100644 --- a/leader/leader_test.go +++ b/leader/leader_test.go @@ -66,7 +66,7 @@ var _ = Describe("Leader election", func() { It("should return an error when POD_NAME is not set", func() { os.Unsetenv("POD_NAME") err := Become(context.TODO(), "leader-test") - Expect(err).ShouldNot(BeNil()) + Expect(err).Should(HaveOccurred()) }) It("should return an ErrNoNamespace", func() { os.Setenv("POD_NAME", "leader-test") @@ -74,9 +74,9 @@ var _ = Describe("Leader election", func() { return "", ErrNoNamespace } err := Become(context.TODO(), "leader-test", WithClient(client)) - Expect(err).ShouldNot(BeNil()) + Expect(err).Should(HaveOccurred()) Expect(err).To(Equal(ErrNoNamespace)) - Expect(errors.Is(err, ErrNoNamespace)).To(Equal(true)) + Expect(errors.Is(err, ErrNoNamespace)).To(BeTrue()) }) It("should not return an error", func() { os.Setenv("POD_NAME", "leader-test") @@ -85,7 +85,7 @@ var _ = Describe("Leader election", func() { } err := Become(context.TODO(), "leader-test", WithClient(client)) - Expect(err).Should(BeNil()) + Expect(err).ShouldNot(HaveOccurred()) }) }) Describe("isPodEvicted", func() { @@ -96,21 +96,21 @@ var _ = Describe("Leader election", func() { leaderPod = &corev1.Pod{} }) It("should return false with an empty status", func() { - Expect(isPodEvicted(*leaderPod)).To(Equal(false)) + Expect(isPodEvicted(*leaderPod)).To(BeFalse()) }) It("should return false if reason is incorrect", func() { leaderPod.Status.Phase = corev1.PodFailed leaderPod.Status.Reason = "invalid" - Expect(isPodEvicted(*leaderPod)).To(Equal(false)) + Expect(isPodEvicted(*leaderPod)).To(BeFalse()) }) It("should return false if pod is in the wrong phase", func() { leaderPod.Status.Phase = corev1.PodRunning - Expect(isPodEvicted(*leaderPod)).To(Equal(false)) + Expect(isPodEvicted(*leaderPod)).To(BeFalse()) }) It("should return true when Phase and Reason are set", func() { leaderPod.Status.Phase = corev1.PodFailed leaderPod.Status.Reason = "Evicted" - Expect(isPodEvicted(*leaderPod)).To(Equal(true)) + Expect(isPodEvicted(*leaderPod)).To(BeTrue()) }) }) Describe("myOwnerRef", func() { @@ -130,17 +130,17 @@ var _ = Describe("Leader election", func() { It("should return an error when POD_NAME is not set", func() { os.Unsetenv("POD_NAME") _, err := myOwnerRef(context.TODO(), client, "") - Expect(err).ShouldNot(BeNil()) + Expect(err).Should(HaveOccurred()) }) It("should return an error if no pod is found", func() { os.Setenv("POD_NAME", "thisisnotthepodyourelookingfor") _, err := myOwnerRef(context.TODO(), client, "") - Expect(err).ShouldNot(BeNil()) + Expect(err).Should(HaveOccurred()) }) It("should return the owner reference without error", func() { os.Setenv("POD_NAME", "mypod") owner, err := myOwnerRef(context.TODO(), client, "testns") - Expect(err).Should(BeNil()) + Expect(err).ShouldNot(HaveOccurred()) Expect(owner.APIVersion).To(Equal("v1")) Expect(owner.Kind).To(Equal("Pod")) Expect(owner.Name).To(Equal("mypod")) @@ -163,17 +163,17 @@ var _ = Describe("Leader election", func() { It("should return an error when POD_NAME is not set", func() { os.Unsetenv("POD_NAME") _, err := getPod(context.TODO(), nil, "") - Expect(err).ShouldNot(BeNil()) + Expect(err).Should(HaveOccurred()) }) It("should return an error if no pod is found", func() { os.Setenv("POD_NAME", "thisisnotthepodyourelookingfor") _, err := getPod(context.TODO(), client, "") - Expect(err).ShouldNot(BeNil()) + Expect(err).Should(HaveOccurred()) }) It("should return the pod with the given name", func() { os.Setenv("POD_NAME", "mypod") pod, err := getPod(context.TODO(), client, "testns") - Expect(err).Should(BeNil()) + Expect(err).ShouldNot(HaveOccurred()) Expect(pod).ShouldNot(BeNil()) Expect(pod.TypeMeta.APIVersion).To(Equal("v1")) Expect(pod.TypeMeta.Kind).To(Equal("Pod")) @@ -196,12 +196,12 @@ var _ = Describe("Leader election", func() { It("should return an error if no node is found", func() { node := corev1.Node{} err := getNode(context.TODO(), client, "", &node) - Expect(err).ShouldNot(BeNil()) + Expect(err).Should(HaveOccurred()) }) It("should return the node with the given name", func() { node := corev1.Node{} err := getNode(context.TODO(), client, "mynode", &node) - Expect(err).Should(BeNil()) + Expect(err).ShouldNot(HaveOccurred()) Expect(node.TypeMeta.APIVersion).To(Equal("v1")) Expect(node.TypeMeta.Kind).To(Equal("Node")) }) @@ -228,33 +228,33 @@ var _ = Describe("Leader election", func() { It("should return false if node is invalid", func() { client = fake.NewClientBuilder().WithObjects().Build() ret := isNotReadyNode(context.TODO(), client, "") - Expect(ret).To(Equal(false)) + Expect(ret).To(BeFalse()) }) It("should return false if no NodeCondition is found", func() { client = fake.NewClientBuilder().WithObjects(node).Build() ret := isNotReadyNode(context.TODO(), client, nodeName) - Expect(ret).To(Equal(false)) + Expect(ret).To(BeFalse()) }) It("should return false if type is incorrect", func() { node.Status.Conditions[0].Type = corev1.NodeMemoryPressure node.Status.Conditions[0].Status = corev1.ConditionFalse client = fake.NewClientBuilder().WithObjects(node).Build() ret := isNotReadyNode(context.TODO(), client, nodeName) - Expect(ret).To(Equal(false)) + Expect(ret).To(BeFalse()) }) It("should return false if NodeReady's type is true", func() { node.Status.Conditions[0].Type = corev1.NodeReady node.Status.Conditions[0].Status = corev1.ConditionTrue client = fake.NewClientBuilder().WithObjects(node).Build() ret := isNotReadyNode(context.TODO(), client, nodeName) - Expect(ret).To(Equal(false)) + Expect(ret).To(BeFalse()) }) It("should return true when Type is set and Status is set to false", func() { node.Status.Conditions[0].Type = corev1.NodeReady node.Status.Conditions[0].Status = corev1.ConditionFalse client = fake.NewClientBuilder().WithObjects(node).Build() ret := isNotReadyNode(context.TODO(), client, nodeName) - Expect(ret).To(Equal(true)) + Expect(ret).To(BeTrue()) }) }) Describe("deleteLeader", func() { @@ -294,27 +294,27 @@ var _ = Describe("Leader election", func() { It("should return an error if existing is not found", func() { client = fake.NewClientBuilder().WithObjects(pod).Build() err := deleteLeader(context.TODO(), client, pod, configmap) - Expect(err).ShouldNot(BeNil()) + Expect(err).Should(HaveOccurred()) }) It("should return an error if pod is not found", func() { client = fake.NewClientBuilder().WithObjects(configmap).Build() err := deleteLeader(context.TODO(), client, pod, configmap) - Expect(err).ShouldNot(BeNil()) + Expect(err).Should(HaveOccurred()) }) It("should return an error if pod is nil", func() { client = fake.NewClientBuilder().WithObjects(pod, configmap).Build() err := deleteLeader(context.TODO(), client, nil, configmap) - Expect(err).ShouldNot(BeNil()) + Expect(err).Should(HaveOccurred()) }) It("should return an error if configmap is nil", func() { client = fake.NewClientBuilder().WithObjects(pod, configmap).Build() err := deleteLeader(context.TODO(), client, pod, nil) - Expect(err).ShouldNot(BeNil()) + Expect(err).Should(HaveOccurred()) }) It("should return nil if pod and configmap exists and configmap's owner is the pod", func() { client = fake.NewClientBuilder().WithObjects(pod, configmap).Build() err := deleteLeader(context.TODO(), client, pod, configmap) - Expect(err).Should(BeNil()) + Expect(err).ShouldNot(HaveOccurred()) }) }) diff --git a/proxy/proxy_test.go b/proxy/proxy_test.go index ba2b331..a680a11 100644 --- a/proxy/proxy_test.go +++ b/proxy/proxy_test.go @@ -40,11 +40,11 @@ var _ = Describe("Retrieving", func() { os.Setenv("HTTP_PROXY", "http_proxy_test") os.Setenv("NO_PROXY", "no_proxy_test") envVars := ReadProxyVarsFromEnv() - Expect(len(envVars)).To(Equal(6)) + Expect(envVars).To(HaveLen(6)) }) It("does not return unset variables", func() { envVars := ReadProxyVarsFromEnv() - Expect(len(envVars)).To(Equal(0)) + Expect(envVars).To(BeEmpty()) }) It("creates upper and lower case environment variables with the same value", func() { @@ -55,9 +55,9 @@ var _ = Describe("Retrieving", func() { for _, envName := range ProxyEnvNames { upperValue, err := checkValueFromEnvObj(envName, envVars) - Expect(err).To(BeNil()) + Expect(err).ToNot(HaveOccurred()) lowerValue, err := checkValueFromEnvObj(strings.ToLower(envName), envVars) - Expect(err).To(BeNil()) + Expect(err).ToNot(HaveOccurred()) Expect(upperValue).To(Equal(lowerValue)) } }) diff --git a/prune/prune_test.go b/prune/prune_test.go index 75e4893..30e9c38 100644 --- a/prune/prune_test.go +++ b/prune/prune_test.go @@ -49,7 +49,7 @@ var _ = Describe("Prune", func() { ) BeforeEach(func() { testScheme, err := createSchemes() - Expect(err).Should(BeNil()) + Expect(err).ShouldNot(HaveOccurred()) fakeClient = crFake.NewClientBuilder().WithScheme(testScheme).Build() fakeObj = &corev1.Pod{} @@ -97,7 +97,7 @@ var _ = Describe("Prune", func() { Kind: "NotReal", }) - Expect(NewRegistry().IsPrunable(obj)).Should(BeNil()) + Expect(NewRegistry().IsPrunable(obj)).Should(Succeed()) }) }) @@ -106,7 +106,7 @@ var _ = Describe("Prune", func() { Describe("NewPruner()", func() { It("Should Return a New Pruner Object", func() { pruner, err := NewPruner(fakeClient, podGVK, myStrategy) - Expect(err).Should(BeNil()) + Expect(err).ShouldNot(HaveOccurred()) Expect(pruner).ShouldNot(BeNil()) }) @@ -118,7 +118,7 @@ var _ = Describe("Prune", func() { myStrategy, WithNamespace(namespace), WithLabels(labels)) - Expect(err).Should(BeNil()) + Expect(err).ShouldNot(HaveOccurred()) Expect(pruner).ShouldNot(BeNil()) Expect(&pruner.registry).Should(Equal(DefaultRegistry())) Expect(pruner.namespace).Should(Equal(namespace)) @@ -131,7 +131,7 @@ var _ = Describe("Prune", func() { It("Should Error if schema.GroupVersionKind Parameter is empty", func() { // empty GVK struct pruner, err := NewPruner(fakeClient, schema.GroupVersionKind{}, myStrategy) - Expect(err).ShouldNot(BeNil()) + Expect(err).Should(HaveOccurred()) Expect(err.Error()).Should(Equal("error when creating a new Pruner: gvk parameter can not be empty")) Expect(pruner).Should(BeNil()) }) @@ -141,123 +141,123 @@ var _ = Describe("Prune", func() { Context("Does not return an Error", func() { testPruneWithDefaultIsPrunableFunc := func(gvk schema.GroupVersionKind) { pruner, err := NewPruner(fakeClient, gvk, myStrategy, WithLabels(appLabels), WithNamespace(namespace)) - Expect(err).Should(BeNil()) + Expect(err).ShouldNot(HaveOccurred()) Expect(pruner).ShouldNot(BeNil()) prunedObjects, err := pruner.Prune(context.Background()) - Expect(err).Should(BeNil()) - Expect(len(prunedObjects)).Should(Equal(2)) + Expect(err).ShouldNot(HaveOccurred()) + Expect(prunedObjects).Should(HaveLen(2)) } It("Should Prune Pods with Default IsPrunableFunc", func() { // Create the test resources - in this case Pods err := createTestPods(fakeClient) - Expect(err).Should(BeNil()) + Expect(err).ShouldNot(HaveOccurred()) // Make sure the pod resources are properly created pods := &unstructured.UnstructuredList{} pods.SetGroupVersionKind(podGVK) err = fakeClient.List(context.Background(), pods) - Expect(err).Should(BeNil()) - Expect(len(pods.Items)).Should(Equal(3)) + Expect(err).ShouldNot(HaveOccurred()) + Expect(pods.Items).Should(HaveLen(3)) testPruneWithDefaultIsPrunableFunc(podGVK) // Get a list of the Pods to make sure we have pruned the ones we expected err = fakeClient.List(context.Background(), pods) - Expect(err).Should(BeNil()) - Expect(len(pods.Items)).Should(Equal(1)) + Expect(err).ShouldNot(HaveOccurred()) + Expect(pods.Items).Should(HaveLen(1)) }) It("Should Prune Jobs with Default IsPrunableFunc", func() { // Create the test resources - in this case Jobs err := createTestJobs(fakeClient) - Expect(err).Should(BeNil()) + Expect(err).ShouldNot(HaveOccurred()) // Make sure the job resources are properly created jobs := &unstructured.UnstructuredList{} jobs.SetGroupVersionKind(jobGVK) err = fakeClient.List(context.Background(), jobs) - Expect(err).Should(BeNil()) - Expect(len(jobs.Items)).Should(Equal(3)) + Expect(err).ShouldNot(HaveOccurred()) + Expect(jobs.Items).Should(HaveLen(3)) testPruneWithDefaultIsPrunableFunc(jobGVK) // Get a list of the job to make sure we have pruned the ones we expected err = fakeClient.List(context.Background(), jobs) - Expect(err).Should(BeNil()) - Expect(len(jobs.Items)).Should(Equal(1)) + Expect(err).ShouldNot(HaveOccurred()) + Expect(jobs.Items).Should(HaveLen(1)) }) It("Should Remove Resource When Using a Custom IsPrunableFunc", func() { // Create the test resources - in this case Jobs err := createTestJobs(fakeClient) - Expect(err).Should(BeNil()) + Expect(err).ShouldNot(HaveOccurred()) // Make sure the job resources are properly created jobs := &unstructured.UnstructuredList{} jobs.SetGroupVersionKind(jobGVK) err = fakeClient.List(context.Background(), jobs) - Expect(err).Should(BeNil()) - Expect(len(jobs.Items)).Should(Equal(3)) + Expect(err).ShouldNot(HaveOccurred()) + Expect(jobs.Items).Should(HaveLen(3)) pruner, err := NewPruner(fakeClient, jobGVK, myStrategy, WithLabels(appLabels), WithNamespace(namespace)) - Expect(err).Should(BeNil()) + Expect(err).ShouldNot(HaveOccurred()) Expect(pruner).ShouldNot(BeNil()) // Register our custom IsPrunableFunc RegisterIsPrunableFunc(jobGVK, myIsPrunable) prunedObjects, err := pruner.Prune(context.Background()) - Expect(err).Should(BeNil()) - Expect(len(prunedObjects)).Should(Equal(2)) + Expect(err).ShouldNot(HaveOccurred()) + Expect(prunedObjects).Should(HaveLen(2)) // Get a list of the jobs to make sure we have pruned the ones we expected err = fakeClient.List(context.Background(), jobs) - Expect(err).Should(BeNil()) - Expect(len(jobs.Items)).Should(Equal(1)) + Expect(err).ShouldNot(HaveOccurred()) + Expect(jobs.Items).Should(HaveLen(1)) }) It("Should Not Prune Resources when using a DryRunClient", func() { // Create the test resources - in this case Pods err := createTestPods(fakeClient) - Expect(err).Should(BeNil()) + Expect(err).ShouldNot(HaveOccurred()) // Make sure the pod resources are properly created pods := &unstructured.UnstructuredList{} pods.SetGroupVersionKind(podGVK) err = fakeClient.List(context.Background(), pods) - Expect(err).Should(BeNil()) - Expect(len(pods.Items)).Should(Equal(3)) + Expect(err).ShouldNot(HaveOccurred()) + Expect(pods.Items).Should(HaveLen(3)) dryRunClient := client.NewDryRunClient(fakeClient) pruner, err := NewPruner(dryRunClient, podGVK, myStrategy, WithLabels(appLabels), WithNamespace(namespace)) - Expect(err).Should(BeNil()) + Expect(err).ShouldNot(HaveOccurred()) Expect(pruner).ShouldNot(BeNil()) prunedObjects, err := pruner.Prune(context.Background()) - Expect(err).Should(BeNil()) - Expect(len(prunedObjects)).Should(Equal(2)) + Expect(err).ShouldNot(HaveOccurred()) + Expect(prunedObjects).Should(HaveLen(2)) // Get a list of the Pods to make sure we haven't pruned any err = fakeClient.List(context.Background(), pods) - Expect(err).Should(BeNil()) - Expect(len(pods.Items)).Should(Equal(3)) + Expect(err).ShouldNot(HaveOccurred()) + Expect(pods.Items).Should(HaveLen(3)) }) It("Should Skip Pruning a Resource If IsPrunable Returns an Error of Type Unprunable", func() { // Create the test resources - in this case Jobs err := createTestJobs(fakeClient) - Expect(err).Should(BeNil()) + Expect(err).ShouldNot(HaveOccurred()) // Make sure the job resources are properly created jobs := &unstructured.UnstructuredList{} jobs.SetGroupVersionKind(jobGVK) err = fakeClient.List(context.Background(), jobs) - Expect(err).Should(BeNil()) - Expect(len(jobs.Items)).Should(Equal(3)) + Expect(err).ShouldNot(HaveOccurred()) + Expect(jobs.Items).Should(HaveLen(3)) pruner, err := NewPruner(fakeClient, jobGVK, myStrategy, WithLabels(appLabels), WithNamespace(namespace)) - Expect(err).Should(BeNil()) + Expect(err).ShouldNot(HaveOccurred()) Expect(pruner).ShouldNot(BeNil()) // IsPrunableFunc that throws Unprunable error @@ -272,13 +272,13 @@ var _ = Describe("Prune", func() { RegisterIsPrunableFunc(jobGVK, errorPrunableFunc) prunedObjects, err := pruner.Prune(context.Background()) - Expect(err).Should(BeNil()) - Expect(len(prunedObjects)).Should(Equal(0)) + Expect(err).ShouldNot(HaveOccurred()) + Expect(prunedObjects).Should(BeEmpty()) // Get a list of the jobs to make sure we have pruned the ones we expected err = fakeClient.List(context.Background(), jobs) - Expect(err).Should(BeNil()) - Expect(len(jobs.Items)).Should(Equal(3)) + Expect(err).ShouldNot(HaveOccurred()) + Expect(jobs.Items).Should(HaveLen(3)) }) }) @@ -286,17 +286,17 @@ var _ = Describe("Prune", func() { It("Should Return an Error if IsPrunableFunc Returns an Error That is not of Type Unprunable", func() { // Create the test resources - in this case Jobs err := createTestJobs(fakeClient) - Expect(err).Should(BeNil()) + Expect(err).ShouldNot(HaveOccurred()) // Make sure the job resources are properly created jobs := &unstructured.UnstructuredList{} jobs.SetGroupVersionKind(jobGVK) err = fakeClient.List(context.Background(), jobs) - Expect(err).Should(BeNil()) - Expect(len(jobs.Items)).Should(Equal(3)) + Expect(err).ShouldNot(HaveOccurred()) + Expect(jobs.Items).Should(HaveLen(3)) pruner, err := NewPruner(fakeClient, jobGVK, myStrategy, WithLabels(appLabels), WithNamespace(namespace)) - Expect(err).Should(BeNil()) + Expect(err).ShouldNot(HaveOccurred()) Expect(pruner).ShouldNot(BeNil()) // IsPrunableFunc that throws non Unprunable error @@ -308,27 +308,27 @@ var _ = Describe("Prune", func() { RegisterIsPrunableFunc(jobGVK, errorPrunableFunc) prunedObjects, err := pruner.Prune(context.Background()) - Expect(err).ShouldNot(BeNil()) + Expect(err).Should(HaveOccurred()) Expect(err.Error()).Should(Equal("TEST")) - Expect(len(prunedObjects)).Should(Equal(0)) + Expect(prunedObjects).Should(BeEmpty()) // Get a list of the jobs to make sure we have pruned the ones we expected err = fakeClient.List(context.Background(), jobs) - Expect(err).Should(BeNil()) - Expect(len(jobs.Items)).Should(Equal(3)) + Expect(err).ShouldNot(HaveOccurred()) + Expect(jobs.Items).Should(HaveLen(3)) }) It("Should Return An Error If Strategy Function Returns An Error", func() { // Create the test resources - in this case Jobs err := createTestJobs(fakeClient) - Expect(err).Should(BeNil()) + Expect(err).ShouldNot(HaveOccurred()) // Make sure the job resources are properly created jobs := &unstructured.UnstructuredList{} jobs.SetGroupVersionKind(jobGVK) err = fakeClient.List(context.Background(), jobs) - Expect(err).Should(BeNil()) - Expect(len(jobs.Items)).Should(Equal(3)) + Expect(err).ShouldNot(HaveOccurred()) + Expect(jobs.Items).Should(HaveLen(3)) // strategy that will return an error prunerStrategy := func(ctx context.Context, objs []client.Object) ([]client.Object, error) { @@ -336,37 +336,37 @@ var _ = Describe("Prune", func() { } pruner, err := NewPruner(fakeClient, jobGVK, prunerStrategy, WithLabels(appLabels), WithNamespace(namespace)) - Expect(err).Should(BeNil()) + Expect(err).ShouldNot(HaveOccurred()) Expect(pruner).ShouldNot(BeNil()) // Register our custom IsPrunableFunc RegisterIsPrunableFunc(jobGVK, myIsPrunable) prunedObjects, err := pruner.Prune(context.Background()) - Expect(err).ShouldNot(BeNil()) + Expect(err).Should(HaveOccurred()) Expect(err.Error()).Should(Equal("error determining prunable objects: TESTERROR")) Expect(prunedObjects).Should(BeNil()) // Get a list of the jobs to make sure we have pruned the ones we expected err = fakeClient.List(context.Background(), jobs) - Expect(err).Should(BeNil()) - Expect(len(jobs.Items)).Should(Equal(3)) + Expect(err).ShouldNot(HaveOccurred()) + Expect(jobs.Items).Should(HaveLen(3)) }) It("Should Return an Error if it can not Prune a Resource", func() { // Create the test resources - in this case Jobs err := createTestJobs(fakeClient) - Expect(err).Should(BeNil()) + Expect(err).ShouldNot(HaveOccurred()) // Make sure the job resources are properly created jobs := &unstructured.UnstructuredList{} jobs.SetGroupVersionKind(jobGVK) err = fakeClient.List(context.Background(), jobs) - Expect(err).Should(BeNil()) - Expect(len(jobs.Items)).Should(Equal(3)) + Expect(err).ShouldNot(HaveOccurred()) + Expect(jobs.Items).Should(HaveLen(3)) pruner, err := NewPruner(fakeClient, jobGVK, myStrategy, WithLabels(appLabels), WithNamespace(namespace)) - Expect(err).Should(BeNil()) + Expect(err).ShouldNot(HaveOccurred()) Expect(pruner).ShouldNot(BeNil()) // IsPrunableFunc that returns nil but also deletes the object @@ -380,14 +380,14 @@ var _ = Describe("Prune", func() { RegisterIsPrunableFunc(jobGVK, prunableFunc) prunedObjects, err := pruner.Prune(context.Background()) - Expect(err).ShouldNot(BeNil()) + Expect(err).Should(HaveOccurred()) Expect(err.Error()).Should(ContainSubstring("error pruning object: jobs.batch \"churro1\" not found")) - Expect(len(prunedObjects)).Should(Equal(0)) + Expect(prunedObjects).Should(BeEmpty()) // Get a list of the jobs to make sure we have pruned the ones we expected err = fakeClient.List(context.Background(), jobs) - Expect(err).Should(BeNil()) - Expect(len(jobs.Items)).Should(Equal(0)) + Expect(err).ShouldNot(HaveOccurred()) + Expect(jobs.Items).Should(BeEmpty()) }) }) @@ -396,7 +396,7 @@ var _ = Describe("Prune", func() { Describe("GVK()", func() { It("Should return the GVK field in the Pruner", func() { pruner, err := NewPruner(fakeClient, podGVK, myStrategy) - Expect(err).Should(BeNil()) + Expect(err).ShouldNot(HaveOccurred()) Expect(pruner).ShouldNot(BeNil()) Expect(pruner.GVK()).Should(Equal(podGVK)) }) @@ -405,7 +405,7 @@ var _ = Describe("Prune", func() { Describe("Labels()", func() { It("Should return the Labels field in the Pruner", func() { pruner, err := NewPruner(fakeClient, podGVK, myStrategy, WithLabels(appLabels)) - Expect(err).Should(BeNil()) + Expect(err).ShouldNot(HaveOccurred()) Expect(pruner).ShouldNot(BeNil()) Expect(pruner.Labels()).Should(Equal(appLabels)) }) @@ -414,7 +414,7 @@ var _ = Describe("Prune", func() { Describe("Namespace()", func() { It("Should return the Namespace field in the Pruner", func() { pruner, err := NewPruner(fakeClient, podGVK, myStrategy, WithNamespace(namespace)) - Expect(err).Should(BeNil()) + Expect(err).ShouldNot(HaveOccurred()) Expect(pruner).ShouldNot(BeNil()) Expect(pruner.Namespace()).Should(Equal(namespace)) }) @@ -438,7 +438,7 @@ var _ = Describe("Prune", func() { // Run it through DefaultPodIsPrunable err := DefaultPodIsPrunable(pod) - Expect(err).Should(BeNil()) + Expect(err).ShouldNot(HaveOccurred()) }) It("Should Panic When client.Object is not of type 'Pod'", func() { @@ -467,7 +467,7 @@ var _ = Describe("Prune", func() { // Run it through DefaultPodIsPrunable err := DefaultPodIsPrunable(pod) - Expect(err).ShouldNot(BeNil()) + Expect(err).Should(HaveOccurred()) var expectErr *Unprunable Expect(errors.As(err, &expectErr)).Should(BeTrue()) Expect(expectErr.Reason).Should(Equal("Pod has not succeeded")) @@ -493,7 +493,7 @@ var _ = Describe("Prune", func() { // Run it through DefaultJobIsPrunable err := DefaultJobIsPrunable(job) - Expect(err).Should(BeNil()) + Expect(err).ShouldNot(HaveOccurred()) }) It("Should Return An Error When Kind Is Not 'Job'", func() { @@ -522,7 +522,7 @@ var _ = Describe("Prune", func() { // Run it through DefaultJobIsPrunable err := DefaultJobIsPrunable(job) - Expect(err).ShouldNot(BeNil()) + Expect(err).Should(HaveOccurred()) var expectErr *Unprunable Expect(errors.As(err, &expectErr)).Should(BeTrue()) Expect(expectErr.Reason).Should(Equal("Job has not completed")) @@ -535,13 +535,13 @@ var _ = Describe("Prune", func() { resources := createDatedResources() It("Should return the 3 oldest resources", func() { resourcesToRemove, err := NewPruneByCountStrategy(2)(context.Background(), resources) - Expect(err).Should(BeNil()) + Expect(err).ShouldNot(HaveOccurred()) Expect(resourcesToRemove).Should(Equal(resources[2:])) }) It("Should return nil", func() { resourcesToRemove, err := NewPruneByCountStrategy(5)(context.Background(), resources) - Expect(err).Should(BeNil()) + Expect(err).ShouldNot(HaveOccurred()) Expect(resourcesToRemove).Should(BeNil()) }) }) @@ -551,15 +551,15 @@ var _ = Describe("Prune", func() { It("Should return 2 resources", func() { date := time.Now().Add(time.Hour * time.Duration(2)) resourcesToRemove, err := NewPruneByDateStrategy(date)(context.Background(), resources) - Expect(err).Should(BeNil()) - Expect(len(resourcesToRemove)).Should(Equal(2)) + Expect(err).ShouldNot(HaveOccurred()) + Expect(resourcesToRemove).Should(HaveLen(2)) }) It("Should return 0 resources", func() { date := time.Now().Add(time.Hour * time.Duration(24)) resourcesToRemove, err := NewPruneByDateStrategy(date)(context.Background(), resources) - Expect(err).Should(BeNil()) - Expect(len(resourcesToRemove)).Should(Equal(0)) + Expect(err).ShouldNot(HaveOccurred()) + Expect(resourcesToRemove).Should(BeEmpty()) }) }) From f93ce10b2c58a25799b558828f00457d46a536fe Mon Sep 17 00:00:00 2001 From: Nahshon Unna-Tsameret Date: Wed, 10 Jan 2024 09:11:35 +0200 Subject: [PATCH 2/3] Fix the assertMetrics test helper function The assertMetrics function in handler/instrumented_enqueue_object_test.go, wasn't really doing anything. The switch-case compared a pointer value to another pointer, so the no case was actually ever selected, because the addresses were not the same. Tested with a debugger - no value were ever selected. This commit fixes this test by selecting the value rather than the address. Signed-off-by: Nahshon Unna-Tsameret --- handler/instrumented_enqueue_object_test.go | 24 +++++++++++---------- 1 file changed, 13 insertions(+), 11 deletions(-) diff --git a/handler/instrumented_enqueue_object_test.go b/handler/instrumented_enqueue_object_test.go index dd69ebb..946854b 100644 --- a/handler/instrumented_enqueue_object_test.go +++ b/handler/instrumented_enqueue_object_test.go @@ -195,27 +195,29 @@ var _ = Describe("InstrumentedEnqueueRequestForObject", func() { func assertMetrics(gauge *dto.MetricFamily, count int, pods []*corev1.Pod) { // need variables to compare the pointers - name := "name" - namespace := "namespace" - g := "group" - v := "version" - k := "kind" + const ( + name = "name" + namespace = "namespace" + g = "group" + v = "version" + k = "kind" + ) Expect(gauge.Metric).To(HaveLen(count)) for i := 0; i < count; i++ { Expect(*gauge.Metric[i].Gauge.Value).To(Equal(float64(pods[i].GetObjectMeta().GetCreationTimestamp().UTC().Unix()))) for _, l := range gauge.Metric[i].Label { - switch l.Name { - case &name: + switch *l.Name { + case name: Expect(l.Value).To(HaveValue(Equal(pods[i].GetObjectMeta().GetName()))) - case &namespace: + case namespace: Expect(l.Value).To(HaveValue(Equal(pods[i].GetObjectMeta().GetNamespace()))) - case &g: + case g: Expect(l.Value).To(HaveValue(Equal(pods[i].GetObjectKind().GroupVersionKind().Group))) - case &v: + case v: Expect(l.Value).To(HaveValue(Equal(pods[i].GetObjectKind().GroupVersionKind().Version))) - case &k: + case k: Expect(l.Value).To(HaveValue(Equal(pods[i].GetObjectKind().GroupVersionKind().Kind))) } } From b85b9e76421a8f5101d64bd3fc28fdb9fa3264ce Mon Sep 17 00:00:00 2001 From: Nahshon Unna-Tsameret Date: Wed, 10 Jan 2024 10:31:14 +0200 Subject: [PATCH 3/3] Simplify error check in tests Replace this patter: ```golang err := someFuncRetOnlyErr() Expect(err).ToNot(HaveOccurred() ``` With ```golang Expect(someFuncRetOnlyErr()).To(Succeed()) ``` Also, use the `MatchError` gomega matcher when checking errors. Signed-off-by: Nahshon Unna-Tsameret --- conditions/conditions_test.go | 35 ++++------ conditions/factory_test.go | 30 +++------ handler/enqueue_annotation_test.go | 24 +++---- leader/leader_test.go | 32 +++------ prune/prune_test.go | 100 ++++++++++------------------- 5 files changed, 74 insertions(+), 147 deletions(-) diff --git a/conditions/conditions_test.go b/conditions/conditions_test.go index a9001f9..581b626 100644 --- a/conditions/conditions_test.go +++ b/conditions/conditions_test.go @@ -64,25 +64,21 @@ var _ = Describe("Condition", func() { } // Create Operator Condition - err := os.Setenv(operatorCondEnvVar, "operator-condition-test") - Expect(err).NotTo(HaveOccurred()) + Expect(os.Setenv(operatorCondEnvVar, "operator-condition-test")).To(Succeed()) readNamespace = func() (string, error) { return ns, nil } // create a new client sch := runtime.NewScheme() - err = apiv2.AddToScheme(sch) - Expect(err).NotTo(HaveOccurred()) + Expect(apiv2.AddToScheme(sch)).To(Succeed()) cl = fake.NewClientBuilder().WithScheme(sch).WithStatusSubresource(operatorCond).Build() // create an operator Condition resource - err = cl.Create(ctx, operatorCond) - Expect(err).NotTo(HaveOccurred()) + Expect(cl.Create(ctx, operatorCond)).To(Succeed()) // Update its status - err = cl.Status().Update(ctx, operatorCond) - Expect(err).NotTo(HaveOccurred()) + Expect(cl.Status().Update(ctx, operatorCond)).To(Succeed()) }) AfterEach(func() { @@ -107,14 +103,12 @@ var _ = Describe("Condition", func() { Expect(err).NotTo(HaveOccurred()) con, err := c.Get(ctx) - Expect(err).To(HaveOccurred()) + Expect(err).To(MatchError(ContainSubstring(fmt.Sprintf("conditionType %v not found", conditionBar)))) Expect(con).To(BeNil()) - Expect(err.Error()).To(ContainSubstring(fmt.Sprintf("conditionType %v not found", conditionBar))) }) It("should error when operator Condition is not present in cluster", func() { - err := os.Setenv(operatorCondEnvVar, "NON_EXISTING_COND") - Expect(err).NotTo(HaveOccurred()) + Expect(os.Setenv(operatorCondEnvVar, "NON_EXISTING_COND")).To(Succeed()) By("setting the status of a new condition") c, err := NewCondition(cl, conditionFoo) @@ -131,13 +125,11 @@ var _ = Describe("Condition", func() { By("setting the status of an existing condition") c, err := NewCondition(cl, conditionFoo) Expect(err).NotTo(HaveOccurred()) - err = c.Set(ctx, metav1.ConditionFalse, WithReason("not_in_foo_state"), WithMessage("test")) - Expect(err).NotTo(HaveOccurred()) + Expect(c.Set(ctx, metav1.ConditionFalse, WithReason("not_in_foo_state"), WithMessage("test"))).To(Succeed()) By("fetching the condition from cluster") op := &apiv2.OperatorCondition{} - err = cl.Get(ctx, objKey, op) - Expect(err).NotTo(HaveOccurred()) + Expect(cl.Get(ctx, objKey, op)).To(Succeed()) By("checking if the condition has been updated") res := op.Spec.Conditions[0] @@ -150,22 +142,19 @@ var _ = Describe("Condition", func() { By("setting the status of a new condition") c, err := NewCondition(cl, conditionBar) Expect(err).NotTo(HaveOccurred()) - err = c.Set(ctx, metav1.ConditionTrue, WithReason("in_bar_state"), WithMessage("test")) - Expect(err).NotTo(HaveOccurred()) + Expect(c.Set(ctx, metav1.ConditionTrue, WithReason("in_bar_state"), WithMessage("test"))).To(Succeed()) By("fetching the condition from cluster") op := &apiv2.OperatorCondition{} - err = cl.Get(ctx, objKey, op) - Expect(err).NotTo(HaveOccurred()) + Expect(cl.Get(ctx, objKey, op)).To(Succeed()) By("checking if the condition has been updated") res := op.Spec.Conditions - Expect(len(res)).To(BeEquivalentTo(2)) + Expect(res).To(HaveLen(2)) Expect(meta.IsStatusConditionTrue(res, string(conditionBar))).To(BeTrue()) }) It("should error when operator Condition is not present in cluster", func() { - err := os.Setenv(operatorCondEnvVar, "NON_EXISTING_COND") - Expect(err).NotTo(HaveOccurred()) + Expect(os.Setenv(operatorCondEnvVar, "NON_EXISTING_COND")).To(Succeed()) By("setting the status of a new condition") c, err := NewCondition(cl, conditionBar) diff --git a/conditions/factory_test.go b/conditions/factory_test.go index fdb339f..5973a3a 100644 --- a/conditions/factory_test.go +++ b/conditions/factory_test.go @@ -36,8 +36,7 @@ var _ = Describe("NewCondition", func() { var cl client.Client BeforeEach(func() { sch := runtime.NewScheme() - err := apiv2.AddToScheme(sch) - Expect(err).NotTo(HaveOccurred()) + Expect(apiv2.AddToScheme(sch)).To(Succeed()) cl = fake.NewClientBuilder().WithScheme(sch).Build() }) @@ -56,8 +55,7 @@ var _ = Describe("InClusterFactory", func() { BeforeEach(func() { sch := runtime.NewScheme() - err := apiv2.AddToScheme(sch) - Expect(err).NotTo(HaveOccurred()) + Expect(apiv2.AddToScheme(sch)).To(Succeed()) cl = fake.NewClientBuilder().WithScheme(sch).Build() f = InClusterFactory{cl} }) @@ -73,8 +71,7 @@ var _ = Describe("InClusterFactory", func() { func testNewCondition(fn func(apiv2.ConditionType) (Condition, error)) { It("should create a new condition", func() { - err := os.Setenv(operatorCondEnvVar, "test-operator-condition") - Expect(err).NotTo(HaveOccurred()) + Expect(os.Setenv(operatorCondEnvVar, "test-operator-condition")).To(Succeed()) readNamespace = func() (string, error) { return "default", nil } @@ -85,8 +82,7 @@ func testNewCondition(fn func(apiv2.ConditionType) (Condition, error)) { }) It("should error when namespacedName cannot be found", func() { - err := os.Unsetenv(operatorCondEnvVar) - Expect(err).NotTo(HaveOccurred()) + Expect(os.Unsetenv(operatorCondEnvVar)).To(Succeed()) c, err := fn(conditionFoo) Expect(err).To(HaveOccurred()) @@ -96,32 +92,27 @@ func testNewCondition(fn func(apiv2.ConditionType) (Condition, error)) { func testGetNamespacedName(fn func() (*types.NamespacedName, error)) { It("should error when name of the operator condition cannot be found", func() { - err := os.Unsetenv(operatorCondEnvVar) - Expect(err).NotTo(HaveOccurred()) + Expect(os.Unsetenv(operatorCondEnvVar)).To(Succeed()) objKey, err := fn() - Expect(err).To(HaveOccurred()) + Expect(err).To(MatchError(ContainSubstring("could not determine operator condition name"))) Expect(objKey).To(BeNil()) - Expect(err.Error()).To(ContainSubstring("could not determine operator condition name")) }) It("should error when object namespace cannot be found", func() { - err := os.Setenv(operatorCondEnvVar, "test") - Expect(err).NotTo(HaveOccurred()) + Expect(os.Setenv(operatorCondEnvVar, "test")).To(Succeed()) readNamespace = func() (string, error) { return "", os.ErrNotExist } objKey, err := fn() - Expect(err).To(HaveOccurred()) + Expect(err).To(MatchError(ContainSubstring("get operator condition namespace: file does not exist"))) Expect(objKey).To(BeNil()) - Expect(err.Error()).To(ContainSubstring("get operator condition namespace: file does not exist")) }) It("should return the right namespaced name from SA namespace file", func() { - err := os.Setenv(operatorCondEnvVar, "test") - Expect(err).NotTo(HaveOccurred()) + Expect(os.Setenv(operatorCondEnvVar, "test")).To(Succeed()) readNamespace = func() (string, error) { return "testns", nil @@ -135,6 +126,5 @@ func testGetNamespacedName(fn func() (*types.NamespacedName, error)) { } func deleteCondition(ctx context.Context, client client.Client, obj client.Object) { - err := client.Delete(ctx, obj) - Expect(err).NotTo(HaveOccurred()) + Expect(client.Delete(ctx, obj)).To(Succeed()) } diff --git a/handler/enqueue_annotation_test.go b/handler/enqueue_annotation_test.go index f36658a..8835aa9 100644 --- a/handler/enqueue_annotation_test.go +++ b/handler/enqueue_annotation_test.go @@ -56,8 +56,7 @@ var _ = Describe("EnqueueRequestForAnnotation", func() { podOwner.SetGroupVersionKind(schema.GroupVersionKind{Group: "", Kind: "Pod"}) - err := SetOwnerAnnotations(podOwner, pod) - Expect(err).ToNot(HaveOccurred()) + Expect(SetOwnerAnnotations(podOwner, pod)).To(Succeed()) instance = EnqueueRequestForAnnotation{ Type: schema.GroupKind{ Group: "", @@ -92,8 +91,7 @@ var _ = Describe("EnqueueRequestForAnnotation", func() { }, } - err := SetOwnerAnnotations(podOwner, repl) - Expect(err).ToNot(HaveOccurred()) + Expect(SetOwnerAnnotations(podOwner, repl)).To(Succeed()) evt := event.CreateEvent{ Object: repl, @@ -248,8 +246,7 @@ var _ = Describe("EnqueueRequestForAnnotation", func() { newPod.Name = pod.Name + "2" newPod.Namespace = pod.Namespace + "2" - err := SetOwnerAnnotations(podOwner, pod) - Expect(err).ToNot(HaveOccurred()) + Expect(SetOwnerAnnotations(podOwner, pod)).To(Succeed()) evt := event.UpdateEvent{ ObjectOld: pod, @@ -337,8 +334,7 @@ var _ = Describe("EnqueueRequestForAnnotation", func() { newPod.Name = pod.Name + "2" newPod.Namespace = pod.Namespace + "2" - err := SetOwnerAnnotations(podOwner, pod) - Expect(err).ToNot(HaveOccurred()) + Expect(SetOwnerAnnotations(podOwner, pod)).To(Succeed()) var podOwner2 = &corev1.Pod{ ObjectMeta: metav1.ObjectMeta{ @@ -348,8 +344,7 @@ var _ = Describe("EnqueueRequestForAnnotation", func() { } podOwner2.SetGroupVersionKind(schema.GroupVersionKind{Group: "", Kind: "Pod"}) - err = SetOwnerAnnotations(podOwner2, newPod) - Expect(err).ToNot(HaveOccurred()) + Expect(SetOwnerAnnotations(podOwner2, newPod)).To(Succeed()) evt := event.UpdateEvent{ ObjectOld: pod, @@ -389,8 +384,7 @@ var _ = Describe("EnqueueRequestForAnnotation", func() { }, } - err := SetOwnerAnnotations(podOwner, nd) - Expect(err).ToNot(HaveOccurred()) + Expect(SetOwnerAnnotations(podOwner, nd)).To(Succeed()) expected := map[string]string{ "my-test-annotation": "should-keep", @@ -409,8 +403,7 @@ var _ = Describe("EnqueueRequestForAnnotation", func() { } podOwner.SetGroupVersionKind(schema.GroupVersionKind{Group: "Pod", Kind: ""}) - err := SetOwnerAnnotations(podOwner, nd) - Expect(err).To(HaveOccurred()) + Expect(SetOwnerAnnotations(podOwner, nd)).ToNot(Succeed()) }) It("should return an error when the owner Name is not set", func() { nd := &corev1.Node{ @@ -426,8 +419,7 @@ var _ = Describe("EnqueueRequestForAnnotation", func() { } ownerNew.SetGroupVersionKind(schema.GroupVersionKind{Group: "Pod", Kind: ""}) - err := SetOwnerAnnotations(ownerNew, nd) - Expect(err).To(HaveOccurred()) + Expect(SetOwnerAnnotations(ownerNew, nd)).ToNot(Succeed()) }) }) }) diff --git a/leader/leader_test.go b/leader/leader_test.go index 7d67a0a..4798795 100644 --- a/leader/leader_test.go +++ b/leader/leader_test.go @@ -16,7 +16,6 @@ package leader import ( "context" - "errors" "os" . "github.com/onsi/ginkgo/v2" @@ -65,8 +64,7 @@ var _ = Describe("Leader election", func() { }) It("should return an error when POD_NAME is not set", func() { os.Unsetenv("POD_NAME") - err := Become(context.TODO(), "leader-test") - Expect(err).Should(HaveOccurred()) + Expect(Become(context.TODO(), "leader-test")).ShouldNot(Succeed()) }) It("should return an ErrNoNamespace", func() { os.Setenv("POD_NAME", "leader-test") @@ -74,9 +72,7 @@ var _ = Describe("Leader election", func() { return "", ErrNoNamespace } err := Become(context.TODO(), "leader-test", WithClient(client)) - Expect(err).Should(HaveOccurred()) - Expect(err).To(Equal(ErrNoNamespace)) - Expect(errors.Is(err, ErrNoNamespace)).To(BeTrue()) + Expect(err).Should(MatchError(ErrNoNamespace)) }) It("should not return an error", func() { os.Setenv("POD_NAME", "leader-test") @@ -84,8 +80,7 @@ var _ = Describe("Leader election", func() { return "testns", nil } - err := Become(context.TODO(), "leader-test", WithClient(client)) - Expect(err).ShouldNot(HaveOccurred()) + Expect(Become(context.TODO(), "leader-test", WithClient(client))).To(Succeed()) }) }) Describe("isPodEvicted", func() { @@ -195,13 +190,11 @@ var _ = Describe("Leader election", func() { }) It("should return an error if no node is found", func() { node := corev1.Node{} - err := getNode(context.TODO(), client, "", &node) - Expect(err).Should(HaveOccurred()) + Expect(getNode(context.TODO(), client, "", &node)).ToNot(Succeed()) }) It("should return the node with the given name", func() { node := corev1.Node{} - err := getNode(context.TODO(), client, "mynode", &node) - Expect(err).ShouldNot(HaveOccurred()) + Expect(getNode(context.TODO(), client, "mynode", &node)).Should(Succeed()) Expect(node.TypeMeta.APIVersion).To(Equal("v1")) Expect(node.TypeMeta.Kind).To(Equal("Node")) }) @@ -293,28 +286,23 @@ var _ = Describe("Leader election", func() { }) It("should return an error if existing is not found", func() { client = fake.NewClientBuilder().WithObjects(pod).Build() - err := deleteLeader(context.TODO(), client, pod, configmap) - Expect(err).Should(HaveOccurred()) + Expect(deleteLeader(context.TODO(), client, pod, configmap)).ToNot(Succeed()) }) It("should return an error if pod is not found", func() { client = fake.NewClientBuilder().WithObjects(configmap).Build() - err := deleteLeader(context.TODO(), client, pod, configmap) - Expect(err).Should(HaveOccurred()) + Expect(deleteLeader(context.TODO(), client, pod, configmap)).ToNot(Succeed()) }) It("should return an error if pod is nil", func() { client = fake.NewClientBuilder().WithObjects(pod, configmap).Build() - err := deleteLeader(context.TODO(), client, nil, configmap) - Expect(err).Should(HaveOccurred()) + Expect(deleteLeader(context.TODO(), client, nil, configmap)).ToNot(Succeed()) }) It("should return an error if configmap is nil", func() { client = fake.NewClientBuilder().WithObjects(pod, configmap).Build() - err := deleteLeader(context.TODO(), client, pod, nil) - Expect(err).Should(HaveOccurred()) + Expect(deleteLeader(context.TODO(), client, pod, nil)).ToNot(Succeed()) }) It("should return nil if pod and configmap exists and configmap's owner is the pod", func() { client = fake.NewClientBuilder().WithObjects(pod, configmap).Build() - err := deleteLeader(context.TODO(), client, pod, configmap) - Expect(err).ShouldNot(HaveOccurred()) + Expect(deleteLeader(context.TODO(), client, pod, configmap)).To(Succeed()) }) }) diff --git a/prune/prune_test.go b/prune/prune_test.go index 30e9c38..9922eec 100644 --- a/prune/prune_test.go +++ b/prune/prune_test.go @@ -61,11 +61,11 @@ var _ = Describe("Prune", func() { Describe("Unprunable", func() { Describe("Error()", func() { It("Should Return a String Representation of Unprunable", func() { - unpruneable := Unprunable{ + unpruneable := &Unprunable{ Obj: &fakeObj, Reason: "TestReason", } - Expect(unpruneable.Error()).To(Equal(fmt.Sprintf("unable to prune %s: %s", client.ObjectKeyFromObject(fakeObj), unpruneable.Reason))) + Expect(unpruneable).To(MatchError(fmt.Sprintf("unable to prune %s: %s", client.ObjectKeyFromObject(fakeObj), unpruneable.Reason))) }) }) }) @@ -131,8 +131,7 @@ var _ = Describe("Prune", func() { It("Should Error if schema.GroupVersionKind Parameter is empty", func() { // empty GVK struct pruner, err := NewPruner(fakeClient, schema.GroupVersionKind{}, myStrategy) - Expect(err).Should(HaveOccurred()) - Expect(err.Error()).Should(Equal("error when creating a new Pruner: gvk parameter can not be empty")) + Expect(err).Should(MatchError("error when creating a new Pruner: gvk parameter can not be empty")) Expect(pruner).Should(BeNil()) }) }) @@ -150,54 +149,46 @@ var _ = Describe("Prune", func() { } It("Should Prune Pods with Default IsPrunableFunc", func() { // Create the test resources - in this case Pods - err := createTestPods(fakeClient) - Expect(err).ShouldNot(HaveOccurred()) + Expect(createTestPods(fakeClient)).To(Succeed()) // Make sure the pod resources are properly created pods := &unstructured.UnstructuredList{} pods.SetGroupVersionKind(podGVK) - err = fakeClient.List(context.Background(), pods) - Expect(err).ShouldNot(HaveOccurred()) + Expect(fakeClient.List(context.Background(), pods)).To(Succeed()) Expect(pods.Items).Should(HaveLen(3)) testPruneWithDefaultIsPrunableFunc(podGVK) // Get a list of the Pods to make sure we have pruned the ones we expected - err = fakeClient.List(context.Background(), pods) - Expect(err).ShouldNot(HaveOccurred()) + Expect(fakeClient.List(context.Background(), pods)).To(Succeed()) Expect(pods.Items).Should(HaveLen(1)) }) It("Should Prune Jobs with Default IsPrunableFunc", func() { // Create the test resources - in this case Jobs - err := createTestJobs(fakeClient) - Expect(err).ShouldNot(HaveOccurred()) + Expect(createTestJobs(fakeClient)).To(Succeed()) // Make sure the job resources are properly created jobs := &unstructured.UnstructuredList{} jobs.SetGroupVersionKind(jobGVK) - err = fakeClient.List(context.Background(), jobs) - Expect(err).ShouldNot(HaveOccurred()) + Expect(fakeClient.List(context.Background(), jobs)).To(Succeed()) Expect(jobs.Items).Should(HaveLen(3)) testPruneWithDefaultIsPrunableFunc(jobGVK) // Get a list of the job to make sure we have pruned the ones we expected - err = fakeClient.List(context.Background(), jobs) - Expect(err).ShouldNot(HaveOccurred()) + Expect(fakeClient.List(context.Background(), jobs)).To(Succeed()) Expect(jobs.Items).Should(HaveLen(1)) }) It("Should Remove Resource When Using a Custom IsPrunableFunc", func() { // Create the test resources - in this case Jobs - err := createTestJobs(fakeClient) - Expect(err).ShouldNot(HaveOccurred()) + Expect(createTestJobs(fakeClient)).To(Succeed()) // Make sure the job resources are properly created jobs := &unstructured.UnstructuredList{} jobs.SetGroupVersionKind(jobGVK) - err = fakeClient.List(context.Background(), jobs) - Expect(err).ShouldNot(HaveOccurred()) + Expect(fakeClient.List(context.Background(), jobs)).To(Succeed()) Expect(jobs.Items).Should(HaveLen(3)) pruner, err := NewPruner(fakeClient, jobGVK, myStrategy, WithLabels(appLabels), WithNamespace(namespace)) @@ -212,21 +203,18 @@ var _ = Describe("Prune", func() { Expect(prunedObjects).Should(HaveLen(2)) // Get a list of the jobs to make sure we have pruned the ones we expected - err = fakeClient.List(context.Background(), jobs) - Expect(err).ShouldNot(HaveOccurred()) + Expect(fakeClient.List(context.Background(), jobs)).To(Succeed()) Expect(jobs.Items).Should(HaveLen(1)) }) It("Should Not Prune Resources when using a DryRunClient", func() { // Create the test resources - in this case Pods - err := createTestPods(fakeClient) - Expect(err).ShouldNot(HaveOccurred()) + Expect(createTestPods(fakeClient)).To(Succeed()) // Make sure the pod resources are properly created pods := &unstructured.UnstructuredList{} pods.SetGroupVersionKind(podGVK) - err = fakeClient.List(context.Background(), pods) - Expect(err).ShouldNot(HaveOccurred()) + Expect(fakeClient.List(context.Background(), pods)).To(Succeed()) Expect(pods.Items).Should(HaveLen(3)) dryRunClient := client.NewDryRunClient(fakeClient) @@ -239,21 +227,18 @@ var _ = Describe("Prune", func() { Expect(prunedObjects).Should(HaveLen(2)) // Get a list of the Pods to make sure we haven't pruned any - err = fakeClient.List(context.Background(), pods) - Expect(err).ShouldNot(HaveOccurred()) + Expect(fakeClient.List(context.Background(), pods)).To(Succeed()) Expect(pods.Items).Should(HaveLen(3)) }) It("Should Skip Pruning a Resource If IsPrunable Returns an Error of Type Unprunable", func() { // Create the test resources - in this case Jobs - err := createTestJobs(fakeClient) - Expect(err).ShouldNot(HaveOccurred()) + Expect(createTestJobs(fakeClient)).To(Succeed()) // Make sure the job resources are properly created jobs := &unstructured.UnstructuredList{} jobs.SetGroupVersionKind(jobGVK) - err = fakeClient.List(context.Background(), jobs) - Expect(err).ShouldNot(HaveOccurred()) + Expect(fakeClient.List(context.Background(), jobs)).To(Succeed()) Expect(jobs.Items).Should(HaveLen(3)) pruner, err := NewPruner(fakeClient, jobGVK, myStrategy, WithLabels(appLabels), WithNamespace(namespace)) @@ -276,8 +261,7 @@ var _ = Describe("Prune", func() { Expect(prunedObjects).Should(BeEmpty()) // Get a list of the jobs to make sure we have pruned the ones we expected - err = fakeClient.List(context.Background(), jobs) - Expect(err).ShouldNot(HaveOccurred()) + Expect(fakeClient.List(context.Background(), jobs)).To(Succeed()) Expect(jobs.Items).Should(HaveLen(3)) }) @@ -285,14 +269,12 @@ var _ = Describe("Prune", func() { Context("Returns an Error", func() { It("Should Return an Error if IsPrunableFunc Returns an Error That is not of Type Unprunable", func() { // Create the test resources - in this case Jobs - err := createTestJobs(fakeClient) - Expect(err).ShouldNot(HaveOccurred()) + Expect(createTestJobs(fakeClient)).To(Succeed()) // Make sure the job resources are properly created jobs := &unstructured.UnstructuredList{} jobs.SetGroupVersionKind(jobGVK) - err = fakeClient.List(context.Background(), jobs) - Expect(err).ShouldNot(HaveOccurred()) + Expect(fakeClient.List(context.Background(), jobs)).To(Succeed()) Expect(jobs.Items).Should(HaveLen(3)) pruner, err := NewPruner(fakeClient, jobGVK, myStrategy, WithLabels(appLabels), WithNamespace(namespace)) @@ -308,26 +290,22 @@ var _ = Describe("Prune", func() { RegisterIsPrunableFunc(jobGVK, errorPrunableFunc) prunedObjects, err := pruner.Prune(context.Background()) - Expect(err).Should(HaveOccurred()) - Expect(err.Error()).Should(Equal("TEST")) + Expect(err).Should(MatchError("TEST")) Expect(prunedObjects).Should(BeEmpty()) // Get a list of the jobs to make sure we have pruned the ones we expected - err = fakeClient.List(context.Background(), jobs) - Expect(err).ShouldNot(HaveOccurred()) + Expect(fakeClient.List(context.Background(), jobs)).To(Succeed()) Expect(jobs.Items).Should(HaveLen(3)) }) It("Should Return An Error If Strategy Function Returns An Error", func() { // Create the test resources - in this case Jobs - err := createTestJobs(fakeClient) - Expect(err).ShouldNot(HaveOccurred()) + Expect(createTestJobs(fakeClient)).To(Succeed()) // Make sure the job resources are properly created jobs := &unstructured.UnstructuredList{} jobs.SetGroupVersionKind(jobGVK) - err = fakeClient.List(context.Background(), jobs) - Expect(err).ShouldNot(HaveOccurred()) + Expect(fakeClient.List(context.Background(), jobs)).To(Succeed()) Expect(jobs.Items).Should(HaveLen(3)) // strategy that will return an error @@ -343,26 +321,22 @@ var _ = Describe("Prune", func() { RegisterIsPrunableFunc(jobGVK, myIsPrunable) prunedObjects, err := pruner.Prune(context.Background()) - Expect(err).Should(HaveOccurred()) - Expect(err.Error()).Should(Equal("error determining prunable objects: TESTERROR")) + Expect(err).Should(MatchError("error determining prunable objects: TESTERROR")) Expect(prunedObjects).Should(BeNil()) // Get a list of the jobs to make sure we have pruned the ones we expected - err = fakeClient.List(context.Background(), jobs) - Expect(err).ShouldNot(HaveOccurred()) + Expect(fakeClient.List(context.Background(), jobs)).To(Succeed()) Expect(jobs.Items).Should(HaveLen(3)) }) It("Should Return an Error if it can not Prune a Resource", func() { // Create the test resources - in this case Jobs - err := createTestJobs(fakeClient) - Expect(err).ShouldNot(HaveOccurred()) + Expect(createTestJobs(fakeClient)).To(Succeed()) // Make sure the job resources are properly created jobs := &unstructured.UnstructuredList{} jobs.SetGroupVersionKind(jobGVK) - err = fakeClient.List(context.Background(), jobs) - Expect(err).ShouldNot(HaveOccurred()) + Expect(fakeClient.List(context.Background(), jobs)).To(Succeed()) Expect(jobs.Items).Should(HaveLen(3)) pruner, err := NewPruner(fakeClient, jobGVK, myStrategy, WithLabels(appLabels), WithNamespace(namespace)) @@ -380,13 +354,11 @@ var _ = Describe("Prune", func() { RegisterIsPrunableFunc(jobGVK, prunableFunc) prunedObjects, err := pruner.Prune(context.Background()) - Expect(err).Should(HaveOccurred()) - Expect(err.Error()).Should(ContainSubstring("error pruning object: jobs.batch \"churro1\" not found")) + Expect(err).Should(MatchError(ContainSubstring("error pruning object: jobs.batch \"churro1\" not found"))) Expect(prunedObjects).Should(BeEmpty()) // Get a list of the jobs to make sure we have pruned the ones we expected - err = fakeClient.List(context.Background(), jobs) - Expect(err).ShouldNot(HaveOccurred()) + Expect(fakeClient.List(context.Background(), jobs)).To(Succeed()) Expect(jobs.Items).Should(BeEmpty()) }) @@ -437,8 +409,7 @@ var _ = Describe("Prune", func() { pod.SetGroupVersionKind(podGVK) // Run it through DefaultPodIsPrunable - err := DefaultPodIsPrunable(pod) - Expect(err).ShouldNot(HaveOccurred()) + Expect(DefaultPodIsPrunable(pod)).To(Succeed()) }) It("Should Panic When client.Object is not of type 'Pod'", func() { @@ -467,12 +438,11 @@ var _ = Describe("Prune", func() { // Run it through DefaultPodIsPrunable err := DefaultPodIsPrunable(pod) - Expect(err).Should(HaveOccurred()) + Expect(err).Should(MatchError(fmt.Sprintf("unable to prune %s: Pod has not succeeded", client.ObjectKeyFromObject(pod)))) var expectErr *Unprunable Expect(errors.As(err, &expectErr)).Should(BeTrue()) Expect(expectErr.Reason).Should(Equal("Pod has not succeeded")) Expect(expectErr.Obj).ShouldNot(BeNil()) - Expect(err.Error()).Should(Equal(fmt.Sprintf("unable to prune %s: Pod has not succeeded", client.ObjectKeyFromObject(pod)))) }) }) @@ -492,8 +462,7 @@ var _ = Describe("Prune", func() { job.SetGroupVersionKind(jobGVK) // Run it through DefaultJobIsPrunable - err := DefaultJobIsPrunable(job) - Expect(err).ShouldNot(HaveOccurred()) + Expect(DefaultJobIsPrunable(job)).To(Succeed()) }) It("Should Return An Error When Kind Is Not 'Job'", func() { @@ -522,12 +491,11 @@ var _ = Describe("Prune", func() { // Run it through DefaultJobIsPrunable err := DefaultJobIsPrunable(job) - Expect(err).Should(HaveOccurred()) + Expect(err).Should(MatchError(ContainSubstring(fmt.Sprintf("unable to prune %s: Job has not completed", client.ObjectKeyFromObject(job))))) var expectErr *Unprunable Expect(errors.As(err, &expectErr)).Should(BeTrue()) Expect(expectErr.Reason).Should(Equal("Job has not completed")) Expect(expectErr.Obj).ShouldNot(BeNil()) - Expect(err.Error()).Should(ContainSubstring(fmt.Sprintf("unable to prune %s: Job has not completed", client.ObjectKeyFromObject(job)))) }) })