chore(trainer): add unit tests for lifecycle tools by Solaris-star · Pull Request #95 · kubeflow/mcp-server

Solaris-star · 2026-07-19T10:52:17Z

Summary

Part of #68 — lifecycle section.

Adds unit coverage for delete_training_job and update_training_job with mocked SDK / CustomObjects API:

confirmed=False preview gate for delete
confirmed=True success path
MCP ownership checks for non-admin personas + platform-admin bypass
invalid name / invalid action / namespace policy short-circuits
not-found and generic SDK error wrapping
suspend/resume patch body (spec.suspend)

Testing

uv run pytest -q tests/unit/trainer/test_lifecycle.py
# 19 passed

make test-python
# 229 passed

ruff check / ruff format --check clean on the new file.

google-oss-prow · 2026-07-19T10:52:23Z

[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 astefanutti 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-07-19T10:52:25Z

🎉 Welcome to the Kubeflow MCP Server! 🎉

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-sdk-team

Join the community:

Slack: Join our #kubeflow-ml-experience Slack channel
Meetings: Attend the Kubeflow SDK and ML Experience 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 lifecycle unit coverage for Trainer job deletion, suspension, and resumption.

Changes:

Tests preview, success, validation, ownership, and error paths.
Verifies suspend/resume patch bodies.

Solaris-star · 2026-07-19T16:14:19Z

+from types import SimpleNamespace
+from unittest.mock import MagicMock, patch
+
+from kubeflow_mcp.trainer.api.lifecycle import delete_training_job, update_training_job


Thanks for flagging this. The shared scaffold in #6 is not on main yet, so this PR cannot import its fixtures without duplicating that open PR. I have kept this branch compatible with current main and tightened the ownership and full SDK call assertions in 23939aa. Once #6 lands, I can rebase and convert these tests to the shared fixtures.

Solaris-star · 2026-07-19T16:14:21Z

+- MCP ownership checks for non-admin personas
+- platform-admin bypass


Updated in 23939aa. The non-admin delete and update paths now capture is_mcp_managed and assert the exact ("job-a", "default") arguments for allowed, denied, and API-error outcomes.

Solaris-star · 2026-07-19T16:14:22Z

+    api.patch_namespaced_custom_object.assert_called_once()
+    kwargs = api.patch_namespaced_custom_object.call_args.kwargs
+    assert kwargs["name"] == "job-a"
+    assert kwargs["namespace"] == "default"
+    assert kwargs["body"] == {"spec": {"suspend": True}}


Updated in 23939aa. Suspend, resume, and the managed non-admin path now use assert_called_once_with for the complete invocation, including group, version, namespace, plural, name, body, and request timeout.

Solaris-star · 2026-07-19T16:14:25Z

Thanks for the review. I agree this slice should use the shared Pattern C harness. That harness currently exists only in #6, which is still open and is not present on main. Copying its fixtures into this PR would duplicate the scaffold.

I will keep this branch based on main until #6 lands, then rebase and convert the confirmed paths to mock_trainer_client + verify_tool_*. At the same time I will tighten the ownership argument checks and assert the complete SDK calls. I will not request maintainer review before that update.

abhijeet-dhumal · 2026-08-10T06:02:09Z

/ok-to-test

abhijeet-dhumal · 2026-08-10T07:15:23Z

+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+"""Tests for lifecycle tools: delete_training_job, update_training_job.


waiting for PR #122 .. update_training_job on main doesn't have a confirmed gate yet. #122 adds one, which will change the function signature and require updates to test_update_training_job_suspend_success_as_admin and siblings. Whichever PR merges second must update the other's tests. Just flagging for awareness.

abhijeet-dhumal · 2026-08-25T17:49:52Z

@Solaris-star Can you please rebase this work.. this PR seems bit outdated

Covers delete/update lifecycle paths with mocked SDK and CustomObjects API: - preview gate, confirmed success, ownership checks, namespace policy - invalid name/action short-circuits, not-found and generic SDK errors Implements the TODO(test) items in the existing lifecycle_test.py. Signed-off-by: Solaris-star <820622658@qq.com>

Solaris-star · 2026-08-29T15:52:38Z

Hi @abhijeet-dhumal - I have rebased this work on top of the current main and ported the lifecycle tool unit tests to the new kubeflow_mcp/trainer/api/ location (matching the recent test relocation on main). The PR is now a single commit (42955e8, SSH-signed, DCO ok) and CI is fully green including pre-commit, tests on Python 3.10-3.12, and DCO. This also fills in the 10 TODO(test) items that were left in the existing lifecycle_test.py on main. Ready for another look whenever convenient. Thanks!

abhijeet-dhumal · 2026-09-02T05:07:14Z


 """Tests for trainer/api/lifecycle.py — delete, suspend, resume.

 Covers input validation. K8s API interaction tests require mocking and are


Nit: module docstring still says "marked as TODOs" worth a one-line update now that this file implements them.

abhijeet-dhumal · 2026-09-02T05:11:24Z

Thanks @Solaris-star, great work 🚀
Just one nit above to resolve.. otherwise LGTM!
Re #122: fine to land this first. When the confirm gate lands on update_training_job, whoever merges second should add preview/confirmed=True tests for update..

Copilot AI review requested due to automatic review settings July 19, 2026 10:52

google-oss-prow Bot requested review from andreyvelich, astefanutti and szaher July 19, 2026 10:52

google-oss-prow Bot added the size/XL label Jul 19, 2026

Copilot started reviewing on behalf of Solaris-star July 19, 2026 10:52 View session

Copilot AI reviewed Jul 19, 2026

View reviewed changes

Nagasirikancha mentioned this pull request Jul 19, 2026

test(trainer): expand unit coverage for trainer MCP tools #68

Open

google-oss-prow Bot added the ok-to-test Approve CI for external contributors label Aug 10, 2026

abhijeet-dhumal reviewed Aug 10, 2026

View reviewed changes

Solaris-star force-pushed the chore/trainer-lifecycle-unit-tests branch 2 times, most recently from d1bd32e to 984be97 Compare August 29, 2026 15:44

Solaris-star force-pushed the chore/trainer-lifecycle-unit-tests branch from 984be97 to 42955e8 Compare August 29, 2026 15:46

abhijeet-dhumal reviewed Sep 2, 2026

View reviewed changes

Navigation Menu

Uh oh!

chore(trainer): add unit tests for lifecycle tools - #95

chore(trainer): add unit tests for lifecycle tools#95
Solaris-star wants to merge 1 commit into
kubeflow:mainfrom
Solaris-star:chore/trainer-lifecycle-unit-tests

Solaris-star commented Jul 19, 2026

Uh oh!

google-oss-prow Bot commented Jul 19, 2026

Uh oh!

github-actions Bot commented Jul 19, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Solaris-star Jul 19, 2026

Uh oh!

Solaris-star Jul 19, 2026

Uh oh!

Solaris-star Jul 19, 2026

Uh oh!

Solaris-star commented Jul 19, 2026

Uh oh!

abhijeet-dhumal commented Aug 10, 2026

Uh oh!

abhijeet-dhumal Aug 10, 2026

Uh oh!

abhijeet-dhumal commented Aug 25, 2026

Uh oh!

Solaris-star commented Aug 29, 2026

Uh oh!

abhijeet-dhumal Sep 2, 2026

Uh oh!

abhijeet-dhumal commented Sep 2, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

		- MCP ownership checks for non-admin personas
		- platform-admin bypass


		"""Tests for trainer/api/lifecycle.py — delete, suspend, resume.

		Covers input validation. K8s API interaction tests require mocking and are

Uh oh!

Conversation

Solaris-star commented Jul 19, 2026

Summary

Testing

Uh oh!

google-oss-prow Bot commented Jul 19, 2026

Uh oh!

github-actions Bot commented Jul 19, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Solaris-star Jul 19, 2026

Choose a reason for hiding this comment

Uh oh!

Solaris-star Jul 19, 2026

Choose a reason for hiding this comment

Uh oh!

Solaris-star Jul 19, 2026

Choose a reason for hiding this comment

Uh oh!

Solaris-star commented Jul 19, 2026

Uh oh!

abhijeet-dhumal commented Aug 10, 2026

Uh oh!

abhijeet-dhumal Aug 10, 2026

Choose a reason for hiding this comment

Uh oh!

abhijeet-dhumal commented Aug 25, 2026

Uh oh!

Solaris-star commented Aug 29, 2026

Uh oh!

abhijeet-dhumal Sep 2, 2026

Choose a reason for hiding this comment

Uh oh!

abhijeet-dhumal commented Sep 2, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants