test(juicefs): migrate controller tests to Ginkgo v2#5690
test(juicefs): migrate controller tests to Ginkgo v2#5690hxrshxz wants to merge 2 commits intofluid-cloudnative:masterfrom
Conversation
- Update suite_test.go to Ginkgo v2 (remove deprecated Done channel) - Add implement_test.go: getRuntime, GetOrCreateEngine, RemoveEngine - Add juicefsruntime_controller_test.go: ControllerName, ManagedResource, NewRuntimeReconciler, NewCacheOption - Coverage: 79.1% (gate >75% PASS) 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 |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the testing infrastructure and coverage for the JuiceFS controller. By migrating to Ginkgo v2 and Gomega, it modernizes the testing framework and introduces comprehensive unit tests for critical controller logic, ensuring greater reliability and maintainability of the codebase. Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
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 successfully migrates the JuiceFS controller tests to Ginkgo v2 and significantly enhances the test suite by adding comprehensive unit tests. The transition from integration-style tests using envtest to faster unit tests with a fake client is a commendable improvement. The new tests are well-structured, covering essential logic including error paths and caching mechanisms. I have one minor suggestion to improve test code conciseness.
| Expect(r.ControllerName()).To(Equal(controllerName)) | ||
| Expect(r.ControllerName()).To(Equal("JuiceFSRuntimeController")) |
There was a problem hiding this comment.
This test contains redundant assertions. Both Expect calls verify the same condition since the unexported constant controllerName is defined as "JuiceFSRuntimeController". Please remove one of them to make the test more concise. Using the string literal is preferred as it decouples the test from the constant's implementation details.
| Expect(r.ControllerName()).To(Equal(controllerName)) | |
| Expect(r.ControllerName()).To(Equal("JuiceFSRuntimeController")) | |
| Expect(r.ControllerName()).To(Equal("JuiceFSRuntimeController")) |
There was a problem hiding this comment.
Pull request overview
Migrates the JuiceFS controller tests under pkg/controllers/v1alpha1/juicefs/ to Ginkgo v2/Gomega and replaces the previous envtest-based suite scaffolding with lightweight unit-test setup using fake clients/loggers.
Changes:
- Update
suite_test.goto a Ginkgo v2 bootstrap (removing the deprecatedDonechannel and envtest bootstrapping). - Add unit tests for
JuiceFSRuntimeReconcilercontroller helpers (ControllerName,ManagedResource,NewRuntimeReconciler,NewCacheOption). - Add unit tests for implementation behaviors (
getRuntime, engine cache create/remove paths, andReconcilenot-found handling).
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| pkg/controllers/v1alpha1/juicefs/suite_test.go | Switch to Ginkgo v2 suite bootstrap; drop envtest setup/teardown. |
| pkg/controllers/v1alpha1/juicefs/juicefsruntime_controller_test.go | Add unit coverage for basic controller methods and cache options. |
| pkg/controllers/v1alpha1/juicefs/implement_test.go | Add unit coverage for runtime lookup, engine caching, removal, and reconcile not-found behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
Remove the duplicate assertion that tested against the unexported constant; use the string literal form only as suggested by code review. Signed-off-by: Harsh <harshmastic@gmail.com>
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #5690 +/- ##
=======================================
Coverage 61.21% 61.21%
=======================================
Files 444 444
Lines 30540 30540
=======================================
Hits 18694 18694
Misses 10306 10306
Partials 1540 1540 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Ⅰ. Describe what this PR does
Migrate
pkg/controllers/v1alpha1/juicefs/to Ginkgo v2/Gomega, adding controller unit tests with 79.1% coverage.Ⅱ. Does this pull request fix one issue?
NONE
Ⅲ. List the added test cases (unit test/integration test) if any, please explain if no tests are needed.
suite_test.go: updated to Ginkgo v2 bootstrap (removes deprecatedDonechannel)juicefsruntime_controller_test.go:ControllerName,ManagedResource,NewRuntimeReconciler,NewCacheOptionimplement_test.go:getRuntime(found/not-found),GetOrCreateEngine(create-error, create+cache-hit),RemoveEngine(remove/no-op),Reconcile(runtime-not-found)Ⅳ. Describe how to verify it
Ⅴ. Special notes for reviews
N/A