test: add unit test for jobset controller #485
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: googs1025 The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
cncf-cla: yes
|
Hi @googs1025. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
k8s-ci-robot
added
the
size/XXL
✅ Deploy Preview for kubernetes-sigs-jobset canceled.
|
No worries! I will take a look at this tomorrow. |
|
/ok-to-test |
k8s-ci-robot
added
ok-to-test
googs1025
force-pushed
the
main
branch
4 times, most recently
from
0abec76 to
780374e
Compare
k8s-ci-robot
added
size/XL
k8s-ci-robot
added
size/XXL
|
@googs1025 I am refactoring createJobs() to make it more testable in #496 since right now this function does a lot things unrelated to creating jobs (enforcing startup policy rules, suspend/resume, etc.) which makes it harder to unit test thoroughly. This will make it easier for you to write the tests, and easier for me to review them as well. I added some preliminary notes on some of your test in the meantime. |
| name: "create jobs with DNSHostname", | ||
| js: testutils.MakeJobSet(jobSetName, ns).EnableDNSHostnames(true). | ||
| 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(), | ||
| }, | ||
| }, |
There was a problem hiding this comment.
There is nothing in the createJobs() function anymore that depends on enableDNShostnames, we can remove this test case
| }, | ||
| }, | ||
| { | ||
| name: "create jobs with StartupPolicyOrder is InOrder", |
There was a problem hiding this comment.
For this test case, we should have 2 replicated jobs, and ensure we only see Job creation calls for the jobs in the 1st replicated Job.
| }, | ||
| }, | ||
| errorFlag: true, | ||
| expectedErr: errors.Join(errors.New("job \"test-jobset-replicated-job-1-0\" creation failed with error: create pod error")), |
There was a problem hiding this comment.
I don't think you need to do errors.Join() on a single error, do you find you needed to in order for the test to pass?
| return nil | ||
| } | ||
| if tc.errorFlag || job == nil { | ||
| return errors.New("create pod error") |
There was a problem hiding this comment.
| return errors.New("create pod error") | |
| return errors.New("example error") |
Let's keep the error message generic and obvious test error, so it's clear when reading the test cases we aren't actually expecting some pod related error.
|
@danielvegamyhre Thank you for your response. I will update the content of this PR after you merge your pr. |
| if tc.errorFlag || job == nil { | ||
| return errors.New("delete pod error") | ||
| } | ||
| job = tc.jobs[0] |
There was a problem hiding this comment.
Why are you overriding the job that was deleted with the test case job at index 0 every time?
In the jobsDeleted slice, we should just track the actual job objects that were in the Delete() calls.
| expected bool | ||
| }{ | ||
| { | ||
| name: "test jobSetFinished with not finished", |
There was a problem hiding this comment.
| name: "test jobSetFinished with not finished", | |
| name: "jobset not finished", |
| Obj()).Obj(), | ||
| }, | ||
| { | ||
| name: "test jobSetFinished with finished", |
There was a problem hiding this comment.
| name: "test jobSetFinished with finished", | |
| name: "jobset finished", |
| expected: true, | ||
| }, | ||
| } | ||
| for _, tc := range tests { |
There was a problem hiding this comment.
Can you add a test case for a failed JobSet? It should be considered finished as well.
| tests := []struct { | ||
| name string | ||
| jobs []*batchv1.Job | ||
| expectedJobsUpdated []*batchv1.Job |
There was a problem hiding this comment.
| expectedJobsUpdated []*batchv1.Job | |
| wantResumed []*batchv1.Job |
| name string | ||
| js *jobset.JobSet | ||
| jobs childJobs | ||
| expectedErr error |
There was a problem hiding this comment.
Can we rename expectedErr to wantErr everywhere in this file? It's more concise and generally adheres to the "want/got" pattern usually seen in Go testing
| js *jobset.JobSet | ||
| jobs childJobs | ||
| expectedErr error | ||
| errorFlag bool |
There was a problem hiding this comment.
Can we rename errorFlag to forceError everywhere in this file? I think this name makes the parameter's purpose more clear.
| if tc.errorFlag || job == nil { | ||
| return errors.New("create pod error") | ||
| } | ||
| job = tc.jobs.active[0] |
There was a problem hiding this comment.
We need to remove this line, should be tracking the job passed in the Create() call, not the defined in the test case.
| record record.EventRecorder | ||
| } | ||
|
|
||
| func makeClient(interceptor interceptor.Funcs, initObjs ...client.Object) *fakeClient { |
There was a problem hiding this comment.
| func makeClient(interceptor interceptor.Funcs, initObjs ...client.Object) *fakeClient { | |
| func makeFakeClient(interceptor interceptor.Funcs, initObjs ...client.Object) *fakeClient { |
| makeJob(&makeJobArgs{ | ||
| jobSetName: jobSetName, | ||
| replicatedJobName: "replicated-job-1", | ||
| jobName: "test-jobset-replicated-job-1-test-job-0", | ||
| ns: ns, | ||
| replicas: 1, | ||
| jobIdx: 0, |
There was a problem hiding this comment.
Many of these makeJob() calls in the test cases are identical, let's just store 1-2 test jobs as variable above the test cases, then re-use those variables as much as possible in the test cases. This applies to all the tests in this file.
This should drastically reduce the number of lines in this file and improve the readability of the test cases.
|
Maybe this PR can wait until @danielvegamyhre these PRs #496 #494 are merged before I make new changes. |
|
We can prioritize handling this PR #490 first. |
|
/hold until comments addressed |
k8s-ci-robot
added
the
do-not-merge/hold
The refactor is complete, feel free to resume this PR |
k8s-ci-robot
added
the
needs-rebase
|
@danielvegamyhre Okay, I plan to spend some time next week working on this PR |
|
@googs1025 Ok please submit PRs with tests for only 1 function at a time, so that it is easier to review. |
Add comment explaining why we don't unconditionally compute firstFailedJob
k8s-ci-robot
added
size/S
|
@googs1025: The following test failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
|
@googs1025 are you still working on this? |
Yes, I have been quite busy with work recently. I will start this PR next week. |
|
Just following up on this @googs1025, do you still plan to finish this? |
|
issue from #476
It's a bit embarrassing. I accidentally deleted the PR during synchronization and resubmitted a new one. Sorry.
When there is rebase, I don’t know how to do it, and I often close the submission.