From a968c7d1ba30b15566b00f61dd271ff00e3e3265 Mon Sep 17 00:00:00 2001 From: Jonathan West Date: Wed, 27 Mar 2024 09:06:54 -0400 Subject: [PATCH] chore: fix unit test data races Signed-off-by: Jonathan West --- analysis/controller_test.go | 21 +++++++--- experiments/controller_test.go | 8 ++-- go.mod | 4 +- .../cmd/get/get_rollout.go | 7 ++++ .../viewcontroller/viewcontroller.go | 22 ++++++++-- .../viewcontroller/viewcontroller_test.go | 22 +++++++++- rollout/canary_test.go | 4 ++ rollout/controller_test.go | 26 +++++++----- rollout/sync_test.go | 2 +- .../ambassador/ambassador_test.go | 6 ++- rollout/trafficrouting/apisix/apisix_test.go | 42 ++++--------------- .../trafficrouting/traefik/traefik_test.go | 26 ++++-------- utils/record/record.go | 27 +++++++++++- utils/record/record_test.go | 2 +- utils/time/now.go | 25 ++++++++++- 15 files changed, 159 insertions(+), 85 deletions(-) diff --git a/analysis/controller_test.go b/analysis/controller_test.go index 8282f09c2a..601139a6d4 100644 --- a/analysis/controller_test.go +++ b/analysis/controller_test.go @@ -5,6 +5,7 @@ import ( "encoding/json" "fmt" "reflect" + "sync" "testing" "time" @@ -50,9 +51,13 @@ type fixture struct { // Actions expected to happen on the client. actions []core.Action // Objects from here preloaded into NewSimpleFake. - objects []runtime.Object - enqueuedObjects map[string]int - unfreezeTime func() error + objects []runtime.Object + + // Acquire 'enqueuedObjectMutex' before accessing enqueuedObjects + enqueuedObjects map[string]int + enqueuedObjectMutex sync.Mutex + + unfreezeTime func() error // fake provider provider *mocks.Provider @@ -66,11 +71,11 @@ func newFixture(t *testing.T) *fixture { f.objects = []runtime.Object{} f.enqueuedObjects = make(map[string]int) f.now = time.Now() - timeutil.Now = func() time.Time { + timeutil.SetNowTimeFunc(func() time.Time { return f.now - } + }) f.unfreezeTime = func() error { - timeutil.Now = time.Now + timeutil.SetNowTimeFunc(time.Now) return nil } return f @@ -122,6 +127,10 @@ func (f *fixture) newController(resync resyncFunc) (*Controller, informers.Share if key, err = cache.MetaNamespaceKeyFunc(obj); err != nil { panic(err) } + + f.enqueuedObjectMutex.Lock() + defer f.enqueuedObjectMutex.Unlock() + count, ok := f.enqueuedObjects[key] if !ok { count = 0 diff --git a/experiments/controller_test.go b/experiments/controller_test.go index 0103e03ce1..55e9c1e917 100644 --- a/experiments/controller_test.go +++ b/experiments/controller_test.go @@ -116,13 +116,15 @@ func newFixture(t *testing.T, objects ...runtime.Object) *fixture { f.kubeclient = k8sfake.NewSimpleClientset(f.kubeobjects...) f.enqueuedObjects = make(map[string]int) now := time.Now() - timeutil.Now = func() time.Time { + + timeutil.SetNowTimeFunc(func() time.Time { return now - } + }) f.unfreezeTime = func() error { - timeutil.Now = time.Now + timeutil.SetNowTimeFunc(time.Now) return nil } + return f } diff --git a/go.mod b/go.mod index 5e423a2e93..3330503d4a 100644 --- a/go.mod +++ b/go.mod @@ -1,6 +1,8 @@ module github.com/argoproj/argo-rollouts -go 1.20 +go 1.21 + +toolchain go1.21.6 require ( github.com/antonmedv/expr v1.15.5 diff --git a/pkg/kubectl-argo-rollouts/cmd/get/get_rollout.go b/pkg/kubectl-argo-rollouts/cmd/get/get_rollout.go index 48c40bad3b..f5b0719509 100644 --- a/pkg/kubectl-argo-rollouts/cmd/get/get_rollout.go +++ b/pkg/kubectl-argo-rollouts/cmd/get/get_rollout.go @@ -5,6 +5,7 @@ import ( "fmt" "io" "strings" + "sync" "time" "github.com/juju/ansiterm" @@ -59,7 +60,11 @@ func NewCmdGetRollout(o *options.ArgoRolloutsOptions) *cobra.Command { getOptions.PrintRollout(ri) } else { rolloutUpdates := make(chan *rollout.RolloutInfo) + var rolloutUpdatesMutex sync.Mutex + controller.RegisterCallback(func(roInfo *rollout.RolloutInfo) { + rolloutUpdatesMutex.Lock() + defer rolloutUpdatesMutex.Unlock() rolloutUpdates <- roInfo }) stopCh := ctx.Done() @@ -72,6 +77,8 @@ func NewCmdGetRollout(o *options.ArgoRolloutsOptions) *cobra.Command { } go getOptions.WatchRollout(stopCh, rolloutUpdates) controller.Run(ctx) + rolloutUpdatesMutex.Lock() + defer rolloutUpdatesMutex.Unlock() close(rolloutUpdates) } return nil diff --git a/pkg/kubectl-argo-rollouts/viewcontroller/viewcontroller.go b/pkg/kubectl-argo-rollouts/viewcontroller/viewcontroller.go index 4aafb0a32a..79dc728428 100644 --- a/pkg/kubectl-argo-rollouts/viewcontroller/viewcontroller.go +++ b/pkg/kubectl-argo-rollouts/viewcontroller/viewcontroller.go @@ -3,6 +3,7 @@ package viewcontroller import ( "context" "reflect" + "sync" "time" "github.com/argoproj/argo-rollouts/utils/queue" @@ -11,7 +12,6 @@ import ( v1 "k8s.io/api/apps/v1" "k8s.io/apimachinery/pkg/labels" "k8s.io/apimachinery/pkg/util/wait" - "k8s.io/client-go/informers" kubeinformers "k8s.io/client-go/informers" "k8s.io/client-go/kubernetes" appslisters "k8s.io/client-go/listers/apps/v1" @@ -32,7 +32,7 @@ type viewController struct { name string namespace string - kubeInformerFactory informers.SharedInformerFactory + kubeInformerFactory kubeinformers.SharedInformerFactory rolloutsInformerFactory rolloutinformers.SharedInformerFactory replicaSetLister appslisters.ReplicaSetNamespaceLister @@ -48,6 +48,8 @@ type viewController struct { prevObj any getObj func() (any, error) callbacks []func(any) + // acquire 'callbacksLock' before reading/writing to 'callbacks' + callbacksLock sync.Mutex } type RolloutViewController struct { @@ -164,7 +166,13 @@ func (c *viewController) processNextWorkItem() bool { return true } if !reflect.DeepEqual(c.prevObj, newObj) { - for _, cb := range c.callbacks { + + // Acquire the mutex and make a thread-local copy of the list of callbacks + c.callbacksLock.Lock() + callbacks := append(make([]func(any), 0), c.callbacks...) + c.callbacksLock.Unlock() + + for _, cb := range callbacks { cb(newObj) } c.prevObj = newObj @@ -173,6 +181,9 @@ func (c *viewController) processNextWorkItem() bool { } func (c *viewController) DeregisterCallbacks() { + c.callbacksLock.Lock() + defer c.callbacksLock.Unlock() + c.callbacks = nil } @@ -218,6 +229,9 @@ func (c *RolloutViewController) RegisterCallback(callback RolloutInfoCallback) { cb := func(i any) { callback(i.(*rollout.RolloutInfo)) } + c.callbacksLock.Lock() + defer c.callbacksLock.Unlock() + c.callbacks = append(c.callbacks, cb) } @@ -246,5 +260,7 @@ func (c *ExperimentViewController) RegisterCallback(callback ExperimentInfoCallb cb := func(i any) { callback(i.(*rollout.ExperimentInfo)) } + c.callbacksLock.Lock() + defer c.callbacksLock.Unlock() c.callbacks = append(c.callbacks, cb) } diff --git a/pkg/kubectl-argo-rollouts/viewcontroller/viewcontroller_test.go b/pkg/kubectl-argo-rollouts/viewcontroller/viewcontroller_test.go index 90752629c2..7e1dd9c30e 100644 --- a/pkg/kubectl-argo-rollouts/viewcontroller/viewcontroller_test.go +++ b/pkg/kubectl-argo-rollouts/viewcontroller/viewcontroller_test.go @@ -2,6 +2,7 @@ package viewcontroller import ( "context" + "sync" "testing" "time" @@ -53,7 +54,11 @@ func TestRolloutControllerCallback(t *testing.T) { } callbackCalled := false + var callbackCalledLock sync.Mutex // acquire before accessing callbackCalled + cb := func(roInfo *rollout.RolloutInfo) { + callbackCalledLock.Lock() + defer callbackCalledLock.Unlock() callbackCalled = true assert.Equal(t, roInfo.ObjectMeta.Name, "foo") } @@ -67,11 +72,16 @@ func TestRolloutControllerCallback(t *testing.T) { go c.Run(ctx) time.Sleep(time.Second) for i := 0; i < 100; i++ { - if callbackCalled { + callbackCalledLock.Lock() + isCallbackCalled := callbackCalled + callbackCalledLock.Unlock() + if isCallbackCalled { break } time.Sleep(10 * time.Millisecond) } + callbackCalledLock.Lock() + defer callbackCalledLock.Unlock() assert.True(t, callbackCalled) } @@ -100,8 +110,11 @@ func TestExperimentControllerCallback(t *testing.T) { }, } + var callbackCalledLock sync.Mutex // acquire before accessing callbackCalled callbackCalled := false cb := func(expInfo *rollout.ExperimentInfo) { + callbackCalledLock.Lock() + defer callbackCalledLock.Unlock() callbackCalled = true assert.Equal(t, expInfo.ObjectMeta.Name, "foo") } @@ -115,10 +128,15 @@ func TestExperimentControllerCallback(t *testing.T) { go c.Run(ctx) time.Sleep(time.Second) for i := 0; i < 100; i++ { - if callbackCalled { + callbackCalledLock.Lock() + isCallbackCalled := callbackCalled + callbackCalledLock.Unlock() + if isCallbackCalled { break } time.Sleep(10 * time.Millisecond) } + callbackCalledLock.Lock() + defer callbackCalledLock.Unlock() assert.True(t, callbackCalled) } diff --git a/rollout/canary_test.go b/rollout/canary_test.go index f89f47494f..0d2065a8db 100644 --- a/rollout/canary_test.go +++ b/rollout/canary_test.go @@ -1156,6 +1156,8 @@ func TestSyncRolloutWaitAddToQueue(t *testing.T) { f.runController(key, true, false, c, i, k8sI) // When the controller starts, it will enqueue the rollout while syncing the informer and during the reconciliation step + f.enqueuedObjectsLock.Lock() + defer f.enqueuedObjectsLock.Unlock() assert.Equal(t, 2, f.enqueuedObjects[key]) } @@ -1204,6 +1206,8 @@ func TestSyncRolloutIgnoreWaitOutsideOfReconciliationPeriod(t *testing.T) { c, i, k8sI := f.newController(func() time.Duration { return 30 * time.Minute }) f.runController(key, true, false, c, i, k8sI) // When the controller starts, it will enqueue the rollout so we expect the rollout to enqueue at least once. + f.enqueuedObjectsLock.Lock() + defer f.enqueuedObjectsLock.Unlock() assert.Equal(t, 1, f.enqueuedObjects[key]) } diff --git a/rollout/controller_test.go b/rollout/controller_test.go index a637d68f29..1409e675e7 100644 --- a/rollout/controller_test.go +++ b/rollout/controller_test.go @@ -99,10 +99,12 @@ type fixture struct { kubeactions []core.Action actions []core.Action // Objects from here preloaded into NewSimpleFake. - kubeobjects []runtime.Object - objects []runtime.Object - enqueuedObjects map[string]int - unfreezeTime func() error + kubeobjects []runtime.Object + objects []runtime.Object + // Acquire 'enqueuedObjectsLock' before accessing enqueuedObjects + enqueuedObjects map[string]int + enqueuedObjectsLock sync.Mutex + unfreezeTime func() error // events holds all the K8s Event Reasons emitted during the run events []string @@ -116,9 +118,12 @@ func newFixture(t *testing.T) *fixture { f.kubeobjects = []runtime.Object{} f.enqueuedObjects = make(map[string]int) now := time.Now() - timeutil.Now = func() time.Time { return now } + + timeutil.SetNowTimeFunc(func() time.Time { + return now + }) f.unfreezeTime = func() error { - timeutil.Now = time.Now + timeutil.SetNowTimeFunc(time.Now) return nil } @@ -598,15 +603,15 @@ func (f *fixture) newController(resync resyncFunc) (*Controller, informers.Share RefResolver: &FakeWorkloadRefResolver{}, }) - var enqueuedObjectsLock sync.Mutex c.enqueueRollout = func(obj any) { var key string var err error if key, err = cache.MetaNamespaceKeyFunc(obj); err != nil { panic(err) } - enqueuedObjectsLock.Lock() - defer enqueuedObjectsLock.Unlock() + + f.enqueuedObjectsLock.Lock() + defer f.enqueuedObjectsLock.Unlock() count, ok := f.enqueuedObjects[key] if !ok { count = 0 @@ -720,7 +725,8 @@ func (f *fixture) runController(rolloutName string, startInformers bool, expectE f.t.Errorf("%d expected actions did not happen:%+v", len(f.kubeactions)-len(k8sActions), f.kubeactions[len(k8sActions):]) } fakeRecorder := c.recorder.(*record.FakeEventRecorder) - f.events = fakeRecorder.Events + + f.events = fakeRecorder.Events() return c } diff --git a/rollout/sync_test.go b/rollout/sync_test.go index a77fc358cb..c29b954804 100644 --- a/rollout/sync_test.go +++ b/rollout/sync_test.go @@ -455,7 +455,7 @@ func TestSendStateChangeEvents(t *testing.T) { recorder := record.NewFakeEventRecorder() roCtx.recorder = recorder roCtx.sendStateChangeEvents(&test.prevStatus, &test.newStatus) - assert.Equal(t, test.expectedEventReasons, recorder.Events) + assert.Equal(t, test.expectedEventReasons, recorder.Events()) } } diff --git a/rollout/trafficrouting/ambassador/ambassador_test.go b/rollout/trafficrouting/ambassador/ambassador_test.go index bd6e4613de..66d193b83a 100644 --- a/rollout/trafficrouting/ambassador/ambassador_test.go +++ b/rollout/trafficrouting/ambassador/ambassador_test.go @@ -136,8 +136,9 @@ type getReturn struct { func (f *fakeClient) Get(ctx context.Context, name string, options metav1.GetOptions, subresources ...string) (*unstructured.Unstructured, error) { invokation := &getInvokation{name: name} f.mu.Lock() + defer f.mu.Unlock() f.getInvokations = append(f.getInvokations, invokation) - f.mu.Unlock() + if len(f.getReturns) == 0 { return nil, nil } @@ -145,7 +146,8 @@ func (f *fakeClient) Get(ctx context.Context, name string, options metav1.GetOpt if len(f.getReturns) >= len(f.getInvokations) { ret = f.getReturns[len(f.getInvokations)-1] } - return ret.obj, ret.err + // We clone the object before returning it, to prevent modification of the fake object in memory by the calling function + return ret.obj.DeepCopy(), ret.err } func (f *fakeClient) Create(ctx context.Context, obj *unstructured.Unstructured, options metav1.CreateOptions, subresources ...string) (*unstructured.Unstructured, error) { diff --git a/rollout/trafficrouting/apisix/apisix_test.go b/rollout/trafficrouting/apisix/apisix_test.go index 823e3bfb10..3749e64d6d 100644 --- a/rollout/trafficrouting/apisix/apisix_test.go +++ b/rollout/trafficrouting/apisix/apisix_test.go @@ -116,10 +116,6 @@ spec: priority: 2 ` -var ( - client *mocks.FakeClient = &mocks.FakeClient{} -) - const ( stableServiceName string = "stable-rollout" fakeStableServiceName string = "fake-stable-rollout" @@ -135,7 +131,7 @@ func TestUpdateHash(t *testing.T) { t.Parallel() cfg := ReconcilerConfig{ Rollout: newRollout(stableServiceName, canaryServiceName, apisixRouteName), - Client: client, + Client: &mocks.FakeClient{}, } r := NewReconciler(&cfg) @@ -152,10 +148,9 @@ func TestSetWeight(t *testing.T) { mocks.ErrorApisixRouteObj = toUnstructured(t, errorApisixRoute) t.Run("SetWeight", func(t *testing.T) { // Given - t.Parallel() cfg := ReconcilerConfig{ Rollout: newRollout(stableServiceName, canaryServiceName, apisixRouteName), - Client: client, + Client: &mocks.FakeClient{}, } r := NewReconciler(&cfg) @@ -195,7 +190,6 @@ func TestSetWeight(t *testing.T) { }) t.Run("SetWeightWithError", func(t *testing.T) { // Given - t.Parallel() cfg := ReconcilerConfig{ Rollout: newRollout(stableServiceName, canaryServiceName, apisixRouteName), Client: &mocks.FakeClient{ @@ -212,7 +206,6 @@ func TestSetWeight(t *testing.T) { }) t.Run("SetWeightWithErrorManifest", func(t *testing.T) { // Given - t.Parallel() cfg := ReconcilerConfig{ Rollout: newRollout(stableServiceName, canaryServiceName, apisixRouteName), Client: &mocks.FakeClient{ @@ -229,10 +222,9 @@ func TestSetWeight(t *testing.T) { }) t.Run("SetWeightWithErrorStableName", func(t *testing.T) { // Given - t.Parallel() cfg := ReconcilerConfig{ Rollout: newRollout(fakeStableServiceName, canaryServiceName, apisixRouteName), - Client: client, + Client: &mocks.FakeClient{}, } r := NewReconciler(&cfg) @@ -244,10 +236,9 @@ func TestSetWeight(t *testing.T) { }) t.Run("SetWeightWithErrorCanaryName", func(t *testing.T) { // Given - t.Parallel() cfg := ReconcilerConfig{ Rollout: newRollout(stableServiceName, fakeCanaryServiceName, apisixRouteName), - Client: client, + Client: &mocks.FakeClient{}, } r := NewReconciler(&cfg) @@ -259,7 +250,6 @@ func TestSetWeight(t *testing.T) { }) t.Run("ApisixUpdateError", func(t *testing.T) { // Given - t.Parallel() cfg := ReconcilerConfig{ Rollout: newRollout(stableServiceName, canaryServiceName, apisixRouteName), Client: &mocks.FakeClient{ @@ -380,7 +370,6 @@ func TestSetHeaderRoute(t *testing.T) { mocks.DuplicateSetHeaderApisixRouteObj = toUnstructured(t, apisixSetHeaderDuplicateRoute) mocks.ErrorApisixRouteObj = toUnstructured(t, errorApisixRoute) t.Run("SetHeaderGetRouteError", func(t *testing.T) { - t.Parallel() cfg := ReconcilerConfig{ Rollout: newRollout(stableServiceName, canaryServiceName, apisixRouteName), Client: &mocks.FakeClient{ @@ -397,7 +386,6 @@ func TestSetHeaderRoute(t *testing.T) { assert.Error(t, err) }) t.Run("SetHeaderGetManagedRouteError", func(t *testing.T) { - t.Parallel() cfg := ReconcilerConfig{ Rollout: newRollout(stableServiceName, canaryServiceName, apisixRouteName), Client: &mocks.FakeClient{ @@ -420,7 +408,6 @@ func TestSetHeaderRoute(t *testing.T) { assert.Error(t, err) }) t.Run("SetHeaderDuplicateManagedRouteError", func(t *testing.T) { - t.Parallel() cfg := ReconcilerConfig{ Rollout: newRollout(stableServiceName, canaryServiceName, apisixRouteName), Client: &mocks.FakeClient{ @@ -445,7 +432,6 @@ func TestSetHeaderRoute(t *testing.T) { }) t.Run("SetHeaderRouteNilMatchWithNew", func(t *testing.T) { // Given - t.Parallel() cfg := ReconcilerConfig{ Rollout: newRollout(stableServiceName, canaryServiceName, apisixRouteName), Client: &mocks.FakeClient{ @@ -466,7 +452,6 @@ func TestSetHeaderRoute(t *testing.T) { t.Run("SetHeaderRouteNilMatch", func(t *testing.T) { client := &mocks.FakeClient{} // Given - t.Parallel() cfg := ReconcilerConfig{ Rollout: newRollout(stableServiceName, canaryServiceName, apisixRouteName), Client: client, @@ -485,7 +470,6 @@ func TestSetHeaderRoute(t *testing.T) { }) t.Run("SetHeaderRoutePriorityWithNew", func(t *testing.T) { // Given - t.Parallel() client := &mocks.FakeClient{ IsGetNotFoundError: true, } @@ -521,7 +505,6 @@ func TestSetHeaderRoute(t *testing.T) { }) t.Run("SetHeaderRoutePriorityWithNew", func(t *testing.T) { // Given - t.Parallel() client := &mocks.FakeClient{ IsGetNotFoundError: false, } @@ -558,7 +541,6 @@ func TestSetHeaderRoute(t *testing.T) { t.Run("SetHeaderRouteExprsWithNew", func(t *testing.T) { // Given - t.Parallel() client := &mocks.FakeClient{ IsGetNotFoundError: true, } @@ -613,7 +595,6 @@ func TestSetHeaderRoute(t *testing.T) { }) t.Run("SetHeaderRouteExprs", func(t *testing.T) { // Given - t.Parallel() client := &mocks.FakeClient{ IsGetNotFoundError: false, } @@ -668,7 +649,6 @@ func TestSetHeaderRoute(t *testing.T) { }) t.Run("SetHeaderDeleteError", func(t *testing.T) { // Given - t.Parallel() client := &mocks.FakeClient{ IsDeleteError: true, } @@ -686,7 +666,6 @@ func TestSetHeaderRoute(t *testing.T) { }) t.Run("SetHeaderCreateError", func(t *testing.T) { // Given - t.Parallel() client := &mocks.FakeClient{ IsCreateError: true, IsGetNotFoundError: true, @@ -710,7 +689,6 @@ func TestSetHeaderRoute(t *testing.T) { }) t.Run("SetHeaderUpdateError", func(t *testing.T) { // Given - t.Parallel() client := &mocks.FakeClient{ UpdateError: true, IsGetNotFoundError: false, @@ -734,7 +712,6 @@ func TestSetHeaderRoute(t *testing.T) { }) t.Run("RemoveManagedRoutesDeleteError", func(t *testing.T) { // Given - t.Parallel() client := &mocks.FakeClient{ IsDeleteError: true, } @@ -749,7 +726,6 @@ func TestSetHeaderRoute(t *testing.T) { }) t.Run("RemoveManagedRoutesNilManagedRoutes", func(t *testing.T) { // Given - t.Parallel() client := &mocks.FakeClient{ IsDeleteError: true, } @@ -799,7 +775,7 @@ func TestSetMirrorRoute(t *testing.T) { t.Parallel() cfg := ReconcilerConfig{ Rollout: newRollout(stableServiceName, canaryServiceName, apisixRouteName), - Client: client, + Client: &mocks.FakeClient{}, } r := NewReconciler(&cfg) @@ -826,7 +802,6 @@ func TestRemoveManagedRoutes(t *testing.T) { t.Run("RemoveManagedRoutes", func(t *testing.T) { client := &mocks.FakeClient{} // Given - t.Parallel() cfg := ReconcilerConfig{ Rollout: newRollout(stableServiceName, canaryServiceName, apisixRouteName), Client: client, @@ -842,7 +817,6 @@ func TestRemoveManagedRoutes(t *testing.T) { IsGetManagedRouteError: true, } // Given - t.Parallel() cfg := ReconcilerConfig{ Rollout: newRollout(stableServiceName, canaryServiceName, apisixRouteName), Client: client, @@ -858,7 +832,6 @@ func TestRemoveManagedRoutes(t *testing.T) { IsGetNotFoundError: true, } // Given - t.Parallel() cfg := ReconcilerConfig{ Rollout: newRollout(stableServiceName, canaryServiceName, apisixRouteName), Client: client, @@ -889,7 +862,7 @@ func TestVerifyWeight(t *testing.T) { t.Parallel() cfg := ReconcilerConfig{ Rollout: newRollout(stableServiceName, canaryServiceName, apisixRouteName), - Client: client, + Client: &mocks.FakeClient{}, } r := NewReconciler(&cfg) @@ -906,10 +879,9 @@ func TestType(t *testing.T) { mocks.ApisixRouteObj = toUnstructured(t, apisixRoute) t.Run("Type", func(t *testing.T) { // Given - t.Parallel() cfg := ReconcilerConfig{ Rollout: newRollout(stableServiceName, canaryServiceName, apisixRouteName), - Client: client, + Client: &mocks.FakeClient{}, } r := NewReconciler(&cfg) diff --git a/rollout/trafficrouting/traefik/traefik_test.go b/rollout/trafficrouting/traefik/traefik_test.go index 892468df0a..0b5b0c9701 100644 --- a/rollout/trafficrouting/traefik/traefik_test.go +++ b/rollout/trafficrouting/traefik/traefik_test.go @@ -38,10 +38,6 @@ metadata: name: mocks-service ` -var ( - client *mocks.FakeClient = &mocks.FakeClient{} -) - const ( stableServiceName string = "stable-rollout" fakeStableServiceName string = "fake-stable-rollout" @@ -67,7 +63,7 @@ func TestUpdateHash(t *testing.T) { t.Parallel() cfg := ReconcilerConfig{ Rollout: newRollout(stableServiceName, canaryServiceName, traefikServiceName), - Client: client, + Client: &mocks.FakeClient{}, } r := NewReconciler(&cfg) @@ -84,10 +80,9 @@ func TestSetWeight(t *testing.T) { mocks.ErrorTraefikServiceObj = toUnstructured(t, errorTraefikService) t.Run("SetWeight", func(t *testing.T) { // Given - t.Parallel() cfg := ReconcilerConfig{ Rollout: newRollout(stableServiceName, canaryServiceName, traefikServiceName), - Client: client, + Client: &mocks.FakeClient{}, } r := NewReconciler(&cfg) @@ -114,7 +109,6 @@ func TestSetWeight(t *testing.T) { }) t.Run("SetWeightWithError", func(t *testing.T) { // Given - t.Parallel() cfg := ReconcilerConfig{ Rollout: newRollout(stableServiceName, canaryServiceName, traefikServiceName), Client: &mocks.FakeClient{ @@ -131,7 +125,6 @@ func TestSetWeight(t *testing.T) { }) t.Run("SetWeightWithErrorManifest", func(t *testing.T) { // Given - t.Parallel() cfg := ReconcilerConfig{ Rollout: newRollout(stableServiceName, canaryServiceName, traefikServiceName), Client: &mocks.FakeClient{ @@ -148,10 +141,9 @@ func TestSetWeight(t *testing.T) { }) t.Run("SetWeightWithErrorStableName", func(t *testing.T) { // Given - t.Parallel() cfg := ReconcilerConfig{ Rollout: newRollout(fakeStableServiceName, canaryServiceName, traefikServiceName), - Client: client, + Client: &mocks.FakeClient{}, } r := NewReconciler(&cfg) @@ -163,10 +155,9 @@ func TestSetWeight(t *testing.T) { }) t.Run("SetWeightWithErrorCanaryName", func(t *testing.T) { // Given - t.Parallel() cfg := ReconcilerConfig{ Rollout: newRollout(stableServiceName, fakeCanaryServiceName, traefikServiceName), - Client: client, + Client: &mocks.FakeClient{}, } r := NewReconciler(&cfg) @@ -178,7 +169,6 @@ func TestSetWeight(t *testing.T) { }) t.Run("TraefikUpdateError", func(t *testing.T) { // Given - t.Parallel() cfg := ReconcilerConfig{ Rollout: newRollout(stableServiceName, canaryServiceName, traefikServiceName), Client: &mocks.FakeClient{ @@ -202,7 +192,7 @@ func TestSetHeaderRoute(t *testing.T) { t.Parallel() cfg := ReconcilerConfig{ Rollout: newRollout(stableServiceName, canaryServiceName, traefikServiceName), - Client: client, + Client: &mocks.FakeClient{}, } r := NewReconciler(&cfg) @@ -231,7 +221,7 @@ func TestSetMirrorRoute(t *testing.T) { t.Parallel() cfg := ReconcilerConfig{ Rollout: newRollout(stableServiceName, canaryServiceName, traefikServiceName), - Client: client, + Client: &mocks.FakeClient{}, } r := NewReconciler(&cfg) @@ -269,7 +259,7 @@ func TestVerifyWeight(t *testing.T) { t.Parallel() cfg := ReconcilerConfig{ Rollout: newRollout(stableServiceName, canaryServiceName, traefikServiceName), - Client: client, + Client: &mocks.FakeClient{}, } r := NewReconciler(&cfg) @@ -289,7 +279,7 @@ func TestType(t *testing.T) { t.Parallel() cfg := ReconcilerConfig{ Rollout: newRollout(stableServiceName, canaryServiceName, traefikServiceName), - Client: client, + Client: &mocks.FakeClient{}, } r := NewReconciler(&cfg) diff --git a/utils/record/record.go b/utils/record/record.go index 801db089b6..6b4f6bfa16 100644 --- a/utils/record/record.go +++ b/utils/record/record.go @@ -9,6 +9,7 @@ import ( "regexp" "sort" "strings" + "sync" "time" argoinformers "github.com/argoproj/argo-rollouts/pkg/client/informers/externalversions/rollouts/v1alpha1" @@ -107,7 +108,29 @@ func NewEventRecorder(kubeclientset kubernetes.Interface, rolloutEventCounter *p // reasons which were emitted type FakeEventRecorder struct { EventRecorderAdapter - Events []string + // acquire eventsLock before using events + events []string + eventsLock sync.Mutex +} + +func (e *FakeEventRecorder) appendEvents(events ...string) { + e.eventsLock.Lock() + defer e.eventsLock.Unlock() + + e.events = append(e.events, events...) +} + +// Events returns a list of received events, with thread safety +func (e *FakeEventRecorder) Events() []string { + + e.eventsLock.Lock() + defer e.eventsLock.Unlock() + + if e.events == nil { + return nil + } + + return append(make([]string, 0), e.events...) } func NewFakeApiFactory() api.Factory { @@ -178,7 +201,7 @@ func NewFakeEventRecorder() *FakeEventRecorder { fakeRecorder := &FakeEventRecorder{} recorder.eventf = func(object runtime.Object, warn bool, opts EventOptions, messageFmt string, args ...any) { recorder.defaultEventf(object, warn, opts, messageFmt, args...) - fakeRecorder.Events = append(fakeRecorder.Events, opts.EventReason) + fakeRecorder.appendEvents(opts.EventReason) } fakeRecorder.EventRecorderAdapter = *recorder return fakeRecorder diff --git a/utils/record/record_test.go b/utils/record/record_test.go index 2cc7afd12c..ab8f3227f0 100644 --- a/utils/record/record_test.go +++ b/utils/record/record_test.go @@ -97,7 +97,7 @@ func TestIncCounter(t *testing.T) { buf := dto.Metric{} m.Write(&buf) assert.Equal(t, float64(3), *buf.Counter.Value) - assert.Equal(t, []string{"FooReason", "FooReason", "FooReason"}, rec.Events) + assert.Equal(t, []string{"FooReason", "FooReason", "FooReason"}, rec.Events()) } func TestSendNotifications(t *testing.T) { diff --git a/utils/time/now.go b/utils/time/now.go index 1b51cb3cc0..87952a91b4 100644 --- a/utils/time/now.go +++ b/utils/time/now.go @@ -1,13 +1,36 @@ package time import ( + "sync" "time" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ) +var ( + timeNowFunc = time.Now + + // Acquire this mutex when accessing now function + nowLock sync.RWMutex +) + // Now is a wrapper around time.Now() and used to override behavior in tests. -var Now = time.Now +// Now invokes time.Now(), or its replacement function +func Now() time.Time { + nowLock.RLock() + defer nowLock.RUnlock() + + return timeNowFunc() +} + +// Replace the function used to return the current time (defaults to time.Now() ) +func SetNowTimeFunc(f func() time.Time) { + nowLock.Lock() + defer nowLock.Unlock() + + timeNowFunc = f + +} // MetaNow is a wrapper around metav1.Now() and used to override behavior in tests. var MetaNow = func() metav1.Time {