Add unit tests for internal packages by abrarshivani · Pull Request #2656 · NVIDIA/gpu-operator

abrarshivani · 2026-07-21T22:37:55Z

Description

Adds unit tests to raise coverage across the internal/ packages. This PR is tests-only — it contains no production changes. Each package was driven to full statement coverage of every reachable path it owns (both success and error paths), verified with go test -covermode=count.

The branch is organized as one commit per package, so each can be reviewed independently.

Packages covered

Commit	Package	Coverage
`Add unit tests for internal/info`	`internal/info`	100%
`Add unit tests for internal/conditions`	`internal/conditions`	`clusterpolicy.go` 100%, `nvidiadriver.go` 100%
`Add unit tests for internal/nodeinfo`	`internal/nodeinfo`	100%
`Add unit tests for internal/nvidiadriver ...`	`internal/nvidiadriver`	98.6%
`Add unit tests for internal/utils`	`internal/utils`	`GetFilesWithSuffix` + object-hash helpers 100%
`Add unit tests for internal/validator ...`	`internal/validator`	100%

internal/info — GetVersionParts / GetVersionString: with/without gitCommit, empty/whitespace values, variadic joining, fresh-slice / no-mutation guarantees.
internal/conditions — ClusterPolicy and NVIDIADriver updaters: Ready/Error success, type-assertion guard, failed-Get, unknown-status-type default branch, retry-on-conflict, non-conflict update-error propagation, and status.state defaulting/preservation. Uses the controller-runtime fake client + interceptors; condition assertions use cmp.Diff (ignoring server-set LastTransitionTime/ObservedGeneration); schemes are built per-test rather than mutating the global client-go scheme. This promotes github.com/google/go-cmp to a direct dependency (a test dependency) and vendors cmp/cmpopts.
internal/nodeinfo — the previously untested Reset methods on both filter builders.
internal/nvidiadriver — AssignOwners error paths (driver-list, node-list, owner-label patch add/remove). The one uncovered line is a defensive nil-Labels guard, unreachable because nodes are listed by the GPU-present label and therefore always carry labels.
internal/utils — GetFilesWithSuffix and the object-hash helpers.
internal/validator — Validate error paths (driver-list failure, invalid listed selector, node-list failure).

Out of scope

Two packages touched here also gained new, untested code on main after this branch was opened. Covering it is left to follow-up PRs, so the package-level numbers are below 100%:

