test(vGPUmonitor): expect short UUID to be skipped, not error - #2407
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe short-UUID test now expects collection to succeed while skipping the invalid device and emitting no metrics. ChangesMetric collection validation
Estimated code review effort: 1 (Trivial) | ~2 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
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:
|
|
yeah, thanks for fixing that, and there's another ut failure here: https://github.com/Project-HAMi/HAMi/actions/runs/31076738697/job/92550580659?pr=2311, perhaps you can solve that too? |
Project-HAMi#2364 changed collectContainerMetrics to skip devices whose UUID is shorter than 40 chars (shared memory not yet initialised) instead of returning an error, but TestCollectContainerMetricsBadInput from Project-HAMi#2350 still asserted the old error behavior, so it fails on master. Align the short-UUID case with the actual behavior (skipped, no metrics, no error), matching the sibling TestCollectContainerMetricsSkipsInvalidUTF8UUID. Signed-off-by: imantaba <itn.taba@gmail.com>
82dbdc8 to
92c7a76
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: archlitchi, imantaba 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 |
I made a comment there here |
What type of PR is this?
/kind bug
What this PR does / why we need it:
TestCollectContainerMetricsBadInputfails onmaster, which blocks unit tests for every open PR.Two recently-merged PRs conflict:
collectContainerMetricsto skip a device whose UUID is shorter than 40 chars (the shared-memory region is not yet initialised) and continue the scrape, instead of returning an error.TestCollectContainerMetricsBadInput, which still asserts that a short UUID returns an error — the old behavior.#2350 merged after #2364 but kept the pre-#2364 expectation, so the test now fails deterministically on
master.This aligns the short-UUID case with the actual behavior: the device is skipped, so
collectContainerreturns no metrics and no error — matching the siblingTestCollectContainerMetricsSkipsInvalidUTF8UUID. Test-only change; no production code touched.Which issue(s) this PR fixes:
Special notes for your reviewer:
go test ./cmd/vGPUmonitor/ -count=1passes with this change.This change was prepared with AI assistance (per CONTRIBUTING.md); all changes were reviewed and verified by me.
Does this PR introduce a user-facing change?:
Summary by CodeRabbit