Skip to content

test: add tests for sandbox Kind and Namespace properties#350

Open
Abhinav-kodes wants to merge 2 commits into
volcano-sh:mainfrom
Abhinav-kodes:test-sandbox-kind-namespace
Open

test: add tests for sandbox Kind and Namespace properties#350
Abhinav-kodes wants to merge 2 commits into
volcano-sh:mainfrom
Abhinav-kodes:test-sandbox-kind-namespace

Conversation

@Abhinav-kodes
Copy link
Copy Markdown
Contributor

What type of PR is this?

/kind cleanup

What this PR does / why we need it:
This PR adds missing unit tests to verify that the Kind and SandboxNamespace fields are correctly populated in the CreateSandboxResponse during sandbox creation. This addresses the reviewer feedback from PR #338 where tests were requested for the newly added fields.

Which issue(s) this PR fixes:
Follow-up to #338

Special notes for your reviewer:
This adds test assertions for resp.Kind and resp.SandboxNamespace in the sandbox creation flows within pkg/router/session_manager_test.go and pkg/workloadmanager/handlers_test.go.

Does this PR introduce a user-facing change?:

NONE

Signed-off-by: Abhinav Singh <abhinavsingh717073@gmail.com>
Copilot AI review requested due to automatic review settings May 19, 2026 10:16
@volcano-sh-bot
Copy link
Copy Markdown
Contributor

[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 yaozengzeng 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:

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

Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request updates the test suite to include and verify the Kind and SandboxNamespace fields in CreateSandboxResponse across different sandbox types. Feedback suggests enhancing the CodeInterpreter success test by adding assertions for SandboxID, Name, and EntryPoints to ensure consistency with the AgentRuntime test coverage.

Comment thread pkg/router/session_manager_test.go Outdated
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR adds unit-test coverage to ensure sandbox creation flows correctly populate the sandbox Kind and namespace-related fields when returning/constructing sandbox metadata (router-side SandboxInfo, workload-manager-side CreateSandboxResponse), as a follow-up to #338.

Changes:

  • Add assertions in workload manager sandbox creation tests for CreateSandboxResponse.Kind.
  • Extend router session manager tests to include CreateSandboxResponse.Kind in mocked responses and assert sandbox.Kind and sandbox.SandboxNamespace after sandbox creation.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File Description
pkg/workloadmanager/handlers_test.go Adds an assertion for resp.Kind in TestServerCreateSandbox.
pkg/router/session_manager_test.go Extends mocked create responses with Kind and adds assertions for sandbox.Kind and sandbox.SandboxNamespace in create flows.
Comments suppressed due to low confidence (3)

pkg/router/session_manager_test.go:233

  • These assertions expect sandbox.Kind to equal the invoke kind (AgentRuntimeKind). After #338, sandbox.Kind is meant to carry the underlying sandbox resource kind (types.SandboxKind or types.SandboxClaimsKind) so downstream logic (JWT signing, deletion) can branch correctly. Update the expected value to match the resource kind you return from the mocked CreateSandboxResponse (typically types.SandboxKind).
	if sandbox.Kind != types.AgentRuntimeKind {
		t.Errorf("expected Kind %s, got %s", types.AgentRuntimeKind, sandbox.Kind)
	}
	if sandbox.SandboxNamespace != "default" {
		t.Errorf("expected SandboxNamespace default, got %s", sandbox.SandboxNamespace)
	}

pkg/router/session_manager_test.go:387

  • Same issue as the AgentRuntime test: the mocked CreateSandboxResponse.Kind is set to the invoke kind (types.CodeInterpreterKind), but the field is intended to be the underlying sandbox resource kind (types.SandboxKind / types.SandboxClaimsKind). Adjust the mocked response accordingly so the test exercises the correct behavior.
		// Send successful response
		resp := types.CreateSandboxResponse{
			Kind:        types.CodeInterpreterKind,
			SessionID:   "ci-session-789",
			SandboxID:   "ci-sandbox-101",
			SandboxName: "ci-sandbox-test",

pkg/router/session_manager_test.go:420

  • These assertions currently expect sandbox.Kind to equal the invoke kind (CodeInterpreterKind). In production, sandbox.Kind should be the sandbox resource kind (types.SandboxKind or types.SandboxClaimsKind) returned by the workload manager so router JWT signing/deletion logic can work. Update the expected value to the resource kind used in the mocked create response.
	if sandbox.Kind != types.CodeInterpreterKind {
		t.Errorf("expected Kind %s, got %s", types.CodeInterpreterKind, sandbox.Kind)
	}
	if sandbox.SandboxNamespace != "default" {
		t.Errorf("expected SandboxNamespace default, got %s", sandbox.SandboxNamespace)
	}

Comment thread pkg/workloadmanager/handlers_test.go
Comment thread pkg/router/session_manager_test.go Outdated
@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented May 19, 2026

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 49.35%. Comparing base (524e55e) to head (cfc6799).
⚠️ Report is 68 commits behind head on main.
❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #350      +/-   ##
==========================================
+ Coverage   47.57%   49.35%   +1.78%     
==========================================
  Files          30       30              
  Lines        2819     2871      +52     
==========================================
+ Hits         1341     1417      +76     
+ Misses       1338     1301      -37     
- Partials      140      153      +13     
Flag Coverage Δ
unittests 49.35% <ø> (+1.78%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Signed-off-by: Abhinav Singh <abhinavsingh717073@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants