NO-JIRA: Add testing guidelines for stability - #1451
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@rikatz: This pull request explicitly references no jira issue. 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. |
|
Warning Ignoring CodeRabbit configuration file changes. For security, only the configuration from the base branch is applied for open source repositories. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughThis pull request adds end-to-end testing guidance in three places: 🚥 Pre-merge checks | ✅ 11 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (11 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
|
[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 |
96db456 to
c457032
Compare
|
/retest e2e-aws-operator: AWS credentials weren't injected, installer hung on interactive prompt until 2h timeout. e2e-hypershift: break-glass-credentials/independent_signers CRR timed out waiting for cert revocation |
|
@rikatz: all tests passed! Full PR test history. Your PR dashboard. 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-sigs/prow repository. I understand the commands that are listed here. |
| Review e2e test code for these reliability patterns (see test/e2e/TESTING.md for full guide): | ||
|
|
||
| 1. CLEANUP: Resource deletion MUST use `t.Cleanup(func() { deleteWithRetryOnError(t, context.Background(), obj, 2*time.Minute) })`. | ||
| Flag any of these anti-patterns: | ||
| - `defer func() { kclient.Delete(...) }()` — use `t.Cleanup` + `deleteWithRetryOnError` instead | ||
| - `defer assertDeleted(...)` or `t.Cleanup(func() { assertDeleted(...) })` — use `deleteWithRetryOnError` instead (assertDeleted fatals on transient errors) | ||
| - Inline `kclient.Delete` in cleanup without retry logic | ||
| - IngressController cleanup: use `t.Cleanup(func() { assertIngressControllerDeleted(t, kclient, ic) })` not `defer` | ||
|
|
||
| 2. CLEANUP SEVERITY: `t.Fatalf()` MUST NOT be used inside `t.Cleanup()` or `defer` functions. Use `t.Errorf()` instead. | ||
| A fatal in cleanup calls runtime.Goexit(), masks the original failure, and prevents other cleanup functions from running. | ||
|
|
||
| 3. RESOURCE UPDATES: Kubernetes resource mutations MUST use retry-on-conflict helpers that re-fetch before update: | ||
| - `updateIngressControllerWithRetryOnConflict()` | ||
| - `updateIngressConfigSpecWithRetryOnConflict()` / `updateIngressConfigStatusWithRetryOnConflict()` | ||
| - `updateRouteWithRetryOnConflict()` | ||
| Flag any pattern that does `Get()` then `Update()` without conflict retry. | ||
|
|
||
| 4. TIMEOUTS: Controller condition polling (`waitForIngressControllerCondition`) MUST use at least `3*time.Minute`. | ||
| Flag any timeout under 1 minute for condition checks (e.g., `10*time.Second`). | ||
| Standard cleanup timeout is `2*time.Minute`. | ||
|
|
||
| 5. ERROR WRAPPING: `fmt.Errorf` SHOULD use `%w` (not `%v`) when the error will be inspected with `errors.Is`/`errors.As`. | ||
|
|
||
| 6. ERROR TYPE CHECKS: Delete operations MUST check `errors.IsNotFound` (not `IsAlreadyExists`). | ||
| Create operations MUST check `errors.IsAlreadyExists` (not `IsNotFound`). | ||
| Flag inverted error type guards — they silently swallow real errors. | ||
|
|
||
| 7. NO UNBOUNDED POLLING: `wait.PollInfinite` MUST NOT be used — it hangs the test suite forever. | ||
| Prefer `wait.PollUntilContextTimeout` over deprecated `wait.PollImmediate` (context-aware, cancellable). | ||
|
|
||
| 8. SHARED MUTABLE STATE: Package-level variables shared between parallel tests MUST use `atomic.Int32` or `sync.Mutex`. | ||
| Never mutate package-level globals (`dnsConfig`, `infraConfig`) from individual tests — read into local variables. | ||
|
|
||
| 9. NIL SAFETY: Guard `infraConfig.Status.PlatformStatus` before accessing `.Type` — it can be nil on some platforms. | ||
| Flag `&value == nil` comparisons (address-of-value is always non-nil, this is dead code). | ||
| Validate slice length before indexing (e.g., `record.Spec.Targets[0]` without length check). | ||
|
|
||
| 10. FUNCTION CONTRACTS: If a helper returns `error`, it MUST NOT call `t.Fatalf` internally (the error return becomes dead code). | ||
| If a helper calls `t.Fatalf`, it should not have an `error` return type. | ||
|
|
||
| 11. DEFER IN LOOPS: `defer` inside a loop accumulates until function exit, causing resource leaks (file descriptors, log streams). | ||
| Close resources explicitly within each loop iteration or use `t.Cleanup`. | ||
|
|
||
| 12. LOG ACCURACY: `t.Log` / `t.Logf` messages MUST accurately describe the operation being performed. | ||
| Flag log messages that contradict the code (e.g., logging "Classic" while setting NLB). | ||
|
|
||
| 13. CONTEXT PROPAGATION: Inner polling loops MUST derive their context from the outer loop's context. | ||
| Flag `context.Background()` inside a poll callback when an outer context parameter is available. | ||
| Flag `defer cancel()` inside poll callbacks — it accumulates deferred calls across iterations. | ||
|
|
||
| 14. TEST HELPERS: Functions accepting `*testing.T` that are not test functions MUST call `t.Helper()`. | ||
| Flag return values of `wait.PollUntilContextTimeout` that are silently discarded (not checked or returned). | ||
|
|
||
| 15. RESOURCE LIFECYCLE: Resources MUST be created (with error check) BEFORE registering `t.Cleanup` for deletion. | ||
| Flag patterns where `buildEcho*`/`buildRoute*` is followed by `t.Cleanup` without an intervening `kclient.Create`. | ||
|
|
||
| See test/e2e/TESTING.md for the full patterns guide. |
There was a problem hiding this comment.
Is it necessary to summarize test/e2e/TESTING.md here in addition to referencing TESTING.md? How do we keep the CodeRabbit summary and TESTING.md synchronized?
|
|
||
| ```go | ||
| // CORRECT — bounded timeout | ||
| wait.PollUntilContextTimeout(ctx, 5*time.Second, 2*time.Minute, true, func(ctx context.Context) (bool, error) { |
There was a problem hiding this comment.
Where does ctx come from? It might be useful to have some guidance on whether it is appropriate to use a new context.Background() for each polling loop, whether it is better to create a single context for the whole test, or to do something else.
|
/assign @grzpiotrowski |
| func testHeaders(t *testing.T, ...) { | ||
| count := podCount.Add(1) | ||
| podName := fmt.Sprintf("test-pod-%d", count) | ||
| } |
There was a problem hiding this comment.
This more verbose example might be clearer:
| func testHeaders(t *testing.T, ...) { | |
| count := podCount.Add(1) | |
| podName := fmt.Sprintf("test-pod-%d", count) | |
| } | |
| // testHeaders is a helper that may be used in in multiple tests. | |
| func testHeaders(t *testing.T, ...) { | |
| t.Helper() | |
| count := podCount.Add(1) | |
| podName := fmt.Sprintf("test-pod-%d", count) | |
| ... | |
| } |
| // CORRECT — read into a local variable | ||
| localDNS := configv1.DNS{} | ||
| if err := kclient.Get(ctx, types.NamespacedName{Name: "cluster"}, &localDNS); err != nil { ... } |
There was a problem hiding this comment.
This example is missing the mutation. If you add the mutation, is this approach really correct? It leaves the global dnsConfig inconsistent with the persisted value. Maybe it would be better to update the global value, but protect it with a mutex.
There was a problem hiding this comment.
Maybe a mutex isn't even necessary; any test that mutates cluster config probably needs to be a serial test anyway.
|
|
||
| ### 5.1 Guard `PlatformStatus` before access | ||
|
|
||
| `infraConfig.Status.PlatformStatus` can be nil on some platforms. Always guard: |
There was a problem hiding this comment.
This is false. See https://bugzilla.redhat.com/show_bug.cgi?id=1890038#c26.
| | IBM Cloud DNS warmup | `7*time.Minute` | Platform-specific negative caching | | ||
| | Quick API fetches | `1*time.Minute` | For simple Get/List operations | | ||
|
|
||
| Never use `10*time.Second` or similar tiny timeouts for controller condition checks. These cause flaky failures under CI load. |
There was a problem hiding this comment.
Short timeouts that cause flakiness is a recurring issue, so maybe it is worth elaborating on this point.
Generally, a timeout should be at least twice the expected worst case. For example, if provisioning a load balancer has been observed to take up to 5 minutes when CI is under load, make the timeout at least 10 minutes. Using a longer timeout does not make the test take more time unless it actually reaches the timeout, and in the case of parallel tests, a long timeout doesn't necessarily increase the overall CI job run time at all. In contrast, timing out early on an operation that would have succeeded with a longer timeout makes the test flakier and causes developers to rerun the CI job with the flaky test, which takes far more time and wastes far more resources than having a longer timeout.
If there is a need to verify that an operation completes within a specific period of time, make a dedicated test specifically to check how long the operation takes. For example, an Origin monitor test could verify that SLB provisioning time does not exceed some threshold based on historical data. However, the purpose of operator E2E tests is not to verify that SLBs do not take too long to provision, and so these tests should use timeouts that are as long as necessary to avoid failure under load.
|
|
||
| ## 7. Error Wrapping | ||
|
|
||
| Use `%w` (not `%v`) in `fmt.Errorf` when the returned error may be inspected with `errors.Is()` or `errors.As()`: |
There was a problem hiding this comment.
Using errors.Wrap or errors.Wrapf is also fine.
|
|
||
| ### 8.1 Consistent function contract | ||
|
|
||
| If a helper function has an `error` return, it must NOT call `t.Fatalf` internally. Either return the error for the caller to handle, or call `t.Fatalf` and return nothing: |
There was a problem hiding this comment.
It is overly complicated to have a helper that can call t.Error or t.Fatal and also can return an error value. Better to do only one or only the other.
|
|
||
| ### 9.2 Log messages must match the actual operation | ||
|
|
||
| Verify that `t.Log` messages accurately describe what the code does. Misleading log messages make debugging flaky tests significantly harder: |
There was a problem hiding this comment.
I would also suggest generally preferring log messages over code comments. Maybe a reasonable guidelines would be this: Use logs to describe what each step of the test does, and use code comments as necessary to explain why the test is dong something.
|
|
||
| ### 10.1 Never `defer` resource cleanup inside a loop | ||
|
|
||
| `defer` in a loop accumulates until function exit. Use explicit cleanup or `t.Cleanup`: |
There was a problem hiding this comment.
This is confusing. How is using t.Cleanup better than using defer in the case of a loop?
(Using t.Cleanup instead of defer for cleanups in general is good practice, of course.)
| for _, pod := range pods.Items { | ||
| logs, _ := getLogStream(pod) | ||
| buf := new(bytes.Buffer) | ||
| buf.ReadFrom(logs) | ||
| logs.Close() | ||
| // ... | ||
| } |
There was a problem hiding this comment.
This is fine for simple cases, but for more involved situations, you might need something like this:
for _, pod := range pods.Items {
func() {
logs, _ := getLogStream(pod)
defer logs.Close()
// The deferred cleanup will happen at the bottom of the loop.
// It is safe to `break` or to call `t.Fatal` from inside the loop.
// ...
}()
}
|
|
||
| ### 11.2 Call cancel explicitly instead of deferring inside poll callbacks | ||
|
|
||
| `defer` inside a poll callback accumulates deferred calls across iterations (the callback is a function called repeatedly, but defers don't fire until the parent function returns). Call cancel explicitly after the inner poll completes: |
There was a problem hiding this comment.
Are you sure about that? That doesn't make sense to me; the callback is a func like any other, so any deferred funcs in the callback func should be called when the callback func returns.
|
|
||
| ### 12.1 Always call `t.Helper()` in test helper functions | ||
|
|
||
| Every function that accepts `*testing.T` and is not a test itself must call `t.Helper()` at the top. This makes failure output point to the caller, not the helper: |
There was a problem hiding this comment.
This is sometimes confusing though. For a more involved helper, knowing the callsite of the helper might be useless; it is helpful to see the point of failure within the helper. (Arguably, some of our helpers are overly complicated.)
| - **Parallel tests** (~90): Use `t.Parallel()`, create their own `IngressController`, independent of each other | ||
| - **Serial tests** (~50): Modify cluster-wide resources (default IC, cluster ingress config, infrastructure config) |
There was a problem hiding this comment.
And each test must be registered as either a parallel test or a serial test in all_tests.go (we have hack/verify-e2e-test-all-presence.sh to verify this).
| ic := newPrivateController(name, domain) | ||
| if err := kclient.Create(context.TODO(), ic); err != nil { | ||
| t.Fatalf("failed to create ingresscontroller: %v", err) | ||
| } |
There was a problem hiding this comment.
Should we promote use of createWithRetryOnError in this example?
| // 1. Create IngressController | ||
| ic := newPrivateController(name, domain) | ||
| if err := kclient.Create(context.TODO(), ic); err != nil { | ||
| t.Fatalf("failed to create ingresscontroller: %v", err) | ||
| } | ||
| t.Cleanup(func() { assertIngressControllerDeleted(t, kclient, ic) }) | ||
|
|
||
| // 2. Wait for readiness | ||
| if err := waitForIngressControllerCondition(t, kclient, 5*time.Minute, name, conditions...); err != nil { | ||
| t.Fatalf("failed to observe expected conditions: %v", err) | ||
| } | ||
|
|
||
| // 3. Create test workload (pod + service + route) |
There was a problem hiding this comment.
The code comments are great examples of code comments that could be log messages.
|
/assign @Miciah |
6 participants
This PR adds a testing guideline to be used by implementer and (human and AI) reviewers on e2e tests on CIO repository. These guidelines should be used to bring consistency and stability on e2e tests, avoiding silent error drop, flaky cleanups, bad fatal testing calls.