test(efc): migrate pkg/controllers/v1alpha1/efc to Ginkgo v2 with fake-client unit tests#5698
test(efc): migrate pkg/controllers/v1alpha1/efc to Ginkgo v2 with fake-client unit tests#5698hxrshxz wants to merge 2 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 |
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 refactors the testing approach for the EFC controller, moving from a heavier integration-style setup to a more agile and focused unit testing methodology. This change aims to improve test execution speed, maintainability, and provide more granular coverage for core controller logic, ensuring robust functionality without relying on a full Kubernetes environment. Highlights
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. 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 EFC controller tests to Ginkgo v2 with fake-client unit tests, which is a significant improvement for test efficiency. The new tests are well-structured and cover essential controller logic. I have one suggestion regarding the usage of context in the test files to align with best practices.
|
|
||
| It("should return the runtime when it exists in the cluster", func() { | ||
| ctx := cruntime.ReconcileRequestContext{ | ||
| Context: context.TODO(), |
There was a problem hiding this comment.
In tests, it is a better practice to use context.Background() instead of context.TODO(). context.Background() is intended for top-level contexts in tests and initializations, whereas context.TODO() is a placeholder for when the correct context is not yet known. Please apply this change to all other occurrences of context.TODO() in this file.
| Context: context.TODO(), | |
| Context: context.Background(), |
There was a problem hiding this comment.
Pull request overview
Migrates pkg/controllers/v1alpha1/efc tests from envtest-based integration style to Ginkgo v2 + controller-runtime fake-client unit tests, improving speed and isolating controller helper behavior.
Changes:
- Replaced envtest bootstrap in
suite_test.gowith a lightweight Ginkgo v2 suite usingfake.NullLogger(). - Added unit tests for controller helpers (
ControllerName,ManagedResource,NewRuntimeReconciler). - Added unit tests for
getRuntime,GetOrCreateEngine,RemoveEngine, and the “runtime not found”Reconcilepath.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| pkg/controllers/v1alpha1/efc/suite_test.go | Removes envtest setup; uses lightweight Ginkgo v2 bootstrap with null logger. |
| pkg/controllers/v1alpha1/efc/implement_test.go | Adds fake-client unit tests covering helper methods and not-found reconcile behavior. |
| pkg/controllers/v1alpha1/efc/efcruntime_controller_test.go | Adds unit tests for controller identity and managed resource metadata. |
💡 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 |
There was a problem hiding this comment.
Code Review
This pull request effectively migrates the tests for the pkg/controllers/v1alpha1/efc package to Ginkgo v2 with a fake client, which is a commendable improvement for test efficiency. The new tests are well-written and cover the intended functionality. My review includes a couple of suggestions to further simplify the test setup code by removing redundant scheme initializations, leveraging the provided helper function more effectively.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request effectively migrates the EFC controller tests to use Ginkgo v2 with a fake client, which is a significant improvement over the previous envtest-based approach. This change simplifies the test setup and should result in faster test execution. The new tests are well-structured and cover the intended functionality. I have one minor suggestion to make an assertion more precise.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #5698 +/- ##
=======================================
Coverage 61.22% 61.22%
=======================================
Files 444 444
Lines 30557 30557
=======================================
Hits 18710 18710
Misses 10307 10307
Partials 1540 1540 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Signed-off-by: Harsh <harshmastic@gmail.com>
Signed-off-by: Harsh <harshmastic@gmail.com>
hxrshxz
force-pushed
the
test/efc-ginkgo-v2-tests
branch
from
7b31315 to
c0c8c17
Compare
|
Ⅰ. Describe what this PR does
Migrate the
pkg/controllers/v1alpha1/efcpackage tests from the legacy Ginkgo v1 + envtest bootstrap to Ginkgo v2 with lightweight fake-client unit tests covering all non-infrastructure functions.Ⅱ. 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.
Updated
pkg/controllers/v1alpha1/efc/suite_test.goto use a lightweight Ginkgo v2 bootstrap withfake.NullLogger(), addedpkg/controllers/v1alpha1/efc/efcruntime_controller_test.gofor controller helper coverage, and addedpkg/controllers/v1alpha1/efc/implement_test.goforgetRuntime,GetOrCreateEngine,RemoveEngine, and the not-foundReconcilepath. The package now has 10 passing specs and 78.6% statement coverage.Ⅳ. Describe how to verify it
Run
go test ./pkg/controllers/v1alpha1/efc/... -count=1 -v, then rungo test -coverprofile=/tmp/fluid-efc.cover ./pkg/controllers/v1alpha1/efc/... -count=1andgo tool cover -func=/tmp/fluid-efc.coverto confirm coverage stays above 75%. Optionally rungo vet ./pkg/controllers/v1alpha1/efc/....Ⅴ. Special notes for reviews
N/A