Add unit tests for internal/state - #2658
Conversation
6d26bb8 to
dbc251d
Compare
7fa5f4c to
6c16861
Compare
|
@abrarshivani thanks for these changes. Can we rebase it to latest main? We can then get these reviewed again and merged. |
|
@rahulait sure. |
6c16861 to
2976b5b
Compare
|
Can you run |
|
Thanks @abrarshivani for trying that out. So FYI @kvalliyurnatt The |
97486b2 to
07b3479
Compare
📝 WalkthroughWalkthroughAdds comprehensive unit-test coverage for the Merge Risk: 🟡 Moderate · up to This test-only change does not alter runtime code, but it adds coverage that preserves behaviors which can report incomplete rollouts as ready, leave managed resources behind after permission errors, collide DaemonSet names, or apply incomplete configuration while reporting success. Merge should wait for those behaviors to be fixed or explicitly accepted, along with the remaining repository validation checks. 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: 5
Note
Quiet mode is enabled, so only the most important comments were posted inline. Other review comments are grouped below.
🟡 Other comments (1)
internal/state/driver_sync_test.go-232-233 (1)
232-233: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAssert the required rendered object.
require.NotEmptypasses if rendering omits the DaemonSet and returns only another manifest object. Require a DaemonSet and validate a stable behavior such as its name or driver-container image.As per path instructions, flag assertions that still pass when the behavior under test is broken.
Source: Path instructions
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: QUIET
Plan: Team
Run ID: 1cc048a7-4a87-4b03-a888-36cb2d7a30ee
📒 Files selected for processing (7)
internal/state/driver_cleanup_test.gointernal/state/driver_manifest_test.gointernal/state/driver_sync_test.gointernal/state/info_source_test.gointernal/state/manager_test.gointernal/state/state_skel_reconcile_test.gointernal/state/state_skel_test.go
Included review availability: Your plan provides up to 8 included reviews per hour; 7 remain after this review.
|
|
||
| // Truncation cuts inside the CR name, so the "-<osVersion>" suffix is dropped | ||
| // entirely: one over-long CR name collides across every OS. | ||
| assert.Equal(t, "nvidia-gpu-driver-"+strings.Repeat("a", 253-len("nvidia-gpu-driver-")), name) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Preserve the OS suffix for long valid CR names.
A legal 253-character NVIDIADriver name also removes the OS suffix. Two node pools for that CR can then render the same DaemonSet name. One pool can replace or prevent the other pool's DaemonSet.
Reserve space for the OS suffix before truncating the CR-name portion. Test the 253-character Kubernetes name boundary and assert distinct names per OS.
| ctx := log.IntoContext(context.Background(), logger) | ||
|
|
||
| objects, err := driverState.getManifestObjects(ctx, driverCR, driverInfoCatalog(fakeClusterInfo{})) | ||
| require.NoError(t, err) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Return the additional-configuration error.
This test expects a missing RepoConfig to log an error and continue. Sync can then apply a DaemonSet without the requested repository configuration and suppress the retry that would repair the CR state.
Return the error from the production rendering path. Change this test to require that error.
As per path instructions, flag “an error that is logged and then returned as success.”
Source: Path instructions
| skel := &stateSkel{} | ||
| ready, err := skel.isDaemonSetReady(toUnstructuredDaemonSet(t, staleDaemonSet), logr.Discard()) | ||
| require.NoError(t, err) | ||
| require.True(t, ready, "known gap: stale-generation status is currently treated as ready") |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Reject stale DaemonSet status.
Line 178 accepts Ready when ObservedGeneration is lower than Generation. This test passes while a stale DaemonSet status advances reconciliation. Expect false, and update isDaemonSetReady to reject this status.
Source: Path instructions
| Spec: appsv1.DeploymentSpec{Replicas: ptr.To[int32](1)}, | ||
| Status: appsv1.DeploymentStatus{Replicas: 2, UpdatedReplicas: 1, AvailableReplicas: 1}, | ||
| }, | ||
| expectedReady: true, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Do not accept the old Deployment replica.
Line 250 accepts Ready while an old replica remains during rollout. This test passes while isDeploymentReady reports completion before the rollout has fully drained. Expect false, and update the readiness predicate.
Source: Path instructions
| syncState, err := skel.handleStateObjectsDeletion(ctx) | ||
| require.NoError(t, err) | ||
| assert.Equal(t, SyncState(SyncStateIgnore), syncState) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Propagate the Forbidden List error.
These assertions accept Ignore after List returns Forbidden. The state-owned DaemonSet remains present, and reconciliation does not retry the cleanup. Return an error with SyncStateError, then update this test to require that result.
Source: Path instructions
07b3479 to
0c71f08
Compare
@tariq1890 I think this PR does not have my changes yet https://github.com/abrarshivani/gpu-operator/blob/07b3479e617c24ed20c6a86c3601e2328448767b/.golangci.yml , I checked locally with a rebase it does show the modernize changes, so I think a rebase should fix it |
Ah ok, I'd assumed that this was already rebased on top of latest main. Thanks for pointing that out! |
0c71f08 to
cd584a7
Compare
Covers the package at 93.9% of statements. Tests only, no production
source changed.
The tests use the controller-runtime fake client with interceptor.Funcs
to inject Get/List/Create/Update/Delete failures, a fake manager, a fake
REST mapper, and the real manifest renderer, so the error paths exercise
real behaviour rather than restating the implementation.
A few blocks encode behaviour that looks questionable but is out of scope
for a tests-only change, so they are pinned by characterization tests
instead: isDaemonSetReady does not re-check ObservedGeneration on the
nonzero-desired path, isDeploymentReady does not require Status.Replicas
to equal desired, and deleteStateRelatedObjects treats a Forbidden List
as nothing to clean up on the invariant that the operator cannot have
created objects of a kind it cannot list. Each is asserted so that
closing the gap later fails the test and becomes a conscious decision.
Some paths are not reachable from a unit test and are left uncovered:
the predicate closure inside GetWatchSources only runs under a live
informer, and the NewManager and newNVIDIADriverStates success returns
need the hardcoded /opt/gpu-operator/manifests/state-driver path, which
is a string literal rather than an overridable var.
go fix is applied to these files, so they use new(x) over ptr.To(x) and
any over interface{}. It is limited to the files added here; running it
across the repo rewrites 48 files including production sources, which
belongs in its own pass.
Signed-off-by: Abrar Shivani <ashivani@nvidia.com>
cd584a7 to
142c46d
Compare
5 participants
Description
Adds unit tests across
internal/state, bringing statement coverage to 93.9%. Tests only, no production source changed.They use the controller-runtime fake client with
interceptor.Funcsto inject Get/List/Create/Update/Delete failures, a fake manager, a fake REST mapper, and the real manifest renderer, so error paths exercise real behaviour rather than restating the implementation.The remaining uncovered statements aren't reachable from a unit test: the
GetWatchSourcespredicate closure needs a live informer, theNewManager/newNVIDIADriverStatessuccess returns need the hardcoded/opt/gpu-operator/manifests/state-driverpath, and a handful of in-loop error branches can't fire with a validNVIDIADriverspec.Behaviours pinned rather than fixed
Out of scope for a tests-only change, so each is pinned by a characterization test — closing the gap later fails the test and becomes a conscious decision.
isDaemonSetReadyskips theObservedGenerationre-check on the nonzero-desired path, so a previous generation's status can report ready right after a spec change.isDeploymentReadydoesn't requireStatus.Replicas == desired, unlikeDeploymentComplete.deleteStateRelatedObjectstreats aForbiddenList as nothing to clean up.state_skel.godocuments this as intentional; the residual risk is list permission revoked after creation.getOSTagerror branch innodepool.gois dead —getOSTagnever returns a non-nil error.Happy to open issues for these and link them from the test comments.
Commits
ptr.To(x)→new(x), 6interface{}→any. Scoped to this PR's files;go fix ./...rewrites 48 files repo-wide including this package's production sources, which belongs in its own pass.Checklist
make lint)make validate-generated-assets)make validate-modules)Testing