NO-JIRA: retry operations in e2e tests - #1071
Conversation
WalkthroughAlert rule E2E tests now use shared polling and retry helpers for creation, deletion, and PrometheusRule verification. A Makefile target builds the E2E test binary, and ChangesAlert rule E2E reliability
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 14 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (14 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
test/e2e/helpers_test.go (1)
23-37: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
pollnever honors a caller-supplied context; it hardcodescontext.Background().
polldoesn't accept acontext.Context, so it can't be cancelled by the outer test context (e.g. a test-level deadline). All three call sites pass their ownctxinto the retried operation but the polling loop itself runs on an unrelated background context. As per path instructions, Go code should usecontext.Context for cancellation and timeouts.♻️ Suggested fix: thread ctx through
-func poll(interval, timeout time.Duration, f func() error) error { +func poll(ctx context.Context, interval, timeout time.Duration, f func() error) error { var lastErr error - err := wait.PollUntilContextTimeout(context.Background(), interval, timeout, true, func(context.Context) (bool, error) { + err := wait.PollUntilContextTimeout(ctx, interval, timeout, true, func(context.Context) (bool, error) { if lastErr = f(); lastErr != nil { return false, nil }(call sites would then pass their existing
ctx)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/e2e/helpers_test.go` around lines 23 - 37, Update poll to accept a context.Context parameter and pass it to wait.PollUntilContextTimeout instead of context.Background(). Update all three poll call sites to provide their existing ctx values, preserving the current retry and error-handling behavior.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/e2e/delete_alert_rule_test.go`:
- Line 95: Handle the error returned by resp.Body.Close() in the defer near the
affected test, rather than discarding it. Update the deferred cleanup to check
the close result and propagate or report any error using the test’s existing
error-handling mechanism.
In `@test/e2e/helpers_test.go`:
- Around line 39-50: The retries around non-idempotent alert-rule operations are
unsafe. In test/e2e/helpers_test.go:39-50, update createRuleViaAPIWithRetry to
verify existing state or otherwise avoid creating a duplicate when the server
may have already processed the request; in
test/e2e/delete_alert_rule_test.go:80-121, make the bulk-delete retry treat
already-deleted/not-found rules as success and avoid repeatedly failing on IDs
removed by an earlier attempt.
---
Nitpick comments:
In `@test/e2e/helpers_test.go`:
- Around line 23-37: Update poll to accept a context.Context parameter and pass
it to wait.PollUntilContextTimeout instead of context.Background(). Update all
three poll call sites to provide their existing ctx values, preserving the
current retry and error-handling behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: db90c7b5-bec6-45bc-81a7-c48e7cf18598
📒 Files selected for processing (3)
test/e2e/create_alert_rule_test.gotest/e2e/delete_alert_rule_test.gotest/e2e/helpers_test.go
| if err != nil { | ||
| return fmt.Errorf("failed to make delete request: %w", err) | ||
| } | ||
| defer resp.Body.Close() |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Unchecked resp.Body.Close() error return.
Flagged by errcheck; per path instructions, Go code should never ignore error returns.
🔧 Suggested fix
- defer resp.Body.Close()
+ defer func() {
+ if cerr := resp.Body.Close(); cerr != nil {
+ t.Logf("failed to close response body: %v", cerr)
+ }
+ }()🧰 Tools
🪛 golangci-lint (2.12.2)
[error] 95-95: Error return value of resp.Body.Close is not checked
(errcheck)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/e2e/delete_alert_rule_test.go` at line 95, Handle the error returned by
resp.Body.Close() in the defer near the affected test, rather than discarding
it. Update the deferred cleanup to check the close result and propagate or
report any error using the test’s existing error-handling mechanism.
Sources: Path instructions, Linters/SAST tools
openshift-ci
Bot
added
the
needs-rebase
330236d to
09b14a5
Compare
openshift-ci
Bot
removed
the
needs-rebase
|
/cc @PeterYurkovich |
|
/test e2e-monitoring |
|
/override-sticky ci/prow/e2e-monitoring |
|
@PeterYurkovich: Overrode contexts on behalf of PeterYurkovich: ci/prow/e2e-monitoring These overrides will persist across retests on the current HEAD SHA. Pushing a new commit will clear them. Use 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 kubernetes-sigs/prow repository. |
Requests to the backend may fail for a variety of reasons such as network connectivity, API blips, etc. Instead of failing a test, this commit implements retries for API requests. Signed-off-by: Simon Pasquier <[email protected]>
09b14a5 to
b1e49b7
Compare
|
/test test-frontend-unit |
|
/override-sticky ci/prow/test-frontend-unit |
|
@PeterYurkovich: Overrode contexts on behalf of PeterYurkovich: ci/prow/test-frontend-unit These overrides will persist across retests on the current HEAD SHA. Pushing a new commit will clear them. Use 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 kubernetes-sigs/prow repository. |
|
/override-sticky ci/prow/e2e-monitoring |
|
@PeterYurkovich: Overrode contexts on behalf of PeterYurkovich: ci/prow/e2e-monitoring These overrides will persist across retests on the current HEAD SHA. Pushing a new commit will clear them. Use 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 kubernetes-sigs/prow repository. |
|
@simonpasquier: 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. |
|
/retitle NO-JIRA: retry operations in e2e tests |
openshift-ci
Bot
changed the title
|
/lgtm |
openshift-ci-robot
added
the
jira/valid-reference
|
@simonpasquier: 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. |
openshift-ci
Bot
added
the
qe-approved
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: PeterYurkovich, simonpasquier 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 |
openshift-ci
Bot
added
the
approved
dda087c
into
openshift:main-alerts-management-api
5 participants
Requests to the backend may fail for a variety of reasons such as network connectivity, API blips, etc. Instead of failing a test, this commit implements retries for API requests.
Summary by CodeRabbit
build-e2etarget to compile the end-to-end test suite into a standalone binary.e2e.testbinary in version control.