test(fluidapp): add Ginkgo controller unit coverage#5828
test(fluidapp): add Ginkgo controller unit coverage#5828hxrshxz wants to merge 2 commits intofluid-cloudnative:masterfrom
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
This PR migrates the pkg/controllers/v1alpha1/fluidapp unit tests to the repository’s current Ginkgo/Gomega style and adds controller-focused unit coverage for the FluidApp reconciler package. It fits into the broader repo effort to standardize testing and raise per-package coverage.
Changes:
- Added a package-level Ginkgo suite bootstrap for the
fluidappcontroller package. - Migrated
implement_test.gofrom stdlibtestingtable tests to Ginkgo/Gomega specs. - Added
fluidapp_controller_ut_test.goto cover reconciler construction, simple accessors, and selectedReconcile/internalReconcilepaths.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
pkg/controllers/v1alpha1/fluidapp/suite_test.go |
Adds the Ginkgo suite entrypoint and test logger setup for the package. |
pkg/controllers/v1alpha1/fluidapp/implement_test.go |
Rewrites fuse-sidecar unmount tests into Ginkgo specs. |
pkg/controllers/v1alpha1/fluidapp/fluidapp_controller_ut_test.go |
Adds new unit coverage for reconciler helpers and main reconcile flows. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| It("returns nil when the fuse sidecar has no mount path", func() { | ||
| patches = gomonkey.ApplyFunc(kubeclient.ExecCommandInContainerWithContext, func(context.Context, string, string, string, []string) (string, string, error) { | ||
| return "", "", nil | ||
| }) |
| It("unmounts each fuse sidecar container", func() { | ||
| patches = gomonkey.ApplyFunc(kubeclient.ExecCommandInContainerWithContext, func(_ context.Context, _, containerName, _ string, cmd []string) (string, string, error) { | ||
| Expect(containerName).To(ContainSubstring(common.FuseContainerName)) | ||
| Expect(cmd).To(Equal([]string{"umount", expectedJuiceFSMountCmd})) | ||
| return "", "", nil | ||
| }) |
| Describe("Reconcile", func() { | ||
| It("returns no requeue when the pod does not exist", func() { |
![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 transitions the FluidApp controller tests to the Ginkgo/Gomega framework, adding a new test suite and refactoring existing implementation tests. The review feedback highlights a potential mismatch in a hardcoded unmount path constant and suggests improvements for test stability, such as addressing the thread-safety of gomonkey patches and ensuring robust cleanup using AfterEach blocks.
| }}, | ||
| }}, | ||
| var _ = Describe("FluidAppReconcilerImplement", func() { | ||
| const expectedJuiceFSMountCmd = "/mnt/jfs/juicefs-fuse" |
There was a problem hiding this comment.
The constant expectedJuiceFSMountCmd includes a hardcoded suffix /juicefs-fuse which seems inconsistent with the MountPath defined in the test cases (e.g., line 105 uses /mnt/jfs). If kubeclient.GetMountPathInContainer simply returns the mount path, the unmount command should be umount /mnt/jfs. Please verify if this suffix is intentional or if it should match the MountPath exactly.
| const expectedJuiceFSMountCmd = "/mnt/jfs/juicefs-fuse" | |
| const expectedJuiceFSMountCmd = "/mnt/jfs" |
| patches := gomonkey.ApplyFunc((*FluidAppReconcilerImplement).umountFuseSidecars, func(_ *FluidAppReconcilerImplement, gotPod *corev1.Pod) error { | ||
| Expect(gotPod.Name).To(Equal(testPodName)) | ||
| return nil | ||
| }) | ||
| defer patches.Reset() |
There was a problem hiding this comment.
Using gomonkey.ApplyFunc to patch methods is globally scoped and not thread-safe. If this test suite is executed in parallel with other tests that also patch the same method or rely on its original behavior, it will lead to race conditions and flaky tests. Consider using an interface-based mocking approach or ensure that parallel execution is disabled for this package.
| patches := gomonkey.ApplyFunc((*FluidAppReconcilerImplement).umountFuseSidecars, func(_ *FluidAppReconcilerImplement, gotPod *corev1.Pod) error { | ||
| Expect(gotPod.Name).To(Equal(testPodName)) | ||
| return nil | ||
| }) | ||
| defer patches.Reset() |
There was a problem hiding this comment.
For consistency with implement_test.go, consider declaring the patches variable at the Describe block level and resetting it in an AfterEach block. This ensures that patches are always cleaned up even if a test panics before reaching the defer statement.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #5828 +/- ##
=======================================
Coverage 58.89% 58.89%
=======================================
Files 479 479
Lines 32443 32443
=======================================
Hits 19107 19107
Misses 11784 11784
Partials 1552 1552 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Signed-off-by: Harsh <harshmastic@gmail.com>
|
cheyang
left a comment
cheyang
left a comment
There was a problem hiding this comment.
感谢贡献！几个建议：
patches变量在 Describe 作用域声明，但 AfterEach 重置逻辑依赖 It 内赋值。建议每个 Context/It 内局部创建 patches 并用 defer Reset()，避免跨测试残留。- 可以考虑补充 Reconcile 中更多错误路径的覆盖（如 updateStatus 失败）。
cheyang
left a comment
cheyang
left a comment
There was a problem hiding this comment.
感谢贡献！几个建议：
patches变量在 Describe 作用域声明，但 AfterEach 重置逻辑依赖 It 内赋值。建议每个 Context/It 内局部创建 patches 并用 defer Reset()，避免跨测试残留。- 可以考虑补充 Reconcile 中更多错误路径的覆盖（如 updateStatus 失败）。
|
感谢贡献！几个建议：
|
I.
Add Ginkgo v2 + Gomega unit coverage for the fluidapp controller package and migrate the touched fluidapp tests to the repo’s current testing style.
II.
#5676
III.
Added a package-level Ginkgo suite bootstrap, migrated
implement_test.gofrom stdlibtestingto Ginkgo/Gomega, and addedfluidapp_controller_ut_test.goto cover constructor/accessor behavior plusReconcileandinternalReconcilepaths.IV.
Run the fluidapp controller package tests and confirm the package coverage stays above the required threshold.
V.
N/A