MGMT-23548: Ensure tests w/ gomock controllers call Finish()

bluesort · bluesort · commit 901eb1da16a2 · 2026-03-23T16:44:43.000-04:00
diff --git a/CLAUDE.md b/CLAUDE.md
@@ -368,6 +368,18 @@ The project has three main test categories:
    - Upstream: [assisted-test-infra](https://github.com/openshift/assisted-test-infra)
    - Downstream: QE maintained tests
 
+### Test Patterns
+
+#### Mock Testing with Gomock
+
+All tests using gomock controllers MUST call `ctrl.Finish()` to verify mock expectations.
+Without `ctrl.Finish()`, gomock EXPECT() calls with constraints are not verified:
+- `.Times(N)` - Expected call count won't be checked
+- `.MaxTimes(0)` - Unexpected calls won't be caught
+- `.Do()` / `.DoAndReturn()` - Mock behavior runs without verification
+
+Each context that creates a controller needs its own `AfterEach` with `ctrl.Finish()`.
+
 ### Environment Variables
 
 #### Test Execution
diff --git a/internal/cluster/common_test.go b/internal/cluster/common_test.go
@@ -82,6 +82,7 @@ var _ = Describe("update_cluster_state", func() {
 	})
 	AfterEach(func() {
 		common.DeleteTestDB(db, dbName)
+		ctrl.Finish()
 	})
 
 })
diff --git a/internal/controller/controllers/clusterdeployments_controller_test.go b/internal/controller/controllers/clusterdeployments_controller_test.go
@@ -418,6 +418,10 @@ var _ = Describe("processMirrorRegistryConfig", func() {
 
 		Expect(err.Error()).To(Equal("failed to load value of registries.conf into toml tree; incorrectly formatted toml: (6, 13): cannot have two dots in one float"))
 	})
+
+	AfterEach(func() {
+		mockCtrl.Finish()
+	})
 })
 
 var _ = Describe("cluster reconcile", func() {
@@ -2050,6 +2054,10 @@ var _ = Describe("cluster reconcile", func() {
 			Expect(err).To(BeNil())
 			Expect(result).To(Equal(ctrl.Result{}))
 		})
+
+		AfterEach(func() {
+			mockCtrl.Finish()
+		})
 	})
 
 	Context("cluster installation", func() {
@@ -5199,6 +5207,10 @@ var _ = Describe("unbindAgents", func() {
 		err = c.Get(ctx, types.NamespacedName{Name: "test-agent2", Namespace: testNamespace}, agent)
 		Expect(k8serrors.IsNotFound(err)).To(BeTrue())
 	})
+
+	AfterEach(func() {
+		mockCtrl.Finish()
+	})
 })
 
 var _ = Describe("day2 cluster", func() {
diff --git a/internal/host/hostcommands/container_image_availability_cmd_test.go b/internal/host/hostcommands/container_image_availability_cmd_test.go
@@ -107,6 +107,7 @@ var _ = Describe("container_image_availability_cmd", func() {
 
 	AfterEach(func() {
 		common.DeleteTestDB(db, dbName)
+		ctrl.Finish()
 	})
 })
 
@@ -173,4 +174,8 @@ var _ = Describe("get images", func() {
 		Expect(images[0]).To(ContainSubstring("registry.mirror.example.com"))
 		Expect(images[0]).NotTo(ContainSubstring("quay.io"))
 	})
+
+	AfterEach(func() {
+		ctrl.Finish()
+	})
 })
diff --git a/internal/host/hostcommands/disk_performance_cmd_test.go b/internal/host/hostcommands/disk_performance_cmd_test.go
@@ -72,5 +72,6 @@ var _ = Describe("disk_performance", func() {
 		common.DeleteTestDB(db, dbName)
 		stepReply = nil
 		stepErr = nil
+		ctrl.Finish()
 	})
 })
