test: deflake TestBaseMetricObject with testing/synctest

TestBaseMetricObject fails intermittently in the Test Retina Image
workflow. It compares two process-wide runtime.NumGoroutine()
snapshots, and unrelated goroutine churn breaks the comparison. It
also sleeps real time before each assertion, and the expire callback
writes expireCalled from the expiration goroutine while the test reads
it, a data race under the race detector.

Move the test body into a testing/synctest bubble. The fake clock
makes the expiration ticker fire deterministically, synctest.Wait
orders the callback write before the test reads it, and the bubble
fails the test if the expiration goroutine leaks. t.Cleanup(b.clean)
stops the goroutine on every exit path. The fake clock also closes a
latent gap: the ticker starts before b.updated, so a late first
assertion could observe the label already expired.

Signed-off-by: Quang Nguyen <28567936+nddq@users.noreply.github.com>

Author: nddq

pkg/module/metrics/basemetricsobject_test.go

@@ -3,9 +3,9 @@
 package metrics
 
 import (
-	"runtime"
 	"slices"
 	"testing"
+	"testing/synctest"
 	"time"
 
 	api "github.com/microsoft/retina/crd/api/v1alpha1"
@@ -42,63 +42,56 @@ func TestBaseMetricObject(t *testing.T) {
 
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			before := runtime.NumGoroutine()
-			expireCalled := new([]string)
-			b := newBaseMetricsObject(
-				&api.MetricsContextOptions{
-					MetricName: "test_metric",
-				},
-				l,
-				localContext,
-				func(lbs []string) bool {
-					*expireCalled = lbs
-					return true
-				},
-				tt.ttl,
-			)
+			// The bubble gives the test a fake clock, so the expiration
+			// ticker fires deterministically. It also fails the test if the
+			// expiration goroutine is still alive when the bubble ends.
+			synctest.Test(t, func(t *testing.T) {
+				var expireCalled []string
+				b := newBaseMetricsObject(
+					&api.MetricsContextOptions{
+						MetricName: "test_metric",
+					},
+					l,
+					localContext,
+					func(lbs []string) bool {
+						expireCalled = lbs
+						return true
+					},
+					tt.ttl,
+				)
+				t.Cleanup(b.clean)
 
-			testLabels := []string{"test"}
-			b.updated(testLabels)
+				testLabels := []string{"test"}
+				b.updated(testLabels)
 
-			metrics := len(b.trackedMetricLabels())
-			if tt.trackMetrics {
-				if metrics != 1 {
-					t.Errorf("expected 1 tracked metric label, got %d", metrics)
+				metrics := len(b.trackedMetricLabels())
+				if tt.trackMetrics {
+					if metrics != 1 {
+						t.Errorf("expected 1 tracked metric label, got %d", metrics)
+					}
+				} else {
+					if metrics != 0 {
+						t.Errorf("expected 0 tracked metric labels, got %d", metrics)
+					}
 				}
-			} else {
-				if metrics != 0 {
-					t.Errorf("expected 0 tracked metric labels, got %d", metrics)
-				}
-			}
 
-			// If we have a positive TTL, we should see the expire function get called after the TTL has passed
-			if tt.ttl > 0 {
-				time.Sleep(tt.ttl + time.Millisecond*100)
-				if !slices.Equal(*expireCalled, testLabels) {
-					t.Errorf("expected expire to be called with %v, got %v", testLabels, *expireCalled)
-				}
-				metrics = len(b.trackedMetricLabels())
-				if metrics != 0 {
-					t.Errorf("expected 0 tracked metric labels after expiration, got %d", metrics)
+				// If we have a positive TTL, we should see the expire function get called after the TTL has passed
+				if tt.ttl > 0 {
+					// Advance the fake clock to the first tick, then wait for
+					// the expiration goroutine to block again.
+					time.Sleep(tt.ttl)
+					synctest.Wait()
+					if !slices.Equal(expireCalled, testLabels) {
+						t.Errorf("expected expire to be called with %v, got %v", testLabels, expireCalled)
+					}
+					metrics = len(b.trackedMetricLabels())
+					if metrics != 0 {
+						t.Errorf("expected 0 tracked metric labels after expiration, got %d", metrics)
+					}
+				} else if len(expireCalled) != 0 {
+					t.Errorf("expected expire to not be called, but got %v", expireCalled)
 				}
-			} else if len(*expireCalled) != 0 {
-				t.Errorf("expected expire to not be called, but got %v", *expireCalled)
-			}
-
-			b.clean()
-			if b.expireFn != nil {
-				<-b.ctx.Done()
-			}
-
-			// Wait for any goroutines to exit after clean is called
-			if tt.trackMetrics {
-				time.Sleep(tt.ttl + time.Millisecond*100)
-			}
-
-			after := runtime.NumGoroutine()
-			if after != before {
-				t.Errorf("expected number of goroutines to be the same as before, expected %d, got %d", before, after)
-			}
+			})
 		})
 	}
 }