test(api/v1alpha1): migrate core API tests to Ginkgo#5889
Conversation
Signed-off-by: Harsh <harshmastic@gmail.com>
There was a problem hiding this comment.
Pull request overview
Migrates existing api/v1alpha1 unit tests from the standard testing table-driven style to Ginkgo v2 + Gomega, and adds new focused coverage for several common API helpers. A Ginkgo test suite entrypoint is introduced so the existing and new specs run under the migrated framework.
Changes:
- Add
suite_test.goto bootstrap a Ginkgo suite for thev1alpha1package. - Convert
dataset_types_test.goandcontainer_network_test.gotoDescribe/DescribeTablespecs. - Add new
common_test.gocoveringMetadataSyncPolicy.AutoSyncEnabled,Dataset.CanbeBound, andDataset.IsExclusiveMode.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| api/v1alpha1/suite_test.go | New Ginkgo suite entrypoint for the package. |
| api/v1alpha1/dataset_types_test.go | Reworks Remove/Set/GetDataOperationInProgress tests as DescribeTable specs. |
| api/v1alpha1/container_network_test.go | Reworks IsHostNetwork tests to DescribeTable, replacing the literal "" case with DefaultNetworkMode. |
| api/v1alpha1/common_test.go | Adds new specs for AutoSyncEnabled, CanbeBound, and IsExclusiveMode. |
Comments suppressed due to low confidence (1)
api/v1alpha1/common_test.go:14
- The license header has stray leading tabs/indentation on lines 5–14 (each line is prefixed with a tab). Other license headers in this PR and across the repo (e.g.,
api/v1alpha1/suite_test.go,container_network.go) place these lines at column 0. The indentation should be removed for consistency.
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.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -0,0 +1,82 @@ | |||
| /* | |||
| Copyright 2026 The Fluid Authors. | |||
![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 unit tests in the api/v1alpha1 package from the standard Go testing package to the Ginkgo and Gomega BDD framework, while also adding new tests for common API helpers. The review feedback suggests improving the test coverage for CanbeBound and IsExclusiveMode by utilizing DescribeTable to verify all components of the runtime identity and all placement modes explicitly.
| 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()) | ||
| }) | ||
| }) |
There was a problem hiding this comment.
The test coverage for CanbeBound can be improved by verifying all components of the runtime identity (Name, Namespace, and Category). Using a DescribeTable would make this more thorough and readable by explicitly testing mismatch scenarios for each field.
Describe("Dataset.CanbeBound", func() {
It("returns true when no runtime is recorded", func() {
dataset := &Dataset{}
Expect(dataset.CanbeBound("runtime", "fluid", common.AccelerateCategory)).To(BeTrue())
})
DescribeTable("matching runtime identity",
func(name, ns string, cat common.Category, expected bool) {
dataset := &Dataset{
Status: DatasetStatus{
Runtimes: []Runtime{
{Name: "target", Namespace: "fluid", Category: common.AccelerateCategory},
},
},
}
Expect(dataset.CanbeBound(name, ns, cat)).To(Equal(expected))
},
Entry("matching identity", "target", "fluid", common.AccelerateCategory, true),
Entry("name mismatch", "other", "fluid", common.AccelerateCategory, false),
Entry("namespace mismatch", "target", "other", common.AccelerateCategory, false),
Entry("category mismatch", "target", "fluid", common.Category("other"), false),
)
})| 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 IsExclusiveMode should explicitly cover the ExclusiveMode case as well, as it is one of the two conditions that return true in the implementation. Refactoring to a DescribeTable improves clarity and ensures all placement modes are accounted for.
DescribeTable("Dataset.IsExclusiveMode",
func(mode PlacementMode, expected bool) {
dataset := &Dataset{Spec: DatasetSpec{PlacementMode: mode}}
Expect(dataset.IsExclusiveMode()).To(Equal(expected))
},
Entry("treats default placement as exclusive", DefaultMode, true),
Entry("treats explicit exclusive mode as exclusive", ExclusiveMode, true),
Entry("returns false for shared placement mode", ShareMode, false),
)
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #5889 +/- ##
=======================================
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>
|
[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 |
|
/gemini review |
| @@ -0,0 +1,47 @@ | |||
| /* | |||
| Copyright 2020 The Fluid Authors. | |||
| DescribeTable("removes the target operation reference", | ||
| func(dataset *Dataset, operationType string, name string, expected string) { | ||
| Expect(dataset.RemoveDataOperationInProgress(operationType, name)).To(Equal(expected)) | ||
| Expect(dataset.GetDataOperationInProgress(operationType)).To(Equal(expected)) | ||
| }, |
There was a problem hiding this comment.
Code Review
This pull request migrates the unit tests in the api/v1alpha1 package from the standard Go testing package to the Ginkgo and Gomega BDD framework. The changes include the addition of a test suite entry point, new tests for MetadataSyncPolicy, and the refactoring of existing tests for IsHostNetwork and Dataset methods into DescribeTable formats. Feedback suggests improving the naming of the top-level Describe block in dataset_types_test.go to better encompass all tested methods and wrapping the IsExclusiveMode table in a Describe block for better structural consistency.
| Status: DatasetStatus{}, | ||
| const dataLoadOperation = "DataLoad" | ||
|
|
||
| var _ = Describe("Dataset operation references", func() { |
There was a problem hiding this comment.
The top-level Describe block name "Dataset operation references" is too narrow, as the suite now includes tests for CanbeBound and IsExclusiveMode which are unrelated to operation references. Consider renaming it to something more inclusive like "Dataset methods" or simply "Dataset".
| DescribeTable("IsExclusiveMode", | ||
| func(mode PlacementMode, expected bool) { | ||
| dataset := &Dataset{Spec: DatasetSpec{PlacementMode: mode}} | ||
|
|
||
| Expect(dataset.IsExclusiveMode()).To(Equal(expected)) | ||
| }, | ||
| } | ||
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| dataset := &Dataset{ | ||
| TypeMeta: tt.fields.TypeMeta, | ||
| ObjectMeta: tt.fields.ObjectMeta, | ||
| Spec: tt.fields.Spec, | ||
| Status: tt.fields.Status, | ||
| } | ||
| dataset.SetDataOperationInProgress(tt.args.operationType, tt.args.name) | ||
| if got := dataset.GetDataOperationInProgress(tt.args.operationType); got != tt.want { | ||
| t.Errorf("SetDataOperationInProgress() = %v, want %v", got, tt.want) | ||
| } | ||
| }) | ||
| } | ||
| } | ||
| Entry("default placement mode", DefaultMode, true), | ||
| Entry("exclusive placement mode", ExclusiveMode, true), | ||
| Entry("shared placement mode", ShareMode, false), | ||
| ) |
There was a problem hiding this comment.
For consistency with other test blocks in this file and to provide better context, consider wrapping the IsExclusiveMode table in a Describe block and using a more descriptive string for the table itself.
Describe("IsExclusiveMode", func() {
DescribeTable("reports whether the placement mode is exclusive",
func(mode PlacementMode, expected bool) {
dataset := &Dataset{Spec: DatasetSpec{PlacementMode: mode}}
Expect(dataset.IsExclusiveMode()).To(Equal(expected))
},
Entry("default placement mode", DefaultMode, true),
Entry("exclusive placement mode", ExclusiveMode, true),
Entry("shared placement mode", ShareMode, false),
)
})Signed-off-by: Harsh <harshmastic@gmail.com>
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors the unit tests for the v1alpha1 API package to use the Ginkgo and Gomega testing frameworks. It introduces a test suite, converts existing table-driven tests for network modes and dataset operations to Ginkgo's DescribeTable format, and adds new test coverage for metadata sync policies and dataset binding logic. Feedback was provided to simplify the test parameters in RemoveDataOperationInProgress by removing redundant variables to improve maintainability.
| func(dataset *Dataset, operationType string, name string, expectedRemoved string, expectedCurrent string) { | ||
| Expect(dataset.RemoveDataOperationInProgress(operationType, name)).To(Equal(expectedRemoved)) | ||
| Expect(dataset.GetDataOperationInProgress(operationType)).To(Equal(expectedCurrent)) | ||
| }, |
There was a problem hiding this comment.
The expectedRemoved and expectedCurrent parameters in this table-driven test are redundant because RemoveDataOperationInProgress returns the updated state of the operation reference, which is exactly what GetDataOperationInProgress retrieves. Simplifying this to a single expected parameter would improve test maintainability.
| func(dataset *Dataset, operationType string, name string, expectedRemoved string, expectedCurrent string) { | |
| Expect(dataset.RemoveDataOperationInProgress(operationType, name)).To(Equal(expectedRemoved)) | |
| Expect(dataset.GetDataOperationInProgress(operationType)).To(Equal(expectedCurrent)) | |
| }, | |
| func(dataset *Dataset, operationType string, name string, expected string) { | |
| Expect(dataset.RemoveDataOperationInProgress(operationType, name)).To(Equal(expected)) | |
| Expect(dataset.GetDataOperationInProgress(operationType)).To(Equal(expected)) | |
| }, |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors the unit tests for the api/v1alpha1 package, migrating from the standard library's testing package to the Ginkgo and Gomega frameworks. Key changes include the addition of a test suite setup, the conversion of existing tests to DescribeTable formats, and the introduction of new tests for MetadataSyncPolicy, CanbeBound, and IsExclusiveMode. Feedback from the review focuses on improving code maintainability by using constants for operation types and increasing test coverage for edge cases in operation removal logic.
| name: "test3", | ||
| fields: fields{ | ||
| Status: DatasetStatus{}, | ||
| const dataLoadOperation = "DataLoad" |
There was a problem hiding this comment.
For better maintainability and consistency, consider defining a constant for the 'DataMigrate' operation type, similar to how dataLoadOperation is defined.
const (
dataLoadOperation = "DataLoad"
dataMigrateOperation = "DataMigrate"
)| Entry("returns empty when no operation refs are recorded", | ||
| &Dataset{Status: DatasetStatus{}}, | ||
| dataLoadOperation, | ||
| "test1", | ||
| "", | ||
| "", | ||
| ), |
There was a problem hiding this comment.
Adding a test case for the scenario where the operation to be removed is not present in the OperationRef list would improve the coverage of edge cases for RemoveDataOperationInProgress.
Entry("returns empty when no operation refs are recorded",
&Dataset{Status: DatasetStatus{}},
dataLoadOperation,
"test1",
"",
"",
),
Entry("returns original list when operation is not found",
&Dataset{Status: DatasetStatus{OperationRef: map[string]string{dataLoadOperation: "test1,test2"}}},
dataLoadOperation,
"test3",
"test1,test2",
"test1,test2",
),| ), | ||
| 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 for consistency.
| "DataMigrate", | |
| dataMigrateOperation, |
Signed-off-by: Harsh <harshmastic@gmail.com>
|
Ⅰ. Describe what this PR does
Migrate the core
api/v1alpha1unit tests in scope to Ginkgo v2 + Gomega and add focused coverage for common API helper behavior.Ⅱ. 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.
IsHostNetworkcoverage to Ginkgo v2 + Gomega.MetadataSyncPolicy.AutoSyncEnabled()coverage for default/nil, explicit true, and explicit false.Dataset.CanbeBound()andDataset.IsExclusiveMode()coverage.Ⅳ. Describe how to verify it
Run the
api/v1alpha1unit tests and confirm the migrated Ginkgo suite passes with only the intended test files changed.Ⅴ. Special notes for reviews
N/A