diff --git a/internal/host/hostcommands/domain_name_resolution_cmd_test.go b/internal/host/hostcommands/domain_name_resolution_cmd_test.go
@@ -81,7 +81,6 @@ var _ = Describe("domainNameResolution", func() {
 	It("Missing cluster name", func() {
 		cluster = common.Cluster{Cluster: models.Cluster{ID: &clusterID, BaseDNSDomain: baseDNSDomain}}
 		Expect(db.Create(&cluster).Error).ShouldNot(HaveOccurred())
-		mockVersions.EXPECT().GetReleaseImage(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return(&models.ReleaseImage{URL: swag.String("quay.io/release")}, nil)
 		stepReply, stepErr = dCmd.GetSteps(ctx, &host)
 		Expect(stepReply).To(BeNil())
 		Expect(stepErr).To(HaveOccurred())
@@ -90,7 +89,6 @@ var _ = Describe("domainNameResolution", func() {
 	It("Missing domain base name", func() {
 		cluster = common.Cluster{Cluster: models.Cluster{ID: &clusterID, Name: name}}
 		Expect(db.Create(&cluster).Error).ShouldNot(HaveOccurred())
-		mockVersions.EXPECT().GetReleaseImage(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return(&models.ReleaseImage{URL: swag.String("quay.io/release")}, nil)
 		stepReply, stepErr = dCmd.GetSteps(ctx, &host)
 		Expect(stepReply).To(BeNil())
 		Expect(stepErr).To(HaveOccurred())
@@ -100,5 +98,6 @@ var _ = Describe("domainNameResolution", func() {
 		common.DeleteTestDB(db, dbName)
 		stepReply = nil
 		stepErr = nil
+		ctrl.Finish()
 	})
 })
diff --git a/internal/host/hostutil/update_host_test.go b/internal/host/hostutil/update_host_test.go
@@ -102,5 +102,6 @@ var _ = Describe("update_host_state", func() {
 
 	AfterEach(func() {
 		common.DeleteTestDB(db, dbName)
+		ctrl.Finish()
 	})
 })
diff --git a/internal/host/refresh_status_preprocessor_test.go b/internal/host/refresh_status_preprocessor_test.go
@@ -61,11 +61,9 @@ var _ = Describe("Cluster Refresh Status Preprocessor", func() {
 			},
 		}, nil)
 		mockOperatorManager = operators.NewMockAPI(ctrl)
-		mockOperatorManager.EXPECT().ValidateHost(gomock.Any(), gomock.Any(), gomock.Any())
 		mockProviderRegistry = registry.NewMockProviderRegistry(ctrl)
 		mockS3WrapperAPI = s3wrapper.NewMockAPI(ctrl)
 		mockVersions = versions.NewMockHandler(ctrl)
-		mockOperatorManager.EXPECT().ValidateCluster(ctx, gomock.Any())
 		db, dbName = common.PrepareTestDB()
 		validatorConfig := &hardware.ValidatorCfg{}
 		disabledHostValidations := make(map[string]struct{})
@@ -82,6 +80,7 @@ var _ = Describe("Cluster Refresh Status Preprocessor", func() {
 
 	AfterEach(func() {
 		common.DeleteTestDB(db, dbName)
+		ctrl.Finish()
 	})
 
 	createHost := func() models.Host {
diff --git a/internal/installercache/installercache_test.go b/internal/installercache/installercache_test.go
@@ -62,6 +62,10 @@ var _ = Describe("release event", func() {
 		).Times(1)
 		Expect(r.Cleanup(ctx)).To(Succeed())
 	})
+
+	AfterEach(func() {
+		ctrl.Finish()
+	})
 })
 
 var _ = Describe("installer cache", func() {
@@ -103,6 +107,7 @@ var _ = Describe("installer cache", func() {
 
 	AfterEach(func() {
 		os.RemoveAll(cacheDir)
+		ctrl.Finish()
 	})
 
 	expectEventsSent := func() {
diff --git a/internal/metrics/directory_usage_collector_test.go b/internal/metrics/directory_usage_collector_test.go
@@ -47,6 +47,7 @@ var _ = Describe("Collection on scrape", func() {
 
 	AfterEach(func() {
 		server.Close()
+		ctrl.Finish()
 	})
 
 	var expectMetricValue = func(metric string, value string) {
diff --git a/internal/provider/registry/registry_test.go b/internal/provider/registry/registry_test.go
@@ -101,6 +101,10 @@ var _ = Describe("Test GetSupportedProvidersByHosts", func() {
 		Expect(err).ToNot(BeNil())
 		Expect(len(platforms)).Should(Equal(0))
 	})
+
+	AfterEach(func() {
+		ctrl.Finish()
+	})
 })
 
 var _ = Describe("IsHostSupported", func() {
@@ -471,6 +475,10 @@ var _ = Describe("Test AddPlatformToInstallConfig", func() {
 			Expect(string(cfg.Platform.External.CloudControllerManager)).To(Equal(cloudControllerManager))
 		})
 	})
+
+	AfterEach(func() {
+		ctrl.Finish()
+	})
 })
 
 var _ = Describe("Test SetPlatformUsages", func() {
@@ -520,6 +528,10 @@ var _ = Describe("Test SetPlatformUsages", func() {
 			Expect(err).To(BeNil())
 		})
 	})
+
+	AfterEach(func() {
+		ctrl.Finish()
+	})
 })
 
 func createHost(isMaster bool, state string, inventory string) *models.Host {
diff --git a/internal/spoke_k8s_client/factory_test.go b/internal/spoke_k8s_client/factory_test.go
@@ -94,6 +94,10 @@ var _ = Describe("Factory", func() {
 			mockSystemInfo = system.NewMockSystemInfo(controller)
 		})
 
+		AfterEach(func() {
+			controller.Finish()
+		})
+
 		It("Can't be created without a logger", func() {
 			client, err := NewFactory(nil, nil, mockSystemInfo)
 			Expect(err).To(MatchError("logger is mandatory"))
@@ -112,6 +116,7 @@ var _ = Describe("Factory", func() {
 			"Fails if secret doesn't contain a valid kubeconfig",
 			func(data map[string][]byte, matcher OmegaMatcher) {
 				controller = gomock.NewController(GinkgoT())
+				defer controller.Finish()
 				mockSystemInfo = system.NewMockSystemInfo(controller)
 
 				// Create the secret:
diff --git a/internal/stream/notification_stream_test.go b/internal/stream/notification_stream_test.go
@@ -35,6 +35,10 @@ var _ = Describe("Close", func() {
 		writer = stream.NewMockStreamWriter(ctrl)
 	})
 
+	AfterEach(func() {
+		ctrl.Finish()
+	})
+
 	It("should close the writer when closing", func() {
 		writer.EXPECT().Close().Times(1)
 		notificationStream := stream.NewNotificationStream(writer, logger, metadata)
@@ -69,6 +73,11 @@ var _ = Describe("Notify", func() {
 		clusterID = strfmt.UUID(uuid.New().String())
 
 	})
+
+	AfterEach(func() {
+		ctrl.Finish()
+	})
+
 	It("succeeds when underlying writer is empty", func() {
 		event := &common.Event{}
 		var emptyWriter stream.StreamWriter
diff --git a/internal/uploader/auth_utils_test.go b/internal/uploader/auth_utils_test.go
@@ -33,6 +33,10 @@ var _ = Describe("getPullSecret", func() {
 		pullSecretWithEmptyAuthFormat = `{"auths":{"%s":{"auth":"","email":"r@empty.com"}}}` // #nosec
 	})
 
+	AfterEach(func() {
+		ctrl.Finish()
+	})
+
 	It("successfully gets a pull secret when the OCM pull secret is present", func() {
 		OCMPullSecret := fmt.Sprintf(pullSecretFormat, openshiftTokenKey, OCMPullSecretToken)
 		data := map[string][]byte{corev1.DockerConfigJsonKey: []byte(OCMPullSecret)}
diff --git a/internal/uploader/events_uploader_test.go b/internal/uploader/events_uploader_test.go
@@ -85,6 +85,7 @@ var _ = Describe("setHeaders", func() {
 	})
 	AfterEach(func() {
 		common.DeleteTestDB(db, dbName)
+		ctrl.Finish()
 	})
 
 	It("correctly sets the request header with a valid auth token", func() {
@@ -192,6 +193,7 @@ var _ = Describe("prepareFiles", func() {
 
 	AfterEach(func() {
 		common.DeleteTestDB(db, dbName)
+		ctrl.Finish()
 	})
 
 	It("successfully prepares all files", func() {
@@ -377,6 +379,7 @@ var _ = Describe("UploadEvents", func() {
 	})
 	AfterEach(func() {
 		common.DeleteTestDB(db, dbName)
+		ctrl.Finish()
 	})
 	It("successfully uploads event data", func() {
 		event := models.Event{
@@ -419,20 +422,12 @@ var _ = Describe("UploadEvents", func() {
 	})
 	It("fails to uploads event data when malformed pullsecret", func() {
 		createOCMPullSecretWithEmptyToken(*mockK8sClient)
-		mockEvents.EXPECT().V2GetEvents(
-			ctx, common.GetDefaultV2GetEventsParams(&clusterID, nil, nil, models.EventCategoryMetrics, models.EventCategoryUser)).
-			Return(&common.V2GetEventsResponse{}, nil).Times(1)
-
 		cluster := createTestObjects(db, &clusterID, &hostID, &infraEnvID)
 		err := uploader.UploadEvents(ctx, cluster, mockEvents)
 		Expect(err.Error()).To(MatchRegexp(`.*failed to get pull secrets to upload event data for cluster.*`))
 	})
 	It("fails to uploads event data when pullsecret not found", func() {
 		createOCMPullSecretNotFound(*mockK8sClient)
-		mockEvents.EXPECT().V2GetEvents(
-			ctx, common.GetDefaultV2GetEventsParams(&clusterID, nil, nil, models.EventCategoryMetrics, models.EventCategoryUser)).
-			Return(&common.V2GetEventsResponse{}, nil).Times(1)
-
 		cluster := createTestObjects(db, &clusterID, &hostID, &infraEnvID)
 		err := uploader.UploadEvents(ctx, cluster, mockEvents)
 		Expect(err.Error()).To(MatchRegexp(`.*failed to get pull secrets to upload event data for cluster.*`))
diff --git a/internal/usage/manager_test.go b/internal/usage/manager_test.go
@@ -51,6 +51,7 @@ var _ = Describe("Feature Usage", func() {
 
 	AfterEach(func() {
 		common.DeleteTestDB(db, dbName)
+		ctrl.Finish()
 	})
 
 	readUsages := func() map[string]models.Usage {
diff --git a/pkg/kafka/json_writer_test.go b/pkg/kafka/json_writer_test.go
@@ -31,6 +31,10 @@ var _ = Describe("Write", func() {
 		}
 	})
 
+	AfterEach(func() {
+		ctrl.Finish()
+	})
+
 	It("should succeed when writing encodable message", func() {
 		key := []byte("my-key")
 		value := map[string]interface{}{
diff --git a/pkg/s3wrapper/filesystem_test.go b/pkg/s3wrapper/filesystem_test.go
@@ -57,6 +57,7 @@ var _ = Describe("s3filesystem", func() {
 		}
 	})
 	AfterEach(func() {
+		ctrl.Finish()
 		err := os.RemoveAll(baseDir)
 		Expect(err).Should(BeNil())
 	})
