Skip to content

Commit

Permalink
Fix more flaky periodic job enqueuer *AfterStart tests (#422)
Browse files Browse the repository at this point in the history
This one continues #416. I fixed a pair of the `*AfterStart` tests, but
didn't look further to see that there were three more, and I guess by a
stroke of luck I was running GitHub Actions at the right time and didn't
encounter anymore failures (today I immediately started seeing them
again when pushing).

Rationale there is similar to #416:

* 500 ms is a short enough period that it seems to be possible in GitHub
  Actions for that amount of time to have elapsed by the time we check
  for the first set of jobs (amazing, I know, but I can't explain it any
  other way).

* The way the tests are written, they're also very slow because each
  requires waiting for one of the 500 ms periods to elapse, so the
  minimum run time of each is 500 ms.

Technically, with the rewrites it can be argued that the test cases are
checking slightly less, but IMO it's good enough, and I was having
trouble thinking of alternative ways to add additional checks without
raciness.

Fixes #386.
  • Loading branch information
brandur authored Jul 4, 2024
1 parent d5b5960 commit 7e1e269
Showing 1 changed file with 19 additions and 31 deletions.
50 changes: 19 additions & 31 deletions internal/maintenance/periodic_job_enqueuer_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -401,28 +401,25 @@ func TestPeriodicJobEnqueuer(t *testing.T) {
startService(t, svc)

handles := svc.AddMany([]*PeriodicJob{
{ScheduleFunc: periodicIntervalSchedule(500 * time.Millisecond), ConstructorFunc: jobConstructorFunc("periodic_job_500ms", false)},
{ScheduleFunc: periodicIntervalSchedule(500 * time.Millisecond), ConstructorFunc: jobConstructorFunc("periodic_job_500ms_start", false), RunOnStart: true},
{ScheduleFunc: periodicIntervalSchedule(5 * time.Second), ConstructorFunc: jobConstructorFunc("periodic_job_5s", false)},
{ScheduleFunc: periodicIntervalSchedule(5 * time.Second), ConstructorFunc: jobConstructorFunc("periodic_job_5s_start", false), RunOnStart: true},
})

svc.TestSignals.InsertedJobs.WaitOrTimeout()
requireNJobs(t, bundle.exec, "periodic_job_500ms", 0)
requireNJobs(t, bundle.exec, "periodic_job_500ms_start", 1)
requireNJobs(t, bundle.exec, "periodic_job_5s", 0)
requireNJobs(t, bundle.exec, "periodic_job_5s_start", 1)

svc.Clear()

require.Empty(t, svc.periodicJobs)

handleAfterClear := svc.Add(
&PeriodicJob{ScheduleFunc: periodicIntervalSchedule(500 * time.Millisecond), ConstructorFunc: jobConstructorFunc("periodic_job_500ms_new", false)},
&PeriodicJob{ScheduleFunc: periodicIntervalSchedule(5 * time.Second), ConstructorFunc: jobConstructorFunc("periodic_job_5s_new", false)},
)

// Handles are not reused.
require.NotEqual(t, handles[0], handleAfterClear)
require.NotEqual(t, handles[1], handleAfterClear)

svc.TestSignals.InsertedJobs.WaitOrTimeout()
requireNJobs(t, bundle.exec, "periodic_job_500ms", 0) // same as before
requireNJobs(t, bundle.exec, "periodic_job_500ms_start", 1) // same as before
requireNJobs(t, bundle.exec, "periodic_job_500ms_new", 1) // new row
})

t.Run("RemoveAfterStart", func(t *testing.T) {
Expand All @@ -433,21 +430,17 @@ func TestPeriodicJobEnqueuer(t *testing.T) {
startService(t, svc)

handles := svc.AddMany([]*PeriodicJob{
{ScheduleFunc: periodicIntervalSchedule(500 * time.Millisecond), ConstructorFunc: jobConstructorFunc("periodic_job_500ms", false)},
{ScheduleFunc: periodicIntervalSchedule(500 * time.Millisecond), ConstructorFunc: jobConstructorFunc("periodic_job_500ms_start", false), RunOnStart: true},
{ScheduleFunc: periodicIntervalSchedule(5 * time.Second), ConstructorFunc: jobConstructorFunc("periodic_job_5s", false)},
{ScheduleFunc: periodicIntervalSchedule(5 * time.Second), ConstructorFunc: jobConstructorFunc("periodic_job_5s_start", false), RunOnStart: true},
})

svc.TestSignals.InsertedJobs.WaitOrTimeout()
requireNJobs(t, bundle.exec, "periodic_job_500ms", 0)
requireNJobs(t, bundle.exec, "periodic_job_500ms_start", 1)
requireNJobs(t, bundle.exec, "periodic_job_5s", 0)
requireNJobs(t, bundle.exec, "periodic_job_5s_start", 1)

svc.Remove(handles[1])

// Each is one because the second job was removed before it was worked
// again.
svc.TestSignals.InsertedJobs.WaitOrTimeout()
requireNJobs(t, bundle.exec, "periodic_job_500ms", 1)
requireNJobs(t, bundle.exec, "periodic_job_500ms_start", 1)
require.Len(t, svc.periodicJobs, 1)
})

t.Run("RemoveManyAfterStart", func(t *testing.T) {
Expand All @@ -458,24 +451,19 @@ func TestPeriodicJobEnqueuer(t *testing.T) {
startService(t, svc)

handles := svc.AddMany([]*PeriodicJob{
{ScheduleFunc: periodicIntervalSchedule(500 * time.Millisecond), ConstructorFunc: jobConstructorFunc("periodic_job_500ms", false)},
{ScheduleFunc: periodicIntervalSchedule(500 * time.Millisecond), ConstructorFunc: jobConstructorFunc("periodic_job_500ms_other", false)},
{ScheduleFunc: periodicIntervalSchedule(500 * time.Millisecond), ConstructorFunc: jobConstructorFunc("periodic_job_500ms_start", false), RunOnStart: true},
{ScheduleFunc: periodicIntervalSchedule(5 * time.Second), ConstructorFunc: jobConstructorFunc("periodic_job_5s", false)},
{ScheduleFunc: periodicIntervalSchedule(5 * time.Second), ConstructorFunc: jobConstructorFunc("periodic_job_5s_other", false)},
{ScheduleFunc: periodicIntervalSchedule(5 * time.Second), ConstructorFunc: jobConstructorFunc("periodic_job_5s_start", false), RunOnStart: true},
})

svc.TestSignals.InsertedJobs.WaitOrTimeout()
requireNJobs(t, bundle.exec, "periodic_job_500ms", 0)
requireNJobs(t, bundle.exec, "periodic_job_500ms_other", 0)
requireNJobs(t, bundle.exec, "periodic_job_500ms_start", 1)
requireNJobs(t, bundle.exec, "periodic_job_5s", 0)
requireNJobs(t, bundle.exec, "periodic_job_5s_other", 0)
requireNJobs(t, bundle.exec, "periodic_job_5s_start", 1)

svc.RemoveMany([]rivertype.PeriodicJobHandle{handles[1], handles[2]})

// Each is one because the second job was removed before it was worked
// again.
svc.TestSignals.InsertedJobs.WaitOrTimeout()
requireNJobs(t, bundle.exec, "periodic_job_500ms", 1)
requireNJobs(t, bundle.exec, "periodic_job_500ms_other", 0)
requireNJobs(t, bundle.exec, "periodic_job_500ms_start", 1)
require.Len(t, svc.periodicJobs, 1)
})

// To suss out any race conditions in the add/remove/clear/run loop code,
Expand Down

0 comments on commit 7e1e269

Please sign in to comment.