test(csi/plugins): add Ginkgo coverage for register paths#5826
Conversation
|
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. |
![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 significantly improves test coverage for the CSI plugins by adding comprehensive unit tests for nodeserver.go and register.go. Key additions include test cases for volume publishing, unpublishing, node labeling, and mount point management, utilizing gomonkey for method patching. Feedback highlights potential brittleness when patching external methods with gomonkey and platform-dependency concerns regarding the use of null bytes in file paths for error simulation.
|
|
||
| fakeMounter := mount.New("") | ||
| isLikelyNotMountPointCalls := 0 | ||
| isLikelyNotMountPointPatch := gomonkey.ApplyMethod(reflect.TypeOf(fakeMounter), "IsLikelyNotMountPoint", func(_ *mount.Mounter, file string) (bool, error) { |
There was a problem hiding this comment.
Using reflect.TypeOf(fakeMounter) to patch a method from an external package like k8s.io/utils/mount can be brittle, especially if the concrete type returned by mount.New("") varies across platforms or if the method is inlined. Since fakeMounter is an interface, it's safer to use the concrete type directly if known, or ensure that the test environment is consistent. Additionally, gomonkey patching of methods in external packages often requires disabling inlining during test execution (e.g., using -gcflags=-l).
| }) | ||
|
|
||
| It("should return stat errors for invalid paths", func() { | ||
| clientset, err := getNodeAuthorizedClientFromKubeletConfig(string([]byte{'\x00'})) |
There was a problem hiding this comment.
Using a null byte \x00 in a file path to trigger an os.Stat error is a common trick in Go tests, but its behavior can be platform-dependent. While it works on most Unix-like systems to trigger an EINVAL or similar error, consider using a more deterministic approach for triggering specific error types if this test is intended to run in diverse environments.
There was a problem hiding this comment.
Pull request overview
This PR migrates and expands unit test coverage for pkg/csi/plugins using Ginkgo v2 + Gomega, focusing on covering CSI plugin register paths and key nodeserver behaviors to keep package coverage above the required threshold.
Changes:
- Added new Ginkgo specs for
getNodeAuthorizedClientFromKubeletConfig,Register, andEnabled. - Added additional deterministic nodeserver tests to cover publish/unpublish success paths, runtime-info fuse label fallback, clean-fuse behavior, and some error branches.
- Aligned the package test suite to run under
package pluginsfor package-scoped coverage.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| pkg/csi/plugins/register_test.go | Adds package-scoped Ginkgo tests for register/kubelet-client init and plugin enablement. |
| pkg/csi/plugins/plugins_suite_test.go | Switches suite to package plugins to support package-scoped coverage/tests. |
| pkg/csi/plugins/nodeserver_test.go | Adds/adjusts nodeserver tests to exercise additional success and fallback branches for coverage. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| resp, err := ns.NodeUnpublishVolume(context.Background(), req) | ||
|
|
||
| Expect(err).NotTo(HaveOccurred()) | ||
| Expect(resp).NotTo(BeNil()) | ||
| Expect(unmountCalls).To(Equal(1)) | ||
| Expect(isLikelyNotMountPointCalls).To(Equal(2)) | ||
| }) |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #5826 +/- ##
==========================================
+ Coverage 59.10% 59.36% +0.26%
==========================================
Files 480 480
Lines 32512 32512
==========================================
+ Hits 19215 19301 +86
+ Misses 11747 11645 -102
- Partials 1550 1566 +16 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
cheyang
left a comment
cheyang
left a comment
There was a problem hiding this comment.
感谢贡献！几个注意点：
- 新增的 bind mount 成功路径测试用 gomonkey 对 cmdguard.Command 打桩，在 ARM 架构上 gomonkey 可能不稳定（因 inline function 限制）。请确认测试在 ARM runner 上也能通过。
- DeferCleanup 用法正确，临时目录清理及时。整体结构不错。
cheyang
left a comment
cheyang
left a comment
There was a problem hiding this comment.
感谢贡献！几个注意点：
- 新增的 bind mount 成功路径测试用 gomonkey 对 cmdguard.Command 打桩，在 ARM 架构上 gomonkey 可能不稳定（因 inline function 限制）。请确认测试在 ARM runner 上也能通过。
- DeferCleanup 用法正确，临时目录清理及时。整体结构不错。
This comment has been minimized.
This comment has been minimized.
|
English translation of the previous review comments:
|
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/csi-plugins-register-ginkgo
branch
from
136652d to
468fbdb
Compare
|
|
Removed the fragile The |
|
[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 |
|
/approve |
Ⅰ. Describe what this PR does
Migrate the CSI plugins register coverage to Ginkgo v2 + Gomega and add package-scoped tests that raise
pkg/csi/pluginscoverage above the required threshold.Ⅱ. 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.
Added Ginkgo unit tests for
getNodeAuthorizedClientFromKubeletConfig,Register, andEnabled, and added deterministicnodeserverpath tests to cover successful publish/unpublish, runtime-info fallback labeling, and clean-fuse execution paths needed for package coverage.Ⅳ. Describe how to verify it
Run the CSI plugins package unit tests and confirm the package coverage stays above 75%.
Ⅴ. Special notes for reviews
N/A