Skip to content

Commit

Permalink
chore: fix race condition in cron tests (#1369)
Browse files Browse the repository at this point in the history
mockDal's `attemptCountMap` was being read without getting a lock
  • Loading branch information
matt2e authored May 1, 2024
1 parent 8816101 commit 5990462
Show file tree
Hide file tree
Showing 2 changed files with 6 additions and 2 deletions.
6 changes: 4 additions & 2 deletions backend/controller/cronjobs/cronjobs_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@ import (
)

func TestServiceWithMockDal(t *testing.T) {
t.Skip("TODO(matt2e): Fails -race, fix me")
t.Parallel()
ctx := log.ContextWithNewDefaultLogger(context.Background())
ctx, cancel := context.WithCancel(ctx)
Expand All @@ -40,7 +39,6 @@ func TestServiceWithMockDal(t *testing.T) {
}

func TestHashRing(t *testing.T) {
t.Skip("TODO(matt2e): Fails -race, fix me")
// This test uses multiple mock clocks to progress time for each controller individually
// This allows us to compare attempts for each cron job and know which controller attempted it
t.Parallel()
Expand Down Expand Up @@ -82,20 +80,24 @@ func TestHashRing(t *testing.T) {
// to build a map of verb to controller keys
controllersForVerbs := map[string][]model.ControllerKey{}
for _, c := range controllers {
mockDal.lock.Lock()
beforeAttemptCount := map[string]int{}
for k, v := range mockDal.attemptCountMap {
beforeAttemptCount[k] = v
}
mockDal.lock.Unlock()

c.mockClock.Add(time.Second * 15)
time.Sleep(time.Millisecond * 100)

mockDal.lock.Lock()
for k, v := range mockDal.attemptCountMap {
if beforeAttemptCount[k] == v {
continue
}
controllersForVerbs[k] = append(controllersForVerbs[k], c.key)
}
mockDal.lock.Unlock()
}

// Check if each job has the same key list
Expand Down
2 changes: 2 additions & 0 deletions backend/controller/cronjobs/cronjobs_utils_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -252,8 +252,10 @@ func testServiceWithDal(ctx context.Context, t *testing.T, dal ExtendedDAL, clk
time.Sleep(time.Second * 2 * 3)
}

verbCallCountLock.Lock()
for _, j := range jobsToCreate {
count := verbCallCount[j.Verb.Name]
assert.Equal(t, count, 3, "expected verb %s to be called 3 times", j.Verb.Name)
}
verbCallCountLock.Unlock()
}

0 comments on commit 5990462

Please sign in to comment.