test(goosefs): migrate node tests to ginkgo #5545
Conversation
Signed-off-by: Harsh <harshmastic@gmail.com>
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
|
[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
This PR migrates the unit tests in pkg/ddc/goosefs/node_test.go from the traditional Go testing framework to Ginkgo/Gomega as part of the broader initiative (#5407) to modernize Fluid's testing framework and improve code quality.
Changes:
- Replaced
testingimport with Ginkgo/Gomega imports - Converted table-driven test
TestSyncScheduleInfoToCacheNodesto use Ginkgo'sDescribeTablepattern - Renamed
runtimeimport toruntimeschemato avoid naming conflicts
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #5545 +/- ##
=======================================
Coverage 61.30% 61.30%
=======================================
Files 444 444
Lines 30540 30540
=======================================
Hits 18723 18723
Misses 10275 10275
Partials 1542 1542 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Signed-off-by: Harsh <harshmastic@gmail.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Signed-off-by: Harsh <harshmastic@gmail.com>
hxrshxz
force-pushed
the
ginkgo/goosefs-node
branch
2 times, most recently
from
c03a506 to
f7a9ae4
Compare
The test assertion should verify the runtime-specific label rather than the common label to accurately validate SyncScheduleInfoToCacheNodes behavior. This ensures nodes with worker pods get the correct runtime-specific label (e.g., fluid.io/s-goosefs-default-hbase) rather than just checking for the common label which might exist from other sources. Signed-off-by: Harsh <harshmastic@gmail.com>
Signed-off-by: Harsh <harshmastic@gmail.com>
hxrshxz
force-pushed
the
ginkgo/goosefs-node
branch
from
902ba32 to
1eb3569
Compare
|
|
/gemini review |
![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 successfully migrates the node tests in pkg/ddc/goosefs/node_test.go to use Ginkgo and Gomega. The refactoring improves test structure and isolation by using DescribeTable and creating a new client for each test case. This is a good improvement for test reliability and readability. I have one suggestion to further align with Go's best practices regarding context usage in tests.
| datasetLabels, err := labels.Parse(fmt.Sprintf(testNodeLabelSelector, engine.runtimeInfo.GetCommonLabelName())) | ||
| Expect(err).NotTo(HaveOccurred()) | ||
|
|
||
| err = c.List(context.TODO(), nodeList, &client.ListOptions{ |
There was a problem hiding this comment.
For clarity and adherence to best practices, it's recommended to use context.Background() as the root context in tests instead of context.TODO(). The Go documentation specifies that context.Background() is typically used by the main function, initialization, and tests.
| err = c.List(context.TODO(), nodeList, &client.ListOptions{ | |
| err = c.List(context.Background(), nodeList, &client.ListOptions{ |
2 participants
Ⅰ. Describe what this PR does
Migrate unit tests in pkg/ddc/goosefs/node_test.go to use Ginkgo/Gomega.
Ⅱ. Does this pull request fix one issue?
part of #5407
Ⅲ. List the added test cases
No new test cases. Migrated existing tests to Ginkgo/Gomega.
Ⅳ. Describe how to verify it
go test -v ./pkg/ddc/goosefs/... -count=1Ⅴ. Special notes for reviews
N/A