Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Increase unit test coverage #476

Closed
googs1025 opened this issue Mar 26, 2024 · 6 comments
Closed

Increase unit test coverage #476

googs1025 opened this issue Mar 26, 2024 · 6 comments

Comments

@googs1025
Copy link
Member

What would you like to be added:
Try to increase the scope of unit testing, the current coverage is relatively low.
Why is this needed:
Improve unit test coverage, improve code robustness, and use this issue to track progress.

reference:#473

@googs1025
Copy link
Member Author

/assign

@googs1025
Copy link
Member Author

@danielvegamyhre
I tried to follow your suggestion and rewrote the demo. Please check if it meets your expectations. This is just a demo.

func TestCreateJobs(t *testing.T) {
	var (
		jobSetName = "test-jobset"
		ns         = "default"
	)
	tests := []struct {
		name                string
		js                  *jobset.JobSet
		jobs                childJobs
		expected            error
		expectedJobsCreated []*batchv1.Job
	}{
		{
			name: "create jobs",
			js: testutils.MakeJobSet(jobSetName, ns).
				ReplicatedJob(testutils.MakeReplicatedJob("replicated-job-1").
					Job(testutils.MakeJobTemplate("test-job", ns).Obj()).
					Replicas(1).
					Obj()).Obj(),
			jobs: childJobs{
				active: []*batchv1.Job{
					makeJob(&makeJobArgs{
						jobSetName:        jobSetName,
						replicatedJobName: "replicated-job-1",
						jobName:           "test-jobset-replicated-job-1-test-job-0",
						ns:                ns,
						replicas:          1,
						jobIdx:            0,
					}).Parallelism(1).
						Completions(2).
						Ready(1).
						Active(1).Obj(),
				},
			},
			expectedJobsCreated: []*batchv1.Job{
				makeJob(&makeJobArgs{
					jobSetName:        jobSetName,
					replicatedJobName: "replicated-job-1",
					jobName:           "test-jobset-replicated-job-1-test-job-0",
					ns:                ns,
					replicas:          1,
					jobIdx:            0,
				}).Parallelism(1).
					Completions(2).
					Ready(1).
					Active(1).Obj(),
			},
		},
	}

	for _, tc := range tests {
		t.Run(tc.name, func(t *testing.T) {
			var jobsCreated []*batchv1.Job
			scheme := runtime.NewScheme()
			utilruntime.Must(jobset.AddToScheme(scheme))
			utilruntime.Must(corev1.AddToScheme(scheme))
			utilruntime.Must(batchv1.AddToScheme(scheme))
			fakeClient := fake.NewClientBuilder().
				WithScheme(scheme).
				WithInterceptorFuncs(interceptor.Funcs{
					Create: func(ctx context.Context, client client.WithWatch, obj client.Object, opts ...client.CreateOption) error {
						job, ok := obj.(*batchv1.Job)
						if !ok {
							return nil
						}
						job = tc.jobs.active[0]
						jobsCreated = append(jobsCreated, job)
						return nil
					},
				}).
				Build()
			r := &JobSetReconciler{
				Client: fakeClient,
				Scheme: scheme,
				Record: nil,
			}

			err := r.createJobs(context.TODO(), tc.js, &tc.jobs, tc.js.Status.ReplicatedJobsStatus)
			if err != tc.expected {
				t.Errorf("createJobs() returned unexpected error: got %v, want %v", err, tc.expected)
			}
			sort.Slice(jobsCreated, func(i, j int) bool {
				return jobsCreated[i].Name < jobsCreated[j].Name
			})
			sort.Slice(tc.expectedJobsCreated, func(i, j int) bool {
				return tc.expectedJobsCreated[i].Name < tc.expectedJobsCreated[j].Name
			})
			if !reflect.DeepEqual(jobsCreated, tc.expectedJobsCreated) {
				t.Errorf("createJobs() did not make the expected job creation calls")
			}
		})
	}
}

@danielvegamyhre
Copy link
Contributor

danielvegamyhre commented Mar 27, 2024

@danielvegamyhre I tried to follow your suggestion and rewrote the demo. Please check if it meets your expectations. This is just a demo.

Thanks! Can you make a PR please so I can comment on it?

@googs1025
Copy link
Member Author

googs1025 commented Mar 27, 2024

@danielvegamyhre /PTAL pr

@googs1025
Copy link
Member Author

/unassign

@danielvegamyhre
Copy link
Contributor

@googs1025 You submitted some PRs improving test coverage but I guess they weren't linked to this issue - I think this is now a stale issue we can close. Feel free to reopen if I missed something.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants