diff --git a/pkg/controller/cronjob/cronjob_controller_test.go b/pkg/controller/cronjob/cronjob_controller_test.go index 45df7ef6c23..a0a49047cff 100644 --- a/pkg/controller/cronjob/cronjob_controller_test.go +++ b/pkg/controller/cronjob/cronjob_controller_test.go @@ -250,113 +250,117 @@ func TestSyncOne_RunOrNot(t *testing.T) { "prev ran but done, long overdue, past short deadline, F": {f, F, onTheHour, shortDead, T, F, weekAfterTheHour(), T, F, 1, 0}, } for name, tc := range testCases { - sj := cronJob() - sj.Spec.ConcurrencyPolicy = tc.concurrencyPolicy - sj.Spec.Suspend = &tc.suspend - sj.Spec.Schedule = tc.schedule - if tc.deadline != noDead { - sj.Spec.StartingDeadlineSeconds = &tc.deadline - } - - var ( - job *batchv1.Job - err error - ) - js := []batchv1.Job{} - if tc.ranPreviously { - sj.ObjectMeta.CreationTimestamp = metav1.Time{Time: justBeforeThePriorHour()} - sj.Status.LastScheduleTime = &metav1.Time{Time: justAfterThePriorHour()} - job, err = getJobFromTemplate(&sj, sj.Status.LastScheduleTime.Time) - if err != nil { - t.Fatalf("%s: unexpected error creating a job from template: %v", name, err) + name := name + tc := tc + t.Run(name, func(t *testing.T) { + sj := cronJob() + sj.Spec.ConcurrencyPolicy = tc.concurrencyPolicy + sj.Spec.Suspend = &tc.suspend + sj.Spec.Schedule = tc.schedule + if tc.deadline != noDead { + sj.Spec.StartingDeadlineSeconds = &tc.deadline } - job.UID = "1234" - job.Namespace = "" - if tc.stillActive { - sj.Status.Active = []v1.ObjectReference{{UID: job.UID}} - js = append(js, *job) - } - } else { - sj.ObjectMeta.CreationTimestamp = metav1.Time{Time: justBeforeTheHour()} - if tc.stillActive { - t.Errorf("%s: test setup error: this case makes no sense", name) - } - } - jc := &fakeJobControl{Job: job} - sjc := &fakeSJControl{} - recorder := record.NewFakeRecorder(10) - - syncOne(&sj, js, tc.now, jc, sjc, recorder) - expectedCreates := 0 - if tc.expectCreate { - expectedCreates = 1 - } - if len(jc.Jobs) != expectedCreates { - t.Errorf("%s: expected %d job started, actually %v", name, expectedCreates, len(jc.Jobs)) - } - for i := range jc.Jobs { - job := &jc.Jobs[i] - controllerRef := metav1.GetControllerOf(job) - if controllerRef == nil { - t.Errorf("%s: expected job to have ControllerRef: %#v", name, job) + var ( + job *batchv1.Job + err error + ) + js := []batchv1.Job{} + if tc.ranPreviously { + sj.ObjectMeta.CreationTimestamp = metav1.Time{Time: justBeforeThePriorHour()} + sj.Status.LastScheduleTime = &metav1.Time{Time: justAfterThePriorHour()} + job, err = getJobFromTemplate(&sj, sj.Status.LastScheduleTime.Time) + if err != nil { + t.Fatalf("%s: unexpected error creating a job from template: %v", name, err) + } + job.UID = "1234" + job.Namespace = "" + if tc.stillActive { + sj.Status.Active = []v1.ObjectReference{{UID: job.UID}} + js = append(js, *job) + } } else { - if got, want := controllerRef.APIVersion, "batch/v1beta1"; got != want { - t.Errorf("%s: controllerRef.APIVersion = %q, want %q", name, got, want) - } - if got, want := controllerRef.Kind, "CronJob"; got != want { - t.Errorf("%s: controllerRef.Kind = %q, want %q", name, got, want) - } - if got, want := controllerRef.Name, sj.Name; got != want { - t.Errorf("%s: controllerRef.Name = %q, want %q", name, got, want) - } - if got, want := controllerRef.UID, sj.UID; got != want { - t.Errorf("%s: controllerRef.UID = %q, want %q", name, got, want) - } - if controllerRef.Controller == nil || *controllerRef.Controller != true { - t.Errorf("%s: controllerRef.Controller is not set to true", name) + sj.ObjectMeta.CreationTimestamp = metav1.Time{Time: justBeforeTheHour()} + if tc.stillActive { + t.Errorf("%s: test setup error: this case makes no sense", name) } } - } - expectedDeletes := 0 - if tc.expectDelete { - expectedDeletes = 1 - } - if len(jc.DeleteJobName) != expectedDeletes { - t.Errorf("%s: expected %d job deleted, actually %v", name, expectedDeletes, len(jc.DeleteJobName)) - } + jc := &fakeJobControl{Job: job} + sjc := &fakeSJControl{} + recorder := record.NewFakeRecorder(10) - // Status update happens once when ranging through job list, and another one if create jobs. - expectUpdates := 1 - expectedEvents := 0 - if tc.expectCreate { - expectedEvents++ - expectUpdates++ - } - if tc.expectDelete { - expectedEvents++ - } - expectedEvents += tc.expectedWarnings - - if len(recorder.Events) != expectedEvents { - t.Errorf("%s: expected %d event, actually %v", name, expectedEvents, len(recorder.Events)) - } - - numWarnings := 0 - for i := 1; i <= len(recorder.Events); i++ { - e := <-recorder.Events - if strings.HasPrefix(e, v1.EventTypeWarning) { - numWarnings++ + syncOne(&sj, js, tc.now, jc, sjc, recorder) + expectedCreates := 0 + if tc.expectCreate { + expectedCreates = 1 + } + if len(jc.Jobs) != expectedCreates { + t.Errorf("%s: expected %d job started, actually %v", name, expectedCreates, len(jc.Jobs)) + } + for i := range jc.Jobs { + job := &jc.Jobs[i] + controllerRef := metav1.GetControllerOf(job) + if controllerRef == nil { + t.Errorf("%s: expected job to have ControllerRef: %#v", name, job) + } else { + if got, want := controllerRef.APIVersion, "batch/v1beta1"; got != want { + t.Errorf("%s: controllerRef.APIVersion = %q, want %q", name, got, want) + } + if got, want := controllerRef.Kind, "CronJob"; got != want { + t.Errorf("%s: controllerRef.Kind = %q, want %q", name, got, want) + } + if got, want := controllerRef.Name, sj.Name; got != want { + t.Errorf("%s: controllerRef.Name = %q, want %q", name, got, want) + } + if got, want := controllerRef.UID, sj.UID; got != want { + t.Errorf("%s: controllerRef.UID = %q, want %q", name, got, want) + } + if controllerRef.Controller == nil || *controllerRef.Controller != true { + t.Errorf("%s: controllerRef.Controller is not set to true", name) + } + } } - } - if numWarnings != tc.expectedWarnings { - t.Errorf("%s: expected %d warnings, actually %v", name, tc.expectedWarnings, numWarnings) - } - if tc.expectActive != len(sjc.Updates[expectUpdates-1].Status.Active) { - t.Errorf("%s: expected Active size %d, got %d", name, tc.expectActive, len(sjc.Updates[expectUpdates-1].Status.Active)) - } + expectedDeletes := 0 + if tc.expectDelete { + expectedDeletes = 1 + } + if len(jc.DeleteJobName) != expectedDeletes { + t.Errorf("%s: expected %d job deleted, actually %v", name, expectedDeletes, len(jc.DeleteJobName)) + } + + // Status update happens once when ranging through job list, and another one if create jobs. + expectUpdates := 1 + expectedEvents := 0 + if tc.expectCreate { + expectedEvents++ + expectUpdates++ + } + if tc.expectDelete { + expectedEvents++ + } + expectedEvents += tc.expectedWarnings + + if len(recorder.Events) != expectedEvents { + t.Errorf("%s: expected %d event, actually %v", name, expectedEvents, len(recorder.Events)) + } + + numWarnings := 0 + for i := 1; i <= len(recorder.Events); i++ { + e := <-recorder.Events + if strings.HasPrefix(e, v1.EventTypeWarning) { + numWarnings++ + } + } + if numWarnings != tc.expectedWarnings { + t.Errorf("%s: expected %d warnings, actually %v", name, tc.expectedWarnings, numWarnings) + } + + if tc.expectActive != len(sjc.Updates[expectUpdates-1].Status.Active) { + t.Errorf("%s: expected Active size %d, got %d", name, tc.expectActive, len(sjc.Updates[expectUpdates-1].Status.Active)) + } + }) } } @@ -486,103 +490,107 @@ func TestCleanupFinishedJobs_DeleteOrNot(t *testing.T) { } for name, tc := range testCases { - sj := cronJob() - suspend := false - sj.Spec.ConcurrencyPolicy = f - sj.Spec.Suspend = &suspend - sj.Spec.Schedule = onTheHour + name := name + tc := tc + t.Run(name, func(t *testing.T) { + sj := cronJob() + suspend := false + sj.Spec.ConcurrencyPolicy = f + sj.Spec.Suspend = &suspend + sj.Spec.Schedule = onTheHour - sj.Spec.SuccessfulJobsHistoryLimit = tc.successfulJobsHistoryLimit - sj.Spec.FailedJobsHistoryLimit = tc.failedJobsHistoryLimit + sj.Spec.SuccessfulJobsHistoryLimit = tc.successfulJobsHistoryLimit + sj.Spec.FailedJobsHistoryLimit = tc.failedJobsHistoryLimit - var ( - job *batchv1.Job - err error - ) + var ( + job *batchv1.Job + err error + ) - // Set consistent timestamps for the CronJob - if len(tc.jobSpecs) != 0 { - firstTime := startTimeStringToTime(tc.jobSpecs[0].StartTime) - lastTime := startTimeStringToTime(tc.jobSpecs[len(tc.jobSpecs)-1].StartTime) - sj.ObjectMeta.CreationTimestamp = metav1.Time{Time: firstTime} - sj.Status.LastScheduleTime = &metav1.Time{Time: lastTime} - } else { - sj.ObjectMeta.CreationTimestamp = metav1.Time{Time: justBeforeTheHour()} - } - - // Create jobs - js := []batchv1.Job{} - jobsToDelete := sets.NewString() - sj.Status.Active = []v1.ObjectReference{} - - for i, spec := range tc.jobSpecs { - job, err = getJobFromTemplate(&sj, startTimeStringToTime(spec.StartTime)) - if err != nil { - t.Fatalf("%s: unexpected error creating a job from template: %v", name, err) + // Set consistent timestamps for the CronJob + if len(tc.jobSpecs) != 0 { + firstTime := startTimeStringToTime(tc.jobSpecs[0].StartTime) + lastTime := startTimeStringToTime(tc.jobSpecs[len(tc.jobSpecs)-1].StartTime) + sj.ObjectMeta.CreationTimestamp = metav1.Time{Time: firstTime} + sj.Status.LastScheduleTime = &metav1.Time{Time: lastTime} + } else { + sj.ObjectMeta.CreationTimestamp = metav1.Time{Time: justBeforeTheHour()} } - job.UID = types.UID(strconv.Itoa(i)) - job.Namespace = "" + // Create jobs + js := []batchv1.Job{} + jobsToDelete := sets.NewString() + sj.Status.Active = []v1.ObjectReference{} - if spec.IsFinished { - var conditionType batchv1.JobConditionType - if spec.IsSuccessful { - conditionType = batchv1.JobComplete - } else { - conditionType = batchv1.JobFailed + for i, spec := range tc.jobSpecs { + job, err = getJobFromTemplate(&sj, startTimeStringToTime(spec.StartTime)) + if err != nil { + t.Fatalf("%s: unexpected error creating a job from template: %v", name, err) } - condition := batchv1.JobCondition{Type: conditionType, Status: v1.ConditionTrue} - job.Status.Conditions = append(job.Status.Conditions, condition) - if spec.IsStillInActiveList { + job.UID = types.UID(strconv.Itoa(i)) + job.Namespace = "" + + if spec.IsFinished { + var conditionType batchv1.JobConditionType + if spec.IsSuccessful { + conditionType = batchv1.JobComplete + } else { + conditionType = batchv1.JobFailed + } + condition := batchv1.JobCondition{Type: conditionType, Status: v1.ConditionTrue} + job.Status.Conditions = append(job.Status.Conditions, condition) + + if spec.IsStillInActiveList { + sj.Status.Active = append(sj.Status.Active, v1.ObjectReference{UID: job.UID}) + } + } else { + if spec.IsSuccessful || spec.IsStillInActiveList { + t.Errorf("%s: test setup error: this case makes no sense", name) + } sj.Status.Active = append(sj.Status.Active, v1.ObjectReference{UID: job.UID}) } - } else { - if spec.IsSuccessful || spec.IsStillInActiveList { - t.Errorf("%s: test setup error: this case makes no sense", name) + + js = append(js, *job) + if spec.ExpectDelete { + jobsToDelete.Insert(job.Name) } - sj.Status.Active = append(sj.Status.Active, v1.ObjectReference{UID: job.UID}) } - js = append(js, *job) - if spec.ExpectDelete { - jobsToDelete.Insert(job.Name) + jc := &fakeJobControl{Job: job} + sjc := &fakeSJControl{} + recorder := record.NewFakeRecorder(10) + + cleanupFinishedJobs(&sj, js, jc, sjc, recorder) + + // Check we have actually deleted the correct jobs + if len(jc.DeleteJobName) != len(jobsToDelete) { + t.Errorf("%s: expected %d job deleted, actually %d", name, len(jobsToDelete), len(jc.DeleteJobName)) + } else { + jcDeleteJobName := sets.NewString(jc.DeleteJobName...) + if !jcDeleteJobName.Equal(jobsToDelete) { + t.Errorf("%s: expected jobs: %v deleted, actually: %v deleted", name, jobsToDelete, jcDeleteJobName) + } } - } - jc := &fakeJobControl{Job: job} - sjc := &fakeSJControl{} - recorder := record.NewFakeRecorder(10) - - cleanupFinishedJobs(&sj, js, jc, sjc, recorder) - - // Check we have actually deleted the correct jobs - if len(jc.DeleteJobName) != len(jobsToDelete) { - t.Errorf("%s: expected %d job deleted, actually %d", name, len(jobsToDelete), len(jc.DeleteJobName)) - } else { - jcDeleteJobName := sets.NewString(jc.DeleteJobName...) - if !jcDeleteJobName.Equal(jobsToDelete) { - t.Errorf("%s: expected jobs: %v deleted, actually: %v deleted", name, jobsToDelete, jcDeleteJobName) + // Check for events + expectedEvents := len(jobsToDelete) + if name == "failed list pod err" { + expectedEvents = len(tc.jobSpecs) + } + if len(recorder.Events) != expectedEvents { + t.Errorf("%s: expected %d event, actually %v", name, expectedEvents, len(recorder.Events)) } - } - // Check for events - expectedEvents := len(jobsToDelete) - if name == "failed list pod err" { - expectedEvents = len(tc.jobSpecs) - } - if len(recorder.Events) != expectedEvents { - t.Errorf("%s: expected %d event, actually %v", name, expectedEvents, len(recorder.Events)) - } - - // Check for jobs still in active list - numActive := 0 - if len(sjc.Updates) != 0 { - numActive = len(sjc.Updates[len(sjc.Updates)-1].Status.Active) - } - if tc.expectActive != numActive { - t.Errorf("%s: expected Active size %d, got %d", name, tc.expectActive, numActive) - } + // Check for jobs still in active list + numActive := 0 + if len(sjc.Updates) != 0 { + numActive = len(sjc.Updates[len(sjc.Updates)-1].Status.Active) + } + if tc.expectActive != numActive { + t.Errorf("%s: expected Active size %d, got %d", name, tc.expectActive, numActive) + } + }) } } @@ -669,97 +677,101 @@ func TestSyncOne_Status(t *testing.T) { } for name, tc := range testCases { - // Setup the test - sj := cronJob() - sj.Spec.ConcurrencyPolicy = tc.concurrencyPolicy - sj.Spec.Suspend = &tc.suspend - sj.Spec.Schedule = tc.schedule - if tc.deadline != noDead { - sj.Spec.StartingDeadlineSeconds = &tc.deadline - } - if tc.ranPreviously { - sj.ObjectMeta.CreationTimestamp = metav1.Time{Time: justBeforeThePriorHour()} - sj.Status.LastScheduleTime = &metav1.Time{Time: justAfterThePriorHour()} - } else { - if tc.hasFinishedJob || tc.hasUnexpectedJob || tc.hasMissingJob { - t.Errorf("%s: test setup error: this case makes no sense", name) + name := name + tc := tc + t.Run(name, func(t *testing.T) { + // Setup the test + sj := cronJob() + sj.Spec.ConcurrencyPolicy = tc.concurrencyPolicy + sj.Spec.Suspend = &tc.suspend + sj.Spec.Schedule = tc.schedule + if tc.deadline != noDead { + sj.Spec.StartingDeadlineSeconds = &tc.deadline } - sj.ObjectMeta.CreationTimestamp = metav1.Time{Time: justBeforeTheHour()} - } - jobs := []batchv1.Job{} - if tc.hasFinishedJob { - ref, err := getRef(&finishedJob) - if err != nil { - t.Errorf("%s: test setup error: failed to get job's ref: %v.", name, err) + if tc.ranPreviously { + sj.ObjectMeta.CreationTimestamp = metav1.Time{Time: justBeforeThePriorHour()} + sj.Status.LastScheduleTime = &metav1.Time{Time: justAfterThePriorHour()} + } else { + if tc.hasFinishedJob || tc.hasUnexpectedJob || tc.hasMissingJob { + t.Errorf("%s: test setup error: this case makes no sense", name) + } + sj.ObjectMeta.CreationTimestamp = metav1.Time{Time: justBeforeTheHour()} } - sj.Status.Active = []v1.ObjectReference{*ref} - jobs = append(jobs, finishedJob) - } - if tc.hasUnexpectedJob { - jobs = append(jobs, unexpectedJob) - } - if tc.hasMissingJob { - ref, err := getRef(&missingJob) - if err != nil { - t.Errorf("%s: test setup error: failed to get job's ref: %v.", name, err) + jobs := []batchv1.Job{} + if tc.hasFinishedJob { + ref, err := getRef(&finishedJob) + if err != nil { + t.Errorf("%s: test setup error: failed to get job's ref: %v.", name, err) + } + sj.Status.Active = []v1.ObjectReference{*ref} + jobs = append(jobs, finishedJob) + } + if tc.hasUnexpectedJob { + jobs = append(jobs, unexpectedJob) + } + if tc.hasMissingJob { + ref, err := getRef(&missingJob) + if err != nil { + t.Errorf("%s: test setup error: failed to get job's ref: %v.", name, err) + } + sj.Status.Active = append(sj.Status.Active, *ref) + } + if tc.beingDeleted { + timestamp := metav1.NewTime(tc.now) + sj.DeletionTimestamp = ×tamp } - sj.Status.Active = append(sj.Status.Active, *ref) - } - if tc.beingDeleted { - timestamp := metav1.NewTime(tc.now) - sj.DeletionTimestamp = ×tamp - } - jc := &fakeJobControl{} - sjc := &fakeSJControl{} - recorder := record.NewFakeRecorder(10) + jc := &fakeJobControl{} + sjc := &fakeSJControl{} + recorder := record.NewFakeRecorder(10) - // Run the code - syncOne(&sj, jobs, tc.now, jc, sjc, recorder) + // Run the code + syncOne(&sj, jobs, tc.now, jc, sjc, recorder) - // Status update happens once when ranging through job list, and another one if create jobs. - expectUpdates := 1 - // Events happens when there's unexpected / finished jobs, and upon job creation / deletion. - expectedEvents := 0 - if tc.expectCreate { - expectUpdates++ - expectedEvents++ - } - if tc.expectDelete { - expectedEvents++ - } - if tc.hasFinishedJob { - expectedEvents++ - } - if tc.hasUnexpectedJob { - expectedEvents++ - } - if tc.hasMissingJob { - expectedEvents++ - } + // Status update happens once when ranging through job list, and another one if create jobs. + expectUpdates := 1 + // Events happens when there's unexpected / finished jobs, and upon job creation / deletion. + expectedEvents := 0 + if tc.expectCreate { + expectUpdates++ + expectedEvents++ + } + if tc.expectDelete { + expectedEvents++ + } + if tc.hasFinishedJob { + expectedEvents++ + } + if tc.hasUnexpectedJob { + expectedEvents++ + } + if tc.hasMissingJob { + expectedEvents++ + } - if len(recorder.Events) != expectedEvents { - t.Errorf("%s: expected %d event, actually %v: %#v", name, expectedEvents, len(recorder.Events), recorder.Events) - } + if len(recorder.Events) != expectedEvents { + t.Errorf("%s: expected %d event, actually %v: %#v", name, expectedEvents, len(recorder.Events), recorder.Events) + } - if expectUpdates != len(sjc.Updates) { - t.Errorf("%s: expected %d status updates, actually %d", name, expectUpdates, len(sjc.Updates)) - } + if expectUpdates != len(sjc.Updates) { + t.Errorf("%s: expected %d status updates, actually %d", name, expectUpdates, len(sjc.Updates)) + } - if tc.hasFinishedJob && inActiveList(sjc.Updates[0], finishedJob.UID) { - t.Errorf("%s: expected finished job removed from active list, actually active list = %#v", name, sjc.Updates[0].Status.Active) - } + if tc.hasFinishedJob && inActiveList(sjc.Updates[0], finishedJob.UID) { + t.Errorf("%s: expected finished job removed from active list, actually active list = %#v", name, sjc.Updates[0].Status.Active) + } - if tc.hasUnexpectedJob && inActiveList(sjc.Updates[0], unexpectedJob.UID) { - t.Errorf("%s: expected unexpected job not added to active list, actually active list = %#v", name, sjc.Updates[0].Status.Active) - } + if tc.hasUnexpectedJob && inActiveList(sjc.Updates[0], unexpectedJob.UID) { + t.Errorf("%s: expected unexpected job not added to active list, actually active list = %#v", name, sjc.Updates[0].Status.Active) + } - if tc.hasMissingJob && inActiveList(sjc.Updates[0], missingJob.UID) { - t.Errorf("%s: expected missing job to be removed from active list, actually active list = %#v", name, sjc.Updates[0].Status.Active) - } + if tc.hasMissingJob && inActiveList(sjc.Updates[0], missingJob.UID) { + t.Errorf("%s: expected missing job to be removed from active list, actually active list = %#v", name, sjc.Updates[0].Status.Active) + } - if tc.expectCreate && !sjc.Updates[1].Status.LastScheduleTime.Time.Equal(topOfTheHour()) { - t.Errorf("%s: expected LastScheduleTime updated to %s, got %s", name, topOfTheHour(), sjc.Updates[1].Status.LastScheduleTime) - } + if tc.expectCreate && !sjc.Updates[1].Status.LastScheduleTime.Time.Equal(topOfTheHour()) { + t.Errorf("%s: expected LastScheduleTime updated to %s, got %s", name, topOfTheHour(), sjc.Updates[1].Status.LastScheduleTime) + } + }) } }