Add unit tests for RuntimeReconciler lifecycle logic#5658
Add unit tests for RuntimeReconciler lifecycle logic#5658mrhapile wants to merge 1 commit 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 @mrhapile, 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 robustness of the Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. 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 @mrhapile. 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
The pull request introduces comprehensive unit tests for the RuntimeReconciler's lifecycle logic, covering owner reference creation, finalizer management, happy-path reconciliation, and error propagation. The tests are well-structured using mock engines and controller-runtime's fake client, which is a good approach for isolating the logic under test. Overall, this is a valuable addition to the codebase, significantly improving test coverage for critical controller functionality. I've identified a couple of areas where the test assertions could be made more robust to ensure they accurately reflect the expected behavior.
| if err.Error() != "Failed to create: induced engine creation failure" && err.Error() != "induced engine creation failure" { | ||
| t.Logf("Got expected error: %v", err) | ||
| } |
There was a problem hiding this comment.
The error message assertion logic here is inverted and could lead to false positives. The if condition currently logs a message if the error is not one of the expected strings, implying it's an expected error. Instead, the test should explicitly t.Fatalf if the error message does not match the expected value. Given that errors.Wrap is used in the actual code, the error message will be prefixed. A precise check for the full expected error string would make this test more robust.
| if err.Error() != "Failed to create: induced engine creation failure" && err.Error() != "induced engine creation failure" { | |
| t.Logf("Got expected error: %v", err) | |
| } | |
| if err == nil { | |
| t.Fatalf("Expected error from ReconcileInternal due to engine failure, got nil") | |
| } | |
| expectedError := "Failed to create: induced engine creation failure" | |
| if err.Error() != expectedError { | |
| t.Fatalf("Expected error message \"%s\", got \"%v\"", expectedError, err) | |
| } |
| if result.Requeue && result.RequeueAfter == 0 { | ||
| t.Errorf("Did not expect immediate Requeue for successful reconcile") | ||
| } |
There was a problem hiding this comment.
The current check if result.Requeue && result.RequeueAfter == 0 only catches a specific case of immediate requeue. To fully verify the utils.NoRequeue() semantics, which implies no requeue at all, it's better to explicitly check that both Requeue is false and RequeueAfter is zero. This ensures that the test fails if any form of requeue (immediate or delayed) is unexpectedly requested.
| if result.Requeue && result.RequeueAfter == 0 { | |
| t.Errorf("Did not expect immediate Requeue for successful reconcile") | |
| } | |
| if result.Requeue || result.RequeueAfter != 0 { | |
| t.Errorf("Expected no requeue for successful reconcile, got %v", result) | |
| } |
There was a problem hiding this comment.
Pull request overview
Adds controller-level unit tests to cover core RuntimeReconciler lifecycle behavior (owner refs, finalizers, deletion cleanup, and engine errors) using a controller-runtime fake client and a mock engine.
Changes:
- Introduces
pkg/controllers/runtime_controller_test.gowith a mockbase.Engineand a harness aroundRuntimeReconciler. - Adds tests for OwnerReference creation, finalizer addition/removal, happy-path reconcile, and engine creation error propagation.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if err.Error() != "Failed to create: induced engine creation failure" && err.Error() != "induced engine creation failure" { | ||
| t.Logf("Got expected error: %v", err) | ||
| } |
There was a problem hiding this comment.
In TestReconcileInternal_EngineError the assertion for the returned error is inverted and never fails on unexpected errors. The current condition logs "Got expected error" only when the error string does NOT match either expected value, and it doesn't fail the test in that case. Please change this to positively assert the error message (preferably using a contains check for the induced message to avoid brittle full-string comparisons) and fail when it doesn't match.
| if result.Requeue && result.RequeueAfter == 0 { | ||
| t.Errorf("Did not expect immediate Requeue for successful reconcile") | ||
| } |
There was a problem hiding this comment.
TestReconcileInternal_ReconcileRuntime is currently non-deterministic / under-asserted because RuntimeReconciler.ReconcileRuntime uses utils.GenerateRandomRequeueDurationFromEnv(), which by default forces a requeue-after (90s) unless FLUID_RUNTIME_RECONCILE_DURATION is set to -1. The test only checks that there is no immediate requeue, so it won't catch regressions. Consider setting utils.RuntimeReconcileDurationEnvVal (and restoring it with t.Cleanup) so the reconciler returns NoRequeue deterministically, and assert the full ctrl.Result you expect.
1 participant
Fixes #5657
This PR adds comprehensive unit test coverage for the core RuntimeReconciler
logic in
pkg/controllers/runtime_controller.go.What this PR does
pkg/controllers/runtime_controller_test.gocontroller-runtimefake clientCoverage added
Implementation details
controller-runtime/pkg/client/fakewith status subresource enabledRuntimeReconcilerbase logic in isolation via a mock enginegomonkeyin favor of interfaces and dependency injectionVerification
go test -v ./pkg/controllers/