kubernetes-sigs / jobset

JobSet: a k8s native API for distributed ML training and HPC workloads
https://jobset.sigs.k8s.io/
Apache License 2.0
152 stars 47 forks source link

Add integration test for changes in PR #562 #563

Open danielvegamyhre opened 6 months ago

danielvegamyhre commented 6 months ago

What would you like to be added: Integration tests for changes in #562

Why is this needed: Improving test coverage

danielvegamyhre commented 6 months ago

@googs1025 if you want to take a shot at this feel free, I was running into some strange issues adding an integration test for this, probably some error I was overlooking with tired eyes. If not no worries, I'll resume work on it tomorrow.

googs1025 commented 6 months ago

@danielvegamyhre Sorry, I just saw it now. Thanks for your invitation.I am not familiar with how to write integration tests. You can finish it first. I will look at this PR to learn how to write integration tests.

googs1025 commented 6 months ago

Hello, @danielvegamyhre , I'm trying to write this integration test case, but I keep encountering an error. It seems that the jobs are not being deleted properly when performing the deletion. I'm not sure where I went wrong as I'm not familiar with integration testing myself.

ginkgo.Entry("jobset with foreground delete should delete all child jobs", &testCase{
            makeJobSet: testJobSet,
            updates: []*update{
                {
                    jobSetUpdateFn: func(js *jobset.JobSet) {
                        gomega.Expect(k8sClient.Delete(ctx, js, client.PropagationPolicy(metav1.DeletePropagationForeground))).To(gomega.Succeed())
                    },
                    checkJobSetState: func(js *jobset.JobSet) {
                        expectedJobNum := 0
                        gomega.Eventually(func() bool {
                            var jobList batchv1.JobList
                            gomega.Expect(k8sClient.List(ctx, &jobList, client.InNamespace(js.Namespace))).To(gomega.Succeed())
                            return len(jobList.Items) == expectedJobNum
                        }, timeout, interval).Should(gomega.Equal(true))
                    },
                },
            },
        }),
danielvegamyhre commented 6 months ago

@googs1025 yeah I ran into the same issue when writing the integration test. Haven't had a chance to debug it yet