test(datamigrate): migrate controller tests to Ginkgo v2#5710
test(datamigrate): migrate controller tests to Ginkgo v2#5710hxrshxz wants to merge 3 commits intofluid-cloudnative:masterfrom
Conversation
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 test suite 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/datamigrate controller tests from testing to Ginkgo v2/Gomega to improve readability and increase package-level coverage.
Changes:
- Add a Ginkgo v2 suite bootstrap for the
datamigratepackage. - Convert existing status handler and operation tests to Ginkgo/Gomega and expand covered paths (once/cron/on-event; operation helpers).
- Add reconciler construction/builder unit tests for
DataMigrateReconciler.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| pkg/controllers/v1alpha1/datamigrate/suite_test.go | Adds Ginkgo v2 bootstrap and sets a null logger for the suite. |
| pkg/controllers/v1alpha1/datamigrate/status_handler_test.go | Migrates status handler tests to Ginkgo and expands coverage across policies and parallel cron behavior. |
| pkg/controllers/v1alpha1/datamigrate/implement_test.go | Migrates/expands dataMigrateOperation unit tests for validation and helper methods. |
| pkg/controllers/v1alpha1/datamigrate/datamigrate_controller_test.go | Adds unit tests for reconciler construction and Build behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| opStatus, err := handler.GetOperationStatus(ctx, &mockDataMigrate.Status) | ||
|
|
||
| // job still running; GetOperationStatus returns nil result (early return) |
There was a problem hiding this comment.
Misleading comment: OnceStatusHandler.GetOperationStatus returns a deep-copied status even when the Job is still running (finished condition is nil). The comment says it returns a nil result; update/remove it to match actual behavior and avoid confusion when debugging failures.
| // job still running; GetOperationStatus returns nil result (early return) | |
| // job still running; GetOperationStatus returns a deep-copied status without modification |
| It("should return PhaseComplete and set NodeAffinity for single-parallelism succeed job", func() { | ||
| parallelism1DataMigrate := v1alpha1.DataMigrate{ | ||
| ObjectMeta: v1.ObjectMeta{ | ||
| Name: "test", | ||
| Namespace: "default", |
There was a problem hiding this comment.
Test name suggests NodeAffinity is set, but the Job here lacks the annotations required by dataflow.GenerateNodeAffinity (AnnotationDataFlowAffinityInject + at least one customized affinity label), and the test doesn’t assert NodeAffinity. Add the annotations + an assertion, or rename the test to only validate PhaseComplete.
| It("should return original status without change when job is still running (no finished condition)", func() { | ||
| runningJob := batchv1.Job{ | ||
| ObjectMeta: v1.ObjectMeta{ | ||
| Name: "test-migrate-migrate", | ||
| Namespace: "default", |
There was a problem hiding this comment.
The test case name claims the running Job path returns the original status 'without change', but the assertions only check non-nil. Consider asserting that Phase/Conditions/Duration/NodeAffinity are unchanged when finishedJobCondition is nil (OnceStatusHandler returns a DeepCopy and exits early).
| It("should return PhasePending and scale StatefulSet when job starts with suspend=true", func() { | ||
| trueFlag := true | ||
| job := batchv1.Job{ | ||
| ObjectMeta: v1.ObjectMeta{ | ||
| Name: "test-migrate-migrate-1", |
There was a problem hiding this comment.
This test aims to validate scaling when job.spec.suspend=true, but the StatefulSet being used starts with Replicas already equal to the target (Parallelism-1), so the ScaleStatefulSet call has no observable effect. Start from a different replica count and assert it changes; also assert the Job’s spec.suspend is flipped to false.
![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 migrates the controller tests for datamigrate to Ginkgo v2, which is a great improvement for test readability and structure. The new tests also increase coverage. My review focuses on further improving the new tests by reducing code duplication and strengthening assertions. I've suggested refactoring one of the new test files to use BeforeEach for common setup, and enhancing a test case to verify the outcome of an API call instead of just checking for errors.
| var _ = Describe("DataMigrateReconciler", func() { | ||
| Describe("NewDataMigrateReconciler", func() { | ||
| It("should create a reconciler with the provided fields", func() { | ||
| scheme := runtime.NewScheme() | ||
| fakeClient := fake.NewFakeClientWithScheme(scheme) | ||
| log := fake.NullLogger() | ||
| recorder := record.NewFakeRecorder(10) | ||
|
|
||
| r := NewDataMigrateReconciler(fakeClient, log, scheme, recorder) | ||
|
|
||
| Expect(r).NotTo(BeNil()) | ||
| Expect(r.Scheme).To(Equal(scheme)) | ||
| Expect(r.OperationReconciler).NotTo(BeNil()) | ||
| }) | ||
| }) | ||
|
|
||
| Describe("ControllerName", func() { | ||
| It("should return DataMigrateReconciler", func() { | ||
| scheme := runtime.NewScheme() | ||
| fakeClient := fake.NewFakeClientWithScheme(scheme) | ||
| log := fake.NullLogger() | ||
| recorder := record.NewFakeRecorder(10) | ||
|
|
||
| r := NewDataMigrateReconciler(fakeClient, log, scheme, recorder) | ||
|
|
||
| Expect(r.ControllerName()).To(Equal("DataMigrateReconciler")) | ||
| }) | ||
| }) | ||
|
|
||
| Describe("Build", func() { | ||
| It("should return a dataMigrateOperation when given a DataMigrate object", func() { | ||
| scheme := runtime.NewScheme() | ||
| fakeClient := fake.NewFakeClientWithScheme(scheme) | ||
| log := fake.NullLogger() | ||
| recorder := record.NewFakeRecorder(10) | ||
|
|
||
| r := NewDataMigrateReconciler(fakeClient, log, scheme, recorder) | ||
|
|
||
| dm := &datav1alpha1.DataMigrate{} | ||
| dm.Name = "test-migrate" | ||
| dm.Namespace = "default" | ||
|
|
||
| op, err := r.Build(dm) | ||
|
|
||
| Expect(err).NotTo(HaveOccurred()) | ||
| Expect(op).NotTo(BeNil()) | ||
| }) | ||
|
|
||
| It("should return an error when given a non-DataMigrate object", func() { | ||
| scheme := runtime.NewScheme() | ||
| fakeClient := fake.NewFakeClientWithScheme(scheme) | ||
| log := fake.NullLogger() | ||
| recorder := record.NewFakeRecorder(10) | ||
|
|
||
| r := NewDataMigrateReconciler(fakeClient, log, scheme, recorder) | ||
|
|
||
| notDM := &datav1alpha1.DataLoad{} | ||
|
|
||
| op, err := r.Build(notDM) | ||
|
|
||
| Expect(err).To(HaveOccurred()) | ||
| Expect(op).To(BeNil()) | ||
| }) | ||
| }) | ||
| }) |
There was a problem hiding this comment.
To avoid repeating the setup logic in each test, you can use a BeforeEach block. This makes the tests DRY (Don't Repeat Yourself) and easier to read and maintain.
var _ = Describe("DataMigrateReconciler", func() {
var (
r *DataMigrateReconciler
scheme *runtime.Scheme
fakeClient client.Client
log logr.Logger
recorder *record.FakeRecorder
)
BeforeEach(func() {
scheme = runtime.NewScheme()
fakeClient = fake.NewFakeClientWithScheme(scheme)
log = fake.NullLogger()
recorder = record.NewFakeRecorder(10)
r = NewDataMigrateReconciler(fakeClient, log, scheme, recorder)
})
Describe("NewDataMigrateReconciler", func() {
It("should create a reconciler with the provided fields", func() {
Expect(r).NotTo(BeNil())
Expect(r.Scheme).To(Equal(scheme))
Expect(r.OperationReconciler).NotTo(BeNil())
})
})
Describe("ControllerName", func() {
It("should return DataMigrateReconciler", func() {
Expect(r.ControllerName()).To(Equal("DataMigrateReconciler"))
})
})
Describe("Build", func() {
It("should return a dataMigrateOperation when given a DataMigrate object", func() {
dm := &datav1alpha1.DataMigrate{}
dm.Name = "test-migrate"
dm.Namespace = "default"
op, err := r.Build(dm)
Expect(err).NotTo(HaveOccurred())
Expect(op).NotTo(BeNil())
})
It("should return an error when given a non-DataMigrate object", func() {
notDM := &datav1alpha1.DataLoad{}
op, err := r.Build(notDM)
Expect(err).To(HaveOccurred())
Expect(op).To(BeNil())
})
})
})| It("should update status without error", func() { | ||
| testScheme := k8sruntime.NewScheme() | ||
| Expect(datav1alpha1.AddToScheme(testScheme)).To(Succeed()) | ||
|
|
||
| dm := &datav1alpha1.DataMigrate{ | ||
| ObjectMeta: v1.ObjectMeta{ | ||
| Name: "test-migrate", | ||
| Namespace: "default", | ||
| }, | ||
| } | ||
| fakeClient := fake.NewFakeClientWithScheme(testScheme, dm) | ||
| op := &dataMigrateOperation{ | ||
| Client: fakeClient, | ||
| dataMigrate: dm, | ||
| } | ||
|
|
||
| newStatus := &datav1alpha1.OperationStatus{ | ||
| Phase: common.PhaseComplete, | ||
| } | ||
| err := op.UpdateOperationApiStatus(newStatus) | ||
|
|
||
| Expect(err).NotTo(HaveOccurred()) | ||
| }) |
There was a problem hiding this comment.
This test only checks that UpdateOperationApiStatus doesn't return an error. To make the test more robust, it should also verify that the status of the DataMigrate object was actually updated as expected. You can do this by fetching the object again from the fake client and asserting its status.
Note: you'll need to add import "context" to the file.
It("should update status without error", func() {
testScheme := k8sruntime.NewScheme()
Expect(datav1alpha1.AddToScheme(testScheme)).To(Succeed())
dm := &datav1alpha1.DataMigrate{
ObjectMeta: v1.ObjectMeta{
Name: "test-migrate",
Namespace: "default",
},
}
fakeClient := fake.NewFakeClientWithScheme(testScheme, dm)
op := &dataMigrateOperation{
Client: fakeClient,
dataMigrate: dm,
}
newStatus := &datav1alpha1.OperationStatus{
Phase: common.PhaseComplete,
}
err := op.UpdateOperationApiStatus(newStatus)
Expect(err).NotTo(HaveOccurred())
// Verify the status is updated
updatedDm := &datav1alpha1.DataMigrate{}
key := types.NamespacedName{Name: "test-migrate", Namespace: "default"}
Expect(fakeClient.Get(context.Background(), key, updatedDm)).To(Succeed())
Expect(updatedDm.Status.Phase).To(Equal(common.PhaseComplete))
})- Fix misleading comment on running-job test (returns DeepCopy, not nil) - Rename NodeAffinity test to reflect actual assertion scope - Improve suspend test: start STS at 0 replicas for observable scaling, assert job.Spec.Suspend is flipped to false Signed-off-by: Harsh <harshmastic@gmail.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #5710 +/- ##
=======================================
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 |
1 similar comment
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request successfully migrates the controller tests for datamigrate to Ginkgo v2 and expands test coverage. The new tests are well-structured and cover various scenarios for different components like the reconciler, operation implementation, and status handlers. I've provided a couple of suggestions to improve the test code further by reducing duplication and adhering to context usage best practices. Overall, this is a solid improvement to the test suite.
There was a problem hiding this comment.
Code Review
This pull request successfully migrates the datamigrate controller tests to Ginkgo v2 and enhances test coverage. The new tests are generally well-structured. I've provided a couple of suggestions to refactor the test setup using BeforeEach blocks. This will reduce code duplication and improve the overall maintainability of the test suite.
Signed-off-by: Harsh <harshmastic@gmail.com>
|
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/datamigratetests to Ginkgo/Gomega and raise package coverage to 76.0%.Ⅱ. 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.
suite_test.gofor the package Ginkgo bootstrapimplement_test.goto Ginkgo/Gomega and expand coverage fordataMigrateOperationstatus_handler_test.goto Ginkgo/Gomega and cover cron/once/on-event handler pathsdatamigrate_controller_test.gofor reconciler construction and builder helpersⅣ. Describe how to verify it
go test ./pkg/controllers/v1alpha1/datamigrate/...go test -cover ./pkg/controllers/v1alpha1/datamigrate/...gofmt -l pkg/controllers/v1alpha1/datamigrateⅤ. Special notes for reviews
N/A