test(thinruntime): add Ginkgo/Gomega unit tests#5694
test(thinruntime): add Ginkgo/Gomega unit tests#5694hxrshxz wants to merge 3 commits intofluid-cloudnative:masterfrom
Conversation
|
[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. |
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 for the Highlights
Changelog
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
|
There was a problem hiding this comment.
Pull request overview
This PR adds Ginkgo/Gomega unit tests for the thinruntime and juicefs controller packages, replacing the heavyweight envtest-based suite bootstrapping with a lightweight fake-client pattern. Tests cover controller helpers (getRuntime, GetOrCreateEngine, RemoveEngine), reconciler construction, cache options, and the not-found reconcile path.
Changes:
- Added unit tests for
thinruntimeandjuicefscontroller packages covering key reconciler methods and helpers - Simplified
suite_test.goin both packages by removingenvtestbootstrapping in favor of a lightweightfake.NullLogger()setup - Added new test files
implement_test.goand*_controller_test.goin both packages
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/controllers/v1alpha1/thinruntime/thinruntime_controller_test.go | New tests for ControllerName, ManagedResource, NewRuntimeReconciler, NewCache |
| pkg/controllers/v1alpha1/thinruntime/suite_test.go | Simplified suite bootstrap, removed envtest dependency |
| pkg/controllers/v1alpha1/thinruntime/implement_test.go | New tests for getRuntime, GetOrCreateEngine, RemoveEngine, Reconcile |
| pkg/controllers/v1alpha1/juicefs/suite_test.go | Simplified suite bootstrap, removed envtest dependency |
| pkg/controllers/v1alpha1/juicefs/juicefsruntime_controller_test.go | New tests for ControllerName, ManagedResource, NewRuntimeReconciler, NewCacheOption |
| pkg/controllers/v1alpha1/juicefs/implement_test.go | New tests for getRuntime, GetOrCreateEngine, RemoveEngine, Reconcile |
💡 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.
|
/gemini review |
1 similar comment
|
/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 introduces a comprehensive set of Ginkgo/Gomega unit tests for the thinruntime controller, significantly improving test coverage. The changes also include a valuable refactoring of the test suite setup for both thinruntime and juicefs controllers, replacing the heavier envtest environment with a more lightweight fake client pattern. The new tests are well-structured and cover important controller logic. I've pointed out a minor code duplication in test helpers that could be refactored for better maintainability. Overall, this is a great contribution to improving the test quality of the project.
There was a problem hiding this comment.
Code Review
This pull request adds valuable unit tests for the thinruntime controller using Ginkgo/Gomega, significantly improving test coverage. The refactoring to a lightweight test pattern by removing envtest is a good simplification for the test suite. I've noticed this pattern has also been applied to the juicefs controller tests, which brings nice consistency. My review includes a couple of suggestions to further improve maintainability and test completeness.
There was a problem hiding this comment.
Code Review
This pull request adds valuable unit tests for the thinruntime controller and refactors the test suite setup for both thinruntime and juicefs to a simpler, more lightweight pattern. The new tests significantly improve coverage for key controller logic.
My main feedback is regarding the significant code duplication in the newly added test files. Both implement_test.go and ..._controller_test.go files for thinruntime and juicefs are nearly identical. I've left a specific comment with suggestions on how to refactor this into shared test helpers, which would improve the maintainability and extensibility of the test suite.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #5694 +/- ##
==========================================
+ Coverage 61.21% 61.22% +0.01%
==========================================
Files 444 444
Lines 30540 30557 +17
==========================================
+ Hits 18694 18710 +16
- Misses 10306 10307 +1
Partials 1540 1540 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
/gemini review |
1 similar comment
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces unit tests for the thinruntime controller and refactors the testing pattern for it and other controllers like efc and juicefs. The move to a more lightweight approach using fake clients instead of envtest is a great improvement for test speed and simplicity.
The new tests are well-structured and cover key functionalities of the reconcilers.
I've added one comment regarding significant code duplication across the new test files. Refactoring this into a shared testing package would improve maintainability.
Overall, this is a solid contribution that improves the test quality of the controllers.
There was a problem hiding this comment.
Code Review
This pull request significantly improves test coverage by adding Ginkgo/Gomega unit tests for the thinruntime, efc, and juicefs controllers. The refactoring to a more lightweight test pattern by removing envtest is a welcome improvement for test execution speed. The new tests cover key reconciler paths and helper functions effectively. I have one suggestion to further improve maintainability by reducing code duplication in the test helpers.
hxrshxz
force-pushed
the
test/thinruntime-ginkgo-tests
branch
from
ac1732f to
0f11364
Compare
- 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>
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>
Signed-off-by: Harsh <harshmastic@gmail.com>
hxrshxz
force-pushed
the
test/thinruntime-ginkgo-tests
branch
from
0f11364 to
c60d0d3
Compare
|
Ⅰ. Describe what this PR does
Adds Ginkgo/Gomega unit tests for
pkg/controllers/v1alpha1/thinruntimeand replaces the empty suite bootstrap with the lightweight test pattern used by nearby controller packages. The package now validates controller helpers and key reconciler paths with measured coverage at 79.2%.Ⅱ. 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 unit tests for
getRuntime,GetOrCreateEngine,RemoveEngine, the not-foundReconcilepath,ControllerName,ManagedResource,NewRuntimeReconciler, andNewCache.Ⅳ. Describe how to verify it
Run
go test -v -coverprofile=/tmp/fluid-thinruntime.cover ./pkg/controllers/v1alpha1/thinruntime/... -count=1and confirm the package passes with total coverage above 75%. Optionally rungo tool cover -func=/tmp/fluid-thinruntime.coverto inspect the 79.2% coverage breakdown.Ⅴ. Special notes for reviews
N/A