add testify, tidy up

Author: piceri

Makefile

@@ -19,4 +19,4 @@ fmt:
 	go fmt ./...
 
 test:
-	KUBEBUILDER_ASSETS=$$(setup-envtest use -p path) go test -count=1 ./...
+	KUBEBUILDER_ASSETS=$$(setup-envtest use -p path) go test ./...

go.mod

@@ -40,6 +40,7 @@ require (
 	github.com/prometheus/common v0.66.1 // indirect
 	github.com/prometheus/procfs v0.16.1 // indirect
 	github.com/spf13/pflag v1.0.9 // indirect
+	github.com/stretchr/testify v1.11.1 // indirect
 	github.com/x448/float16 v0.8.4 // indirect
 	go.yaml.in/yaml/v2 v2.4.3 // indirect
 	go.yaml.in/yaml/v3 v3.0.4 // indirect

internal/controller/controller_integration_test.go

@@ -9,11 +9,14 @@ import (
 
 	"github.com/github/deployment-tracker/internal/metadata"
 	"github.com/github/deployment-tracker/pkg/deploymentrecord"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
 	appsv1 "k8s.io/api/apps/v1"
 	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/client-go/kubernetes"
 	k8smetadata "k8s.io/client-go/metadata"
+	"k8s.io/client-go/tools/cache"
 	"sigs.k8s.io/controller-runtime/pkg/envtest"
 )
 
@@ -30,14 +33,14 @@ func (m *mockRecordPoster) PostOne(_ context.Context, record *deploymentrecord.D
 	return m.err
 }
 
-// Helper that allows tests to read captured records safely
+// Helper that allows tests to read captured records safely.
 func (m *mockRecordPoster) getRecords() []*deploymentrecord.DeploymentRecord {
 	m.mu.Lock()
 	defer m.mu.Unlock()
 	return slices.Clone(m.records)
 }
 
-func setup(t *testing.T, namespace string) (*envtest.Environment, context.CancelFunc, *kubernetes.Clientset, *mockRecordPoster) {
+func setup(t *testing.T, namespace string) (*kubernetes.Clientset, *mockRecordPoster) {
 	t.Helper()
 	testEnv := &envtest.Environment{}
 
@@ -52,6 +55,10 @@ func setup(t *testing.T, namespace string) (*envtest.Environment, context.Cancel
 	}
 
 	ctx, cancel := context.WithCancel(context.Background())
+	t.Cleanup(func() {
+		cancel()
+		_ = testEnv.Stop()
+	})
 
 	ns := &corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: namespace}}
 	_, err = clientset.CoreV1().Namespaces().Create(ctx, ns, metav1.CreateOptions{})
@@ -90,10 +97,14 @@ func setup(t *testing.T, namespace string) (*envtest.Environment, context.Cancel
 	mockDeploymentrecord := &mockRecordPoster{}
 	ctrl.apiClient = mockDeploymentrecord
 
-	go ctrl.Run(ctx, 1)
-	time.Sleep(1 * time.Second)
+	go func() {
+		_ = ctrl.Run(ctx, 1)
+	}()
+	if !cache.WaitForCacheSync(ctx.Done(), ctrl.podInformer.HasSynced) {
+		t.Fatal("timed out waiting for informer cache to sync")
+	}
 
-	return testEnv, cancel, clientset, mockDeploymentrecord
+	return clientset, mockDeploymentrecord
 }
 
 func makeDeployment(t *testing.T, clientset *kubernetes.Clientset, owners []metav1.OwnerReference, namespace, name string) *appsv1.Deployment {
@@ -207,52 +218,12 @@ func deletePod(t *testing.T, clientset *kubernetes.Clientset, namespace, name st
 	}
 }
 
-// pollForRecords polls until the mock has at least minCount records, then returns them.
-func pollForRecords(t *testing.T, mock *mockRecordPoster, minCount int, timeout time.Duration) []*deploymentrecord.DeploymentRecord {
-	t.Helper()
-	deadline := time.After(timeout)
-	for {
-		records := mock.getRecords()
-		if len(records) >= minCount {
-			return records
-		}
-		select {
-		case <-deadline:
-			t.Fatalf("timed out waiting for at least %d records, got %d", minCount, len(records))
-		case <-time.After(100 * time.Millisecond):
-		}
-	}
-}
-
-// assertNoNewRecords polls for the given duration and fails if the record count deviates from expectedCount.
-func assertNoNewRecords(t *testing.T, mock *mockRecordPoster, expectedCount int, timeout time.Duration) {
-	t.Helper()
-	deadline := time.After(timeout)
-	for {
-		select {
-		case <-deadline:
-			if got := len(mock.getRecords()); got != expectedCount {
-				t.Fatalf("expected %d records, got %d", expectedCount, got)
-			}
-			return
-		case <-time.After(100 * time.Millisecond):
-			if got := len(mock.getRecords()); got != expectedCount {
-				t.Fatalf("expected %d records, got %d", expectedCount, got)
-			}
-		}
-	}
-}
-
 func TestControllerIntegration_KubernetesDeployment(t *testing.T) {
 	if testing.Short() {
 		t.Skip("skipping integration test in short mode")
 	}
 	namespace := "test-namespace"
-	testEnv, cancel, clientset, mock := setup(t, namespace)
-	defer func(testEnv *envtest.Environment, cancelFunc context.CancelFunc) {
-		_ = testEnv.Stop()
-		cancel()
-	}(testEnv, cancel)
+	clientset, mock := setup(t, namespace)
 
 	// Create deployment, replicaset, and pod; expect 1 record
 	deployment := makeDeployment(t, clientset, []metav1.OwnerReference{}, namespace, "test-deployment")
@@ -269,13 +240,12 @@ func TestControllerIntegration_KubernetesDeployment(t *testing.T) {
 		UID:        replicaSet.UID,
 	}}, namespace, "test-deployment-123456-1")
 
