test(dataset): add Ginkgo unit coverage for dataset controller - #5827
Conversation
|
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. |
There was a problem hiding this comment.
Code Review
This pull request introduces a comprehensive unit test suite for the Dataset controller, replacing the previous integration tests. The new tests cover core reconciliation logic, including finalizer handling and dataset deletion. Feedback focuses on improving test reliability and maintainability by suggesting dependency injection over monkey patching to avoid race conditions, using context.Background() instead of context.TODO(), strengthening error assertions for DNS validation, and ensuring deterministic checks for finalizer removal in the fake client.
| scaleoutPatch = gomonkey.ApplyFunc(deploy.ScaleoutRuntimeControllerOnDemand, | ||
| func(client.Client, types.NamespacedName, logr.Logger) (string, bool, error) { | ||
| return "", false, nil | ||
| }) |
There was a problem hiding this comment.
Using gomonkey to patch global functions can lead to race conditions and flaky tests if they are executed in parallel (e.g., using ginkgo -p). It also requires disabling compiler inlining to work reliably. A more robust approach for long-term maintainability would be to refactor DatasetReconciler to use dependency injection (e.g., an interface) for the scale-out logic.
| Recorder: record.NewFakeRecorder(16), | ||
| } | ||
|
|
||
| result, err := r.Reconcile(context.TODO(), ctrl.Request{ |
There was a problem hiding this comment.
In unit tests, context.Background() is generally preferred over context.TODO(). context.TODO() is intended for use when it's unclear which context to use, whereas tests should typically use a clean background context. This applies to multiple locations throughout this file.
| result, err := r.Reconcile(context.TODO(), ctrl.Request{ | |
| result, err := r.Reconcile(context.Background(), ctrl.Request{ |
| NamespacedName: types.NamespacedName{Name: invalidName, Namespace: "default"}, | ||
| }) | ||
|
|
||
| Expect(err).To(HaveOccurred()) |
There was a problem hiding this comment.
It is better to verify the specific error returned to ensure the test fails for the expected reason (DNS-1035 validation) rather than any other potential error during reconciliation.
| Expect(err).To(HaveOccurred()) | |
| Expect(err).To(MatchError(ContainSubstring("metadata.name"))) |
| err = r.Get(context.TODO(), types.NamespacedName{Name: storedDataset.Name, Namespace: storedDataset.Namespace}, updatedDataset) | ||
| if err == nil { | ||
| Expect(updatedDataset.GetFinalizers()).NotTo(ContainElement(finalizer)) | ||
| } else { | ||
| Expect(client.IgnoreNotFound(err)).To(BeNil()) | ||
| } |
There was a problem hiding this comment.
This assertion is non-deterministic. Since the fake client typically does not simulate the asynchronous deletion logic of the Kubernetes API server (where objects are removed after finalizers are cleared), you should explicitly assert the expected state in the fake client's cache. If the object is expected to persist without the finalizer, assert that err is nil and the finalizer is absent.
Expect(r.Get(context.Background(), types.NamespacedName{Name: storedDataset.Name, Namespace: storedDataset.Namespace}, updatedDataset)).To(Succeed())
Expect(updatedDataset.GetFinalizers()).NotTo(ContainElement(finalizer))There was a problem hiding this comment.
Pull request overview
This PR migrates the dataset controller package toward the repository’s newer Ginkgo/Gomega unit-testing approach as part of the broader testing modernization work in #5676. It removes the package’s old envtest-heavy suite and replaces it with focused unit coverage around DatasetReconciler behavior.
Changes:
- Replace the dataset package’s envtest-based
suite_test.gowith a minimal Ginkgo suite bootstrap. - Add package-scoped unit tests for dataset reconcile paths such as not-found handling, name validation, finalizer behavior, phase initialization, deletion flows, datasetRef pruning, and reference-dataset runtime creation.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
pkg/controllers/v1alpha1/dataset/suite_test.go |
Simplifies the package test bootstrap to a minimal Ginkgo suite runner. |
pkg/controllers/v1alpha1/dataset/dataset_controller_ut_test.go |
Adds new unit tests covering key DatasetReconciler helper and reconcile scenarios. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| DeletionTimestamp: &deletionTime, | ||
| }, | ||
| Status: datav1alpha1.DatasetStatus{ | ||
| DatasetRef: []string{"consumer-a"}, |
| Context: context.TODO(), | ||
| Dataset: currentDataset, |
| Context: context.TODO(), | ||
| Dataset: currentDataset, |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #5827 +/- ##
==========================================
+ Coverage 59.10% 66.29% +7.19%
==========================================
Files 480 490 +10
Lines 32512 35812 +3300
==========================================
+ Hits 19215 23742 +4527
+ Misses 11747 10330 -1417
- Partials 1550 1740 +190 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
cheyang
left a comment
There was a problem hiding this comment.
感谢贡献！建议：
- AfterEach 只重置 scaleoutPatch，但部分 It 内还创建了其他 gomonkey patches（如 kubeclient 相关）。请确认所有 patches 都在对应 It 内用 defer Reset() 清理，或在 AfterEach 中统一管理。
- 建议补充 Reconcile 中 dataset found 但 ScaleoutRuntimeControllerOnDemand 返回错误时的场景覆盖。
This comment was marked as duplicate.
This comment was marked as duplicate.
Sorry, something went wrong.
This comment has been minimized.
This comment has been minimized.
|
English translation of the previous review comments:
|
|
This PR has merge conflicts with the base branch. Please rebase your branch onto the latest master and resolve the conflicts so we can proceed with the review. |
Signed-off-by: Harsh <harshmastic@gmail.com>
Signed-off-by: Harsh <harshmastic@gmail.com>
Signed-off-by: Harsh <harshmastic@gmail.com>
221f3b6 to
70a1c3a
Compare
|
|
Tightened the patch cleanup so non- I also added coverage for the Reconcile path where the dataset is found but |
|
/ok-to-test |
|
Solid controller unit test coverage. Tests cover not-found, DNS-1035 validation, finalizer lifecycle, phase initialization, scaleout requeue, deletion with finalizer removal, datasetRef pruning, and reference dataset runtime creation. Good use of gomonkey for isolating ScaleoutRuntimeControllerOnDemand. The suite_test.go simplification from envtest to fake-client is a welcome improvement for test speed. LGTM. |
|
@cheyang: The specified target(s) for
DetailsIn response to this:
Instructions 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. |
|
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: cheyang The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
I. Describe what this PR does
Add package-scoped Ginkgo v2 + Gomega unit coverage for the dataset controller and replace the dataset package’s legacy envtest-heavy suite with a minimal package bootstrap.
II. Does this pull request fix one issue?
#5676
III. List the added test cases (unit test/integration test) if any, please explain if no tests are needed.
Adds dataset controller unit tests covering not-found reconcile, invalid name validation, finalizer add/requeue, phase initialization, scaleout requeue behavior, deletion/finalizer removal, datasetRef pruning, and reference dataset runtime creation.
IV. Describe how to verify it
Run the dataset controller package tests and confirm the package coverage stays above 75 percent.
V. Special notes for reviews
N/A