test(alluxio): migrate suite to Ginkgo v2 and add unit tests for controller and implement#5686
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 test coverage and modernizes the testing framework for the AlluxioRuntime controller. By migrating to Ginkgo v2 and adding comprehensive unit tests for key controller and implementation methods, it improves the reliability and maintainability of the Alluxio integration. The changes ensure that core functionalities like runtime reconciliation, engine management, and object retrieval behave as expected under various conditions. 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
|
There was a problem hiding this comment.
Pull request overview
Migrates the Alluxio controller test suite to Ginkgo v2 style and adds unit tests for key controller and implement behaviors.
Changes:
- Updated the controller suite bootstrap from Ginkgo v1
BeforeSuite(done Done)to Ginkgo v2BeforeSuite(func()) - Added unit tests for
getRuntime, engine map management (GetOrCreateEngine,RemoveEngine), and controller basics (NewRuntimeReconciler,ControllerName,Reconcile) - Simplified suite configuration by removing existing-cluster toggling
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| pkg/controllers/v1alpha1/alluxio/suite_test.go | Migrates suite setup to Ginkgo v2 BeforeSuite style and adjusts envtest bootstrap behavior |
| pkg/controllers/v1alpha1/alluxio/implement_test.go | Adds unit tests for getRuntime, RemoveEngine, and GetOrCreateEngine |
| pkg/controllers/v1alpha1/alluxio/alluxio_runtime_controller_test.go | Adds unit tests for controller construction, naming, and Reconcile 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.
| var _ = BeforeSuite(func() { | ||
| logf.SetLogger(zap.New(zap.WriteTo(GinkgoWriter), zap.UseDevMode(true))) | ||
| if env := os.Getenv("USE_EXISTING_CLUSTER"); env == "true" { | ||
| useExistingCluster = true | ||
| } | ||
|
|
||
| By("bootstrapping test environment") | ||
| testEnv = &envtest.Environment{ |
There was a problem hiding this comment.
The prior Ginkgo v1 suite used an explicit 60s timeout for BeforeSuite, but the v2 migration removes it. If envtest startup hangs (assets missing, etc.), the suite may block indefinitely. Consider adding an explicit Ginkgo v2 timeout (e.g., a NodeTimeout(...) option on BeforeSuite or suite-level timeout configuration) to restore the previous bounded behavior.
| if err != nil { | ||
| Expect(err.Error()).ToNot(ContainSubstring("not found")) | ||
| } |
There was a problem hiding this comment.
This assertion is brittle and can fail for legitimate reasons: ReconcileInternal may attempt to fetch other resources (the comment even mentions a missing Dataset) which can also produce a "not found" error string. Instead of substring-matching the error message, prefer a structured check (e.g., use Kubernetes apierrors.IsNotFound(err) and, if it's NotFound, assert the missing kind/resource matches the AlluxioRuntime rather than any dependency), or restructure the test to isolate/verify only the getRuntime portion (e.g., by directly testing getRuntime or mocking/stubbing the internal reconcile step).
| // Inject a sentinel value directly so we can verify removal. | ||
| r.mutex.Lock() | ||
| r.engines[id] = nil | ||
| r.mutex.Unlock() |
There was a problem hiding this comment.
Using nil as the sentinel map value makes this test less robust: depending on RemoveEngine's implementation, code might treat a nil interface value as equivalent to an absent engine (or skip deletion when the value is nil). It’s safer to store a non-nil fake/stub base.Engine implementation so the test definitively exercises the 'engine present' removal behavior.
|
/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 successfully migrates the Alluxio test suite to Ginkgo v2 and expands unit test coverage for the controller and its implementation. The changes are well-structured and the new tests cover important functions. My review includes a few suggestions to further improve the tests: consistently using context.Background() instead of context.TODO() for root contexts in tests, and strengthening a test assertion in alluxio_runtime_controller_test.go to more accurately reflect the behavior under test.
I am having trouble creating individual review comments. Click here to see my feedback.
pkg/controllers/v1alpha1/alluxio/alluxio_runtime_controller_test.go (85)
In Go tests, it's a best practice to use context.Background() for the root context of a test case instead of context.TODO(). This makes the intent clearer that you are starting a new context tree for the test.
result, err := r.Reconcile(context.Background(), req)
pkg/controllers/v1alpha1/alluxio/alluxio_runtime_controller_test.go (110-115)
This test's assertion could be stronger, and the accompanying comment is misleading. When a Dataset is missing, ReconcileInternal returns a requeue result, not an error. This means err is nil, and the if err != nil block is dead code in this scenario.
To make the test more accurate, you should assert that a requeue is requested. This also provides an opportunity to use context.Background() as per Go testing best practices.
result, err := r.Reconcile(context.Background(), req)
Expect(err).ToNot(HaveOccurred())
Expect(result.Requeue).To(BeTrue())
pkg/controllers/v1alpha1/alluxio/implement_test.go (75)
In Go tests, it's a best practice to use context.Background() for the root context of a test case instead of context.TODO(). This makes the intent clearer that you are starting a new context tree for the test. This improvement can be applied to all uses of context.TODO() in this file.
Context: context.Background(),
There was a problem hiding this comment.
Code Review
This pull request successfully migrates the Alluxio controller test suite to Ginkgo v2 and adds several new unit tests, significantly improving test coverage. The changes are well-structured. My review includes a few suggestions to further improve the new tests, mainly focusing on strengthening an assertion to check for an expected error instead of just a lack of panic, and consistently using context.Background() instead of context.TODO() for clarity in test contexts.
| // Reconcile proceeds past getRuntime, builds the engine, then hits the | ||
| // "no dataset bound" branch which returns RequeueAfter(5s) with no error. | ||
| result, err := r.Reconcile(context.TODO(), req) | ||
| Expect(err).ToNot(HaveOccurred()) | ||
| Expect(result.RequeueAfter).ToNot(BeZero()) |
There was a problem hiding this comment.
The assertion on line 111 only checks that the Reconcile call doesn't panic, while ignoring the returned error with nolint:errcheck. Since ReconcileInternal is expected to fail in this unit test, a more robust test would be to assert that an error is indeed returned. This makes the test's intent clearer and removes the need for the nolint directive.
Additionally, it's preferable to use context.Background() over context.TODO().
| // Reconcile proceeds past getRuntime, builds the engine, then hits the | |
| // "no dataset bound" branch which returns RequeueAfter(5s) with no error. | |
| result, err := r.Reconcile(context.TODO(), req) | |
| Expect(err).ToNot(HaveOccurred()) | |
| Expect(result.RequeueAfter).ToNot(BeZero()) | |
| // Reconcile will proceed past getRuntime (runtime exists) and call | |
| // ReconcileInternal. We expect an error because ReconcileInternal | |
| // will fail on missing Dataset or other resources in unit-test conditions. | |
| _, err := r.Reconcile(context.Background(), req) | |
| Expect(err).To(HaveOccurred()) |
| Namespace: testNamespace, | ||
| }, | ||
| } | ||
| result, err := r.Reconcile(context.TODO(), req) |
There was a problem hiding this comment.
It's a good practice to use context.Background() instead of context.TODO() in tests when no specific context is needed. context.Background() is more explicit that this is a root context for a test operation.
| result, err := r.Reconcile(context.TODO(), req) | |
| result, err := r.Reconcile(context.Background(), req) |
| } | ||
| r := makeReconciler(s, rt) | ||
| ctx := cruntime.ReconcileRequestContext{ | ||
| Context: context.TODO(), |
There was a problem hiding this comment.
It's a good practice to use context.Background() instead of context.TODO() in tests when no specific context is needed. This applies to other usages of context.TODO() in this file as well (e.g., lines 91, 114, 129, 142).
| Context: context.TODO(), | |
| Context: context.Background(), |
There was a problem hiding this comment.
Code Review
This pull request successfully migrates the Alluxio controller test suite to Ginkgo v2 and introduces new unit tests, enhancing test coverage for the controller and its implementation. The changes are well-executed. My review includes a suggestion to replace context.TODO() with context.Background() in the new test files to align with Go's best practices for context management.
| Namespace: testNamespace, | ||
| }, | ||
| } | ||
| result, err := r.Reconcile(context.TODO(), req) |
There was a problem hiding this comment.
For top-level contexts in tests, context.Background() is preferred over context.TODO(). context.TODO() is a placeholder, while context.Background() is the standard for a root context. Please apply this change to all occurrences of context.TODO() in this file.
| result, err := r.Reconcile(context.TODO(), req) | |
| result, err := r.Reconcile(context.Background(), req) |
| } | ||
| r := makeReconciler(s, rt) | ||
| ctx := cruntime.ReconcileRequestContext{ | ||
| Context: context.TODO(), |
There was a problem hiding this comment.
For top-level contexts in tests, context.Background() is preferred over context.TODO(). context.TODO() is a placeholder, while context.Background() is the standard for a root context. Please apply this change to all occurrences of context.TODO() in this file.
| Context: context.TODO(), | |
| Context: context.Background(), |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #5686 +/- ##
=======================================
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:
|
hxrshxz
force-pushed
the
test/alluxio-controller-ginkgo-v2
branch
from
5886324 to
4f431d2
Compare
…roller and implement - Migrate BeforeSuite to Ginkgo v2 style with NodeTimeout(60s) - Add unit tests for NewRuntimeReconciler, ControllerName, and Reconcile - Add unit tests for getRuntime, RemoveEngine, GetOrCreateEngine - Use context.Background() throughout tests - Use non-nil MockEngine stub in RemoveEngine test Signed-off-by: Harsh <harshmastic@gmail.com>
hxrshxz
force-pushed
the
test/alluxio-controller-ginkgo-v2
branch
from
e9c794b to
4662d24
Compare
|
Ⅰ. Describe what this PR does
Migrate the AlluxioRuntime controller package to Ginkgo v2 suite style and add unit tests covering
NewRuntimeReconciler,ControllerName,Reconcile,getRuntime,GetOrCreateEngine, andRemoveEngine.Ⅱ. 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.
implement_test.go:getRuntime— returns runtime when present, errors when absent;RemoveEngine— removes entry, no-op when missing;GetOrCreateEngine— returns error for unknown engine implalluxio_runtime_controller_test.go:NewRuntimeReconciler— fields initialised;ControllerName— returns expected constant;Reconcile— early-exit on not-found runtime, proceeds past getRuntime when runtime existsⅣ. Describe how to verify it
Ⅴ. Special notes for reviews
N/A