-	records := pollForRecords(t, mock, 1, 5*time.Second)
-	if len(records) != 1 {
-		t.Fatalf("expected 1 record, got %d", len(records))
-	}
-	if records[0].Status != deploymentrecord.StatusDeployed {
-		t.Errorf("expected %s, got %s", deploymentrecord.StatusDeployed, records[0].Status)
-	}
+	require.Eventually(t, func() bool {
+		return len(mock.getRecords()) >= 1
+	}, 5*time.Second, 100*time.Millisecond)
+	records := mock.getRecords()
+	require.Len(t, records, 1)
+	assert.Equal(t, deploymentrecord.StatusDeployed, records[0].Status)
 
 	// Create another pod in replicaset; the dedup cache should prevent a new record as there is only one worker
 	// and no risk of multiple works processing before cache is set.
@@ -285,22 +255,25 @@ func TestControllerIntegration_KubernetesDeployment(t *testing.T) {
 		Name:       replicaSet.Name,
 		UID:        replicaSet.UID,
 	}}, namespace, "test-deployment-123456-2")
-	assertNoNewRecords(t, mock, 1, 5*time.Second)
+	require.Never(t, func() bool {
+		return len(mock.getRecords()) != 1
+	}, 5*time.Second, 100*time.Millisecond)
 
 	// Delete second pod; still expect 1 record
 	deletePod(t, clientset, namespace, "test-deployment-123456-2")
-	assertNoNewRecords(t, mock, 1, 5*time.Second)
+	require.Never(t, func() bool {
+		return len(mock.getRecords()) != 1
+	}, 5*time.Second, 100*time.Millisecond)
 
 	// Delete deployment, replicaset, and first pod; expect 2 records
 	deleteDeployment(t, clientset, namespace, "test-deployment")
 	deleteReplicaSet(t, clientset, namespace, "test-deployment-123456")
 	deletePod(t, clientset, namespace, "test-deployment-123456-1")
 
-	records = pollForRecords(t, mock, 2, 5*time.Second)
-	if len(records) != 2 {
-		t.Fatalf("expected 2 records after deletion, got %d", len(records))
-	}
-	if records[1].Status != deploymentrecord.StatusDecommissioned {
-		t.Errorf("expected second record to be %s, got %s", deploymentrecord.StatusDecommissioned, records[1].Status)
-	}
+	require.Eventually(t, func() bool {
+		return len(mock.getRecords()) >= 2
+	}, 5*time.Second, 100*time.Millisecond)
+	records = mock.getRecords()
+	require.Len(t, records, 2)
+	assert.Equal(t, deploymentrecord.StatusDecommissioned, records[1].Status)
 }