chore(test): add UTs for Build, Status, and ReconcilerBuilders inter… by Hitanshi7556 · Pull Request #3215 · kubeflow/trainer

Hitanshi7556 · 2026-02-17T07:45:59Z

Adds missing unit tests for JobSet plugin interfaces as tracked in #2468.

TestJobSet and TestValidate already existed, but JobSet implements 5 interfaces.
This PR adds tests for the 3 remaining ones:

TestReconcilerBuilders: verifies watcher wiring returns non-nil builder
TestBuild: covers object generation, nil inputs, suspended/unsuspended cases
TestStatus: covers condition mapping (Complete/Failed) and ReplicatedJobsStatus to JobsStatus

related #2468

google-oss-prow · 2026-02-17T07:46:06Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

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

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

Details

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

github-actions · 2026-02-17T07:46:07Z

🎉 Welcome to the Kubeflow Trainer! 🎉

Thanks for opening your first PR! We're happy to have you as part of our community 🚀

Here's what happens next:

If you haven't already, please check out our Contributing Guide for repo-specific guidelines and the Kubeflow Contributor Guide for general community standards.
Our team will review your PR soon! cc @kubeflow/kubeflow-trainer-team

Join the community:

Slack: Join our #kubeflow-trainer Slack channel.
Meetings: Attend the Kubeflow AutoML and Training Working Group bi-weekly meetings.

Feel free to ask questions in the comments if you need any help or clarification!
Thanks again for contributing to Kubeflow! 🙏

…faces Adds missing unit tests for JobSet plugin interfaces as tracked in kubeflow#2468. - TestReconcilerBuilders: verifies watcher wiring returns non-nil builder - TestBuild: covers object generation, nil inputs, and suspended/unsuspended cases - TestStatus: covers condition mapping and ReplicatedJobsStatus to JobsStatus Signed-off-by: Hitanshi Goklani <hitanshigoklani@gmail.com> Signed-off-by: Hitanshi Goklani <hitanshigoklani33@gmail.com>

Copilot

Pull request overview

Adds unit tests for the JobSet runtime plugin to cover the remaining framework interfaces and improve decoupling of plugin UTs from the framework tests (Issue #2468).

Changes:

Removed the TODO placeholder about missing interface tests.
Added new unit tests for ReconcilerBuilders, Build, and Status.
Updated error assertions in the existing IdentifyPodNetwork test block.

Copilot · 2026-02-17T07:50:57Z

pkg/runtime/framework/plugins/jobset/jobset_test.go

+		trainJob  *trainer.TrainJob
+		info      *runtime.Info
+		jobSet    *jobsetv1alpha2.JobSet
+		clientErr error
+		wantObjs  int


TestBuild defines clientErr and sets up an interceptor when it’s non-nil, but none of the test cases set clientErr, so this branch is currently dead code; either remove clientErr/interceptor wiring or add a case that simulates a non-NotFound Get error to assert Build returns that error.

Copilot · 2026-02-17T07:50:57Z

pkg/runtime/framework/plugins/jobset/jobset_test.go

+			}
+			if err == nil && len(objs) != tc.wantObjs {
+				t.Errorf("Expected %d objects, got %d", tc.wantObjs, len(objs))
+			}


TestBuild only asserts the number of returned objects, which can still pass if the generated JobSet is malformed or missing key fields (labels/annotations, suspend, owner refs, parallelism/completions updates); consider converting the returned ApplyConfigurations to typed objects and asserting important metadata/spec fields (similar to other plugin Build tests).

Suggested change

}

}

// When build succeeds and we expect objects, verify the JobSet object is well-formed.

if err == nil && tc.wantObjs > 0 {

var js *jobsetv1alpha2.JobSet

for _, obj := range objs {

if typed, ok := obj.(*jobsetv1alpha2.JobSet); ok {

js = typed

break

}

}

if js == nil {

t.Fatalf("expected at least one JobSet object, but got types: %T", objs)

}

if tc.trainJob != nil {

if js.Namespace != tc.trainJob.Namespace {

t.Errorf("JobSet namespace = %q, want %q", js.Namespace, tc.trainJob.Namespace)

}

if js.Name != tc.trainJob.Name {

t.Errorf("JobSet name = %q, want %q", js.Name, tc.trainJob.Name)

}

}

if js.Spec.Suspend == nil {

t.Errorf("JobSet.Spec.Suspend must be set")

}

}

Copilot · 2026-02-17T07:50:58Z

