Skip to content

test(plugin): fix TestGetNumaNode failure caused by existing PCI bus ID - #2546

Merged
hami-robot[bot] merged 1 commit into
Project-HAMi:masterfrom
shinigami-777:test/fix-TestGetNumaNode-failure
Aug 11, 2026
Merged

test(plugin): fix TestGetNumaNode failure caused by existing PCI bus ID#2546
hami-robot[bot] merged 1 commit into
Project-HAMi:masterfrom
shinigami-777:test/fix-TestGetNumaNode-failure

Conversation

@shinigami-777

@shinigami-777 shinigami-777 commented Aug 10, 2026

Copy link
Copy Markdown
Contributor

What type of PR is this?
/kind failing-test

What this PR does / why we need it:
The TestGetNumaNode test in register_test.go previously hardcoded the PCI bus ID to be 00000000:02:00.0 with the assumption that this device would not exist on the test host (and therefore lack a /sys/bus/pci/devices/.../numa_node file). However, on machines where a device actually does exist at 0000:02:00.0 such as local dev environments or specific CI nodes, the GetNumaNode function successfully reads the file instead of returning the expected error, causing the test to fail (my own machine had this case and thus the test ./pkg/device-plugin/nvidiadevice/nvinternal/plugin/ kept failing).

Change Made:

  • pkg/device-plugin/nvidiadevice/nvinternal/plugin/register_test.go: Updated the mocked PciInfo.BusId in the test from 00000000:02:00.0 to an impossible bus ID 0000DEAD:BE:EF.0.

By using DEAD:BE:EF.0, we can guarantee that the numa_node sysfs path will never exist on any machine regardless of their local hardware setup, ensuring the error branch is deterministically triggered and the test passes consistently .
Which issue(s) this PR fixes:
Fixes #

Special notes for your reviewer:

AI assistance disclosure:
I saw that make test was failing on my machine while doing something and used Gemini Pro 3.1 for investigating some of the recent commits to find the cause. Verified everything manually before creating the PR.

Does this PR introduce a user-facing change?:
No, this only modifies a unit test that might fail for some machines.

Summary by CodeRabbit

  • Tests
    • Updated a test fixture’s simulated PCI bus ID while preserving existing test behavior and assertions.

@coderabbitai

coderabbitai Bot commented Aug 10, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: d48d1ecf-6bd7-4be2-9ce7-8bfd61f37cab

📥 Commits

Reviewing files that changed from the base of the PR and between 91f0248 and 48a3654.

📒 Files selected for processing (1)
  • pkg/device-plugin/nvidiadevice/nvinternal/plugin/register_test.go

📝 Walkthrough

Walkthrough

The absent-NUMA-node test fixture now uses 0000:DEAD:BE:EF.0 as its mocked PCI bus ID. Test behavior and assertions remain unchanged.

Changes

PCI fixture update

Layer / File(s) Summary
Update absent-NUMA-node test fixture
pkg/device-plugin/nvidiadevice/nvinternal/plugin/register_test.go
The test uses the non-zero PCI bus ID 0000:DEAD:BE:EF.0.

Estimated code review effort: 1 (Trivial) | ~2 minutes

Possibly related PRs

Suggested reviewers: fouof

Poem

A rabbit checks the bus ID bright,
DEAD:BE:EF.0 fits just right.
The NUMA test hops on through,
With assertions unchanged and true.
Nibble nibble, test suite green!

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly identifies the test fix and its cause, matching the pull request's main change.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches 💡 1
🛠️ Fix failing CI checks 💡
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai
coderabbitai Bot requested a review from FouoF August 10, 2026 16:06
Signed-off-by: shinigami-777 <chattopadhyaytamaghna@gmail.com>
@shinigami-777
shinigami-777 force-pushed the test/fix-TestGetNumaNode-failure branch from 48a3654 to 6eff773 Compare August 10, 2026 16:10
@codecov

codecov Bot commented Aug 10, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

Flag Coverage Δ
unittests 62.32% <ø> (+0.02%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.
see 1 file with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@FouoF

FouoF commented Aug 11, 2026

Copy link
Copy Markdown
Contributor

/lgtm

@hami-robot hami-robot Bot added the lgtm label Aug 11, 2026
@hami-robot

hami-robot Bot commented Aug 11, 2026

Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: FouoF, shinigami-777

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@hami-robot hami-robot Bot added the approved label Aug 11, 2026
@hami-robot
hami-robot Bot merged commit c963de3 into Project-HAMi:master Aug 11, 2026
16 checks passed
@shinigami-777
shinigami-777 deleted the test/fix-TestGetNumaNode-failure branch August 11, 2026 04:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants