test(plugin): raise register.go coverage - #2195
Conversation
|
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 change adds an injectable NVML initialization hook. Shutdown runs only after successful initialization. Tests cover NVML failure and success paths, PCI information errors, and missing sysfs NUMA data. ChangesNVML plugin tests
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 |
![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-plugin/nvidiadevice/nvinternal/plugin/register_test.go`:
- Around line 123-138: Make getAPIDevices use an injectable NVML initialization
function or interface, defaulting to the real nvml.Init implementation in
production. Update TestGetAPIDevices_PanicsWithoutNVMLDriver to inject a stub
that returns an explicit NVML initialization error, then verify getAPIDevices
panics without depending on host GPU hardware.
🪄 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: e79360be-4b95-40ab-b672-790ea955b2b2
📒 Files selected for processing (1)
pkg/device-plugin/nvidiadevice/nvinternal/plugin/register_test.go
|
no ai assistance disclosure is present. if any ai tool was used, it must be disclosed per CONTRIBUTING.md: https://github.com/Project-HAMi/HAMi/blob/master/CONTRIBUTING.md#ai-assistance-notice |
I am updating this in the description |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/device-plugin/nvidiadevice/nvinternal/plugin/register_test.go (1)
134-149: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winVerify that the panic comes from the NVML initialization branch.
The deferred
recoveraccepts any panic.plugin.Devices()runs beforenvmlInit()inregister.goLines [95-97]. If that call or the mock panics, this test passes without exercising the intended branch. Record that the injected function was called and assert it before accepting the panic.Proposed test adjustment
func TestGetAPIDevices_PanicsOnNVMLInitFailure(t *testing.T) { + called := false origInit := nvmlInit - nvmlInit = func() nvml.Return { return nvml.ERROR_LIBRARY_NOT_FOUND } + nvmlInit = func() nvml.Return { + called = true + return nvml.ERROR_LIBRARY_NOT_FOUND + } defer func() { nvmlInit = origInit }() // ... defer func() { + if !called { + t.Fatal("nvmlInit was not called") + } if r := recover(); r == nil { t.Fatal("expected getAPIDevices to panic when nvml.Init fails") } }()🤖 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-plugin/nvidiadevice/nvinternal/plugin/register_test.go` around lines 134 - 149, Update TestGetAPIDevices_PanicsOnNVMLInitFailure to track whether the injected nvmlInit function runs, then require that flag to be set before accepting the recovered panic. Keep the existing panic assertion, but fail if recovery occurs without nvmlInit having been called so the test specifically validates the NVML initialization branch.
🤖 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-plugin/nvidiadevice/nvinternal/plugin/register_test.go`:
- Around line 105-114: Update the mock.Device fixture in the GetNumaNode test so
its NVML BusId resolves to an impossible PCI address, such as 00000000:ff:ff.0,
ensuring the sysfs lookup is missing on every host. Preserve the existing
coverage of domain-prefix trimming without relying on the host’s actual PCI
devices.
---
Outside diff comments:
In `@pkg/device-plugin/nvidiadevice/nvinternal/plugin/register_test.go`:
- Around line 134-149: Update TestGetAPIDevices_PanicsOnNVMLInitFailure to track
whether the injected nvmlInit function runs, then require that flag to be set
before accepting the recovered panic. Keep the existing panic assertion, but
fail if recovery occurs without nvmlInit having been called so the test
specifically validates the NVML initialization branch.
🪄 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: 19941657-f0ad-4f49-9b04-0cbb51a5da2e
📒 Files selected for processing (2)
pkg/device-plugin/nvidiadevice/nvinternal/plugin/register.gopkg/device-plugin/nvidiadevice/nvinternal/plugin/register_test.go
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.
... and 2 files with indirect coverage changes 🚀 New features to boost your workflow:
|
|
@archlitchi PTAL. This PR ready to merged |
|
/assign |
Add tests for GetNumaNode's two deterministic error branches (PCI info lookup failure, and the numa_node sysfs read failure that always occurs since the domain prefix is stripped before building the path) and a test that pins down getAPIDevices' existing panic-on-no-driver behavior via recover(). getAPIDevices, RegisterInAnnotation, and most of WatchAndRegister's active branch remain uncovered: getAPIDevices calls the real nvml.Init() directly (not an injectable interface), which fails and panics on any machine without an NVIDIA driver - this one included - so exercising their actual logic isn't possible without either real GPU hardware or refactoring nvml access behind an interface, which is out of scope for this test-only change. Signed-off-by: Aditya Raut <araut7798@gmail.com>
nvml.Shutdown() was deferred before checking whether nvml.Init() succeeded. On a host with no NVML library at all, Init fails gracefully, but the deferred Shutdown() still runs during the panic(0) unwind and crashes the whole process with a dynamic symbol lookup error, since the library was never loaded. Defer Shutdown only after a successful Init. Also route Init through a package-level nvmlInit var so TestGetAPIDevices_PanicsOnNVMLInitFailure can simulate an init failure directly instead of depending on the test host's driver state, and fix the numa_node mock/comment to use NVML's real 8-digit-domain BusId format per review feedback. Signed-off-by: Aditya Raut <araut7798@gmail.com>
The only existing getAPIDevices test forces nvmlInit to fail, so the defer nvml.Shutdown() moved after the success check in 5a1cd13 was never exercised, leaving codecov/patch failing on that line. Add a success-path test that stubs both nvmlInit and nvml.Shutdown (both are reassignable package vars) so it never touches the real, unloaded NVML library on this host. Signed-off-by: Aditya Raut <araut7798@gmail.com>
Signed-off-by: Aditya Raut <araut7798@gmail.com>
archlitchi asked for unnecessary comments to be removed from
register.go and register_test.go, with method/test doc comments
trimmed to one sentence describing what the code does. Also fixes a
comment above defer nvml.Shutdown() that was missing its lead-in
clause ("succeeded: calling it after a failed Init crashes the
process...") and read as a sentence fragment.
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>
0a8cf88 to
ab770f2
Compare
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: adity1raut, 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 |
3 participants
What type of PR is this?
/kind feature
What this PR does / why we need it:
pkg/device-plugin/nvidiadevice/nvinternal/plugin/register.go:GetNumaNode's two deterministic error branches (PCI info lookup failure,and the sysfs
numa_noderead failure — the code strips the PCI domainprefix before building the path, so it never matches a real sysfs
directory).
getAPIDevices' existing panic-on-no-driver behaviorvia
recover().Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Does this PR introduce a user-facing change?:
Summary by CodeRabbit
Bug Fixes
Tests