test(api/v1alpha1): add data operation type coverage#5890
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. |
There was a problem hiding this comment.
Pull request overview
Migrates and expands api/v1alpha1 unit tests to Ginkgo v2 + Gomega, adding scheme registration, DeepCopyObject, and spec/status construction coverage for the four data operation types (DataLoad, DataMigrate, DataBackup, DataProcess). Part of the broader Ginkgo/Gomega migration tracked in #5676 and stacked on #5889.
Changes:
- Adds new Ginkgo specs for DataLoad/DataMigrate/DataBackup/DataProcess covering scheme GVKs, deep-copy semantics, and representative spec/status structures.
- Re-converts existing
dataset_types_test.goandcontainer_network_test.gointo GinkgoDescribeTablestyle and introduces a sharedapiGVKForhelper usingUnitTestScheme. - Adds the v1alpha1 Ginkgo suite bootstrap and a
common_test.goGinkgo suite forMetadataSyncPolicy.AutoSyncEnabled,Dataset.CanbeBound, andDataset.IsExclusiveMode.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| api/v1alpha1/suite_test.go | New Ginkgo test entrypoint for the v1alpha1 package suite. |
| api/v1alpha1/common_test.go | Ginkgo coverage for MetadataSyncPolicy.AutoSyncEnabled, Dataset.CanbeBound, Dataset.IsExclusiveMode. |
| api/v1alpha1/container_network_test.go | Migrates IsHostNetwork table test to DescribeTable. |
| api/v1alpha1/dataset_types_test.go | Migrates Set/RemoveDataOperationInProgress tests to Ginkgo DescribeTable. |
| api/v1alpha1/dataload_types_test.go | New tests for DataLoad GVK registration, DeepCopyObject, and spec/status construction; defines shared apiGVKFor helper. |
| api/v1alpha1/databackup_types_test.go | New tests for DataBackup GVK registration, DeepCopyObject, and spec/status construction. |
| api/v1alpha1/datamigrate_types_test.go | New tests for DataMigrate GVK registration, DeepCopyObject, and spec/status construction. |
| api/v1alpha1/dataprocess_types_test.go | New tests for DataProcess GVK registration, DeepCopyObject, and spec/status construction. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| you may not use this file except in compliance with the License. | ||
| You may obtain a copy of the License at | ||
|
|
||
| http://www.apache.org/licenses/LICENSE-2.0 | ||
|
|
||
| Unless required by applicable law or agreed to in writing, software | ||
| distributed under the License is distributed on an "AS IS" BASIS, | ||
| WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| See the License for the specific language governing permissions and | ||
| limitations under the License. | ||
| */ |
![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 refactors existing tests and introduces new unit tests for the v1alpha1 API package using the Ginkgo and Gomega frameworks, covering data operations such as DataBackup, DataLoad, DataMigrate, and DataProcess. The reviewer suggests improving test organization by moving Dataset-specific tests and shared helpers to centralized locations. Additionally, feedback was provided to enhance DeepCopyObject tests by explicitly verifying that nested pointers are deep-copied to ensure object independence.
| Describe("Dataset.CanbeBound", func() { | ||
| It("returns true when no runtime is recorded", func() { | ||
| dataset := &Dataset{} | ||
|
|
||
| Expect(dataset.CanbeBound("runtime", "fluid", common.AccelerateCategory)).To(BeTrue()) | ||
| }) | ||
|
|
||
| It("returns true only for a matching runtime identity", func() { | ||
| dataset := &Dataset{ | ||
| Status: DatasetStatus{ | ||
| Runtimes: []Runtime{ | ||
| {Name: "target", Namespace: "fluid", Category: common.AccelerateCategory}, | ||
| }, | ||
| }, | ||
| } | ||
|
|
||
| Expect(dataset.CanbeBound("target", "fluid", common.AccelerateCategory)).To(BeTrue()) | ||
| Expect(dataset.CanbeBound("other", "fluid", common.AccelerateCategory)).To(BeFalse()) | ||
| }) | ||
| }) | ||
|
|
||
| Describe("Dataset.IsExclusiveMode", func() { | ||
| It("treats default placement as exclusive", func() { | ||
| dataset := &Dataset{} | ||
|
|
||
| Expect(dataset.IsExclusiveMode()).To(BeTrue()) | ||
| }) | ||
|
|
||
| It("returns false for shared placement mode", func() { | ||
| dataset := &Dataset{Spec: DatasetSpec{PlacementMode: ShareMode}} | ||
|
|
||
| Expect(dataset.IsExclusiveMode()).To(BeFalse()) | ||
| }) | ||
| }) |
There was a problem hiding this comment.
The tests for Dataset.CanbeBound and Dataset.IsExclusiveMode are better suited for api/v1alpha1/dataset_types_test.go, which already contains tests for other Dataset methods. This keeps common_test.go focused on the types defined in common.go (like MetadataSyncPolicy).
| copiedObject := dataBackup.DeepCopyObject() | ||
| copiedDataBackup, ok := copiedObject.(*DataBackup) | ||
| Expect(ok).To(BeTrue()) | ||
| Expect(copiedDataBackup).NotTo(BeIdenticalTo(dataBackup)) | ||
| Expect(copiedDataBackup.Spec).To(Equal(dataBackup.Spec)) | ||
| Expect(copiedDataBackup.Status).To(Equal(dataBackup.Status)) |
There was a problem hiding this comment.
The DeepCopyObject test should verify that nested pointers are actually deep-copied. Currently, it only checks value equality using Equal (which uses reflect.DeepEqual), which doesn't distinguish between shallow and deep copies of pointer fields like Spec.RunAs and Spec.RunAfter. To ensure independence of the copy, you should verify that these pointers point to different memory addresses.
| copiedObject := dataBackup.DeepCopyObject() | |
| copiedDataBackup, ok := copiedObject.(*DataBackup) | |
| Expect(ok).To(BeTrue()) | |
| Expect(copiedDataBackup).NotTo(BeIdenticalTo(dataBackup)) | |
| Expect(copiedDataBackup.Spec).To(Equal(dataBackup.Spec)) | |
| Expect(copiedDataBackup.Status).To(Equal(dataBackup.Status)) | |
| copiedObject := dataBackup.DeepCopyObject() | |
| copiedDataBackup, ok := copiedObject.(*DataBackup) | |
| Expect(ok).To(BeTrue()) | |
| Expect(copiedDataBackup).NotTo(BeIdenticalTo(dataBackup)) | |
| Expect(copiedDataBackup.Spec).To(Equal(dataBackup.Spec)) | |
| // Verify deep copy of nested pointers | |
| if dataBackup.Spec.RunAs != nil { | |
| Expect(copiedDataBackup.Spec.RunAs).NotTo(BeIdenticalTo(dataBackup.Spec.RunAs)) | |
| } | |
| if dataBackup.Spec.RunAfter != nil { | |
| Expect(copiedDataBackup.Spec.RunAfter).NotTo(BeIdenticalTo(dataBackup.Spec.RunAfter)) | |
| } | |
| Expect(copiedDataBackup.Status).To(Equal(dataBackup.Status)) |
| copiedObject := dataLoad.DeepCopyObject() | ||
| copiedDataLoad, ok := copiedObject.(*DataLoad) | ||
| Expect(ok).To(BeTrue()) | ||
| Expect(copiedDataLoad).NotTo(BeIdenticalTo(dataLoad)) | ||
| Expect(copiedDataLoad.Spec).To(Equal(dataLoad.Spec)) | ||
| Expect(copiedDataLoad.Status).To(Equal(dataLoad.Status)) |
There was a problem hiding this comment.
Similar to other DeepCopyObject tests, this should verify that nested pointers (like Spec.RunAfter) are deep-copied to ensure that the copy is independent of the original object.
| copiedObject := dataLoad.DeepCopyObject() | |
| copiedDataLoad, ok := copiedObject.(*DataLoad) | |
| Expect(ok).To(BeTrue()) | |
| Expect(copiedDataLoad).NotTo(BeIdenticalTo(dataLoad)) | |
| Expect(copiedDataLoad.Spec).To(Equal(dataLoad.Spec)) | |
| Expect(copiedDataLoad.Status).To(Equal(dataLoad.Status)) | |
| copiedObject := dataLoad.DeepCopyObject() | |
| copiedDataLoad, ok := copiedObject.(*DataLoad) | |
| Expect(ok).To(BeTrue()) | |
| Expect(copiedDataLoad).NotTo(BeIdenticalTo(dataLoad)) | |
| Expect(copiedDataLoad.Spec).To(Equal(dataLoad.Spec)) | |
| // Verify deep copy of nested pointers | |
| if dataLoad.Spec.RunAfter != nil { | |
| Expect(copiedDataLoad.Spec.RunAfter).NotTo(BeIdenticalTo(dataLoad.Spec.RunAfter)) | |
| } | |
| Expect(copiedDataLoad.Status).To(Equal(dataLoad.Status)) |
| }) | ||
| }) | ||
|
|
||
| func apiGVKFor(object runtime.Object) (schema.GroupVersionKind, error) { |
There was a problem hiding this comment.
The apiGVKFor helper function is used by multiple test files in this package (databackup_types_test.go, datamigrate_types_test.go, etc.). Consider moving it to suite_test.go to centralize shared test utilities and improve code organization.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #5890 +/- ##
=======================================
Coverage 61.65% 61.65%
=======================================
Files 480 480
Lines 32613 32613
=======================================
Hits 20108 20108
Misses 10897 10897
Partials 1608 1608 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Signed-off-by: Harsh <harshmastic@gmail.com>
hxrshxz
force-pushed
the
test/api-v1alpha1-dataops-ginkgo
branch
from
d1da84f to
1c55f9b
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request migrates the v1alpha1 API unit tests to the Ginkgo and Gomega testing frameworks and introduces comprehensive test coverage for several API types, including DataBackup, DataLoad, DataMigrate, and DataProcess. The new tests cover scheme registration, DeepCopyObject functionality, and spec/status construction. Feedback was provided to strengthen the DeepCopyObject tests for DataMigrate and DataProcess by explicitly verifying that nested pointers are deep-copied rather than just comparing their values.
| Expect(copiedDataMigrate.Spec.From).To(Equal(dataMigrate.Spec.From)) | ||
| Expect(copiedDataMigrate.Spec.To).To(Equal(dataMigrate.Spec.To)) |
There was a problem hiding this comment.
The DeepCopyObject test for DataMigrate should verify that the copy is truly deep by ensuring that nested pointers (such as DataSet and ExternalStorage) are not identical to the original pointers. Currently, it only verifies value equality, which would pass even for a shallow copy of the pointers.
| Expect(copiedDataMigrate.Spec.From).To(Equal(dataMigrate.Spec.From)) | |
| Expect(copiedDataMigrate.Spec.To).To(Equal(dataMigrate.Spec.To)) | |
| Expect(copiedDataMigrate.Spec.From).To(Equal(dataMigrate.Spec.From)) | |
| if dataMigrate.Spec.From.DataSet != nil { | |
| Expect(copiedDataMigrate.Spec.From.DataSet).NotTo(BeIdenticalTo(dataMigrate.Spec.From.DataSet)) | |
| } | |
| Expect(copiedDataMigrate.Spec.To).To(Equal(dataMigrate.Spec.To)) | |
| if dataMigrate.Spec.To.ExternalStorage != nil { | |
| Expect(copiedDataMigrate.Spec.To.ExternalStorage).NotTo(BeIdenticalTo(dataMigrate.Spec.To.ExternalStorage)) | |
| } |
| Expect(copiedDataProcess.Spec.Processor.Script).NotTo(BeNil()) | ||
| Expect(*copiedDataProcess.Spec.Processor.Script).To(Equal(*dataProcess.Spec.Processor.Script)) |
There was a problem hiding this comment.
To properly validate the DeepCopyObject implementation for DataProcess, the test should verify that the Processor.Script pointer in the copied object is not identical to the one in the original object. This ensures that a deep copy was performed rather than a shallow one, maintaining consistency with the other data operation tests in this pull request.
| Expect(copiedDataProcess.Spec.Processor.Script).NotTo(BeNil()) | |
| Expect(*copiedDataProcess.Spec.Processor.Script).To(Equal(*dataProcess.Spec.Processor.Script)) | |
| Expect(copiedDataProcess.Spec.Processor.Script).NotTo(BeNil()) | |
| Expect(copiedDataProcess.Spec.Processor.Script).NotTo(BeIdenticalTo(dataProcess.Spec.Processor.Script)) | |
| Expect(copiedDataProcess.Spec.Processor.Script).To(Equal(dataProcess.Spec.Processor.Script)) |
| @@ -0,0 +1,47 @@ | |||
| /* | |||
| Copyright 2020 The Fluid Authors. | |||
Signed-off-by: Harsh <harshmastic@gmail.com>
hxrshxz
force-pushed
the
test/api-v1alpha1-dataops-ginkgo
branch
from
1bd8382 to
b1600b2
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request migrates the v1alpha1 API unit tests to the Ginkgo and Gomega frameworks, adding new tests for DataBackup, DataLoad, DataMigrate, and DataProcess types. The changes cover scheme registration, deep copy logic, and object construction. The review feedback identifies opportunities to simplify the test code by removing redundant nil checks and suggests strengthening the deep copy verification by ensuring nested pointers are distinct memory addresses.
| if dataBackup.Spec.RunAs != nil { | ||
| Expect(copiedDataBackup.Spec.RunAs).NotTo(BeIdenticalTo(dataBackup.Spec.RunAs)) | ||
| } | ||
| if dataBackup.Spec.RunAfter != nil { | ||
| Expect(copiedDataBackup.Spec.RunAfter).NotTo(BeIdenticalTo(dataBackup.Spec.RunAfter)) | ||
| } | ||
| Expect(copiedDataBackup.Status).To(Equal(dataBackup.Status)) | ||
| if dataBackup.Spec.RunAs != nil { | ||
| if dataBackup.Spec.RunAs.UID != nil { | ||
| Expect(copiedDataBackup.Spec.RunAs.UID).NotTo(BeIdenticalTo(dataBackup.Spec.RunAs.UID)) | ||
| } | ||
| if dataBackup.Spec.RunAs.GID != nil { | ||
| Expect(copiedDataBackup.Spec.RunAs.GID).NotTo(BeIdenticalTo(dataBackup.Spec.RunAs.GID)) | ||
| } | ||
| } |
There was a problem hiding this comment.
These if checks are redundant because the dataBackup object is explicitly initialized with these fields (RunAs, RunAfter, UID, GID) being non-nil in the test setup above. Removing the conditionals makes the test code cleaner and more direct.
| if dataBackup.Spec.RunAs != nil { | |
| Expect(copiedDataBackup.Spec.RunAs).NotTo(BeIdenticalTo(dataBackup.Spec.RunAs)) | |
| } | |
| if dataBackup.Spec.RunAfter != nil { | |
| Expect(copiedDataBackup.Spec.RunAfter).NotTo(BeIdenticalTo(dataBackup.Spec.RunAfter)) | |
| } | |
| Expect(copiedDataBackup.Status).To(Equal(dataBackup.Status)) | |
| if dataBackup.Spec.RunAs != nil { | |
| if dataBackup.Spec.RunAs.UID != nil { | |
| Expect(copiedDataBackup.Spec.RunAs.UID).NotTo(BeIdenticalTo(dataBackup.Spec.RunAs.UID)) | |
| } | |
| if dataBackup.Spec.RunAs.GID != nil { | |
| Expect(copiedDataBackup.Spec.RunAs.GID).NotTo(BeIdenticalTo(dataBackup.Spec.RunAs.GID)) | |
| } | |
| } | |
| // Verify deep copy of nested pointers. | |
| Expect(copiedDataBackup.Spec.RunAs).NotTo(BeIdenticalTo(dataBackup.Spec.RunAs)) | |
| Expect(copiedDataBackup.Spec.RunAfter).NotTo(BeIdenticalTo(dataBackup.Spec.RunAfter)) | |
| Expect(copiedDataBackup.Status).To(Equal(dataBackup.Status)) | |
| Expect(copiedDataBackup.Spec.RunAs.UID).NotTo(BeIdenticalTo(dataBackup.Spec.RunAs.UID)) | |
| Expect(copiedDataBackup.Spec.RunAs.GID).NotTo(BeIdenticalTo(dataBackup.Spec.RunAs.GID)) |
| if dataLoad.Spec.RunAfter != nil { | ||
| Expect(copiedDataLoad.Spec.RunAfter).NotTo(BeIdenticalTo(dataLoad.Spec.RunAfter)) | ||
| dataLoad.Spec.RunAfter.Name = "changed-after-copy" | ||
| Expect(copiedDataLoad.Spec.RunAfter.ObjectRef).To(Equal(ObjectRef{Kind: "DataLoad", Name: "prepare"})) | ||
| } |
There was a problem hiding this comment.
The if check for RunAfter is unnecessary here as it is guaranteed to be non-nil by the test setup on line 48. Simplifying the test by removing the conditional improves readability.
// Verify deep copy of nested pointers.
Expect(copiedDataLoad.Spec.RunAfter).NotTo(BeIdenticalTo(dataLoad.Spec.RunAfter))
dataLoad.Spec.RunAfter.Name = "changed-after-copy"
Expect(copiedDataLoad.Spec.RunAfter.ObjectRef).To(Equal(ObjectRef{Kind: "DataLoad", Name: "prepare"}))| Expect(copiedDataMigrate.Spec.From).To(Equal(dataMigrate.Spec.From)) | ||
| Expect(copiedDataMigrate.Spec.To).To(Equal(dataMigrate.Spec.To)) | ||
| Expect(copiedDataMigrate.Status).To(Equal(dataMigrate.Status)) |
There was a problem hiding this comment.
The DeepCopyObject test should also verify that nested pointers (like DataSet and ExternalStorage) are actually deep-copied by checking that they point to different memory addresses. Currently, it only verifies value equality, which would also pass for a shallow copy.
| Expect(copiedDataMigrate.Spec.From).To(Equal(dataMigrate.Spec.From)) | |
| Expect(copiedDataMigrate.Spec.To).To(Equal(dataMigrate.Spec.To)) | |
| Expect(copiedDataMigrate.Status).To(Equal(dataMigrate.Status)) | |
| Expect(copiedDataMigrate.Spec.From).To(Equal(dataMigrate.Spec.From)) | |
| Expect(copiedDataMigrate.Spec.From.DataSet).NotTo(BeIdenticalTo(dataMigrate.Spec.From.DataSet)) | |
| Expect(copiedDataMigrate.Spec.To).To(Equal(dataMigrate.Spec.To)) | |
| Expect(copiedDataMigrate.Spec.To.ExternalStorage).NotTo(BeIdenticalTo(dataMigrate.Spec.To.ExternalStorage)) | |
| Expect(copiedDataMigrate.Status).To(Equal(dataMigrate.Status)) |
| Expect(copiedDataProcess.Spec.Processor.Script).NotTo(BeNil()) | ||
| Expect(*copiedDataProcess.Spec.Processor.Script).To(Equal(*dataProcess.Spec.Processor.Script)) |
There was a problem hiding this comment.
To properly verify a deep copy, the test should ensure that the Script pointer in the copied object is distinct from the original one. Checking only value equality is insufficient to distinguish between a deep copy and a shallow copy of the pointer.
| Expect(copiedDataProcess.Spec.Processor.Script).NotTo(BeNil()) | |
| Expect(*copiedDataProcess.Spec.Processor.Script).To(Equal(*dataProcess.Spec.Processor.Script)) | |
| Expect(copiedDataProcess.Spec.Processor.Script).NotTo(BeNil()) | |
| Expect(copiedDataProcess.Spec.Processor.Script).NotTo(BeIdenticalTo(dataProcess.Spec.Processor.Script)) | |
| Expect(*copiedDataProcess.Spec.Processor.Script).To(Equal(*dataProcess.Spec.Processor.Script)) |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request migrates the v1alpha1 API unit tests to the Ginkgo and Gomega frameworks, introducing a centralized test suite and comprehensive coverage for DataBackup, DataLoad, DataMigrate, DataProcess, and Dataset types. The changes include refactoring existing tests into descriptive tables and adding deep copy verification. Feedback focuses on improving code maintainability by replacing magic strings with constants for data operation types.
| name: "test3", | ||
| fields: fields{ | ||
| Status: DatasetStatus{}, | ||
| const dataLoadOperation = "DataLoad" |
There was a problem hiding this comment.
For better maintainability and consistency, consider defining constants for all data operation types used in the tests instead of using magic strings.
const (
dataLoadOperation = "DataLoad"
dataMigrateOperation = "DataMigrate"
)| ), | ||
| Entry("records a different operation type independently", | ||
| &Dataset{Status: DatasetStatus{OperationRef: map[string]string{dataLoadOperation: "test1"}}}, | ||
| "DataMigrate", |
There was a problem hiding this comment.
Use the newly defined constant dataMigrateOperation here for consistency.
| "DataMigrate", | |
| dataMigrateOperation, |
Signed-off-by: Harsh <harshmastic@gmail.com>
Signed-off-by: Harsh <harshmastic@gmail.com>
Signed-off-by: Harsh <harshmastic@gmail.com>
Signed-off-by: Harsh <harshmastic@gmail.com>
Signed-off-by: Harsh <harshmastic@gmail.com>
Signed-off-by: Harsh <harshmastic@gmail.com>
Signed-off-by: Harsh <harshmastic@gmail.com>
Signed-off-by: Harsh <harshmastic@gmail.com>
hxrshxz
force-pushed
the
test/api-v1alpha1-dataops-ginkgo
branch
from
90f778a to
806f1ed
Compare
|
|
/ok-to-test |
Ⅰ. Describe what this PR does
Add Ginkgo/Gomega coverage for the api/v1alpha1 data operation API types, including registration, DeepCopyObject sanity, and representative spec/status construction.
Ⅱ. 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.
Ⅳ. Describe how to verify it
Run the api/v1alpha1 Go test package and confirm the new Ginkgo specs pass for the data operation API types.
Ⅴ. Special notes for reviews
This PR is stacked after #5889 because that PR adds the shared api/v1alpha1 suite_test.go bootstrap; the upstream base branch for #5889 was not available as a direct PR base target, so this PR targets master and should be reviewed after #5889.