Add unit tests for internal/conditions/gpucluster - #2889
Open
abrarshivani wants to merge 1 commit into
Open
abrarshivani wants to merge 1 commit into
abrarshivani wants to merge 1 commit into
Conversation
Takes the conditions package from 64.6% to 100.0% of statements. Tests only, no production source changed. The tests follow the shape already used by nvidiadriver_test.go and clusterpolicy_test.go: the controller-runtime fake client with a status subresource for the happy paths, and interceptor.Funcs to inject a conflict on the first status update and a plain failure on every one, so the retry wrapper and the error return are exercised rather than restated. Most of gpucluster.go mirrors nvidiadriver.go, so most of these tests mirror its tests. The one place the two genuinely diverge is how the error branch resolves status.state, and that is what the bulk of the new assertions pin. NVIDIADriverReconciler sets status.state in memory without persisting it and then writes conditions, so its updater has to copy the caller's value over unconditionally. GPUClusterReconciler.updateCRStatus persists the desired state before every SetConditionsError call, so the stored value is already authoritative and the updater only fills it in when empty. SetConditionsErrorStateResolution covers the four combinations of stored and caller state, and SetConditionsReadyLeavesStateAlone covers the ready branch, which does not touch status.state at all. Together they fail if the two updaters are ever folded into one without accounting for the difference. GetError asserts apierrors.IsNotFound through the wrapper rather than only matching the wrapper text, so a %w that regresses to %v is caught. RetryOnConflict counts Get calls as well as update calls, which is what actually distinguishes a refetch inside the retry loop from one hoisted out of it. The error logged in setConditions before the return is not asserted. Checking it needs a log sink and the returned error already covers the same path. The backoff policy in retry.RetryOnConflict is not asserted either; the tests check that a conflict is retried and that a non-conflict error is not, which is the part this package owns. A typed-nil *nvidiav1alpha1.GPUCluster satisfies the type assertion in SetConditionsReady and SetConditionsError and then panics on cr.Name. All three updaters in this package share that gap, so fixing it belongs in a change that can touch production code. Signed-off-by: Abrar Shivani <ashivani@nvidia.com>
abrarshivani
requested review from
cdesiniotis,
karthikvetrivel,
rahulait,
rajathagasthya,
shivamerla and
tariq1890
as code owners
September 11, 2026 20:25
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: QUIET Plan: Enterprise Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 12 included reviews per hour; 11 remain after this review. 📝 WalkthroughWalkthroughAdded comprehensive tests for the GPUCluster condition updater. The tests cover construction, object validation, not-found errors, ready and error transitions, status resolution, state preservation, unknown status errors, conflict retries with refetching, and non-conflict update failures. Priority: ⬇️ Low Merge Risk: ⚪ Minimal · up to The added tests do not introduce an actionable merge risk. Comment |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
Contributor
|
@coderabbitai approve |
✅ Action performedComments resolved. Approval is disabled; enable |
rahulait
approved these changes
Sep 14, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
internal/conditions/gpucluster.gohad no unit tests. This adds them, taking theconditionspackage from 64.6% to 100.0% of statements. Tests only, no productioncode changed.
They follow the shape of the existing
nvidiadriver_test.goandclusterpolicy_test.go: fake client with a status subresource for the happy paths,interceptor.Funcsto inject a conflict on the first status update and a plainfailure on every one.
gpucluster.golargely mirrorsnvidiadriver.go. The one real difference is how theerror branch resolves
status.state:NVIDIADriverReconcilersetsstatus.statein memory without persisting it and thenwrites conditions, so its updater has to copy the caller's value over.
GPUClusterReconciler.updateCRStatuspersists the desired state first, so the storedvalue is already authoritative and is only filled in when empty. Both are correct for
their own controller, and neither was covered.
SetConditionsErrorStateResolutioncovers the four combinations of stored and caller state;
SetConditionsReadyLeavesStateAlonecovers the ready branch, which does not touchstatus.stateat all.Not addressed here because it needs a production change: a typed-nil
*nvidiav1alpha1.GPUClustersatisfies the type assertion inSetConditionsReadyandSetConditionsErrorand then panics oncr.Name. All three updaters in this packageshare that gap.
Checklist
make lint) — no new findings.make lintdoes report 3pre-existing
staticcheckdeprecation warnings incontrollers/object_controls.goand
internal/state/driver_volumes.go; both files are untouched by this PR and thewarnings are present on
main.make validate-generated-assets)make validate-modules)Testing
Unit tests only.
-racegpucluster.goat 100% of statements, every function; package 64.6% to 100.0%gpucluster.goand confirminga test fails: 11 mutants, all killed. One assertion did not start out live.
GetErrormatched only the wrapper text, so a%wto%vregression survived; itnow asserts
apierrors.IsNotFoundthrough the wrapper.RetryOnConflictcountsGetcalls as well as update calls, which is what distinguishes a refetch insidethe retry loop from one hoisted out of it.