test(nvidia): raise calculate_score.go coverage from 18% to 94% - #2174
test(nvidia): raise calculate_score.go coverage from 18% to 94%#2174adity1raut wants to merge 3 commits into
Conversation
|
[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 |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR adds mocked tests for NVIDIA device construction and device-list building. It also adds table-driven coverage for device filtering, topology and NVLink pair scoring, error cases, symmetry, and all NVLink levels. ChangesNVIDIA device and scoring tests
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related issues
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 |
6f67ac3 to
4a139e8
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: 2
🤖 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/kunlun/topo_test.go`:
- Around line 274-298: Require TestInterconnect_TwoDevices and
TestInterconnect_FourDevices to assert exactly 2 and 4 selected devices
respectively when all devices are available. Remove the len(got) == 0 acceptance
while preserving the existing duplicate-index checks.
In `@pkg/device/nvidia/calculate_score_build_test.go`:
- Around line 124-129: Extend the success-case assertions around newDevice to
verify that the constructed device’s PCI BusID matches the expected value
derived from the fake device’s PCI address. Keep the existing index and UUID
assertions unchanged, and use the populated PCI-info field returned by
newDevice.
🪄 Autofix (Beta)
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: 47484058-7f09-4635-9388-3b3ddfac922d
📒 Files selected for processing (3)
pkg/device/kunlun/topo_test.gopkg/device/nvidia/calculate_score_build_test.gopkg/device/nvidia/calculate_score_test.go
Addresses a CodeRabbit review comment on PR Project-HAMi#2174: the newDevice success case didn't verify the PciInfo -> BusID conversion, so a regression there would have passed silently. Signed-off-by: Aditya Raut <araut7798@gmail.com>
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:
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/device/nvidia/calculate_score_test.go (1)
114-117: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd a regression test for aggregate error propagation.
The asymmetric-link test validates
calculateGPUPairScore, but no visible case drives that error throughcalculateGPUScoreand its newreturn nil, errbranch. Add an asymmetric fixture toTest_calculateGPUScoreand assert the propagated error.🤖 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 `@pkg/device/nvidia/calculate_score_test.go` around lines 114 - 117, Add an asymmetric-link fixture to Test_calculateGPUScore that causes calculateGPUPairScore to return an error, then assert calculateGPUScore returns that same error with a nil score list. Keep the existing successful cases unchanged and cover the new return nil, err branch directly.
🤖 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.
Nitpick comments:
In `@pkg/device/nvidia/calculate_score_test.go`:
- Around line 114-117: Add an asymmetric-link fixture to Test_calculateGPUScore
that causes calculateGPUPairScore to return an error, then assert
calculateGPUScore returns that same error with a nil score list. Keep the
existing successful cases unchanged and cover the new return nil, err branch
directly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 3a3b2f15-5ec8-4dc4-b359-b98f4e072622
📒 Files selected for processing (2)
pkg/device/nvidia/calculate_score.gopkg/device/nvidia/calculate_score_test.go
|
Production behavior also changed (panic to error), worth reflecting in the title/description. |
You're right, I missed that. I'll update the title and description to reflect the behavior change as well. Thanks! |
|
asymmetric pair errors out here, so RegisterInAnnotation never patches the node, blocking all device registration on it. #2306 scores it 0 instead and keeps registering. /hold |
|
please resolve these conflicts |
…o bool-based asymmetric API Master merged Project-HAMi#2306 while this PR was open, which fixed the same calculateGPUPairScore panic-on-asymmetric-links bug this PR targeted, but with a different, already-accepted approach: score the pair as 0 and set an asymmetric bool flag instead of returning an error. That change is not reapplied here — calculate_score.go is left as-is. This carries forward the still-valid parts of this PR: the new calculate_score_build_test.go (NVML mock helpers covering device construction, list building, UUID filtering, and topology/NVLink derivation) and the calculateGPUPairScore / DeviceList.Filter test coverage from calculate_score_test.go, updated to call the current (int, bool) signature instead of the (int, error) signature this PR originally introduced. The old panic/error-specific asymmetric-link test is dropped since master's Test_calculateGPUPairScore_asymmetric already covers that scenario against the merged behavior. Signed-off-by: Aditya Raut <araut7798@gmail.com>
26d8791 to
b8b413a
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. |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/nvidia/calculate_score_build_test.go`:
- Around line 124-130: Update the newDevice test fixture to use an
eight-character zero-padded PCI domain in the address passed to newFakeDevice,
then adjust the PCI BusID assertion to expect the valid four-character domain
remaining after BusID conversion. Keep the existing device and topology
assertions unchanged.
In `@pkg/device/nvidia/calculate_score_test.go`:
- Around line 236-238: Add an asymmetric-link test case in the table-driven
tests around the linkTypes cases, using different forward and backward link
counts such as one link versus zero. Assert that calculateGPUPairScore reports
asymmetric as true and verify the expected score for that condition, while
preserving the existing symmetric cases.
- Around line 219-221: Update the wantErr branch of the DeviceList.Filter test
to assert that the returned got value is nil, in addition to checking the
existing error text. Preserve the current early return after both assertions.
🪄 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: 6f8f0d1b-0c8f-48f5-8930-9c76005d3a64
📒 Files selected for processing (2)
pkg/device/nvidia/calculate_score_build_test.gopkg/device/nvidia/calculate_score_test.go
- Use a realistic 8-digit PCI domain fixture (00000000:00:00.0) so the asserted BusID() output (0000:00:00.0) looks like a real bus ID, instead of asserting the domain-stripped :00:00.0. - Assert Filter's wantErr branch also returns a nil DeviceList, not just the expected error text. - Add an asymmetric-link-count case to the calculateGPUPairScore table test so the wantAsymmetric=true branch is actually exercised. Signed-off-by: Aditya Raut <araut7798@gmail.com>
Project-HAMi#2313 added a LimitSet flag that FitQuota now gates on instead of Limit != 0, and updated TestFitResourceQuota's fixture accordingly, but missed three sibling tests in the same file that also build device.Quota directly: TestFitResourceQuotaNonNvidia, TestFitResourceQuotaCountsEveryDevice, and TestFitResourceQuotaAscendMemoryFactor. Their fixtures defaulted to LimitSet: false, so FitQuota treated the configured limits as unset and admitted every pod, failing the denial assertions in all three tests on current master. Set LimitSet: true on the five affected fixture entries to match what AddQuota produces, mirroring the fix already applied to TestFitResourceQuota. Signed-off-by: Aditya Raut <araut7798@gmail.com>
|
Thanks for the NVIDIA scoring tests. This test-only branch has remained on hold, and the production issue it was intended to support has since been addressed by the merged #2306. Keeping the old branch would preserve tests against a superseded implementation and require another large review without a current gap. We are closing this PR. If #2306 still lacks an important regression case, please propose only that case from the latest master and explain the failure it protects. |
4 participants
What type of PR is this?
/kind feature
What this PR does / why we need it:
Adds unit tests for
pkg/device/nvidia/calculate_score.go, raising itsstatement coverage from ~18% to ~94%.
Filter,calculateGPUScore, and every branch ofcalculateGPUPairScore(all P2PLink/NVLink types, nil handling, andthe asymmetric-link panic) are now directly tested.
newDeviceanddeviceListBuilder.build()(including its NVMLInit/GetDevices/GetP2PLink/GetNVLink error paths) are exercised using
fake
nvml.Interfaceandnvlib/device.Interfaceimplementationsbuilt on the moq-generated mocks already vendored under
go-nvml/pkg/nvml/mock, so no real GPU/driver is required.CalculateGPUScore's success path still isn't covered:NewDevices()calls
nvml.New()directly, so exercising it would need either a realNVML driver or a source change to inject the interface — out of scope
for this test-only PR.
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Does this PR introduce a user-facing change?:
Summary by CodeRabbit