From af670c8c3bd2302b825e68f83ef18d82065d4b1e Mon Sep 17 00:00:00 2001 From: Karl Isenberg Date: Mon, 7 Oct 2024 11:08:41 -0700 Subject: [PATCH] test: fix flaky controller-manager tests Go Testing doesn't handle panics in goroutines well. So test fatals need to happen from the main thread. But errors can still happen in goroutines. So change the controller-manager stopping to assert instead of require no errors. --- .../reposync_controller_manager_test.go | 24 ++--------- .../rootsync_controller_manager_test.go | 43 +++++++------------ 2 files changed, 18 insertions(+), 49 deletions(-) diff --git a/pkg/reconcilermanager/controllers/reposync_controller_manager_test.go b/pkg/reconcilermanager/controllers/reposync_controller_manager_test.go index 28c538094..d61628eac 100644 --- a/pkg/reconcilermanager/controllers/reposync_controller_manager_test.go +++ b/pkg/reconcilermanager/controllers/reposync_controller_manager_test.go @@ -64,13 +64,7 @@ func TestRepoSyncReconcilerDeploymentLifecycle(t *testing.T) { errCh := startControllerManager(ctx, t, fakeClient, testReconciler) // Wait for manager to exit before returning - defer func() { - cancel() - t.Log("waiting for controller-manager to stop") - for err := range errCh { - require.NoError(t, err) - } - }() + defer stopControllerManager(t, cancel, errCh) watchCtx, watchCancel := context.WithTimeout(ctx, 10*time.Second) defer watchCancel() @@ -187,13 +181,7 @@ func TestReconcileInvalidRepoSyncLifecycle(t *testing.T) { errCh := startControllerManager(ctx, t, fakeClient, testReconciler) // Wait for manager to exit before returning - defer func() { - cancel() - t.Log("waiting for controller-manager to stop") - for err := range errCh { - require.NoError(t, err) - } - }() + defer stopControllerManager(t, cancel, errCh) t.Log("watching for RepoSync status update") watchCtx, watchCancel := context.WithTimeout(ctx, 10*time.Second) @@ -260,13 +248,7 @@ func TestReconcileRepoSyncLifecycleValidToInvalid(t *testing.T) { errCh := startControllerManager(ctx, t, fakeClient, testReconciler) // Wait for manager to exit before returning - defer func() { - cancel() - t.Log("waiting for controller-manager to stop") - for err := range errCh { - require.NoError(t, err) - } - }() + defer stopControllerManager(t, cancel, errCh) reconcilerKey := core.NsReconcilerObjectKey(rs.Namespace, rs.Name) diff --git a/pkg/reconcilermanager/controllers/rootsync_controller_manager_test.go b/pkg/reconcilermanager/controllers/rootsync_controller_manager_test.go index 0c2044745..d5d21b0ae 100644 --- a/pkg/reconcilermanager/controllers/rootsync_controller_manager_test.go +++ b/pkg/reconcilermanager/controllers/rootsync_controller_manager_test.go @@ -24,6 +24,7 @@ import ( "time" "github.com/go-logr/logr/testr" + "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" appsv1 "k8s.io/api/apps/v1" corev1 "k8s.io/api/core/v1" @@ -78,13 +79,7 @@ func TestRootSyncReconcilerDeploymentLifecycle(t *testing.T) { errCh := startControllerManager(ctx, t, fakeClient, testReconciler) // Wait for manager to exit before returning - defer func() { - cancel() - t.Log("waiting for controller-manager to stop") - for err := range errCh { - require.NoError(t, err) - } - }() + defer stopControllerManager(t, cancel, errCh) watchCtx, watchCancel := context.WithTimeout(ctx, 10*time.Second) defer watchCancel() @@ -201,13 +196,7 @@ func TestReconcileInvalidRootSyncLifecycle(t *testing.T) { errCh := startControllerManager(ctx, t, fakeClient, testReconciler) // Wait for manager to exit before returning - defer func() { - cancel() - t.Log("waiting for controller-manager to stop") - for err := range errCh { - require.NoError(t, err) - } - }() + defer stopControllerManager(t, cancel, errCh) t.Log("watching for RootSync status update") watchCtx, watchCancel := context.WithTimeout(ctx, 10*time.Second) @@ -274,13 +263,7 @@ func TestReconcileRootSyncLifecycleValidToInvalid1(t *testing.T) { errCh := startControllerManager(ctx, t, fakeClient, testReconciler) // Wait for manager to exit before returning - defer func() { - cancel() - t.Log("waiting for controller-manager to stop") - for err := range errCh { - require.NoError(t, err) - } - }() + defer stopControllerManager(t, cancel, errCh) reconcilerKey := core.RootReconcilerObjectKey(rs.Name) @@ -517,13 +500,7 @@ func testDriftProtection(t *testing.T, fakeClient *syncerFake.Client, testReconc errCh := startControllerManager(ctx, t, fakeClient, testReconciler) // Wait for manager to exit before returning - defer func() { - cancel() - t.Log("waiting for controller-manager to stop") - for err := range errCh { - require.NoError(t, err) - } - }() + defer stopControllerManager(t, cancel, errCh) key := objKeyFunc(client.ObjectKeyFromObject(syncObj)) @@ -650,6 +627,16 @@ func startControllerManager(ctx context.Context, t *testing.T, fakeClient *synce return errCh } +func stopControllerManager(t *testing.T, cancel context.CancelFunc, errCh <-chan error) { + cancel() + t.Log("waiting for controller-manager to stop") + for err := range errCh { + // Error/Assert instead of Fatal/Require, to avoid panic when called async. + // Go tests that panic in a defer can be flakey. + assert.NoError(t, err) + } +} + func logObjectYAMLIfFailed(t *testing.T, fakeClient *syncerFake.Client, obj client.Object) { if t.Failed() { err := fakeClient.Get(context.Background(), client.ObjectKeyFromObject(obj), obj)