test(databackup): migrate controller tests to Ginkgo/Gomega#5713
test(databackup): migrate controller tests to Ginkgo/Gomega#5713hxrshxz wants to merge 2 commits intofluid-cloudnative:masterfrom
Conversation
- Add suite_test.go with package-level Ginkgo bootstrap (TestAPIs) - Migrate status_handler_test.go from stdlib testing to Ginkgo/Gomega - Add databackup_controller_test.go: NewDataBackupReconciler, Build, ControllerName - Add implement_test.go: covers all dataBackupOperation methods including GetOperationObject, GetChartsDirectory, HasPrecedingOperation, Validate, UpdateStatusInfoForCompleted, GetTTL, GetStatusHandler, GetTargetDataset, UpdateOperationApiStatus, GetOperationType, GetPossibleTargetDatasetNamespacedNames, GetReleaseNameSpacedName, GetParallelTaskNumber, SetTargetDatasetStatusInProgress, RemoveTargetDatasetStatusInProgress Coverage: 73.2% -> 77.5% (exceeds 75% gate) Signed-off-by: Harsh <harshmastic@gmail.com>
|
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. |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly refactors the testing infrastructure for the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
There was a problem hiding this comment.
Pull request overview
Migrates pkg/controllers/v1alpha1/databackup unit tests from testing/testify-style to the Ginkgo/Gomega BDD framework, aligning this controller package with the repo’s newer Ginkgo-based test suites and aiming to improve coverage.
Changes:
- Added a Ginkgo suite entrypoint for the
databackupcontroller test package. - Rewrote
DataBackupReconcilertests using Ginkgo/Gomega. - Rewrote
dataBackupOperationandOnceHandlertests using Ginkgo/Gomega, adding additional scenarios.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| pkg/controllers/v1alpha1/databackup/suite_test.go | Adds Ginkgo suite runner and sets controller-runtime logger to a null logger for tests. |
| pkg/controllers/v1alpha1/databackup/databackup_controller_test.go | Migrates reconciler unit tests to Ginkgo/Gomega. |
| pkg/controllers/v1alpha1/databackup/implement_test.go | Adds Ginkgo/Gomega unit tests covering dataBackupOperation methods. |
| pkg/controllers/v1alpha1/databackup/status_handler_test.go | Migrates OnceHandler status handler tests to Ginkgo/Gomega and expands scenarios. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| It("should return an OnceHandler", func() { | ||
| handler := op.GetStatusHandler() | ||
| Expect(handler).NotTo(BeNil()) | ||
| _, ok := handler.(*OnceHandler) | ||
| Expect(ok).To(BeTrue()) |
There was a problem hiding this comment.
GetStatusHandler() is only asserted to return an *OnceHandler, but the production implementation currently returns &OnceHandler{} without setting dataBackup. Since OnceHandler.GetOperationStatus() dereferences o.dataBackup, using the returned handler as-is will panic. Please either (a) change the production GetStatusHandler() to return a fully initialized handler, or (b) update this test to exercise handler.GetOperationStatus(...) (or at least assert the handler is configured) so this regression is caught.
| It("should return an OnceHandler", func() { | |
| handler := op.GetStatusHandler() | |
| Expect(handler).NotTo(BeNil()) | |
| _, ok := handler.(*OnceHandler) | |
| Expect(ok).To(BeTrue()) | |
| It("should return a properly configured OnceHandler", func() { | |
| handler := op.GetStatusHandler() | |
| Expect(handler).NotTo(BeNil()) | |
| onceHandler, ok := handler.(*OnceHandler) | |
| Expect(ok).To(BeTrue()) | |
| // Ensure the OnceHandler is fully initialized so that GetOperationStatus will not panic. | |
| Expect(onceHandler.dataBackup).NotTo(BeNil()) |
| It("should use conditions LastTransitionTime when conditions are set on succeeded pod", func() { | ||
| conditionTime := v1.Now() | ||
| mockPodWithConditions := corev1.Pod{ | ||
| ObjectMeta: v1.ObjectMeta{ | ||
| Name: "test-pod", |
There was a problem hiding this comment.
This test case name says it verifies LastTransitionTime is used, but it only asserts the phase and that Conditions has length 1. Please assert something that actually depends on conditionTime (e.g., opStatus.Conditions[0].LastTransitionTime equals conditionTime, and/or Duration matches the expected value when CreationTimestamp is set) so the test will fail if the code falls back to time.Now() instead.
![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
The pull request successfully migrates the controller tests for the databackup package to use Ginkgo/Gomega, enhancing the test suite's structure and readability. The new tests cover various functionalities of the DataBackupReconciler and dataBackupOperation.
However, there is a logical error in one of the test expectations that needs to be addressed to ensure correctness.
| Expect(names).To(HaveLen(1)) | ||
| Expect(names[0]).To(Equal(types.NamespacedName{ | ||
| Namespace: "default", | ||
| Name: "test-backup", | ||
| })) |
There was a problem hiding this comment.
The GetPossibleTargetDatasetNamespacedNames method is expected to return the NamespacedName of the target dataset, which is specified in dataBackup.Spec.Dataset. However, this test currently asserts that the returned name is "test-backup", which is the name of the DataBackup object itself, not the dataset it targets. This is a logical error in the test expectation and should be corrected to "test-dataset" to accurately reflect the method's purpose.
Expect(names[0]).To(Equal(types.NamespacedName{
Namespace: "default",
Name: "test-dataset",
}))
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #5713 +/- ##
=======================================
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:
|
Strengthen the 'should use conditions LastTransitionTime' test to actually verify the transition time value, not just the condition count. Uses BeTemporally to handle metav1.Time second-precision truncation. Signed-off-by: Harsh <harshmastic@gmail.com>
|
/gemini review |
|
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request successfully migrates the tests for the databackup controller to use Ginkgo/Gomega, improving the test structure and coverage. The changes are generally well-implemented. I've included a couple of suggestions to refactor duplicated code in the new tests, which will enhance their readability and maintainability.
| It("should return a dataBackupOperation for a valid DataBackup object", func() { | ||
| s := runtime.NewScheme() | ||
| _ = datav1alpha1.AddToScheme(s) | ||
| fakeClient := fake.NewFakeClientWithScheme(s) | ||
| log := ctrl.Log.WithName("test") | ||
| recorder := record.NewFakeRecorder(10) | ||
| r := NewDataBackupReconciler(fakeClient, log, s, recorder) | ||
|
|
||
| dataBackup := &datav1alpha1.DataBackup{} | ||
| op, err := r.Build(dataBackup) | ||
| Expect(err).NotTo(HaveOccurred()) | ||
| Expect(op).NotTo(BeNil()) | ||
| }) | ||
|
|
||
| It("should return an error for a non-DataBackup object", func() { | ||
| s := runtime.NewScheme() | ||
| fakeClient := fake.NewFakeClientWithScheme(s) | ||
| log := ctrl.Log.WithName("test") | ||
| recorder := record.NewFakeRecorder(10) | ||
| r := NewDataBackupReconciler(fakeClient, log, s, recorder) | ||
|
|
||
| } | ||
| dataset := &datav1alpha1.Dataset{} | ||
| op, err := r.Build(dataset) | ||
| Expect(err).To(HaveOccurred()) | ||
| Expect(op).To(BeNil()) | ||
| }) |
There was a problem hiding this comment.
The setup for the reconciler is duplicated in both test cases within this Describe block. To improve readability and maintainability, you can extract the common setup into a BeforeEach block.
var r *DataBackupReconciler
BeforeEach(func() {
s := runtime.NewScheme()
_ = datav1alpha1.AddToScheme(s)
fakeClient := fake.NewFakeClientWithScheme(s)
log := ctrl.Log.WithName("test")
recorder := record.NewFakeRecorder(10)
r = NewDataBackupReconciler(fakeClient, log, s, recorder)
})
It("should return a dataBackupOperation for a valid DataBackup object", func() {
dataBackup := &datav1alpha1.DataBackup{}
op, err := r.Build(dataBackup)
Expect(err).NotTo(HaveOccurred())
Expect(op).NotTo(BeNil())
})
It("should return an error for a non-DataBackup object", func() {
dataset := &datav1alpha1.Dataset{}
op, err := r.Build(dataset)
Expect(err).To(HaveOccurred())
Expect(op).To(BeNil())
})| testScheme3 := runtime.NewScheme() | ||
| _ = datav1alpha1.AddToScheme(testScheme3) | ||
| _ = corev1.AddToScheme(testScheme3) | ||
| fakeClient3 := fake.NewFakeClientWithScheme(testScheme3, dataBackup, dataset) | ||
| op3 := &dataBackupOperation{ | ||
| Client: fakeClient3, | ||
| Log: ctrl.Log.WithName("test"), | ||
| Recorder: record.NewFakeRecorder(10), | ||
| dataBackup: dataBackup, | ||
| } | ||
|
|
||
| got, err := op3.GetTargetDataset() |
There was a problem hiding this comment.
This block contains redundant setup code. The testScheme is already created in the BeforeEach block and can be reused. Also, creating a new operation object (op3) for this test makes it verbose. A better approach is to update the client of the existing op object, which is reset for each test. This makes the test cleaner and easier to read.
A similar pattern of redundant setup is present in other tests like UpdateStatusInfoForCompleted and UpdateOperationApiStatus and could also be refactored.
| testScheme3 := runtime.NewScheme() | |
| _ = datav1alpha1.AddToScheme(testScheme3) | |
| _ = corev1.AddToScheme(testScheme3) | |
| fakeClient3 := fake.NewFakeClientWithScheme(testScheme3, dataBackup, dataset) | |
| op3 := &dataBackupOperation{ | |
| Client: fakeClient3, | |
| Log: ctrl.Log.WithName("test"), | |
| Recorder: record.NewFakeRecorder(10), | |
| dataBackup: dataBackup, | |
| } | |
| got, err := op3.GetTargetDataset() | |
| op.Client = fake.NewFakeClientWithScheme(testScheme, dataBackup, dataset) | |
| got, err := op.GetTargetDataset() |
There was a problem hiding this comment.
Code Review
This pull request successfully migrates the controller tests for databackup to Ginkgo/Gomega, enhancing test readability and coverage. The changes are well-structured and the new tests cover more scenarios. I've included a few suggestions to further improve the test code by removing some redundancies, which will enhance maintainability.
| testScheme2 := runtime.NewScheme() | ||
| _ = datav1alpha1.AddToScheme(testScheme2) | ||
| _ = corev1.AddToScheme(testScheme2) | ||
| dataBackup2 := dataBackup.DeepCopy() | ||
| dataBackup2.Spec.BackupPath = "local:///tmp/backup" | ||
| fakeClient2 := fake.NewFakeClientWithScheme(testScheme2, dataBackup2, backupPod) |
There was a problem hiding this comment.
The testScheme is already initialized in the BeforeEach block and is accessible within this test case. Creating a new scheme testScheme2 is redundant. You can reuse testScheme when creating the fake client to make the test code cleaner and less repetitive.
| testScheme2 := runtime.NewScheme() | |
| _ = datav1alpha1.AddToScheme(testScheme2) | |
| _ = corev1.AddToScheme(testScheme2) | |
| dataBackup2 := dataBackup.DeepCopy() | |
| dataBackup2.Spec.BackupPath = "local:///tmp/backup" | |
| fakeClient2 := fake.NewFakeClientWithScheme(testScheme2, dataBackup2, backupPod) | |
| dataBackup2 := dataBackup.DeepCopy() | |
| dataBackup2.Spec.BackupPath = "local:///tmp/backup" | |
| fakeClient2 := fake.NewFakeClientWithScheme(testScheme, dataBackup2, backupPod) |
| testScheme4 := runtime.NewScheme() | ||
| _ = datav1alpha1.AddToScheme(testScheme4) | ||
| _ = corev1.AddToScheme(testScheme4) | ||
| fakeClient4 := fake.NewFakeClientWithScheme(testScheme4, dataBackup) |
There was a problem hiding this comment.
The testScheme is already initialized in the BeforeEach block. You can reuse it here instead of creating a new testScheme4.
| testScheme4 := runtime.NewScheme() | |
| _ = datav1alpha1.AddToScheme(testScheme4) | |
| _ = corev1.AddToScheme(testScheme4) | |
| fakeClient4 := fake.NewFakeClientWithScheme(testScheme4, dataBackup) | |
| fakeClient4 := fake.NewFakeClientWithScheme(testScheme, dataBackup) |
| testScheme3 := runtime.NewScheme() | ||
| _ = datav1alpha1.AddToScheme(testScheme3) | ||
| _ = corev1.AddToScheme(testScheme3) | ||
| fakeClient3 := fake.NewFakeClientWithScheme(testScheme3, dataBackup, dataset) |
There was a problem hiding this comment.
The testScheme is already initialized in the BeforeEach block. You can reuse it here instead of creating a new testScheme3.
| testScheme3 := runtime.NewScheme() | |
| _ = datav1alpha1.AddToScheme(testScheme3) | |
| _ = corev1.AddToScheme(testScheme3) | |
| fakeClient3 := fake.NewFakeClientWithScheme(testScheme3, dataBackup, dataset) | |
| fakeClient3 := fake.NewFakeClientWithScheme(testScheme, dataBackup, dataset) |
cheyang
left a comment
cheyang
left a comment
There was a problem hiding this comment.
/lgtm
/approve
Clean migration to Ginkgo/Gomega testing framework:
- Well-structured tests using Describe/Context/It
- Proper use of fake client for unit testing
- Good coverage of both normal and error scenarios
- Clear test naming (should...)
- All CI checks pass
|
[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 |
Ⅰ. Describe what this PR does
Migrate
pkg/controllers/v1alpha1/databackup/tests from stdlibtestingto Ginkgo/Gomega and raise measured package coverage to 77.5%.Ⅱ. Does this pull request fix one issue?
NONE
Ⅲ. List the added test cases (unit test/integration test) if any, please explain if no tests are needed.
pkg/controllers/v1alpha1/databackup/databackup_controller_test.goto Ginkgo/Gomega.pkg/controllers/v1alpha1/databackup/status_handler_test.goto Ginkgo/Gomega.pkg/controllers/v1alpha1/databackup/implement_test.goto Ginkgo/Gomega.pkg/controllers/v1alpha1/databackup/suite_test.gofor the package test suite.Ⅳ. Describe how to verify it
make fmtgo test -coverprofile=/tmp/fluid-databackup.cover ./pkg/controllers/v1alpha1/databackup/... -count=1go tool cover -func=/tmp/fluid-databackup.cover | grep totalrg 'testify' pkg/controllers/v1alpha1/databackup/*_test.goⅤ. Special notes for reviews
N/A