internal/conditions is 64.6% overall — gpucluster.go (added by Add GPUCluster CRD and controller for DRA-based stack #2571) is untested.
internal/utils is 67.2% overall — PrependPathListEnvvar, SetEnvVar, and WriteFileAtomically are untested. GetStringHash is at 75%: its panic on a hasher.Write error is a deliberate assertion of an invariant and is unreachable by design, so it is left as-is.

The internal/image tests that previously lived in this PR have been dropped: the ImagePath work was split into #2682, which has since merged and already brings internal/image to 100%.

Production changes split out

The small internal/utils production cleanups that previously lived here (the GetFilesWithSuffix dedupe break and the GetObjectHashIgnoreEmptyKeys precondition doc) have been moved to their own PR, #2690, so this PR stays tests-only. utils.go here is unchanged from main.

Checklist

No secrets, sensitive information, or unrelated changes
Lint checks passing (make lint)
Generated assets in-sync (make validate-generated-assets)
Go mod artifacts in-sync (make validate-modules)
Test cases are added for new code paths

Testing

Every commit was verified to build and pass independently (git rebase --exec), so the branch is bisect-clean:

go build ./internal/... ./api/... ./controllers/...
go test ./internal/...

Full-branch checks:

GOOS=linux go build ./...                    # pathrs-lite is Linux-only
go test ./internal/... ./api/... ./controllers/...
go mod tidy && go mod vendor                 # no diff
GOOS=linux golangci-lint run ./internal/...  # 0 issues

copy-pr-bot · 2026-07-21T22:37:59Z

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

tariq1890 · 2026-07-28T15:09:26Z

Can you condense the commit history and split it by the package instead? For eg:- If you have two or more commits writing tests for code in the same package, those commits can be merged to one.

abrarshivani · 2026-07-28T16:51:16Z

@tariq1890 Sure, will segregate each package tests into its own PR.

tariq1890 · 2026-07-28T16:58:17Z

Thanks @abrarshivani. To clarify, it doesn't have to be separate PRs. I just meant that we reorganize this into commits per package. If you want to make it a separate PR for each commit, that's fine too. I'll leave that for you to decide.

abrarshivani · 2026-07-28T17:30:48Z

Oh, ok. I was thinking about this from a reviewer's perspective. I can also group the commits by package to make it easier to review.

tariq1890 · 2026-07-28T22:10:07Z

+				// Stop at the first matching suffix so a file that matches
+				// more than one of the provided suffixes is returned only once.
+				break


Are we sure that this is correct? Won't this lead to a bug as we need to collect multiple yaml files under a state- directory for template rendering?

@rahulait Can you check this one too?

Also, even if this change is valid, it warrants its own PR IMO. We shouldn't slip in any behavioural change in a PR that claims addition of unit tests.

Agreed, even though it looks safe, this behavioral change should be in a separate PR

Let me move the production code-related changes to another PR.

Cover GetVersionParts and GetVersionString: with and without gitCommit, empty and whitespace-only values, variadic joining with empty elements, and the fresh-slice / no-mutation guarantees. internal/info reaches 100% statement coverage. Signed-off-by: Abrar Shivani <ashivani@nvidia.com>

Cover every path in the ClusterPolicy and NVIDIADriver updaters: SetConditionsReady/SetConditionsError success cases, the type-assertion guard for foreign objects, the Ready->Error transition, the failed-Get path, the unknown-status-type default branch, retry-on-conflict, non-conflict status-update error propagation, and the status.state defaulting/preservation logic in the Error case. Conditions are compared with a single cmp.Diff against the expected slice (ignoring server-set LastTransitionTime/ObservedGeneration) rather than field by field, and each test builds a fresh runtime.NewScheme() instead of mutating the global client-go scheme. This promotes go-cmp to a direct dependency and vendors cmp/cmpopts. clusterpolicy.go and nvidiadriver.go both reach 100% statement coverage. Signed-off-by: Abrar Shivani <ashivani@nvidia.com>

Cover the previously untested Reset methods on NodeLabelFilterBuilder and NodeLabelNoValFilterBuilder, verifying that Reset clears accumulated label criteria. internal/nodeinfo reaches 100% statement coverage. Signed-off-by: Abrar Shivani <ashivani@nvidia.com>

Cover the driver-list failure, the GPU-node-list failure, and the owner-label patch failures for both the add/update and removal cases, using fake-client interceptors. internal/nvidiadriver reaches 98.6% statement coverage; the only remaining uncovered line is the defensive nil-Labels guard, which is unreachable in AssignOwners because nodes are listed by the GPU-present label and therefore always carry labels. Signed-off-by: Abrar Shivani <ashivani@nvidia.com>

Cover GetFilesWithSuffix and the object-hash helpers. Signed-off-by: Abrar Shivani <ashivani@nvidia.com>

Cover the driver-list failure, an invalid nodeSelector on a listed driver, and the node-list failure, using fake-client interceptors. internal/validator reaches 100% statement coverage. Signed-off-by: Abrar Shivani <ashivani@nvidia.com>

abrarshivani changed the title ~~Add unit tests for internal/image and internal/info~~ Add unit tests for internal packages Jul 22, 2026

abrarshivani self-assigned this Jul 22, 2026

abrarshivani marked this pull request as ready for review July 22, 2026 20:50

abrarshivani requested review from cdesiniotis, karthikvetrivel, rahulait, rajathagasthya, shivamerla and tariq1890 as code owners July 22, 2026 20:50

abrarshivani force-pushed the unit-test-internal branch from aa9c69d to 88f0709 Compare July 23, 2026 19:38

tariq1890 reviewed Jul 27, 2026

View reviewed changes

Comment thread api/nvidia/v1/clusterpolicy_types.go Outdated

abrarshivani mentioned this pull request Jul 27, 2026

Reject incomplete image specs in ImagePath #2682

Merged

5 tasks

abrarshivani force-pushed the unit-test-internal branch from 88f0709 to b6fafd7 Compare July 27, 2026 18:29

tariq1890 reviewed Jul 27, 2026

View reviewed changes

Comment thread internal/utils/utils.go Outdated

abrarshivani force-pushed the unit-test-internal branch 2 times, most recently from 0f65be4 to f897418 Compare July 28, 2026 19:19

tariq1890 reviewed Jul 28, 2026

View reviewed changes

abrarshivani mentioned this pull request Jul 28, 2026

Dedupe GetFilesWithSuffix and document hash struct precondition #2690

Open

5 tasks

abrarshivani added 4 commits July 28, 2026 16:10

abrarshivani force-pushed the unit-test-internal branch from f897418 to 0afb86b Compare July 28, 2026 23:12

abrarshivani added 2 commits July 28, 2026 16:23

Add unit tests for internal/utils

d3173fb

Cover GetFilesWithSuffix and the object-hash helpers. Signed-off-by: Abrar Shivani <ashivani@nvidia.com>

abrarshivani force-pushed the unit-test-internal branch from 0afb86b to a674f06 Compare July 28, 2026 23:23

tariq1890 approved these changes Jul 28, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Add unit tests for internal packages - #2656

Add unit tests for internal packages#2656
abrarshivani wants to merge 6 commits into
NVIDIA:mainfrom
abrarshivani:unit-test-internal

abrarshivani commented Jul 21, 2026 •

edited

Loading

Uh oh!

copy-pr-bot Bot commented Jul 21, 2026

Uh oh!

Uh oh!

Uh oh!

tariq1890 commented Jul 28, 2026

Uh oh!

abrarshivani commented Jul 28, 2026

Uh oh!

tariq1890 commented Jul 28, 2026

Uh oh!

abrarshivani commented Jul 28, 2026

Uh oh!

tariq1890 Jul 28, 2026

Uh oh!

tariq1890 Jul 28, 2026

Uh oh!

tariq1890 Jul 28, 2026 •

edited

Loading

Uh oh!

rahulait Jul 28, 2026

Uh oh!

abrarshivani Jul 28, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

abrarshivani commented Jul 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Packages covered

Out of scope

Production changes split out

Checklist

Testing

Uh oh!

copy-pr-bot Bot commented Jul 21, 2026

Uh oh!

Uh oh!

Uh oh!

tariq1890 commented Jul 28, 2026

Uh oh!

abrarshivani commented Jul 28, 2026

Uh oh!

tariq1890 commented Jul 28, 2026

Uh oh!

abrarshivani commented Jul 28, 2026

Uh oh!

tariq1890 Jul 28, 2026

Choose a reason for hiding this comment

Uh oh!

tariq1890 Jul 28, 2026

Choose a reason for hiding this comment

Uh oh!

tariq1890 Jul 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rahulait Jul 28, 2026

Choose a reason for hiding this comment

Uh oh!

abrarshivani Jul 28, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

abrarshivani commented Jul 21, 2026 •

edited

Loading

tariq1890 Jul 28, 2026 •

edited

Loading