Impove deploy-image controller-test.go

Fixes a weakness in the controller-test.  Previously, the test did not
check for duplicates.  Duplicate conditions can be a problem, and this
new way of writing the test ensures that this is caught early.

* To(Not()) → NotTo()
* Use default eventually timeouts
* Eventually(func() error) → Eventually(func(g Gomega)), and then use
  g.Expect inside those Eventually() functions
* Remove use of Eventually when nothing else is happening
* Use Gomega assertions for the condition check
* Check for uniqueness for the conditions
* Use gomega's format string support to explain the context of failures

Author: mogsie

docs/book/src/getting-started/testdata/project/internal/controller/memcached_controller_test.go

@@ -18,8 +18,6 @@ package controller
 
 import (
 	"context"
-	"fmt"
-	"time"
 
 	. "github.com/onsi/ginkgo/v2"
 	. "github.com/onsi/gomega"
@@ -83,31 +81,19 @@ var _ = Describe("Memcached Controller", func() {
 			})
 			Expect(err).NotTo(HaveOccurred())
 			By("Checking if Deployment was successfully created in the reconciliation")
-			Eventually(func() error {
+			Eventually(func(g Gomega) {
 				found := &appsv1.Deployment{}
-				return k8sClient.Get(ctx, typeNamespacedName, found)
-			}, time.Minute, time.Second).Should(Succeed())
+				g.Expect(k8sClient.Get(ctx, typeNamespacedName, found)).To(Succeed())
+			}).Should(Succeed())
 
 			By("Checking the latest Status Condition added to the Memcached instance")
-			Eventually(func() error {
-				if memcached.Status.Conditions != nil &&
-					len(memcached.Status.Conditions) != 0 {
-					latestStatusCondition := memcached.Status.Conditions[len(memcached.Status.Conditions)-1]
-					expectedLatestStatusCondition := metav1.Condition{
-						Type:   typeAvailableMemcached,
-						Status: metav1.ConditionTrue,
-						Reason: "Reconciling",
-						Message: fmt.Sprintf(
-							"Deployment for custom resource (%s) with %d replicas created successfully",
-							memcached.Name,
-							memcached.Spec.Size),
-					}
-					if latestStatusCondition != expectedLatestStatusCondition {
-						return fmt.Errorf("The latest status condition added to the Memcached instance is not as expected")
-					}
-				}
-				return nil
-			}, time.Minute, time.Second).Should(Succeed())
+			Expect(k8sClient.Get(ctx, typeNamespacedName, memcached)).To(Succeed())
+			conditions := []metav1.Condition{}
+			Expect(memcached.Status.Conditions).To(ContainElement(
+				HaveField("Type", Equal(typeAvailableMemcached)), &conditions))
+			Expect(conditions).To(HaveLen(1), "Multiple conditions of type %s", typeAvailableMemcached)
+			Expect(conditions[0].Status).To(Equal(metav1.ConditionTrue), "condition %s", typeAvailableMemcached)
+			Expect(conditions[0].Reason).To(Equal("Reconciling"), "condition %s", typeAvailableMemcached)
 		})
 	})
 })

hack/docs/internal/getting-started/generate_getting_started.go

@@ -48,15 +48,6 @@ func (sp *Sample) UpdateTutorial() {
 func (sp *Sample) updateControllerTest() {
 	file := "internal/controller/memcached_controller_test.go"
 	err := pluginutil.ReplaceInFile(
-		filepath.Join(sp.ctx.Dir, file),
-		"\"context\"",
-		`"context"
-	"fmt"
-	"time"`,
-	)
-	hackutils.CheckError("add imports", err)
-
-	err = pluginutil.ReplaceInFile(
 		filepath.Join(sp.ctx.Dir, file),
 		". \"github.com/onsi/gomega\"",
 		`. "github.com/onsi/gomega"
@@ -78,31 +69,19 @@ func (sp *Sample) updateControllerTest() {
 		`// TODO(user): Add more specific assertions depending on your controller's reconciliation logic.
 			// Example: If you expect a certain status condition after reconciliation, verify it here.`,
 		`By("Checking if Deployment was successfully created in the reconciliation")
-			Eventually(func() error {
+			Eventually(func(g Gomega) {
 				found := &appsv1.Deployment{}
-				return k8sClient.Get(ctx, typeNamespacedName, found)
-			}, time.Minute, time.Second).Should(Succeed())
+				g.Expect(k8sClient.Get(ctx, typeNamespacedName, found)).To(Succeed())
+			}).Should(Succeed())
 
 			By("Checking the latest Status Condition added to the Memcached instance")
-			Eventually(func() error {
-				if memcached.Status.Conditions != nil &&
-					len(memcached.Status.Conditions) != 0 {
-					latestStatusCondition := memcached.Status.Conditions[len(memcached.Status.Conditions)-1]
-					expectedLatestStatusCondition := metav1.Condition{
-						Type:   typeAvailableMemcached,
-						Status: metav1.ConditionTrue,
-						Reason: "Reconciling",
-						Message: fmt.Sprintf(
-							"Deployment for custom resource (%s) with %d replicas created successfully",
-							memcached.Name,
-							memcached.Spec.Size),
-					}
-					if latestStatusCondition != expectedLatestStatusCondition {
-						return fmt.Errorf("The latest status condition added to the Memcached instance is not as expected")
-					}
-				}
-				return nil
-			}, time.Minute, time.Second).Should(Succeed())`,
+			Expect(k8sClient.Get(ctx, typeNamespacedName, memcached)).To(Succeed())
+			conditions := []metav1.Condition{}
+			Expect(memcached.Status.Conditions).To(ContainElement(
+				HaveField("Type", Equal(typeAvailableMemcached)), &conditions))
+			Expect(conditions).To(HaveLen(1), "Multiple conditions of type %s", typeAvailableMemcached)
+			Expect(conditions[0].Status).To(Equal(metav1.ConditionTrue), "condition %s", typeAvailableMemcached)
+			Expect(conditions[0].Reason).To(Equal("Reconciling"), "condition %s", typeAvailableMemcached)`,
 	)
 	hackutils.CheckError("add spec apis", err)
 }

pkg/plugins/golang/deploy-image/v1alpha1/scaffolds/internal/templates/controllers/controller-test.go

@@ -68,7 +68,6 @@ import (
 	"context"
 	"os"
 	"time"
-	"fmt"
 
 	//nolint:golint
     . "github.com/onsi/ginkgo/v2"
@@ -105,14 +104,17 @@ var _ = Describe("{{ .Resource.Kind }} controller", func() {
 		}
 		{{ lower .Resource.Kind }} := &{{ .Resource.ImportAlias }}.{{ .Resource.Kind }}{}
 
+		SetDefaultEventuallyTimeout(2 * time.Minute)
+		SetDefaultEventuallyPollingInterval(time.Second)
+
 		BeforeEach(func() {
 			By("Creating the Namespace to perform the tests")
 			err := k8sClient.Create(ctx, namespace);
-			Expect(err).To(Not(HaveOccurred()))
+			Expect(err).NotTo(HaveOccurred())
 
 			By("Setting the Image ENV VAR which stores the Operand image")
 			err= os.Setenv("{{ upper .Resource.Kind }}_IMAGE", "example.com/image:test")
-			Expect(err).To(Not(HaveOccurred()))
+			Expect(err).NotTo(HaveOccurred())
 
 			By("creating the custom resource for the Kind {{ .Resource.Kind }}")
 			err = k8sClient.Get(ctx, typeNamespacedName, {{ lower .Resource.Kind }})
@@ -133,19 +135,19 @@ var _ = Describe("{{ .Resource.Kind }} controller", func() {
 				}
 				
 				err = k8sClient.Create(ctx, {{ lower .Resource.Kind }})
-				Expect(err).To(Not(HaveOccurred()))
+				Expect(err).NotTo(HaveOccurred())
 			}
 		})
 
 		AfterEach(func() {
 			By("removing the custom resource for the Kind {{ .Resource.Kind }}")
 			found := &{{ .Resource.ImportAlias }}.{{ .Resource.Kind }}{}
 			err := k8sClient.Get(ctx, typeNamespacedName, found)
-			Expect(err).To(Not(HaveOccurred()))
+			Expect(err).NotTo(HaveOccurred())
 
-			Eventually(func() error {
-				return k8sClient.Delete(context.TODO(), found)
-			}, 2*time.Minute, time.Second).Should(Succeed())
+			Eventually(func(g Gomega) {
+				g.Expect(k8sClient.Delete(context.TODO(), found)).To(Succeed())
+			}).Should(Succeed())
 
 			// TODO(user): Attention if you improve this code by adding other context test you MUST
 			// be aware of the current delete namespace limitations. 
@@ -159,10 +161,10 @@ var _ = Describe("{{ .Resource.Kind }} controller", func() {
 
 		It("should successfully reconcile a custom resource for {{ .Resource.Kind }}", func() {
 			By("Checking if the custom resource was successfully created")
-			Eventually(func() error {
+			Eventually(func(g Gomega) {
 				found := &{{ .Resource.ImportAlias }}.{{ .Resource.Kind }}{}
-				return k8sClient.Get(ctx, typeNamespacedName, found)
-			}, time.Minute, time.Second).Should(Succeed())
+				Expect(k8sClient.Get(ctx, typeNamespacedName, found)).To(Succeed())
+			}).Should(Succeed())
 
 			By("Reconciling the custom resource created")
 			{{ lower .Resource.Kind }}Reconciler := &{{ .Resource.Kind }}Reconciler{
@@ -173,34 +175,22 @@ var _ = Describe("{{ .Resource.Kind }} controller", func() {
 			_, err := {{ lower .Resource.Kind }}Reconciler.Reconcile(ctx, reconcile.Request{
 				NamespacedName: typeNamespacedName,
 			})
-			Expect(err).To(Not(HaveOccurred()))
+			Expect(err).NotTo(HaveOccurred())
 
 			By("Checking if Deployment was successfully created in the reconciliation")
-			Eventually(func() error {
+			Eventually(func(g Gomega) {
 				found := &appsv1.Deployment{}
-				return k8sClient.Get(ctx, typeNamespacedName, found)
-			}, time.Minute, time.Second).Should(Succeed())
+				g.Expect(k8sClient.Get(ctx, typeNamespacedName, found)).To(Succeed())
+			}).Should(Succeed())
 
 			By("Checking the latest Status Condition added to the {{ .Resource.Kind }} instance")
-			Eventually(func() error {
-				if {{ lower .Resource.Kind }}.Status.Conditions != nil &&
-					len({{ lower .Resource.Kind }}.Status.Conditions) != 0 {
-					latestStatusCondition := {{ lower .Resource.Kind }}.Status.Conditions[len({{ lower .Resource.Kind }}.Status.Conditions)-1]
-					expectedLatestStatusCondition := metav1.Condition{
-						Type:    typeAvailable{{ .Resource.Kind }},
-						Status:  metav1.ConditionTrue,
-						Reason:  "Reconciling",
-						Message: fmt.Sprintf(
-							"Deployment for custom resource (%s) with %d replicas created successfully", 
-							{{ lower .Resource.Kind }}.Name,
-							{{ lower .Resource.Kind }}.Spec.Size),
-					}
-					if latestStatusCondition != expectedLatestStatusCondition {
-						return fmt.Errorf("The latest status condition added to the {{ .Resource.Kind }} instance is not as expected")
-					}
-				}
-				return nil
-			}, time.Minute, time.Second).Should(Succeed())
+			Expect(k8sClient.Get(ctx, typeNamespacedName, {{ lower .Resource.Kind }})).To(Succeed())
+			conditions := []metav1.Condition{}
+			Expect({{ lower .Resource.Kind }}.Status.Conditions).To(ContainElement(
+				HaveField("Type", Equal(typeAvailable{{ .Resource.Kind }})), &conditions))
+			Expect(conditions).To(HaveLen(1), "Multiple conditions of type %s", typeAvailable{{ .Resource.Kind }})
+			Expect(conditions[0].Status).To(Equal(metav1.ConditionTrue), "condition %s", typeAvailable{{ .Resource.Kind }})
+			Expect(conditions[0].Reason).To(Equal("Reconciling"), "condition %s", typeAvailable{{ .Resource.Kind }})
 		})
 	})
 })

testdata/project-v4-multigroup/internal/controller/example.com/busybox_controller_test.go

@@ -18,7 +18,6 @@ package examplecom
 
 import (
 	"context"
-	"fmt"
 	"os"
 	"time"
 
@@ -55,14 +54,17 @@ var _ = Describe("Busybox controller", func() {
 		}
 		busybox := &examplecomv1alpha1.Busybox{}
 
+		SetDefaultEventuallyTimeout(2 * time.Minute)
+		SetDefaultEventuallyPollingInterval(time.Second)
+
 		BeforeEach(func() {
 			By("Creating the Namespace to perform the tests")
 			err := k8sClient.Create(ctx, namespace)
-			Expect(err).To(Not(HaveOccurred()))
+			Expect(err).NotTo(HaveOccurred())
 
 			By("Setting the Image ENV VAR which stores the Operand image")
 			err = os.Setenv("BUSYBOX_IMAGE", "example.com/image:test")
-			Expect(err).To(Not(HaveOccurred()))
+			Expect(err).NotTo(HaveOccurred())
 
 			By("creating the custom resource for the Kind Busybox")
 			err = k8sClient.Get(ctx, typeNamespacedName, busybox)
@@ -80,19 +82,19 @@ var _ = Describe("Busybox controller", func() {
 				}
 
 				err = k8sClient.Create(ctx, busybox)
-				Expect(err).To(Not(HaveOccurred()))
+				Expect(err).NotTo(HaveOccurred())
 			}
 		})
 
 		AfterEach(func() {
 			By("removing the custom resource for the Kind Busybox")
 			found := &examplecomv1alpha1.Busybox{}
 			err := k8sClient.Get(ctx, typeNamespacedName, found)
-			Expect(err).To(Not(HaveOccurred()))
+			Expect(err).NotTo(HaveOccurred())
 
-			Eventually(func() error {
-				return k8sClient.Delete(context.TODO(), found)
-			}, 2*time.Minute, time.Second).Should(Succeed())
+			Eventually(func(g Gomega) {
+				g.Expect(k8sClient.Delete(context.TODO(), found)).To(Succeed())
+			}).Should(Succeed())
 
 			// TODO(user): Attention if you improve this code by adding other context test you MUST
 			// be aware of the current delete namespace limitations.
@@ -106,10 +108,10 @@ var _ = Describe("Busybox controller", func() {
 
 		It("should successfully reconcile a custom resource for Busybox", func() {
 			By("Checking if the custom resource was successfully created")
-			Eventually(func() error {
+			Eventually(func(g Gomega) {
 				found := &examplecomv1alpha1.Busybox{}
-				return k8sClient.Get(ctx, typeNamespacedName, found)
-			}, time.Minute, time.Second).Should(Succeed())
+				Expect(k8sClient.Get(ctx, typeNamespacedName, found)).To(Succeed())
+			}).Should(Succeed())
 
 			By("Reconciling the custom resource created")
 			busyboxReconciler := &BusyboxReconciler{
@@ -120,34 +122,22 @@ var _ = Describe("Busybox controller", func() {
 			_, err := busyboxReconciler.Reconcile(ctx, reconcile.Request{
 				NamespacedName: typeNamespacedName,
 			})
-			Expect(err).To(Not(HaveOccurred()))
+			Expect(err).NotTo(HaveOccurred())
 
 			By("Checking if Deployment was successfully created in the reconciliation")
-			Eventually(func() error {
+			Eventually(func(g Gomega) {
 				found := &appsv1.Deployment{}
-				return k8sClient.Get(ctx, typeNamespacedName, found)
-			}, time.Minute, time.Second).Should(Succeed())
+				g.Expect(k8sClient.Get(ctx, typeNamespacedName, found)).To(Succeed())
+			}).Should(Succeed())
 
 			By("Checking the latest Status Condition added to the Busybox instance")
-			Eventually(func() error {
-				if busybox.Status.Conditions != nil &&
-					len(busybox.Status.Conditions) != 0 {
-					latestStatusCondition := busybox.Status.Conditions[len(busybox.Status.Conditions)-1]
-					expectedLatestStatusCondition := metav1.Condition{
-						Type:   typeAvailableBusybox,
-						Status: metav1.ConditionTrue,
-						Reason: "Reconciling",
-						Message: fmt.Sprintf(
-							"Deployment for custom resource (%s) with %d replicas created successfully",
-							busybox.Name,
-							busybox.Spec.Size),
-					}
-					if latestStatusCondition != expectedLatestStatusCondition {
-						return fmt.Errorf("The latest status condition added to the Busybox instance is not as expected")
-					}
-				}
-				return nil
-			}, time.Minute, time.Second).Should(Succeed())
+			Expect(k8sClient.Get(ctx, typeNamespacedName, busybox)).To(Succeed())
+			conditions := []metav1.Condition{}
+			Expect(busybox.Status.Conditions).To(ContainElement(
+				HaveField("Type", Equal(typeAvailableBusybox)), &conditions))
+			Expect(conditions).To(HaveLen(1), "Multiple conditions of type %s", typeAvailableBusybox)
+			Expect(conditions[0].Status).To(Equal(metav1.ConditionTrue), "condition %s", typeAvailableBusybox)
+			Expect(conditions[0].Reason).To(Equal("Reconciling"), "condition %s", typeAvailableBusybox)
 		})
 	})
 })