test(device): add coverage for PodManager update, delete, and ListPodsUID paths - #2358
Conversation
|
Welcome @aaka24! It looks like this is your first PR to Project-HAMi/HAMi 🎉 |
📝 WalkthroughWalkthroughThis test-only change adds coverage for ChangesPodManager test coverage
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…PodsUID PodManager.AddPod's existing-pod update branch, DelPod, and ListPodsUID had no direct unit tests, only incidental use inside scheduler_test.go. This adds focused tests for: - AddPod re-adding an already-tracked pod (devices replaced, NodeID preserved, return value false) - DelPod removing a tracked pod, and DelPod being a no-op (no panic) when called on an already-removed or unknown pod - ListPodsUID on an empty manager and with multiple tracked pods Signed-off-by: aaka24 <yaarsuperstar44@gmail.com>
0e88441 to
d466020
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/device/pod_test.go`:
- Around line 316-321: Update the second AddPod call in the podManager test to
pass "node2" instead of "node1", while continuing to assert that the retrieved
pod's NodeID remains "node1".
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: bad35942-23d8-4211-9f43-3acb14aec060
📒 Files selected for processing (1)
pkg/device/pod_test.go
Signed-off-by: aaka24 <yaarsuperstar44@gmail.com>
|
Updated the AddPod update test based on review feedback by using a different NodeID on the second AddPod call to verify that the original NodeID is preserved. |
|
@aaka24 I see you mentioned the following go test ./pkg/device/... was attempted locally, but the test environment was Windows and the build failed due to missing Linux NVIDIA dependency headers (dlfcn.h). well I would suggest to run the code before submitting... would you like me to test out your changes and review them? |
|
@SK8-infi Thanks! I’d really appreciate that. Let me get my testing environment set up first, though. If you have any recommendations or a guide on the preferred setup for this project, I’d love to follow it. Once I’ve got everything working, I’ll rerun the tests and update the PR. |
Sure. I would say since you are using windows wsl2 would be the easiest and closest. You may give it a shot. |
|
@SK8-infi Followed your suggestion and switched to WSL2. go test ./pkg/device/... passes now. Thanks! |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: aaka24, archlitchi The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Codecov Report✅ All modified and coverable lines are covered by tests.
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
3 participants
What this PR does / why we need it:
Adds focused unit tests for previously uncovered
PodManagerpaths:AddPodbehavior when updating an existing podDelPodremoval behavior and handling of unknown podsListPodsUIDwith empty and populated managersThese tests improve coverage without changing runtime behavior.
Which issue(s) this PR fixes:
Fixes #2357
Special notes for reviewer:
Tests were added only; no production code changes were made.
Tests were verified in a WSL2 (Ubuntu) environment by running:
go test ./pkg/device/...All tests passed successfully.
Does this PR introduce a user-facing change?:
No. This is a test-only change.
Summary by CodeRabbit