From d937baa51ef38738789199741b1c04b96591a3d7 Mon Sep 17 00:00:00 2001
From: Sergi Castro <sergikstro@gmail.com>
Date: Thu, 29 Feb 2024 11:32:44 +0100
Subject: [PATCH] Fix more races in secret controller test (#66)

fix more races in secret controller
---
 .../k8s/secret_controller_lifecycle_test.go   | 73 ++++++++++++++++---
 1 file changed, 63 insertions(+), 10 deletions(-)

diff --git a/internal/k8s/secret_controller_lifecycle_test.go b/internal/k8s/secret_controller_lifecycle_test.go
index 5063e5e..1e17af6 100644
--- a/internal/k8s/secret_controller_lifecycle_test.go
+++ b/internal/k8s/secret_controller_lifecycle_test.go
@@ -17,6 +17,7 @@ package k8s
 import (
 	"context"
 	"sync"
+	"sync/atomic"
 	"testing"
 	"time"
 
@@ -55,9 +56,14 @@ func TestManagerStarts(t *testing.T) {
 		controller = NewSecretController(&cfg.Config)
 	)
 
+	manual := &manualPreRun{
+		preRunStarted: new(atomic.Bool),
+		finishPreRun:  make(chan struct{}),
+	}
+
 	controller.restConf = startEnv(t)
 	controller.namespace = defaultNamespace
-	g.Register(irq, &cfg, logging, controller)
+	g.Register(irq, &cfg, logging, controller, manual)
 
 	wg := sync.WaitGroup{}
 	wg.Add(1)
@@ -67,22 +73,25 @@ func TestManagerStarts(t *testing.T) {
 		require.NoError(t, err)
 	}()
 
-	t.Run("controller is setup at preRun", func(t *testing.T) {
-		require.Eventually(t, func() bool {
-			return controller.k8sClient != nil
-		}, defaultWait, defaultTick, "Controller manager is not setup")
-	})
+	// eventually, the controller's PreRun should be done
+	require.Eventually(t, func() bool { return manual.preRunStarted.Load() },
+		defaultWait, defaultTick, "controller PreRun not done")
 
-	mgrStarted := false
+	// once preRun is done, the manager should initialize, and we can add our own test manager.Runnable
+	mgrStarted := &atomic.Bool{}
 	err := controller.manager.Add(manager.RunnableFunc(func(ctx context.Context) error {
-		mgrStarted = true
+		mgrStarted.Store(true)
 		<-ctx.Done()
 		return ctx.Err()
 	}))
 	require.NoError(t, err)
 
+	// signale the prerun phase to complete
+	close(manual.finishPreRun)
+
+	// at some point of serve phase, the manager should be started
 	t.Run("manager is started", func(t *testing.T) {
-		require.Eventually(t, func() bool { return mgrStarted },
+		require.Eventually(t, func() bool { return mgrStarted.Load() },
 			defaultWait, defaultTick, "manager not started")
 	})
 
@@ -101,9 +110,13 @@ func TestManagerNotInitializedIfNothingToWatch(t *testing.T) {
 		controller = NewSecretController(&cfg.Config)
 	)
 
+	manual := &manualService{
+		serveStarted: new(atomic.Bool),
+	}
+
 	controller.restConf = startEnv(t)
 	controller.namespace = defaultNamespace
-	g.Register(irq, &cfg, logging, controller)
+	g.Register(irq, &cfg, logging, controller, manual)
 
 	wg := sync.WaitGroup{}
 	wg.Add(1)
@@ -113,6 +126,10 @@ func TestManagerNotInitializedIfNothingToWatch(t *testing.T) {
 		require.NoError(t, err)
 	}()
 
+	// wait for the run group to be fully started
+	require.Eventually(t, func() bool { return manual.serveStarted.Load() },
+		defaultWait, defaultTick, "run group not fully started")
+
 	// signal group termination and wait for it
 	require.NoError(t, irq.Close())
 	wg.Wait()
@@ -131,3 +148,39 @@ func startEnv(t *testing.T) *rest.Config {
 	})
 	return cfg
 }
+
+var (
+	_ run.PreRunner      = (*manualPreRun)(nil)
+	_ run.ServiceContext = (*manualService)(nil)
+)
+
+type (
+	manualPreRun struct {
+		preRunStarted *atomic.Bool
+		finishPreRun  chan struct{}
+	}
+
+	manualService struct {
+		serveStarted *atomic.Bool
+	}
+)
+
+func (l *manualPreRun) Name() string {
+	return "manual preRun"
+}
+
+func (l *manualPreRun) PreRun() error {
+	l.preRunStarted.Store(true)
+	<-l.finishPreRun
+	return nil
+}
+
+func (l *manualService) Name() string {
+	return "manual service"
+}
+
+func (l *manualService) ServeContext(ctx context.Context) error {
+	l.serveStarted.Store(true)
+	<-ctx.Done()
+	return ctx.Err()
+}