test(dataprocess): migrate package tests and fix status handling#5739
test(dataprocess): migrate package tests and fix status handling#5739hxrshxz wants to merge 2 commits intofluid-cloudnative:masterfrom
Conversation
Signed-off-by: Harsh <harshmastic@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 @hxrshxz. 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 introduces comprehensive unit tests for the DataProcess controller and its operations using the Ginkgo and Gomega frameworks. It also includes a bug fix in the status handler to correctly identify the finished job condition from the status conditions slice, rather than assuming it is the first element. Additionally, the test suite has been simplified by removing the dependency on envtest. Feedback was provided regarding redundant code in the test files that can be removed to improve clarity.
| // Ensure fake.NullLogger is usable in tests (imported via suite_test.go BeforeSuite). | ||
| var _ = fake.NullLogger |
There was a problem hiding this comment.
This line and its accompanying comment are unnecessary and potentially confusing. The fake package is already imported and used on line 42 for fake.NewFakeClientWithScheme, so this line isn't needed to prevent an 'unused import' error. The test logger is configured globally in suite_test.go. This block appears to be dead code and can be removed to improve clarity.
There was a problem hiding this comment.
Pull request overview
This PR migrates pkg/controllers/v1alpha1/dataprocess/ tests to Ginkgo/Gomega, adds new unit test coverage for the controller and operation implementation, and updates OnceStatusHandler to use the “finished” Job condition (instead of assuming the first condition is terminal) to handle out-of-order Job conditions.
Changes:
- Reworked the test suite bootstrap to use a fake-client setup (instead of envtest) for DataProcess controller unit tests.
- Added Ginkgo/Gomega unit tests for
DataProcessReconcileranddataProcessOperationvalidation/helpers. - Updated
OnceStatusHandler.GetOperationStatusto use the finished Job condition for status updates when Job conditions are out of order.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/controllers/v1alpha1/dataprocess/suite_test.go | Switches suite bootstrap to a lightweight fake-client/Ginkgo setup and standardizes suite naming/logger setup. |
| pkg/controllers/v1alpha1/dataprocess/status_handler_test.go | Migrates and expands status handler tests, including regressions for running/not-found/out-of-order conditions. |
| pkg/controllers/v1alpha1/dataprocess/status_handler.go | Uses the finished Job condition when computing status (fix for out-of-order Job conditions). |
| pkg/controllers/v1alpha1/dataprocess/implement_test.go | Adds unit tests for dataProcessOperation helpers and Validate behavior. |
| pkg/controllers/v1alpha1/dataprocess/controller_test.go | Adds unit tests for reconciler construction, Build(), and Reconcile() paths. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // job either failed or complete, update DataLoad's phase status | ||
| jobCondition := job.Status.Conditions[0] | ||
| jobCondition := *finishedJobCondition | ||
|
|
There was a problem hiding this comment.
GetOperationStatus still returns the original err when the Job is NotFound (even after successfully deleting the Helm release). This makes the NotFound path behave like an error and will conflict with the new unit test expectation that Job-not-found is handled cleanly. Consider returning early (or clearing err) after the NotFound cleanup so the handler returns the unchanged status without error, and avoid logging it as "can't get dataprocess job" in that case.
| Context("when the job is not found", func() { | ||
| It("should delete the helm release and return the unchanged status copy without error", func() { | ||
| // Patch helm.DeleteReleaseIfExists to avoid exec'ing ddc-helm in a unit test. | ||
| patch := gomonkey.ApplyFunc(helm.DeleteReleaseIfExists, | ||
| func(_ string, _ string) error { return nil }) | ||
| defer patch.Reset() | ||
|
|
||
| fakeClient := fake.NewFakeClientWithScheme(testScheme, mockDataProcess) | ||
| handler := &OnceStatusHandler{ | ||
| Client: fakeClient, | ||
| dataProcess: mockDataProcess, | ||
| } | ||
|
|
||
| opStatus := &datav1alpha1.OperationStatus{Phase: common.PhaseExecuting} | ||
| result, err := handler.GetOperationStatus(ctx, opStatus) | ||
| Expect(err).NotTo(HaveOccurred()) | ||
| Expect(result).NotTo(BeNil()) | ||
| Expect(result).NotTo(BeIdenticalTo(opStatus)) | ||
| Expect(result.Phase).To(Equal(common.PhaseExecuting)) | ||
| }) |
There was a problem hiding this comment.
This test asserts that Job-not-found should be handled without error, but the current OnceStatusHandler.GetOperationStatus implementation returns the NotFound error after attempting Helm cleanup. Either adjust the production code to return nil error after the NotFound cleanup, or update this test to match the current behavior.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a comprehensive suite of unit tests for the DataProcess controller and its operations using the Ginkgo and Gomega frameworks. Key changes include the addition of controller_test.go and implement_test.go, and a significant refactoring of status_handler_test.go and suite_test.go to replace the envtest dependency with a more efficient fake client setup. Furthermore, a bug fix was implemented in status_handler.go to ensure the controller correctly identifies the finished job condition rather than simply selecting the first condition in the status list. I have no feedback to provide as there were no review comments.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #5739 +/- ##
=======================================
Coverage 61.22% 61.22%
=======================================
Files 444 444
Lines 30557 30557
=======================================
Hits 18710 18710
Misses 10307 10307
Partials 1540 1540 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
/gemini review |
Signed-off-by: Harsh <harshmastic@gmail.com>
|
There was a problem hiding this comment.
Code Review
This pull request introduces comprehensive unit tests for the DataProcess controller, including tests for the reconciler, operation implementation, and status handling. It also includes a minor fix in the status handler to correctly use the finished job condition. The review feedback highlights several instances where errors from AddToScheme are ignored in test helpers and test setups, recommending the use of panic or Expect(...).To(Succeed()) to ensure robust test execution.
| if s == nil { | ||
| s = runtime.NewScheme() | ||
| _ = datav1alpha1.AddToScheme(s) | ||
| } |
There was a problem hiding this comment.
The potential error from datav1alpha1.AddToScheme(s) is ignored. In a test helper, it's better to panic on error to cause an immediate and clear test failure. This ensures the test setup is robust. A similar issue of ignoring errors from AddToScheme exists on lines 60, 92, and 105, where Expect(...).To(Succeed()) should be used to be consistent with other tests in this PR.
| if s == nil { | |
| s = runtime.NewScheme() | |
| _ = datav1alpha1.AddToScheme(s) | |
| } | |
| if s == nil { | |
| s = runtime.NewScheme() | |
| if err := datav1alpha1.AddToScheme(s); err != nil { | |
| panic(err) | |
| } | |
| } |
| if s == nil { | ||
| s = runtime.NewScheme() | ||
| _ = datav1alpha1.AddToScheme(s) | ||
| } |
There was a problem hiding this comment.
The potential error from datav1alpha1.AddToScheme(s) is ignored. In a test helper, it's better to panic on error to cause an immediate and clear test failure. This ensures the test setup is robust. A similar issue exists on line 235, where Expect(...).To(Succeed()) should be used.
| if s == nil { | |
| s = runtime.NewScheme() | |
| _ = datav1alpha1.AddToScheme(s) | |
| } | |
| if s == nil { | |
| s = runtime.NewScheme() | |
| if err := datav1alpha1.AddToScheme(s); err != nil { | |
| panic(err) | |
| } | |
| } |
|
|
||
| BeforeEach(func() { | ||
| testScheme = runtime.NewScheme() | ||
| _ = datav1alpha1.AddToScheme(testScheme) |
There was a problem hiding this comment.
The potential error from datav1alpha1.AddToScheme(testScheme) is ignored. It's better to handle this to prevent obscure test failures. Please use Expect(datav1alpha1.AddToScheme(testScheme)).To(Succeed()) to make the test fail immediately with a clear message. This is also consistent with other tests in this PR (e.g., in status_handler_test.go).
| _ = datav1alpha1.AddToScheme(testScheme) | |
| Expect(datav1alpha1.AddToScheme(testScheme)).To(Succeed()) |
Ⅰ. Describe what this PR does
Migrate
pkg/controllers/v1alpha1/dataprocess/package tests to Ginkgo/Gomega, add controller and implement coverage, and fixOnceStatusHandlerto use the finished job condition when job conditions are out of order.Ⅱ. Does this pull request fix one issue?
#5676
Ⅲ. List the added test cases (unit test/integration test) if any, please explain if no tests are needed.
controller_test.goforDataProcessReconcilerconstruction, build, and reconcile pathsimplement_test.gofordataProcessOperationvalidation and helper methodsstatus_handler_test.goto Ginkgo/Gomega and added running, not-found, and out-of-order condition regressionssuite_test.goto a fake-client unit-test suite bootstrapⅣ. Describe how to verify it
Run
go test -coverprofile=/tmp/fluid-requested-package.cover ./pkg/controllers/v1alpha1/dataprocess/... -count=1and confirmgo tool cover -func=/tmp/fluid-requested-package.coverreportstotal: 89.0%.Ⅴ. Special notes for reviews
N/A