test(jindo): migrate controller tests to Ginkgo/Gomega#5688
test(jindo): migrate controller tests to Ginkgo/Gomega#5688hxrshxz wants to merge 3 commits intofluid-cloudnative:masterfrom
Conversation
Migrate pkg/controllers/v1alpha1/jindo/ unit tests to Ginkgo v2/Gomega. Fix broken suite bootstrap (Ginkgo v1 Unknown Decorator error) and add full coverage for implement.go and jindoruntime_controller.go (86.0%). 13/13 specs pass.
|
[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 upgrades the testing infrastructure for the Jindo controller. By migrating to Ginkgo v2/Gomega and expanding test coverage, it improves the robustness and maintainability of the controller's core logic, making future development and refactoring safer and more efficient. 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
Migrates Jindo controller unit tests to Ginkgo v2/Gomega, removing the old envtest-based suite bootstrap and adding new unit coverage for controller/implement logic.
Changes:
- Replaced the Ginkgo v1 suite bootstrap with a minimal Ginkgo v2
RunSpecsentrypoint. - Added new Ginkgo v2/Gomega unit tests for controller behavior and engine/runtime helpers.
- Expanded coverage for not-found and error paths (e.g., engine impl selection, runtime retrieval).
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| pkg/controllers/v1alpha1/jindo/suite_test.go | Simplifies test suite bootstrap for Ginkgo v2 and removes envtest setup. |
| pkg/controllers/v1alpha1/jindo/implement_test.go | Adds unit tests for getRuntime, GetOrCreateEngine, and RemoveEngine. |
| pkg/controllers/v1alpha1/jindo/controller_test.go | Adds unit tests for reconciler construction, managed resource metadata, and reconcile paths. |
💡 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-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 Jindo controller unit tests to Ginkgo/Gomega, improving test structure and coverage. The changes are well-executed, particularly the cleanup of suite_test.go. My feedback focuses on improving the new test files (controller_test.go and implement_test.go) by reducing code duplication and consolidating redundant test cases for better maintainability.
- Replace silent _ = datav1alpha1.AddToScheme(s) with Expect(datav1alpha1.AddToScheme(s)).NotTo(HaveOccurred()) in all It blocks across controller_test.go and implement_test.go so scheme-registration failures surface immediately rather than producing misleading test behaviour. - Change r.mutex.Lock()/Expect/r.mutex.Unlock() sequence in RemoveEngine test to use defer r.mutex.Unlock() so the lock is always released even when an Expect assertion fails.
|
/gemini review |
|
/gemini review |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #5688 +/- ##
=======================================
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:
|
There was a problem hiding this comment.
Code Review
This pull request successfully migrates the Jindo controller unit tests to Ginkgo v2/Gomega, resolving the Unknown Decorator error and significantly expanding test coverage for implement.go and jindoruntime_controller.go. The new tests are well-structured and cover various scenarios, enhancing the overall reliability and maintainability of the codebase. One minor improvement could be made to ensure consistency and precision in APIVersion assertions.
There was a problem hiding this comment.
Code Review
This pull request successfully migrates the Jindo controller tests to Ginkgo v2/Gomega, addressing the Unknown Decorator error and significantly enhancing test coverage for implement.go and jindoruntime_controller.go. The newly added test files, controller_test.go and implement_test.go, are well-structured and cover essential functionalities, including runtime creation, controller naming, managed resource type meta, and various Reconcile, getRuntime, GetOrCreateEngine, and RemoveEngine scenarios. The changes in suite_test.go correctly remove the outdated Ginkgo v1 setup and envtest dependencies, streamlining the testing environment. Overall, the changes improve the robustness and maintainability of the Jindo controller's test suite.
|
Ⅰ. Describe what this PR does
Migrate
pkg/controllers/v1alpha1/jindo/unit tests to Ginkgo v2/Gomega, fix the broken suite bootstrap (Ginkgo v1Unknown Decoratorerror), and add full test coverage forimplement.goandjindoruntime_controller.go.Ⅱ. 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.
New test file
controller_test.go:NewRuntimeReconciler— verifies non-nil Scheme, engines map, mutex, and embedded RuntimeReconcilerControllerName— verifies constant valueJindoRuntimeControllerManagedResource— verifies returned JindoRuntime has correct Kind and APIVersion TypeMetaReconcile(not-found path) — verifiesctrl.Result{}, nilwhen runtime is absentReconcile(invalid DNS name) — verifies error returned for runtime name starting with a digitNew test file
implement_test.go:getRuntime(found) — verifies correct JindoRuntime is returned from fake clientgetRuntime(not-found) — verifies error is returnedGetOrCreateEngine(create) — verifies new engine is created and non-nilGetOrCreateEngine(cached) — verifies second call returns same engine instanceGetOrCreateEngine(unknown impl) — verifies error for unregistered engine implRemoveEngine(removes) — verifies engine map is empty after removalRemoveEngine(no-op) — verifies no panic when engine not presentUpdated
suite_test.go:BeforeSuite(func(done Done){...}, 60)pattern (causedUnknown Decoratorpanic)Ⅳ. Describe how to verify it
Ⅴ. Special notes for reviews
N/A