test: add tests for sandbox Kind and Namespace properties#350
test: add tests for sandbox Kind and Namespace properties#350Abhinav-kodes wants to merge 2 commits into
Conversation
Signed-off-by: Abhinav Singh <abhinavsingh717073@gmail.com>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.Kindin mocked responses and assertsandbox.Kindandsandbox.SandboxNamespaceafter 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.Kindto equal the invoke kind (AgentRuntimeKind). After #338,sandbox.Kindis meant to carry the underlying sandbox resource kind (types.SandboxKindortypes.SandboxClaimsKind) so downstream logic (JWT signing, deletion) can branch correctly. Update the expected value to match the resource kind you return from the mockedCreateSandboxResponse(typicallytypes.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.Kindis 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.Kindto equal the invoke kind (CodeInterpreterKind). In production,sandbox.Kindshould be the sandbox resource kind (types.SandboxKindortypes.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)
}
|
Codecov Report✅ All modified and coverable lines are covered by tests. 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
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Signed-off-by: Abhinav Singh <abhinavsingh717073@gmail.com>
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
KindandSandboxNamespacefields are correctly populated in theCreateSandboxResponseduring 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.Kindandresp.SandboxNamespacein the sandbox creation flows withinpkg/router/session_manager_test.goandpkg/workloadmanager/handlers_test.go.Does this PR introduce a user-facing change?: