From 4a57125579a078630be7d228e3a27830365dff1a Mon Sep 17 00:00:00 2001 From: John Roesler Date: Thu, 9 Nov 2023 15:04:18 -0600 Subject: [PATCH] test all remaining With*, and test logger (#609) --- CONTRIBUTING.md | 2 - errors.go | 62 +++++------ executor.go | 42 ++++---- logger.go | 16 +++ logger_test.go | 77 ++++++++++++++ scheduler.go | 36 ++++--- scheduler_test.go | 256 +++++++++++++++++++++++++++++++++++++++++++--- 7 files changed, 414 insertions(+), 77 deletions(-) create mode 100644 logger_test.go diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index b2d3be8..99e1e88 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -32,8 +32,6 @@ Please always make sure a pull request has been: - Unit tested with `make test` - Linted with `make lint` -- Vetted with `make vet` -- Formatted with `make fmt` or validated with `make check-fmt` ## Writing Tests diff --git a/errors.go b/errors.go index a331982..b9e4669 100644 --- a/errors.go +++ b/errors.go @@ -4,36 +4,38 @@ import "fmt" // Public error definitions var ( - ErrCronJobParse = fmt.Errorf("gocron: CronJob: crontab parse failure") - ErrDailyJobAtTimeNil = fmt.Errorf("gocron: DailyJob: atTime within atTimes must not be nil") - ErrDailyJobAtTimesNil = fmt.Errorf("gocron: DailyJob: atTimes must not be nil") - ErrDailyJobHours = fmt.Errorf("gocron: DailyJob: atTimes hours must be between 0 and 23 inclusive") - ErrDailyJobMinutesSeconds = fmt.Errorf("gocron: DailyJob: atTimes minutes and seconds must be between 0 and 59 inclusive") - ErrDurationRandomJobMinMax = fmt.Errorf("gocron: DurationRandomJob: minimum duration must be less than maximum duration") - ErrEventListenerFuncNil = fmt.Errorf("gocron: eventListenerFunc must not be nil") - ErrJobNotFound = fmt.Errorf("gocron: job not found") - ErrMonthlyJobDays = fmt.Errorf("gocron: MonthlyJob: daysOfTheMonth must be between 31 and -31 inclusive, and not 0") - ErrMonthlyJobAtTimeNil = fmt.Errorf("gocron: MonthlyJob: atTime within atTimes must not be nil") - ErrMonthlyJobAtTimesNil = fmt.Errorf("gocron: MonthlyJob: atTimes must not be nil") - ErrMonthlyJobDaysNil = fmt.Errorf("gocron: MonthlyJob: daysOfTheMonth must not be nil") - ErrMonthlyJobHours = fmt.Errorf("gocron: MonthlyJob: atTimes hours must be between 0 and 23 inclusive") - ErrMonthlyJobMinutesSeconds = fmt.Errorf("gocron: MonthlyJob: atTimes minutes and seconds must be between 0 and 59 inclusive") - ErrNewJobTask = fmt.Errorf("gocron: NewJob: Task.Function must be of kind reflect.Func") - ErrNewJobTaskNil = fmt.Errorf("gocron: NewJob: Task must not be nil") - ErrStopExecutorTimedOut = fmt.Errorf("gocron: timed out waiting for executor to stop") - ErrStopJobsTimedOut = fmt.Errorf("gocron: timed out waiting for jobs to finish") - ErrStopSchedulerTimedOut = fmt.Errorf("gocron: timed out waiting for scheduler to stop") - ErrWeeklyJobAtTimeNil = fmt.Errorf("gocron: WeeklyJob: atTime within atTimes must not be nil") - ErrWeeklyJobAtTimesNil = fmt.Errorf("gocron: WeeklyJob: atTimes must not be nil") - ErrWeeklyJobDaysOfTheWeekNil = fmt.Errorf("gocron: WeeklyJob: daysOfTheWeek must not be nil") - ErrWeeklyJobHours = fmt.Errorf("gocron: WeeklyJob: atTimes hours must be between 0 and 23 inclusive") - ErrWeeklyJobMinutesSeconds = fmt.Errorf("gocron: WeeklyJob: atTimes minutes and seconds must be between 0 and 59 inclusive") - ErrWithDistributedElector = fmt.Errorf("gocron: WithDistributedElector: elector must not be nil") - ErrWithClockNil = fmt.Errorf("gocron: WithClock: clock must not be nil") - ErrWithLocationNil = fmt.Errorf("gocron: WithLocation: location must not be nil") - ErrWithLoggerNil = fmt.Errorf("gocron: WithLogger: logger must not be nil") - ErrWithNameEmpty = fmt.Errorf("gocron: WithName: name must not be empty") - ErrWithStartDateTimePast = fmt.Errorf("gocron: WithStartDateTime: start must not be in the past") + ErrCronJobParse = fmt.Errorf("gocron: CronJob: crontab parse failure") + ErrDailyJobAtTimeNil = fmt.Errorf("gocron: DailyJob: atTime within atTimes must not be nil") + ErrDailyJobAtTimesNil = fmt.Errorf("gocron: DailyJob: atTimes must not be nil") + ErrDailyJobHours = fmt.Errorf("gocron: DailyJob: atTimes hours must be between 0 and 23 inclusive") + ErrDailyJobMinutesSeconds = fmt.Errorf("gocron: DailyJob: atTimes minutes and seconds must be between 0 and 59 inclusive") + ErrDurationRandomJobMinMax = fmt.Errorf("gocron: DurationRandomJob: minimum duration must be less than maximum duration") + ErrEventListenerFuncNil = fmt.Errorf("gocron: eventListenerFunc must not be nil") + ErrJobNotFound = fmt.Errorf("gocron: job not found") + ErrMonthlyJobDays = fmt.Errorf("gocron: MonthlyJob: daysOfTheMonth must be between 31 and -31 inclusive, and not 0") + ErrMonthlyJobAtTimeNil = fmt.Errorf("gocron: MonthlyJob: atTime within atTimes must not be nil") + ErrMonthlyJobAtTimesNil = fmt.Errorf("gocron: MonthlyJob: atTimes must not be nil") + ErrMonthlyJobDaysNil = fmt.Errorf("gocron: MonthlyJob: daysOfTheMonth must not be nil") + ErrMonthlyJobHours = fmt.Errorf("gocron: MonthlyJob: atTimes hours must be between 0 and 23 inclusive") + ErrMonthlyJobMinutesSeconds = fmt.Errorf("gocron: MonthlyJob: atTimes minutes and seconds must be between 0 and 59 inclusive") + ErrNewJobTaskNil = fmt.Errorf("gocron: NewJob: Task must not be nil") + ErrNewJobTaskNotFunc = fmt.Errorf("gocron: NewJob: Task.Function must be of kind reflect.Func") + ErrStopExecutorTimedOut = fmt.Errorf("gocron: timed out waiting for executor to stop") + ErrStopJobsTimedOut = fmt.Errorf("gocron: timed out waiting for jobs to finish") + ErrStopSchedulerTimedOut = fmt.Errorf("gocron: timed out waiting for scheduler to stop") + ErrWeeklyJobAtTimeNil = fmt.Errorf("gocron: WeeklyJob: atTime within atTimes must not be nil") + ErrWeeklyJobAtTimesNil = fmt.Errorf("gocron: WeeklyJob: atTimes must not be nil") + ErrWeeklyJobDaysOfTheWeekNil = fmt.Errorf("gocron: WeeklyJob: daysOfTheWeek must not be nil") + ErrWeeklyJobHours = fmt.Errorf("gocron: WeeklyJob: atTimes hours must be between 0 and 23 inclusive") + ErrWeeklyJobMinutesSeconds = fmt.Errorf("gocron: WeeklyJob: atTimes minutes and seconds must be between 0 and 59 inclusive") + ErrWithClockNil = fmt.Errorf("gocron: WithClock: clock must not be nil") + ErrWithDistributedElectorNil = fmt.Errorf("gocron: WithDistributedElector: elector must not be nil") + ErrWithLimitConcurrentJobsZero = fmt.Errorf("gocron: WithLimitConcurrentJobs: limit must be greater than 0") + ErrWithLocationNil = fmt.Errorf("gocron: WithLocation: location must not be nil") + ErrWithLoggerNil = fmt.Errorf("gocron: WithLogger: logger must not be nil") + ErrWithNameEmpty = fmt.Errorf("gocron: WithName: name must not be empty") + ErrWithStartDateTimePast = fmt.Errorf("gocron: WithStartDateTime: start must not be in the past") + ErrWithStopTimeoutZeroOrNegative = fmt.Errorf("gocron: WithStopTimeout: timeout must be greater than 0") ) // internal errors diff --git a/executor.go b/executor.go index ea83b7b..9f5bee3 100644 --- a/executor.go +++ b/executor.go @@ -326,30 +326,36 @@ func (e *executor) limitModeRunner(name string, in chan uuid.UUID, wg *waitGroup } func (e *executor) runJob(j internalJob) { + if j.ctx == nil { + return + } select { case <-e.ctx.Done(): + return case <-j.ctx.Done(): + return default: - if e.elector != nil { - if err := e.elector.IsLeader(j.ctx); err != nil { - return - } - } - _ = callJobFuncWithParams(j.beforeJobRuns, j.id) + } - select { - case <-e.ctx.Done(): + if e.elector != nil { + if err := e.elector.IsLeader(j.ctx); err != nil { return - case <-j.ctx.Done(): - return - case e.jobIDsOut <- j.id: - } - - err := callJobFuncWithParams(j.function, j.parameters...) - if err != nil { - _ = callJobFuncWithParams(j.afterJobRunsWithError, j.id, err) - } else { - _ = callJobFuncWithParams(j.afterJobRuns, j.id) } } + _ = callJobFuncWithParams(j.beforeJobRuns, j.id) + + select { + case <-e.ctx.Done(): + return + case <-j.ctx.Done(): + return + case e.jobIDsOut <- j.id: + } + + err := callJobFuncWithParams(j.function, j.parameters...) + if err != nil { + _ = callJobFuncWithParams(j.afterJobRunsWithError, j.id, err) + } else { + _ = callJobFuncWithParams(j.afterJobRuns, j.id) + } } diff --git a/logger.go b/logger.go index c03e03c..dd102b8 100644 --- a/logger.go +++ b/logger.go @@ -52,6 +52,10 @@ func (l *logger) Debug(msg string, args ...interface{}) { if l.level < LogLevelDebug { return } + if len(args) == 0 { + log.Printf("DEBUG: %s\n", msg) + return + } log.Printf("DEBUG: %s, %v\n", msg, args) } @@ -59,6 +63,10 @@ func (l *logger) Error(msg string, args ...interface{}) { if l.level < LogLevelError { return } + if len(args) == 0 { + log.Printf("ERROR: %s\n", msg) + return + } log.Printf("ERROR: %s, %v\n", msg, args) } @@ -66,6 +74,10 @@ func (l *logger) Info(msg string, args ...interface{}) { if l.level < LogLevelInfo { return } + if len(args) == 0 { + log.Printf("INFO: %s\n", msg) + return + } log.Printf("INFO: %s, %v\n", msg, args) } @@ -73,5 +85,9 @@ func (l *logger) Warn(msg string, args ...interface{}) { if l.level < LogLevelWarn { return } + if len(args) == 0 { + log.Printf("WARN: %s\n", msg) + return + } log.Printf("WARN: %s, %v\n", msg, args) } diff --git a/logger_test.go b/logger_test.go new file mode 100644 index 0000000..d4bdb86 --- /dev/null +++ b/logger_test.go @@ -0,0 +1,77 @@ +package gocron + +import ( + "bytes" + "log" + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestNoOpLogger(_ *testing.T) { + noOp := noOpLogger{} + noOp.Debug("debug", "arg1", "arg2") + noOp.Error("error", "arg1", "arg2") + noOp.Info("info", "arg1", "arg2") + noOp.Warn("warn", "arg1", "arg2") +} + +func TestNewLogger(t *testing.T) { + tests := []struct { + name string + level LogLevel + }{ + { + "debug", + LogLevelDebug, + }, + { + "info", + LogLevelInfo, + }, + { + "warn", + LogLevelWarn, + }, + { + "error", + LogLevelError, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + var results bytes.Buffer + log.SetOutput(&results) + l := NewLogger(tt.level) + + l.Debug("debug", "arg1", "arg2") + if tt.level >= LogLevelDebug { + assert.Contains(t, results.String(), "DEBUG: debug, [arg1 arg2]\n") + } else { + assert.Empty(t, results.String()) + } + + l.Info("info", "arg1", "arg2") + if tt.level >= LogLevelInfo { + assert.Contains(t, results.String(), "INFO: info, [arg1 arg2]\n") + } else { + assert.Empty(t, results.String()) + } + + l.Warn("warn", "arg1", "arg2") + if tt.level >= LogLevelWarn { + assert.Contains(t, results.String(), "WARN: warn, [arg1 arg2]\n") + } else { + assert.Empty(t, results.String()) + } + + l.Error("error", "arg1", "arg2") + if tt.level >= LogLevelError { + assert.Contains(t, results.String(), "ERROR: error, [arg1 arg2]\n") + } else { + assert.Empty(t, results.String()) + } + }) + } +} diff --git a/scheduler.go b/scheduler.go index 5909100..87a348f 100644 --- a/scheduler.go +++ b/scheduler.go @@ -391,7 +391,7 @@ func (s *scheduler) addOrUpdateJob(id uuid.UUID, definition JobDefinition, taskW } if taskFunc.Kind() != reflect.Func { - return nil, ErrNewJobTask + return nil, ErrNewJobTaskNotFunc } j.function = tsk.function @@ -506,20 +506,6 @@ func (s *scheduler) Update(id uuid.UUID, jobDefinition JobDefinition, task Task, // options on the Scheduler. type SchedulerOption func(*scheduler) error -// WithDistributedElector sets the elector to be used by multiple -// Scheduler instances to determine who should be the leader. -// Only the leader runs jobs, while non-leaders wait and continue -// to check if a new leader has been elected. -func WithDistributedElector(elector Elector) SchedulerOption { - return func(s *scheduler) error { - if elector == nil { - return ErrWithDistributedElector - } - s.exec.elector = elector - return nil - } -} - // WithClock sets the clock used by the Scheduler // to the clock provided. See https://github.com/jonboulle/clockwork func WithClock(clock clockwork.Clock) SchedulerOption { @@ -532,6 +518,20 @@ func WithClock(clock clockwork.Clock) SchedulerOption { } } +// WithDistributedElector sets the elector to be used by multiple +// Scheduler instances to determine who should be the leader. +// Only the leader runs jobs, while non-leaders wait and continue +// to check if a new leader has been elected. +func WithDistributedElector(elector Elector) SchedulerOption { + return func(s *scheduler) error { + if elector == nil { + return ErrWithDistributedElectorNil + } + s.exec.elector = elector + return nil + } +} + // WithGlobalJobOptions sets JobOption's that will be applied to // all jobs added to the scheduler. JobOption's set on the job // itself will override if the same JobOption is set globally. @@ -585,6 +585,9 @@ const ( // a given time. func WithLimitConcurrentJobs(limit uint, mode LimitMode) SchedulerOption { return func(s *scheduler) error { + if limit == 0 { + return ErrWithLimitConcurrentJobsZero + } s.exec.limitMode = &limitModeConfig{ mode: mode, limit: limit, @@ -627,6 +630,9 @@ func WithLogger(logger Logger) SchedulerOption { // Default: 10 * time.Second func WithStopTimeout(timeout time.Duration) SchedulerOption { return func(s *scheduler) error { + if timeout <= 0 { + return ErrWithStopTimeoutZeroOrNegative + } s.exec.stopTimeout = timeout return nil } diff --git a/scheduler_test.go b/scheduler_test.go index d5cb9c9..ee8f758 100644 --- a/scheduler_test.go +++ b/scheduler_test.go @@ -7,6 +7,8 @@ import ( "testing" "time" + "github.com/google/uuid" + "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "go.uber.org/goleak" @@ -78,8 +80,7 @@ func TestScheduler_OneSecond_NoOptions(t *testing.T) { <-tt.ch runCount++ } - err = s.Shutdown() - require.NoError(t, err) + require.NoError(t, s.Shutdown()) stopTime := time.Now() select { @@ -154,8 +155,7 @@ func TestScheduler_LongRunningJobs(t *testing.T) { s.Start() time.Sleep(1600 * time.Millisecond) - err = s.Shutdown() - require.NoError(t, err) + require.NoError(t, s.Shutdown()) var runCount int timeout := make(chan struct{}) @@ -238,8 +238,7 @@ func TestScheduler_Update(t *testing.T) { default: } } - err = s.Shutdown() - require.NoError(t, err) + require.NoError(t, s.Shutdown()) stopTime := time.Now() select { @@ -735,8 +734,114 @@ func TestScheduler_NewJobErrors(t *testing.T) { _, err = s.NewJob(tt.jd, NewTask(func() {}), tt.opts...) assert.ErrorIs(t, err, tt.err) - err = s.Shutdown() + require.NoError(t, s.Shutdown()) + }) + t.Run(tt.name+" global", func(t *testing.T) { + s, err := newTestScheduler( + WithStopTimeout(time.Millisecond*50), + WithGlobalJobOptions(tt.opts...), + ) require.NoError(t, err) + + _, err = s.NewJob(tt.jd, NewTask(func() {})) + assert.ErrorIs(t, err, tt.err) + require.NoError(t, s.Shutdown()) + }) + } +} + +func TestScheduler_NewJobTask(t *testing.T) { + goleak.VerifyNone(t) + + testFuncPtr := func() {} + + tests := []struct { + name string + tsk Task + err error + }{ + { + "task nil", + nil, + ErrNewJobTaskNil, + }, + { + "task not func - nil", + NewTask(nil), + ErrNewJobTaskNotFunc, + }, + { + "task not func - string", + NewTask("not a func"), + ErrNewJobTaskNotFunc, + }, + { + "task func is pointer", + NewTask(&testFuncPtr), + nil, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + s, err := newTestScheduler() + require.NoError(t, err) + + _, err = s.NewJob(DurationJob(time.Second), tt.tsk) + assert.ErrorIs(t, err, tt.err) + require.NoError(t, s.Shutdown()) + }) + } +} + +func TestScheduler_WithOptionsErrors(t *testing.T) { + goleak.VerifyNone(t) + tests := []struct { + name string + opt SchedulerOption + err error + }{ + { + "WithClock nil", + WithClock(nil), + ErrWithClockNil, + }, + { + "WithDistributedElector nil", + WithDistributedElector(nil), + ErrWithDistributedElectorNil, + }, + { + "WithLimitConcurrentJobs limit 0", + WithLimitConcurrentJobs(0, LimitModeWait), + ErrWithLimitConcurrentJobsZero, + }, + { + "WithLocation nil", + WithLocation(nil), + ErrWithLocationNil, + }, + { + "WithLogger nil", + WithLogger(nil), + ErrWithLoggerNil, + }, + { + "WithStopTimeout 0", + WithStopTimeout(0), + ErrWithStopTimeoutZeroOrNegative, + }, + { + "WithStopTimeout -1", + WithStopTimeout(-1), + ErrWithStopTimeoutZeroOrNegative, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + _, err := newTestScheduler(tt.opt) + assert.ErrorIs(t, err, tt.err) }) } } @@ -766,7 +871,8 @@ func TestScheduler_Singleton(t *testing.T) { jobRanCh := make(chan struct{}, 10) s, err := newTestScheduler( - WithStopTimeout(1 * time.Second), + WithStopTimeout(1*time.Second), + WithLocation(time.Local), ) require.NoError(t, err) @@ -869,8 +975,7 @@ func TestScheduler_LimitMode(t *testing.T) { } } stop := time.Now() - err = s.Shutdown() - require.NoError(t, err) + require.NoError(t, s.Shutdown()) assert.GreaterOrEqual(t, stop.Sub(start), tt.expectedMin) assert.LessOrEqual(t, stop.Sub(start), tt.expectedMax) @@ -927,6 +1032,8 @@ func TestScheduler_WithDistributedElector(t *testing.T) { ) require.NoError(t, err) + s.Start() + _, err = s.NewJob( DurationJob( time.Second, @@ -942,8 +1049,6 @@ func TestScheduler_WithDistributedElector(t *testing.T) { ) require.NoError(t, err) - s.Start() - <-ctx.Done() err = s.Shutdown() require.NoError(t, err) @@ -979,3 +1084,130 @@ func TestScheduler_WithDistributedElector(t *testing.T) { }) } } + +func TestScheduler_RemoveJob(t *testing.T) { + goleak.VerifyNone(t) + tests := []struct { + name string + addJob bool + err error + }{ + { + "success", + true, + nil, + }, + { + "job not found", + false, + ErrJobNotFound, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + s, err := newTestScheduler() + require.NoError(t, err) + + var id uuid.UUID + if tt.addJob { + j, err := s.NewJob(DurationJob(time.Second), NewTask(func() {})) + require.NoError(t, err) + id = j.ID() + } else { + id = uuid.New() + } + + time.Sleep(50 * time.Millisecond) + err = s.RemoveJob(id) + assert.ErrorIs(t, err, err) + require.NoError(t, s.Shutdown()) + }) + } +} + +func TestScheduler_WithEventListeners(t *testing.T) { + goleak.VerifyNone(t) + + listenerRunCh := make(chan error, 1) + testErr := fmt.Errorf("test error") + tests := []struct { + name string + tsk Task + el EventListener + expectRun bool + expectErr error + }{ + { + "AfterJobRuns", + NewTask(func() {}), + AfterJobRuns(func(_ uuid.UUID) { + listenerRunCh <- nil + }), + true, + nil, + }, + { + "AfterJobRunsWithError - error", + NewTask(func() error { return testErr }), + AfterJobRunsWithError(func(_ uuid.UUID, err error) { + listenerRunCh <- err + }), + true, + testErr, + }, + { + "AfterJobRunsWithError - no error", + NewTask(func() error { return nil }), + AfterJobRunsWithError(func(_ uuid.UUID, err error) { + listenerRunCh <- err + }), + false, + nil, + }, + { + "BeforeJobRuns", + NewTask(func() {}), + BeforeJobRuns(func(_ uuid.UUID) { + listenerRunCh <- nil + }), + true, + nil, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + s, err := newTestScheduler() + require.NoError(t, err) + _, err = s.NewJob( + DurationJob(time.Minute*10), + tt.tsk, + WithStartAt( + WithStartImmediately(), + ), + WithEventListeners(tt.el), + WithLimitedRuns(1), + ) + require.NoError(t, err) + + s.Start() + if tt.expectRun { + select { + case err = <-listenerRunCh: + assert.ErrorIs(t, err, tt.expectErr) + case <-time.After(time.Second): + t.Fatal("timed out waiting for listener to run") + } + } else { + select { + case <-listenerRunCh: + t.Fatal("listener ran when it shouldn't have") + case <-time.After(time.Millisecond * 100): + } + } + + require.NoError(t, s.Shutdown()) + }) + } +}