feat(test): Add integration tests for status server by digvijay-y · Pull Request #3373 · kubeflow/trainer

digvijay-y · 2026-03-22T14:31:18Z

What this PR does / why we need it:
Adds integration tests for the status server handleTrainJobRuntimeStatus handler.

The tests include coverage of:

The happy path (updating the train job status)
invalid data is rejected with a useful error message progressPercentage greater than 100 +kubebuilder:validation:Minimum=0 and +kubebuilder:validation:Maximum=100 with this handler would return 422
updating a non-existing trainjob is rejected with a useful error message

Which issue(s) this PR fixes (optional, in Fixes #<issue number>, #<issue number>, ... format, will close the issue(s) when PR gets merged):
Fixes #3346

Checklist:

Docs included if any changes are user facing

github-actions · 2026-03-22T14:31:27Z

🎉 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! 🙏

Copilot

Pull request overview

Adds Ginkgo integration coverage for the status server’s handleTrainJobRuntimeStatus endpoint, exercising real API-server validation behavior in the envtest-based integration suite.

Changes:

Introduces a new integration spec that boots a TLS-enabled status server and posts status updates against it.
Covers success, invalid payload validation (e.g., progressPercentage > 100), and not-found TrainJob updates.

test/integration/controller/statusserver_test.go

robert-bell

Hey @digvijay-y, thanks for your patience and the reminder.

It's looking good so far, but I have a few comments. We should look at the copilot suggestions too.

There's a couple of extra scenarios we could check - would you be happy to take a look?

testing that an update overwrites an existing trainer status. It shouldn't merge the status (e.g. if a progress percent is there initially, but not in the update, then it should be removed by the update).
testing that an empty request UpdateTrainJobStatusRequest{} is valid but doesn't overwrite an existing trainer status.

I think the current implementation should already handle both those cases correctly.

test/integration/controller/statusserver_test.go

robert-bell · 2026-04-02T16:37:02Z

test/integration/controller/statusserver_test.go

+	return true, nil
+}
+
+func generateTestTLSConfig() (*tls.Config, error) {


This is quite a chunky utility - can we use crypto/tls/testcert instead?

Maybe something like

func generateTestTLSConfig() (*tls.Config, error) { cert, err := tls.X509KeyPair(testcert.LocalhostCert, testcert.LocalhostKey) if err != nil { return nil, err } return &tls.Config{Certificates: []tls.Certificate{cert}}, nil }

crypto/tls/testcert is an internal stdlib package and can't be imported outside the standard library. Used runtime self-signed cert generation via crypto/ecdsa and crypto/x509 instead.

robert-bell · 2026-04-02T16:39:29Z

test/integration/controller/statusserver_test.go

+		gomega.Expect(k8sClient.Create(ctx, trainJob)).To(gomega.Succeed())
+
+		ginkgo.By("POSTing a valid status update")
+		resp := postStatus(httpClient, serverAddr, ns.Name, jobName, trainer.UpdateTrainJobStatusRequest{


can we add a test for an empty update request? The TrainJob status should be unchanged; any existing status should not be removed.

trainer.UpdateTrainJobStatusRequest{}

Added as a dedicated It block , "Should accept an empty update request but not overwrite existing status". It POSTs an initial status, then POSTs UpdateTrainJobStatusRequest{}, and uses Consistently to verify the existing TrainerStatus is unchanged

test/integration/controller/statusserver_test.go

robert-bell · 2026-04-02T16:46:16Z

test/integration/controller/statusserver_test.go

+		var status metav1.Status
+		gomega.Expect(json.NewDecoder(resp.Body).Decode(&status)).To(gomega.Succeed())
+		gomega.Expect(status.Message).To(gomega.ContainSubstring("progressPercentage"),
+			"error message should identify the invalid field")


should we check the train job status isn't updated?

Added a Consistently block after the 422 assertions that polls the TrainJob for 2 seconds and asserts TrainerStatus remains nil, confirming the rejected request had no side-effects.

test/integration/controller/statusserver_test.go

digvijay-y · 2026-04-06T13:16:50Z

@robert-bell I've Implemented the changes, would you take a look, whenever possible.

robert-bell

Thanks for the updates @digvijay-y. Just one small extra comment, but otherwise this is looking good. It'll need a review from a maintainer before it can be merged. @astefanutti @andreyvelich please could you take a look when you have bandwidth?

robert-bell · 2026-04-08T15:01:46Z

test/integration/controller/statusserver_test.go

+		err := k8sClient.Get(ctx, types.NamespacedName{
+			Name:      jobName,
+			Namespace: ns.Name,
+		}, notFound)


should this be wrapped in ginkgo.Consistently too?

Wrapped it up. Thank you for the review.

Signed-off-by: DIGVIJAY <144053736+digvijay-y@users.noreply.github.com>

review-notebook-app · 2026-04-11T18:22:11Z

Check out this pull request on

See visual diffs & provide feedback on Jupyter Notebooks.

Powered by ReviewNB

google-oss-prow · 2026-04-11T18:40:08Z

[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 electronic-waste 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

Signed-off-by: DIGVIJAY <144053736+digvijay-y@users.noreply.github.com>

Signed-off-by: DIGVIJAY <144053736+digvijay-y@users.noreply.github.com> signed-off-by: DIGVIJAY <yewaredigvijay@gmail.com> Signed-off-by: DIGVIJAY <144053736+digvijay-y@users.noreply.github.com>

digvijay-y · 2026-04-11T19:18:36Z

@astefanutti @andreyvelich I have implemented changes suggested by @robert-bell , Could please take a look whenever possible?

Copilot AI review requested due to automatic review settings March 22, 2026 14:31

google-oss-prow bot added the size/L label Mar 22, 2026

google-oss-prow bot requested review from jinchihe and kuizhiqing March 22, 2026 14:31

Copilot started reviewing on behalf of digvijay-y March 22, 2026 14:31 View session

digvijay-y changed the title ~~feat(Tests): Add integration tests for status server~~ feat(test): Add integration tests for status server Mar 22, 2026

Copilot AI reviewed Mar 22, 2026

View reviewed changes

robert-bell reviewed Apr 2, 2026

View reviewed changes

digvijay-y requested a review from robert-bell April 3, 2026 18:34

robert-bell reviewed Apr 8, 2026

View reviewed changes

digvijay-y added 4 commits April 11, 2026 18:21

Added statusserver_test.go

4292d49

Signed-off-by: DIGVIJAY <144053736+digvijay-y@users.noreply.github.com>

fix: linting and format

9a94da6

Signed-off-by: DIGVIJAY <144053736+digvijay-y@users.noreply.github.com>

fix: added suggest comments by copilot

e22d0c9

Signed-off-by: DIGVIJAY <144053736+digvijay-y@users.noreply.github.com>

Address review comments by robert-bell

0ae72f9

Signed-off-by: DIGVIJAY <144053736+digvijay-y@users.noreply.github.com>

digvijay-y force-pushed the test-Server branch from a4fac51 to 576230b Compare April 11, 2026 18:22

google-oss-prow bot added size/XXL and removed size/L labels Apr 11, 2026

digvijay-y closed this Apr 11, 2026

digvijay-y force-pushed the test-Server branch from 576230b to 5959ae7 Compare April 11, 2026 18:30

google-oss-prow bot added size/XS and removed size/XXL labels Apr 11, 2026

digvijay-y reopened this Apr 11, 2026

google-oss-prow bot added size/L and removed size/XS labels Apr 11, 2026

digvijay-y added 2 commits April 11, 2026 19:00

test: namespace error wrapped in ginkgo.Consistent

8c91db1

Signed-off-by: DIGVIJAY <144053736+digvijay-y@users.noreply.github.com>

update error message

d392970

Signed-off-by: DIGVIJAY <144053736+digvijay-y@users.noreply.github.com> signed-off-by: DIGVIJAY <yewaredigvijay@gmail.com> Signed-off-by: DIGVIJAY <144053736+digvijay-y@users.noreply.github.com>

digvijay-y force-pushed the test-Server branch from 65ebd68 to d392970 Compare April 11, 2026 19:02

Conversation

digvijay-y commented Mar 22, 2026

Uh oh!

github-actions bot commented Mar 22, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

robert-bell left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

digvijay-y commented Apr 6, 2026

Uh oh!

robert-bell left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

review-notebook-app bot commented Apr 11, 2026

Uh oh!

google-oss-prow bot commented Apr 11, 2026

Uh oh!

digvijay-y commented Apr 11, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants