CONSOLE-5196: Fix flaky Playwright e2e tests caused by OAuth redirect timing#16740
CONSOLE-5196: Fix flaky Playwright e2e tests caused by OAuth redirect timing#16740rhamilto wants to merge 1 commit into
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci-robot
added
the
jira/valid-reference
|
@rhamilto: This pull request references CONSOLE-5196 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 epic to target the "5.0.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. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: openshift/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (7)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (4)
WalkthroughPlaywright E2E setup now uses shared SPA warmup behavior with explicit navigation and visibility timeouts. Quota tests verify cluster resource creation through the Kubernetes client and extend list visibility waits. ChangesE2E warmup and quota readiness
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 12 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (12 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: rhamilto 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
|
/retest backend |
|
/test backend |
1 similar comment
|
/test backend |
|
/pipeline required |
|
Scheduling tests matching the |
|
/pipeline required |
|
Scheduling tests matching the |
|
/test e2e-playwright |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
frontend/e2e/tests/console/crud/quotas.spec.ts (1)
93-102: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract duplicated retry-and-reload logic into a shared helper.
The same 3-attempt retry/reload/re-filter pattern is duplicated verbatim in both tests. Consider extracting it into a reusable helper (e.g., in
list-page.ts) to avoid drift if the retry strategy needs tuning later. Also consider logging or narrowing the caught error so unrelated failures (e.g., wrong selector) aren't silently retried and masked.♻️ Proposed helper extraction
+// in list-page.ts (or a shared e2e util) +async function waitForCellVisibleWithRetry( + page: Page, + listPage: ListPage, + resourceName: string, + attempts = 3, + timeout = 10_000, +): Promise<void> { + for (let attempt = 0; attempt < attempts; attempt++) { + try { + await expect(listPage.getCell(resourceName)).toBeVisible({ timeout }); + return; + } catch (err) { + if (attempt === attempts - 1) throw err; + await page.reload({ waitUntil: 'domcontentloaded' }); + await listPage.filterByName(resourceName); + } + } +}- for (let attempt = 0; attempt < 3; attempt++) { - try { - await expect(listPage.getCell(clusterQuotaName)).toBeVisible({ timeout: 10_000 }); - break; - } catch { - await page.reload({ waitUntil: 'domcontentloaded' }); - await listPage.filterByName(clusterQuotaName); - } - } - await expect(listPage.getCell(clusterQuotaName)).toBeVisible({ timeout: 10_000 }); + await waitForCellVisibleWithRetry(page, listPage, clusterQuotaName);Also applies to: 125-134
🤖 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 `@frontend/e2e/tests/console/crud/quotas.spec.ts` around lines 93 - 102, The 3-attempt retry, reload, and re-filter flow is duplicated in the quota CRUD tests and should be extracted into a shared helper to keep the behavior consistent. Move the repeated logic into a reusable utility such as a method on listPage in list-page.ts, then update both tests to call that helper instead of inlining the loop. While refactoring, avoid catching everything silently: narrow or log the caught error so only the intended transient failures are retried and unrelated selector/test issues are not masked.
🤖 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.
Nitpick comments:
In `@frontend/e2e/tests/console/crud/quotas.spec.ts`:
- Around line 93-102: The 3-attempt retry, reload, and re-filter flow is
duplicated in the quota CRUD tests and should be extracted into a shared helper
to keep the behavior consistent. Move the repeated logic into a reusable utility
such as a method on listPage in list-page.ts, then update both tests to call
that helper instead of inlining the loop. While refactoring, avoid catching
everything silently: narrow or log the caught error so only the intended
transient failures are retried and unrelated selector/test issues are not
masked.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 4175aa4d-a6a2-4dce-a4da-36b32d40ebcf
📒 Files selected for processing (6)
frontend/e2e/pages/base-page.tsfrontend/e2e/pages/web-terminal-page.tsfrontend/e2e/tests/console/app/masthead.spec.tsfrontend/e2e/tests/console/crud/quotas.spec.tsfrontend/e2e/tests/console/favorites/favorites.spec.tsfrontend/e2e/tests/webterminal/web-terminal-admin.spec.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- frontend/e2e/tests/console/favorites/favorites.spec.ts
- frontend/e2e/pages/base-page.ts
- frontend/e2e/tests/webterminal/web-terminal-admin.spec.ts
- frontend/e2e/pages/web-terminal-page.ts
- frontend/e2e/tests/console/app/masthead.spec.ts
… timing
Several e2e tests fail intermittently because they interact with the page
before the OAuth redirect chain completes. The SPA navigates through
/oauth/authorize and /auth/callback before rendering, and tests using bare
page.goto('/') without waiting for the app to load hit elements that don't
exist yet.
- warmupSPA: increase goto timeout to 60s and toBeVisible to 30s to
accommodate OAuth redirects in CI
- quotas: increase ClusterResourceQuota visibility timeout to 30s for
cluster-scoped resource propagation
- masthead: use warmupSPA in beforeEach instead of bare page.goto('/')
- favorites: use warmupSPA instead of bare page.goto('/')
- web-terminal: use warmupSPA in waitForTerminalIconVisible so the
console fully loads before looking for the terminal icon
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
/test backend |
2 similar comments
|
/test backend |
|
/test backend |
|
/test e2e-playwright |
|
@rhamilto: The following test failed, say
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. |
2 participants
Analysis / Root cause:
Several Playwright e2e tests fail intermittently in CI because they interact with the page before the OAuth redirect chain (
/ → /oauth/authorize → /auth/callback → /) completes. Tests using barepage.goto('/')without waiting for the SPA to fully render attempt to find elements that don't exist yet, causing timeout failures.Five flaky tests were identified:
quotas.spec.ts— ClusterResourceQuota visibility assertions use default 5s timeout, too tight for cluster-scoped resource propagationmasthead.spec.ts— barepage.goto('/')without waiting for OAuth flowfavorites.spec.ts— barepage.goto('/')without waiting for OAuth flowweb-terminal-admin.spec.ts—BasePage.goTo('/')doesn't verify the SPA rendered; console never fully loadsbase-page.ts(warmupSPA) — default 5stoBeVisibletimeout too tight for OAuth redirects in CIAdditionally, an accessibility bug was identified in the
other-routes.spec.tstest — a PatternFlyDataViewFiltersMenuTogglehas no accessible name. This is an upstream bug filed as patternfly/react-data-view#680.Solution description:
warmupSPA(base-page.ts): Increasepage.goto('/')timeout to 60s withwaitUntil: 'domcontentloaded'andtoBeVisibletimeout to 30s to accommodate OAuth redirects in CIquotas.spec.ts: Increase ClusterResourceQuotatoBeVisibletimeout to 30s (lines 93, 116) for cluster-scoped resource propagationmasthead.spec.ts: AddwarmupSPAinbeforeEachand remove redundant barepage.goto('/')from each testfavorites.spec.ts: Replace barepage.goto('/')withwarmupSPAweb-terminal-admin.spec.ts/web-terminal-page.ts: Replacethis.goTo('/')withwarmupSPAinwaitForTerminalIconVisibleso the console fully loads before looking for the terminal iconScreenshots / screen recording:
Test setup:
Run the affected Playwright e2e tests in CI to verify they no longer flake.
Test cases:
quotas.spec.ts— "All Projects shows ClusterResourceQuotas" and "project namespace shows AppliedClusterResourceQuotas" should pass reliablymasthead.spec.ts— "quick create should open Deploy Image" and all other masthead tests should pass reliablyfavorites.spec.ts— "adds, displays, removes, and limits favorites" should pass reliablyweb-terminal-admin.spec.ts— "open and close multiple terminal tabs" should pass reliablyBrowser conformance:
Additional info:
Upstream PatternFly issue for the
DataViewFiltersa11y bug: patternfly/react-data-view#680Summary by CodeRabbit