test: add coverage for buildSandboxObject and buildSandboxClaimObject#323
test: add coverage for buildSandboxObject and buildSandboxClaimObject#323HarshitPal25 wants to merge 1 commit into
Conversation
Add two new test functions: - TestBuildSandboxObject_WorkloadNameLabel: verifies WorkloadNameLabelKey is correctly propagated from params.workloadName to the Sandbox object metadata labels. Covers both set and empty workloadName cases. - TestBuildSandboxClaimObject: verifies SandboxClaim fields are populated correctly including namespace, name, templateRef, session labels, idle timeout annotations, and owner references. Tests both with and without owner reference scenarios. Signed-off-by: HarshitPal25 <harshit13082006@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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces unit tests for buildSandboxObject and buildSandboxClaimObject to ensure correct metadata and label assignment. Feedback focuses on improving test coverage by adding assertions for missing fields such as SandboxNameLabelKey and full OwnerReference details, as well as recommending the use of dynamic duration strings for better maintainability.
| if claim.Labels[SessionIdLabelKey] != "session-claim-test" { | ||
| t.Errorf("expected label %s=session-claim-test, got %q", SessionIdLabelKey, claim.Labels[SessionIdLabelKey]) | ||
| } | ||
| if claim.Annotations[IdleTimeoutAnnotationKey] != "10m0s" { | ||
| t.Errorf("expected annotation %s=10m0s, got %q", IdleTimeoutAnnotationKey, claim.Annotations[IdleTimeoutAnnotationKey]) | ||
| } | ||
| if len(claim.OwnerReferences) != 1 { | ||
| t.Fatalf("expected 1 owner reference, got %d", len(claim.OwnerReferences)) | ||
| } | ||
| if claim.OwnerReferences[0].Name != "my-ci" { | ||
| t.Errorf("expected owner ref name my-ci, got %q", claim.OwnerReferences[0].Name) | ||
| } |
There was a problem hiding this comment.
The test for buildSandboxClaimObject is missing assertions for several fields that are explicitly set by the function, such as the SandboxNameLabelKey label and the full details of the OwnerReference (APIVersion, Kind, and UID). Additionally, using params.idleTimeout.String() is preferred over hardcoded duration strings for better maintainability and consistency with other tests.
if claim.Labels[SessionIdLabelKey] != "session-claim-test" {
t.Errorf("expected label %s=session-claim-test, got %q", SessionIdLabelKey, claim.Labels[SessionIdLabelKey])
}
if claim.Labels[SandboxNameLabelKey] != "claim-abc" {
t.Errorf("expected label %s=claim-abc, got %q", SandboxNameLabelKey, claim.Labels[SandboxNameLabelKey])
}
if claim.Annotations[IdleTimeoutAnnotationKey] != params.idleTimeout.String() {
t.Errorf("expected annotation %s=%s, got %q", IdleTimeoutAnnotationKey, params.idleTimeout.String(), claim.Annotations[IdleTimeoutAnnotationKey])
}
if len(claim.OwnerReferences) != 1 {
t.Fatalf("expected 1 owner reference, got %d", len(claim.OwnerReferences))
}
ref := claim.OwnerReferences[0]
if ref.Name != "my-ci" || ref.Kind != "CodeInterpreter" || ref.APIVersion != "runtime.agentcube.volcano.sh/v1alpha1" || ref.UID != "test-uid-123" {
t.Errorf("unexpected owner reference: %+v", ref)
}| if len(claim.OwnerReferences) != 0 { | ||
| t.Errorf("expected 0 owner references, got %d", len(claim.OwnerReferences)) | ||
| } | ||
| if claim.Annotations[IdleTimeoutAnnotationKey] != DefaultSandboxIdleTimeout.String() { | ||
| t.Errorf("expected default idle timeout annotation %s, got %q", | ||
| DefaultSandboxIdleTimeout.String(), claim.Annotations[IdleTimeoutAnnotationKey]) | ||
| } |
There was a problem hiding this comment.
The "without owner reference" subtest only verifies the owner references and the idle timeout annotation. It should also verify basic fields like Namespace, Name, TemplateRef, and Labels to ensure the object is correctly constructed in this scenario as well, maintaining the same level of thoroughness as the "with owner reference" subtest.
There was a problem hiding this comment.
Pull request overview
Adds unit tests to increase coverage of the Workload Manager’s workload builder helpers, focusing on label propagation and SandboxClaim construction used when provisioning sandboxes for sessions.
Changes:
- Add a table-driven test ensuring
WorkloadNameLabelKeyis set onSandboxmetadata labels frombuildSandboxParams.workloadName. - Add tests covering
buildSandboxClaimObjectfield population, idle timeout annotation behavior (explicit vs default), and owner reference handling.
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #323 +/- ##
==========================================
+ Coverage 47.57% 48.72% +1.15%
==========================================
Files 30 30
Lines 2819 2855 +36
==========================================
+ Hits 1341 1391 +50
+ Misses 1338 1316 -22
- Partials 140 148 +8
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:
|
Description:
What type of PR is this?
/kind enhancement
What this PR does / why we need it:
Adds two new test functions to improve test coverage for the workload builder package:
TestBuildSandboxObject_WorkloadNameLabel: Verifies thatWorkloadNameLabelKeyis correctly propagated fromparams.workloadNameto the Sandbox object metadata labels.TestBuildSandboxClaimObject: VerifiesbuildSandboxClaimObjectcorrectly populates all fields including namespace, name, templateRef, session labels, idle timeout annotations, and owner references (covers both with and without owner reference scenarios).Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Does this PR introduce a user-facing change?: