test: add unit test about jobset controller

[googs1025]

Hello everyone, when I was studying the project, I found that the degree of unit testing was a bit low. I tried to add some unit testing code, hoping to help the project.
Add some unit tests about jobset controller. Improve overall unit test coverage

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: googs1025
Once this PR has been reviewed and has the lgtm label, please assign ahg-g for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[k8s-ci-robot]

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 /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

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.

[netlify]

✅ Deploy Preview for kubernetes-sigs-jobset canceled.

Name	Link
🔨 Latest commit	c203131
🔍 Latest deploy log	https://app.netlify.com/sites/kubernetes-sigs-jobset/deploys/65ff859598fbf000085bc364

[kannon92]

/ok-to-test
/cc

[danielvegamyhre]

Thanks for working on improving the unit test coverage!

One thing I notice about many of these unit tests is they seem to all just check that no unexpected error occurred, but don't seem to verify the actual behavior of the functions.

I think to proper way to unit test this is to create the fake client WithInterceptorFuncs, which we can use to inject functions which are called instead of the client's underlying CRUD methods (see here).

For example, in this test for createJobs() you could define an interceptor func for Create(...), and use it to track the Job creation calls made by the fake client, and we can compare that against what Job creation calls we expected to be made when calling createJobs() with the given parameters.

Here is a rough example of what I am talking about.

for _, tc := range cases {
    // create fake client with interceptor func for Create
    var jobsCreated []*batchv1.Job
    client := NewClient(wrappedClient, Funcs{
			    Create: func(ctx context.Context, client client.WithWatch, obj client.Object, opts ...client.CreateOption) error {
				    job, err := obj.(*batchv1.Job)
                                     if err != nil {
                                        // ignore creation calls for non-job objects
                                        return nil
                                     }
                                     // track the job creation calls the client makes here
				    jobsCreated = append(jobsCreated, job)
				    return nil
			    },
		    })
		    
    // create reconciler using this fake client
    r := &JobSetReconciler{
		    Client: c,
		    Scheme: scheme,
		    Record: recorder,
	    }
	
   err := r.createJobs(context.TODO(), tc.js, tc.jobs, tc.rjobStatuses)
   // compare errors
   assert.Equal(err, tc.wantErr)
   
   // ...sort slices here before comparing because order matters in deepequal...

   // check client made all the expected job creation calls
   assert.DeepEqual(jobsCreated, tc.wantJobsCreated)
}

[googs1025]

Thank your review. Indeed, in the test cases, the robustness of the code was not taken into consideration. At that time, I only focused on whether the methods had exceptions.

[googs1025]

I will take your suggestions into account and make code modifications. Thank you for your advice.

[googs1025]

@danielvegamyhre Hello, thank you for your suggestion. What I'm thinking is that currently, there are quite a few methods that need unit testing. Can I raise an issue to track them and submit multiple pull requests? This way, it will be easier for the reviewers as well. I will close this pull request and submit a new issue to track the progress.

[danielvegamyhre]

@danielvegamyhre Hello, thank you for your suggestion. What I'm thinking is that currently, there are quite a few methods that need unit testing. Can I raise an issue to track them and submit multiple pull requests? This way, it will be easier for the reviewers as well. I will close this pull request and submit a new issue to track the progress.

Yes that would be great