pkg/runtime/framework/plugins/jobset/jobset_test.go

+		"maps ReplicatedJobsStatus to JobsStatus": {
+			trainJob: utiltesting.MakeTrainJobWrapper(metav1.NamespaceDefault, "test").Obj(),
+			jobSet: utiltesting.MakeJobSetWrapper(metav1.NamespaceDefault, "test").
+				ReplicatedJobsStatuses([]jobsetv1alpha2.ReplicatedJobStatus{
+					{


The ReplicatedJobsStatus -> JobsStatus test uses a single ReplicatedJobStatus entry, so it won’t catch issues that only appear with multiple entries (e.g., pointer reuse when taking addresses inside a range loop); consider adding at least one more ReplicatedJobStatus and asserting the mapped per-entry values are distinct/correct.

Copilot · 2026-02-17T07:50:58Z

pkg/runtime/framework/plugins/jobset/jobset_test.go

+	builders := p.(framework.WatchExtensionPlugin).ReconcilerBuilders()
+	if len(builders) == 0 {
+		t.Error("ReconcilerBuilders must return at least one builder")
+	}


TestReconcilerBuilders only asserts that the returned slice is non-empty, but it doesn’t verify that the builder functions themselves are non-nil (or that invoking them returns a non-nil *builder.Builder), so it can pass even if wiring is broken; consider iterating builders to assert each entry is non-nil and/or updating the test to exercise the builder function contract (or adjust the PR description accordingly).

Copilot AI review requested due to automatic review settings February 17, 2026 07:46

google-oss-prow bot requested review from akshaychitneni and jinchihe February 17, 2026 07:46

google-oss-prow bot added the size/L label Feb 17, 2026

Copilot started reviewing on behalf of Hitanshi7556 February 17, 2026 07:46 View session

Hitanshi7556 changed the title ~~Test(jobset): add UTs for Build, Status, and ReconcilerBuilders inter…~~ chore(jobset): add UTs for Build, Status, and ReconcilerBuilders inter… Feb 17, 2026

Copilot AI reviewed Feb 17, 2026

View reviewed changes

Hitanshi7556 force-pushed the test/jobset-plugin-interfaces-2468 branch from 29c03a2 to 66b2255 Compare February 17, 2026 07:51

Hitanshi7556 changed the title ~~chore(jobset): add UTs for Build, Status, and ReconcilerBuilders inter…~~ chore(test): add UTs for Build, Status, and ReconcilerBuilders inter… Feb 17, 2026

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore(test): add UTs for Build, Status, and ReconcilerBuilders inter…#3215

chore(test): add UTs for Build, Status, and ReconcilerBuilders inter…#3215
Hitanshi7556 wants to merge 1 commit intokubeflow:masterfrom
Hitanshi7556:test/jobset-plugin-interfaces-2468

Hitanshi7556 commented Feb 17, 2026

Uh oh!

google-oss-prow bot commented Feb 17, 2026

Uh oh!

github-actions bot commented Feb 17, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Feb 17, 2026

Uh oh!

Copilot AI Feb 17, 2026

Uh oh!

Copilot AI Feb 17, 2026

Uh oh!

Copilot AI Feb 17, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Comments

-			}
+			}
+			// When build succeeds and we expect objects, verify the JobSet object is well-formed.
+			if err == nil && tc.wantObjs > 0 {
+				var js *jobsetv1alpha2.JobSet
+				for _, obj := range objs {
+					if typed, ok := obj.(*jobsetv1alpha2.JobSet); ok {
+						js = typed
+						break
+					}
+				}
+				if js == nil {
+					t.Fatalf("expected at least one JobSet object, but got types: %T", objs)
+				}
+				if tc.trainJob != nil {
+					if js.Namespace != tc.trainJob.Namespace {
+						t.Errorf("JobSet namespace = %q, want %q", js.Namespace, tc.trainJob.Namespace)
+					}
+					if js.Name != tc.trainJob.Name {
+						t.Errorf("JobSet name = %q, want %q", js.Name, tc.trainJob.Name)
+					}
+				}
+				if js.Spec.Suspend == nil {
+					t.Errorf("JobSet.Spec.Suspend must be set")
+				}
+			}

Conversation

Hitanshi7556 commented Feb 17, 2026

Uh oh!

google-oss-prow bot commented Feb 17, 2026

Uh oh!

github-actions bot commented Feb 17, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Feb 17, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 17, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 17, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 17, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Comments