MGMT-23404: fix flaky Tenant StorageClass test - #170
Conversation
The test was flaky because it reconciled immediately after creating or deleting StorageClass objects without waiting for the controller's informer cache to sync. Replace the separate doReconcile()+assertStatus() calls with a single reconcileAndAssertStatus() helper that retries both reconciliation and assertion inside an Eventually block, giving the cache time to observe the latest state. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
@eranco74: This pull request references MGMT-23404 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
Instructions 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 openshift-eng/jira-lifecycle-plugin repository. |
📝 WalkthroughWalkthroughTest code is refactored to introduce a combined Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@eranco74: This pull request references MGMT-23404 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
Instructions 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 openshift-eng/jira-lifecycle-plugin repository. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/controller/tenant_controller_test.go (1)
90-105: Consider clarifying the comment about ignored errors.The term "transient errors" typically implies temporary API/network failures. However, per
handleUpdate()(lines 127-135 intenant_controller.go), the namespace-not-found error is an expected state in certain test steps—not a temporary failure that will resolve on retry.The approach itself is sound since status conditions are set before returning errors, but the comment could be more precise.
📝 Suggested wording
- // The reconcile error is intentionally ignored because the - // reconciler may return transient errors (e.g. namespace not - // found) while still correctly setting status conditions. + // The reconcile error is intentionally ignored because the + // reconciler returns errors for missing prerequisites (e.g., + // namespace not found) while still correctly setting status + // conditions. The test validates conditions, not error returns.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/controller/tenant_controller_test.go` around lines 90 - 105, The comment for reconcileAndAssertStatus should be clarified to avoid calling namespace-not-found a generic "transient error"; update the comment to explain that errors from doReconcile() are intentionally ignored because the reconciler (see handleUpdate in tenant_controller.go) may return expected state errors such as "namespace not found" as part of normal flow, and status conditions are set before those errors are returned; mention reconcileAndAssertStatus and doReconcile by name so future readers understand why the test ignores the reconcile error and still asserts status conditions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@internal/controller/tenant_controller_test.go`:
- Around line 90-105: The comment for reconcileAndAssertStatus should be
clarified to avoid calling namespace-not-found a generic "transient error";
update the comment to explain that errors from doReconcile() are intentionally
ignored because the reconciler (see handleUpdate in tenant_controller.go) may
return expected state errors such as "namespace not found" as part of normal
flow, and status conditions are set before those errors are returned; mention
reconcileAndAssertStatus and doReconcile by name so future readers understand
why the test ignores the reconcile error and still asserts status conditions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: fe97db94-cea1-4f0d-829a-af8fb7ed307b
📒 Files selected for processing (1)
internal/controller/tenant_controller_test.go
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: adriengentil, eranco74 The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
3 participants
Summary
should transition through all Ready/Progressing phases with conditionsintenant_controller_test.godoReconcile()+assertStatus()calls with a singlereconcileAndAssertStatus()helper that retries both reconciliation and assertion inside anEventuallyblock, giving the cache time to observe the latest stateTest plan
go test ./internal/controller/ -count=1)gofmt -s -l .reports no formatting issues🤖 Generated with Claude Code
Summary by CodeRabbit