test(scheduler): add unit tests for metrics helper functions - #2447
test(scheduler): add unit tests for metrics helper functions#2447adity1raut wants to merge 1 commit into
Conversation
normalizeAMDCoreMetrics, sendMetric, and sendLegacyMetric in cmd/scheduler/metrics.go had no direct unit test coverage; they were only exercised indirectly through the collector-level tests. Add focused tests for the AMD core normalization math and for the success/error paths of both metric-sending helpers. Signed-off-by: Aditya Raut <araut7798@gmail.com>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: adity1raut 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 |
📝 WalkthroughWalkthroughAdded scheduler metric tests for AMD core normalization, standard metric sending, and legacy metric sending. The tests cover valid, invalid, nil, case-insensitive, rounding, and error-handling cases. ChangesScheduler metrics test coverage
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested labels: 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 |
![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 `@cmd/scheduler/metrics_test.go`:
- Around line 236-243: Add a separate negative-total AMD test case in the table
covering normalizeAMDCoreMetrics, using a negative total and expected total and
allocated values unchanged. Keep the existing zero-total case, and ensure the
new case verifies the total <= 0 pass-through behavior.
🪄 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: 03aff813-b048-4f35-a28b-2e63457538f3
📒 Files selected for processing (1)
cmd/scheduler/metrics_test.go
| { | ||
| name: "AMD device with non-positive total is passed through unchanged", | ||
| deviceType: "AMD", | ||
| total: 0, | ||
| allocated: 0, | ||
| wantTotal: 0, | ||
| wantAllocated: 0, | ||
| }, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Cover the negative-total branch.
This case tests total == 0 only. normalizeAMDCoreMetrics passes through every total <= 0 value. Add an AMD case with a negative total to detect a regression that changes the condition to total == 0.
Proposed test case
+ {
+ name: "AMD device with negative total is passed through unchanged",
+ deviceType: "AMD",
+ total: -1,
+ allocated: 1,
+ wantTotal: -1,
+ wantAllocated: 1,
+ },📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| { | |
| name: "AMD device with non-positive total is passed through unchanged", | |
| deviceType: "AMD", | |
| total: 0, | |
| allocated: 0, | |
| wantTotal: 0, | |
| wantAllocated: 0, | |
| }, | |
| { | |
| name: "AMD device with non-positive total is passed through unchanged", | |
| deviceType: "AMD", | |
| total: 0, | |
| allocated: 0, | |
| wantTotal: 0, | |
| wantAllocated: 0, | |
| }, | |
| { | |
| name: "AMD device with negative total is passed through unchanged", | |
| deviceType: "AMD", | |
| total: -1, | |
| allocated: 1, | |
| wantTotal: -1, | |
| wantAllocated: 1, | |
| }, |
🤖 Prompt for 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.
In `@cmd/scheduler/metrics_test.go` around lines 236 - 243, Add a separate
negative-total AMD test case in the table covering normalizeAMDCoreMetrics,
using a negative total and expected total and allocated values unchanged. Keep
the existing zero-total case, and ensure the new case verifies the total <= 0
pass-through behavior.
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:
|
|
Thanks for the contribution. Most of this PR adds direct coverage for thin metrics helpers that are already exercised through collector-level tests, without addressing a concrete failure. The AMD normalization edge cases may be worth testing if they are tied to a confirmed behavior contract, but they do not justify a separate coverage-only PR in the current form. We are closing this PR. A future focused change should first confirm the expected behavior with a maintainer and keep only the tests that protect that contract. |
2 participants
What type of PR is this?
/kind feature
What this PR does / why we need it:
Adds direct unit tests for
normalizeAMDCoreMetrics,sendMetric, andsendLegacyMetricincmd/scheduler/metrics.go. These three helpers had no direct test coverage — they were only exercised indirectly through the collector-levelCollect/Gathertests, so their branches (AMD vs. non-AMD normalization, non-positive total, thesendMetricerror path on mismatched labels, and the nil-descriptor no-op insendLegacyMetric) weren't independently verified.This is a small, standalone piece of the metrics gap analysis discussed in #2126 (test coverage for the existing scheduler metrics surface before further changes are layered on top).
Which issue(s) this PR fixes:
Relates to #2126
Special notes for your reviewer:
Test-only change, no production code touched. Ran locally:
Does this PR introduce a user-facing change?:
AI assistance disclosure: This PR was written primarily by Claude Code (test cases and implementation), based on my own review of the existing metrics.go coverage gaps.
Summary by CodeRabbit