test(device-plugin): add unit tests for imex and cdi packages - #2355
test(device-plugin): add unit tests for imex and cdi packages#2355Labreo wants to merge 1 commit into
Conversation
Signed-off-by: Labreo <74888611+Labreo@users.noreply.github.com>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: Labreo 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 |
|
Welcome @Labreo! It looks like this is your first PR to Project-HAMi/HAMi 🎉 |
📝 WalkthroughWalkthroughThe pull request adds unit tests for the NVIDIA device plugin’s ChangesNVIDIA internal package tests
Estimated code review effort: 2 (Simple) | ~10 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
pkg/device-plugin/nvidiadevice/nvinternal/cdi/imex_test.go (1)
58-65: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the generated device-node paths.
The test supplies
PathandHostPath, but it does not checkContainerEdits.DeviceNodes. A change that drops or changes either path inGetSpecwill pass this test. Assert one device node and compare both fields.Proposed test extension
if rawSpec.Devices[0].Name != "0" { t.Errorf("expected device name '0', got %s", rawSpec.Devices[0].Name) } +deviceNodes := rawSpec.Devices[0].ContainerEdits.DeviceNodes +if len(deviceNodes) != 1 { + t.Fatalf("expected 1 device node, got %d", len(deviceNodes)) +} +if deviceNodes[0].Path != channels[0].Path { + t.Errorf("expected device path %q, got %q", channels[0].Path, deviceNodes[0].Path) +} +if deviceNodes[0].HostPath != channels[0].HostPath { + t.Errorf("expected host path %q, got %q", channels[0].HostPath, deviceNodes[0].HostPath) +}🤖 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/cdi/imex_test.go` around lines 58 - 65, Extend the test assertions after validating rawSpec.Devices to inspect rawSpec.ContainerEdits.DeviceNodes: require exactly one device node, then compare its Path and HostPath against the input values supplied by the test. Keep the existing Kind and device-name assertions unchanged.pkg/device-plugin/nvidiadevice/nvinternal/imex/imex_test.go (1)
28-110: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffConsider adding a happy-path test for an existing character device channel.
Every subtest here exercises an empty list or a failure branch (missing channel, non-character file). None confirms that
GetChannelsappends a channel to the result when the underlying path is a valid character device. Creating a real character device in a test typically requiressyscall.Mknodwith elevated privileges, which likely explains the omission, so this is a good-to-have rather than a blocking gap.🤖 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/imex/imex_test.go` around lines 28 - 110, Add a happy-path subtest to TestGetChannels that provides an existing valid character-device channel and verifies GetChannels returns it without error. Use an appropriate test setup or mockable filesystem mechanism consistent with the implementation, and assert the returned channel collection contains the expected channel while preserving the existing failure-case tests.
🤖 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-plugin/nvidiadevice/nvinternal/cdi/imex_test.go`:
- Around line 58-65: Extend the test assertions after validating rawSpec.Devices
to inspect rawSpec.ContainerEdits.DeviceNodes: require exactly one device node,
then compare its Path and HostPath against the input values supplied by the
test. Keep the existing Kind and device-name assertions unchanged.
In `@pkg/device-plugin/nvidiadevice/nvinternal/imex/imex_test.go`:
- Around line 28-110: Add a happy-path subtest to TestGetChannels that provides
an existing valid character-device channel and verifies GetChannels returns it
without error. Use an appropriate test setup or mockable filesystem mechanism
consistent with the implementation, and assert the returned channel collection
contains the expected channel while preserving the existing failure-case tests.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 4e196ed2-3fa0-44a6-98ec-aae209296808
📒 Files selected for processing (4)
pkg/device-plugin/nvidiadevice/nvinternal/cdi/imex_test.gopkg/device-plugin/nvidiadevice/nvinternal/cdi/null_test.gopkg/device-plugin/nvidiadevice/nvinternal/cdi/options_test.gopkg/device-plugin/nvidiadevice/nvinternal/imex/imex_test.go
|
duplicate #2351 |
2 participants
What type of PR is this?
/kind cleanup
What this PR does / why we need it:
This PR adds comprehensive unit test coverage for the
imex(pkg/device-plugin/nvidiadevice/nvinternal/imex) andcdi(pkg/device-plugin/nvidiadevice/nvinternal/cdi) packages in the NVIDIA device plugin internal implementation. Previously, both packages had zero unit test coverage (coverage: 0.0% of statements).Specifically, this PR introduces:
pkg/device-plugin/nvidiadevice/nvinternal/imex/imex_test.go: TestsGetChannels()for empty channel lists, non-required channel skips, required channel error handling, and character device file mode verification (83.3% statement coverage).pkg/device-plugin/nvidiadevice/nvinternal/cdi/null_test.go: TestsNewNullHandler()no-op CDI handler logic (AdditionalDevices,CreateSpecFile,QualifiedName).pkg/device-plugin/nvidiadevice/nvinternal/cdi/options_test.go: Tests option configuration setters (WithDeviceListStrategies,WithDriverRoot,WithDevRoot,WithTargetDriverRoot,WithTargetDevRoot,WithNvidiaCTKPath,WithDeviceIDStrategy,WithVendor,WithGdrcopyEnabled,WithGdsEnabled,WithMofedEnabled,WithImexChannels).pkg/device-plugin/nvidiadevice/nvinternal/cdi/imex_test.go: Tests IMEX channel CDI spec generation (newImexChannelSpecGenerator()andGetSpec()).Which issue(s) this PR fixes:
Fixes #2354
Special notes for your reviewer:
./hack/verify-license.sh.go test -v -cover ./pkg/device-plugin/nvidiadevice/nvinternal/imex/... ./pkg/device-plugin/nvidiadevice/nvinternal/cdi/....Does this PR introduce a user-facing change?:
NONE
Summary by CodeRabbit