test all remaining With*, and test logger (#609)

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
 

errors.go

@@ -18,8 +18,8 @@ var (
 	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")
+	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")
@@ -28,12 +28,14 @@ var (
 	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")
+	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

executor.go

@@ -326,10 +326,17 @@ 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
@@ -352,4 +359,3 @@ func (e *executor) runJob(j internalJob) {
 		_ = callJobFuncWithParams(j.afterJobRuns, j.id)
 	}
 }
-}

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)
 }

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())
+			}
+		})
+	}
+}

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
 	}

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)
 		})
 	}
 }
@@ -767,6 +872,7 @@ func TestScheduler_Singleton(t *testing.T) {
 
 			s, err := newTestScheduler(
 				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())
+		})
+	}
+}