test: add NotFound/helm-delete path tests for DataProcess status han…#6068
test: add NotFound/helm-delete path tests for DataProcess status han…#6068adityaupasani2 wants to merge 2 commits into
Conversation
…dlers Add unit tests covering the job/CronJob NotFound code path in all three DataProcess status handlers: - TestOnceGetOperationStatusJobNotFound: Job not found triggers helm release deletion and returns unchanged status (no error) - TestOnEventGetOperationStatusJobNotFound: same for OnEventStatusHandler - TestCronGetOperationStatusCronJobNotFound: CronJob not found triggers helm release deletion and returns unchanged status Uses gomonkey.ApplyFunc to patch helm.DeleteReleaseIfExists so the tests do not require the ddc-helm binary (which is not available in unit test environments). This was promised as a follow-up to PR fluid-cloudnative#5969 (review comment by cheyang). Closes fluid-cloudnative#6066 Signed-off-by: Aditya Upasani <adityaupasani29@gmail.com>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 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 |
|
Hi @adityaupasani2. Thanks for your PR. I'm waiting for a fluid-cloudnative member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds unit tests to verify the behavior of the status handlers when a job or cronjob is not found. The review feedback highlights that the tests mock helm.DeleteReleaseIfExists but do not assert that it was actually called with the correct release name and namespace. Additionally, in TestCronGetOperationStatusCronJobNotFound, the context ctx is initialized without a namespace, which would cause the deletion to be requested in an empty namespace instead of "default". Code suggestions are provided to add these assertions and fix the context initialization.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| func TestOnceGetOperationStatusJobNotFound(t *testing.T) { | ||
| testScheme := runtime.NewScheme() | ||
| _ = v1alpha1.AddToScheme(testScheme) | ||
| _ = batchv1.AddToScheme(testScheme) | ||
|
|
||
| // Patch helm.DeleteReleaseIfExists to avoid shelling out to ddc-helm binary | ||
| helmPatch := gomonkey.ApplyFunc(helm.DeleteReleaseIfExists, func(name, namespace string) error { | ||
| return nil | ||
| }) | ||
| defer helmPatch.Reset() | ||
|
|
||
| mockDataProcess := v1alpha1.DataProcess{ | ||
| ObjectMeta: v1.ObjectMeta{ | ||
| Name: "test", | ||
| Namespace: "default", | ||
| }, | ||
| Spec: v1alpha1.DataProcessSpec{ | ||
| Policy: v1alpha1.Once, | ||
| }, | ||
| Status: v1alpha1.OperationStatus{ | ||
| Phase: common.PhasePending, | ||
| }, | ||
| } | ||
|
|
||
| // No job in fake client - simulates NotFound | ||
| client := fake.NewFakeClientWithScheme(testScheme, &mockDataProcess) | ||
| handler := &OnceStatusHandler{Client: client, dataProcess: &mockDataProcess} | ||
| ctx := cruntime.ReconcileRequestContext{ | ||
| NamespacedName: types.NamespacedName{Namespace: "default", Name: ""}, | ||
| Log: fake.NullLogger(), | ||
| } | ||
|
|
||
| // When job is not found, helm release is deleted and we get early return with unchanged status | ||
| opStatus, err := handler.GetOperationStatus(ctx, &mockDataProcess.Status) | ||
| if err != nil { | ||
| t.Errorf("unexpected error on NotFound path: %v", err) | ||
| } | ||
| if opStatus == nil { | ||
| t.Error("expected non-nil opStatus") | ||
| } | ||
| if opStatus.Phase != common.PhasePending { | ||
| t.Errorf("expected phase %s, got %s", common.PhasePending, opStatus.Phase) | ||
| } | ||
| } |
There was a problem hiding this comment.
The unit test mocks helm.DeleteReleaseIfExists but does not assert that it was actually called, nor does it verify that the correct release name and namespace were passed. Adding assertions to verify the mock call ensures that the helm release deletion path is actually executed and prevents false positives (e.g., if the deletion call is accidentally removed or bypassed in the handler).
func TestOnceGetOperationStatusJobNotFound(t *testing.T) {
testScheme := runtime.NewScheme()
_ = v1alpha1.AddToScheme(testScheme)
_ = batchv1.AddToScheme(testScheme)
// Patch helm.DeleteReleaseIfExists to avoid shelling out to ddc-helm binary
var called bool
var calledName, calledNamespace string
helmPatch := gomonkey.ApplyFunc(helm.DeleteReleaseIfExists, func(name, namespace string) error {
called = true
calledName = name
calledNamespace = namespace
return nil
})
defer helmPatch.Reset()
mockDataProcess := v1alpha1.DataProcess{
ObjectMeta: v1.ObjectMeta{
Name: "test",
Namespace: "default",
},
Spec: v1alpha1.DataProcessSpec{
Policy: v1alpha1.Once,
},
Status: v1alpha1.OperationStatus{
Phase: common.PhasePending,
},
}
// No job in fake client - simulates NotFound
client := fake.NewFakeClientWithScheme(testScheme, &mockDataProcess)
handler := &OnceStatusHandler{Client: client, dataProcess: &mockDataProcess}
ctx := cruntime.ReconcileRequestContext{
NamespacedName: types.NamespacedName{Namespace: "default", Name: ""},
Log: fake.NullLogger(),
}
// When job is not found, helm release is deleted and we get early return with unchanged status
opStatus, err := handler.GetOperationStatus(ctx, &mockDataProcess.Status)
if err != nil {
t.Errorf("unexpected error on NotFound path: %v", err)
}
if opStatus == nil {
t.Error("expected non-nil opStatus")
}
if opStatus.Phase != common.PhasePending {
t.Errorf("expected phase %s, got %s", common.PhasePending, opStatus.Phase)
}
if !called {
t.Error("expected helm.DeleteReleaseIfExists to be called")
}
expectedReleaseName := utils.GetDataProcessReleaseName(mockDataProcess.GetName())
if calledName != expectedReleaseName || calledNamespace != "default" {
t.Errorf("expected helm.DeleteReleaseIfExists to be called with (%s, %s), got (%s, %s)", expectedReleaseName, "default", calledName, calledNamespace)
}
}| func TestOnEventGetOperationStatusJobNotFound(t *testing.T) { | ||
| testScheme := runtime.NewScheme() | ||
| _ = v1alpha1.AddToScheme(testScheme) | ||
| _ = batchv1.AddToScheme(testScheme) | ||
|
|
||
| // Patch helm.DeleteReleaseIfExists to avoid shelling out to ddc-helm binary | ||
| helmPatch := gomonkey.ApplyFunc(helm.DeleteReleaseIfExists, func(name, namespace string) error { | ||
| return nil | ||
| }) | ||
| defer helmPatch.Reset() | ||
|
|
||
| mockDataProcess := v1alpha1.DataProcess{ | ||
| ObjectMeta: v1.ObjectMeta{ | ||
| Name: "test", | ||
| Namespace: "default", | ||
| }, | ||
| Spec: v1alpha1.DataProcessSpec{ | ||
| Policy: v1alpha1.OnEvent, | ||
| }, | ||
| Status: v1alpha1.OperationStatus{ | ||
| Phase: common.PhasePending, | ||
| }, | ||
| } | ||
|
|
||
| // No job in fake client - simulates NotFound | ||
| client := fake.NewFakeClientWithScheme(testScheme, &mockDataProcess) | ||
| handler := &OnEventStatusHandler{Client: client, dataProcess: &mockDataProcess} | ||
| ctx := cruntime.ReconcileRequestContext{ | ||
| NamespacedName: types.NamespacedName{Namespace: "default", Name: ""}, | ||
| Log: fake.NullLogger(), | ||
| } | ||
|
|
||
| // When job is not found, helm release is deleted and we get early return with unchanged status | ||
| opStatus, err := handler.GetOperationStatus(ctx, &mockDataProcess.Status) | ||
| if err != nil { | ||
| t.Errorf("unexpected error on NotFound path: %v", err) | ||
| } | ||
| if opStatus == nil { | ||
| t.Error("expected non-nil opStatus") | ||
| } | ||
| if opStatus.Phase != common.PhasePending { | ||
| t.Errorf("expected phase %s, got %s", common.PhasePending, opStatus.Phase) | ||
| } | ||
| } |
There was a problem hiding this comment.
The unit test mocks helm.DeleteReleaseIfExists but does not assert that it was actually called, nor does it verify that the correct release name and namespace were passed. Adding assertions to verify the mock call ensures that the helm release deletion path is actually executed and prevents false positives.
func TestOnEventGetOperationStatusJobNotFound(t *testing.T) {
testScheme := runtime.NewScheme()
_ = v1alpha1.AddToScheme(testScheme)
_ = batchv1.AddToScheme(testScheme)
// Patch helm.DeleteReleaseIfExists to avoid shelling out to ddc-helm binary
var called bool
var calledName, calledNamespace string
helmPatch := gomonkey.ApplyFunc(helm.DeleteReleaseIfExists, func(name, namespace string) error {
called = true
calledName = name
calledNamespace = namespace
return nil
})
defer helmPatch.Reset()
mockDataProcess := v1alpha1.DataProcess{
ObjectMeta: v1.ObjectMeta{
Name: "test",
Namespace: "default",
},
Spec: v1alpha1.DataProcessSpec{
Policy: v1alpha1.OnEvent,
},
Status: v1alpha1.OperationStatus{
Phase: common.PhasePending,
},
}
// No job in fake client - simulates NotFound
client := fake.NewFakeClientWithScheme(testScheme, &mockDataProcess)
handler := &OnEventStatusHandler{Client: client, dataProcess: &mockDataProcess}
ctx := cruntime.ReconcileRequestContext{
NamespacedName: types.NamespacedName{Namespace: "default", Name: ""},
Log: fake.NullLogger(),
}
// When job is not found, helm release is deleted and we get early return with unchanged status
opStatus, err := handler.GetOperationStatus(ctx, &mockDataProcess.Status)
if err != nil {
t.Errorf("unexpected error on NotFound path: %v", err)
}
if opStatus == nil {
t.Error("expected non-nil opStatus")
}
if opStatus.Phase != common.PhasePending {
t.Errorf("expected phase %s, got %s", common.PhasePending, opStatus.Phase)
}
if !called {
t.Error("expected helm.DeleteReleaseIfExists to be called")
}
expectedReleaseName := utils.GetDataProcessReleaseName(mockDataProcess.GetName())
if calledName != expectedReleaseName || calledNamespace != "default" {
t.Errorf("expected helm.DeleteReleaseIfExists to be called with (%s, %s), got (%s, %s)", expectedReleaseName, "default", calledName, calledNamespace)
}
}| func TestCronGetOperationStatusCronJobNotFound(t *testing.T) { | ||
| testScheme := runtime.NewScheme() | ||
| _ = v1alpha1.AddToScheme(testScheme) | ||
| _ = batchv1.AddToScheme(testScheme) | ||
|
|
||
| patch := gomonkey.ApplyFunc(compatibility.IsBatchV1CronJobSupported, func() bool { | ||
| return true | ||
| }) | ||
| defer patch.Reset() | ||
|
|
||
| // Patch helm.DeleteReleaseIfExists to avoid shelling out to ddc-helm binary | ||
| helmPatch := gomonkey.ApplyFunc(helm.DeleteReleaseIfExists, func(name, namespace string) error { | ||
| return nil | ||
| }) | ||
| defer helmPatch.Reset() | ||
|
|
||
| mockDataProcess := v1alpha1.DataProcess{ | ||
| ObjectMeta: v1.ObjectMeta{ | ||
| Name: "test", | ||
| Namespace: "default", | ||
| }, | ||
| Spec: v1alpha1.DataProcessSpec{ | ||
| Policy: v1alpha1.Cron, | ||
| Schedule: "* * * * *", | ||
| }, | ||
| Status: v1alpha1.OperationStatus{ | ||
| Phase: common.PhasePending, | ||
| }, | ||
| } | ||
|
|
||
| // No CronJob in fake client - simulates NotFound | ||
| client := fake.NewFakeClientWithScheme(testScheme, &mockDataProcess) | ||
| handler := &CronStatusHandler{Client: client, dataProcess: &mockDataProcess} | ||
| ctx := cruntime.ReconcileRequestContext{Log: fake.NullLogger()} | ||
|
|
||
| // When CronJob is not found, helm release is deleted and we get early return with unchanged status | ||
| opStatus, err := handler.GetOperationStatus(ctx, &mockDataProcess.Status) | ||
| if err != nil { | ||
| t.Errorf("unexpected error on NotFound path: %v", err) | ||
| } | ||
| if opStatus == nil { | ||
| t.Error("expected non-nil opStatus") | ||
| } | ||
| if opStatus.Phase != common.PhasePending { | ||
| t.Errorf("expected phase %s, got %s", common.PhasePending, opStatus.Phase) | ||
| } | ||
| } |
There was a problem hiding this comment.
There are two issues here:
- The unit test mocks
helm.DeleteReleaseIfExistsbut does not assert that it was actually called, nor does it verify that the correct release name and namespace were passed. - The
ctxcontext is initialized without a namespace (ctx := cruntime.ReconcileRequestContext{Log: fake.NullLogger()}). However,CronStatusHandler.GetOperationStatususesctx.Namespacewhen callinghelm.DeleteReleaseIfExists(releaseName, ctx.Namespace). This means the deletion would be requested in an empty namespace""instead of"default".
Initializing ctx with the correct namespace and adding assertions to verify the mock call makes the test robust and correct.
func TestCronGetOperationStatusCronJobNotFound(t *testing.T) {
testScheme := runtime.NewScheme()
_ = v1alpha1.AddToScheme(testScheme)
_ = batchv1.AddToScheme(testScheme)
patch := gomonkey.ApplyFunc(compatibility.IsBatchV1CronJobSupported, func() bool {
return true
})
defer patch.Reset()
// Patch helm.DeleteReleaseIfExists to avoid shelling out to ddc-helm binary
var called bool
var calledName, calledNamespace string
helmPatch := gomonkey.ApplyFunc(helm.DeleteReleaseIfExists, func(name, namespace string) error {
called = true
calledName = name
calledNamespace = namespace
return nil
})
defer helmPatch.Reset()
mockDataProcess := v1alpha1.DataProcess{
ObjectMeta: v1.ObjectMeta{
Name: "test",
Namespace: "default",
},
Spec: v1alpha1.DataProcessSpec{
Policy: v1alpha1.Cron,
Schedule: "* * * * *",
},
Status: v1alpha1.OperationStatus{
Phase: common.PhasePending,
},
}
// No CronJob in fake client - simulates NotFound
client := fake.NewFakeClientWithScheme(testScheme, &mockDataProcess)
handler := &CronStatusHandler{Client: client, dataProcess: &mockDataProcess}
ctx := cruntime.ReconcileRequestContext{
NamespacedName: types.NamespacedName{Namespace: "default", Name: ""},
Log: fake.NullLogger(),
}
// When CronJob is not found, helm release is deleted and we get early return with unchanged status
opStatus, err := handler.GetOperationStatus(ctx, &mockDataProcess.Status)
if err != nil {
t.Errorf("unexpected error on NotFound path: %v", err)
}
if opStatus == nil {
t.Error("expected non-nil opStatus")
}
if opStatus.Phase != common.PhasePending {
t.Errorf("expected phase %s, got %s", common.PhasePending, opStatus.Phase)
}
if !called {
t.Error("expected helm.DeleteReleaseIfExists to be called")
}
expectedReleaseName := utils.GetDataProcessReleaseName(mockDataProcess.GetName())
if calledName != expectedReleaseName || calledNamespace != "default" {
t.Errorf("expected helm.DeleteReleaseIfExists to be called with (%s, %s), got (%s, %s)", expectedReleaseName, "default", calledName, calledNamespace)
}
}
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #6068 +/- ##
=======================================
Coverage 64.77% 64.77%
=======================================
Files 484 484
Lines 33892 33892
=======================================
Hits 21954 21954
Misses 10215 10215
Partials 1723 1723 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
| if opStatus == nil { | ||
| t.Error("expected non-nil opStatus") | ||
| } | ||
| if opStatus.Phase != common.PhasePending { |
There was a problem hiding this comment.
The lint and staticcheck jobs are red on this PR because of SA5011 here (also at lines 515 and 563). After if opStatus == nil { t.Error(...) } you fall through and immediately read opStatus.Phase without return, so staticcheck flags this as a possible nil pointer dereference. The pattern needs to either short-circuit (t.Fatal(...) / return) or guard the phase check inside an else. PR is currently BLOCKED on Project Check because of these three findings.
| if opStatus == nil { | ||
| t.Error("expected non-nil opStatus") | ||
| } | ||
| if opStatus.Phase != common.PhasePending { |
There was a problem hiding this comment.
Same SA5011 pattern as the Once case: line 512 checks opStatus == nil and continues, then line 515 dereferences opStatus.Phase. This is the second of three lint failures gating the PR.
| if opStatus == nil { | ||
| t.Error("expected non-nil opStatus") | ||
| } | ||
| if opStatus.Phase != common.PhasePending { |
There was a problem hiding this comment.
Same pattern, third occurrence: nil check at 560 does not return/t.Fatal, so 563 still dereferences opStatus.Phase. Once all three are fixed (e.g., switch to t.Fatal for the nil branch), staticcheck and lint should turn green and merge can unblock.
| // No CronJob in fake client - simulates NotFound | ||
| client := fake.NewFakeClientWithScheme(testScheme, &mockDataProcess) | ||
| handler := &CronStatusHandler{Client: client, dataProcess: &mockDataProcess} | ||
| ctx := cruntime.ReconcileRequestContext{Log: fake.NullLogger()} |
There was a problem hiding this comment.
ctx here is built without a Namespace, but in production CronStatusHandler.GetOperationStatus passes ctx.Namespace to helm.DeleteReleaseIfExists (status_handler.go:113). With your no-op patch the test still passes, but it would also pass if the production code silently called DeleteReleaseIfExists(releaseName, ""). Setting NamespacedName: types.NamespacedName{Namespace: "default"} (as the Once/OnEvent tests do) makes the test reflect the real call signature and matches the Gemini bot's observation.
| _ = batchv1.AddToScheme(testScheme) | ||
|
|
||
| // Patch helm.DeleteReleaseIfExists to avoid shelling out to ddc-helm binary | ||
| helmPatch := gomonkey.ApplyFunc(helm.DeleteReleaseIfExists, func(name, namespace string) error { |
There was a problem hiding this comment.
The whole point of these tests is to prove the NotFound branch reaches helm.DeleteReleaseIfExists. Right now the patched stub just returns nil and is never checked, so a future refactor that removes the delete call entirely would still keep the test green. Capturing the call (e.g., flip a local called bool or record name/namespace and assert them after GetOperationStatus returns) would lock in the contract this PR is meant to cover. The Gemini review flagged the same gap.
…ests - Replace t.Error with t.Fatal for nil opStatus checks to prevent nil pointer dereference (SA5011 staticcheck finding that was blocking lint/staticcheck CI) - Add called bool + name/namespace capture to verify helm.DeleteReleaseIfExists is actually invoked with the correct release name and namespace, so a future removal of the delete call would fail the test - Fix CronStatusHandler test: set ctx.Namespace to "default" so it matches the namespace passed to helm.DeleteReleaseIfExists in production code (ctx.Namespace), making the assertion meaningful Addresses cheyang and Gemini review comments on fluid-cloudnative#6068 Signed-off-by: Aditya Upasani <adityaupasani29@gmail.com>
|
2 participants
Ⅰ. Describe what this PR does
Adds unit tests covering the job/CronJob NotFound code path in all three DataProcess status handlers — the path that was flagged as untested in the PR #5969 review.
When the job or CronJob is not found, each handler deletes the helm release and returns early with the unchanged status and no error. Previously this path had zero coverage.
Uses
gomonkey.ApplyFuncto patchhelm.DeleteReleaseIfExistsso the tests do not require theddc-helmbinary (which is not available in unit test environments — the same approach used forcompatibility.IsBatchV1CronJobSupportedin the existing Cron tests).Ⅱ. Does this pull request fix one issue?
Closes #6066
Ⅲ. List the added test cases
TestOnceGetOperationStatusJobNotFound— Job not found → helm release deleted, unchanged status returned, no errorTestOnEventGetOperationStatusJobNotFound— same for OnEventStatusHandlerTestCronGetOperationStatusCronJobNotFound— CronJob not found → helm release deleted, unchanged status returned, no errorⅣ. Describe how to verify it
Note:
pkg/controllers/v1alpha1/dataprocesshas a pre-existingBeforeSuiteGinkgo decorator failure insuite_test.gothat prevents the test runner from executing any tests in this package locally. This is present on upstream master too and is unrelated to this PR. The tests compile cleanly (go build,go vetpass).Ⅴ. Special notes for reviews
This was promised as a follow-up to PR #5969 (review comment by cheyang).