NO-JIRA: Remove redundant waitFor calls in Playwright e2e tests#16583
Open
rhamilto wants to merge 2 commits into
Open
NO-JIRA: Remove redundant waitFor calls in Playwright e2e tests#16583rhamilto wants to merge 2 commits into
rhamilto wants to merge 2 commits into
Conversation
openshift-ci-robot
added
the
jira/valid-reference
Contributor
|
@rhamilto: 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. |
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis PR updates Cypress→Playwright migration docs and the migrate-cypress skill, replaces many explicit Playwright locator.waitFor({ state }) calls with expect-based assertions or removes redundant waits in page objects/tests, adds an ESLint rule flagging waitFor(), and adds clickAdvancedTimeout(). ChangesPlaywright Auto-Waiting Pattern Migration
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 14 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (14 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
openshift-ci
Bot
added
the
approved
rhamilto
force-pushed
the
playwright-waitfor-cleanup
branch
from
f90e9c2 to
be0795b
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 @.claude/migration-context.md:
- Around line 498-512: Update the guidance paragraph to state that only
KubernetesClient.deleteNamespace and KubernetesClient.deleteCustomResource
swallow 404s (they catch errors and call isNotFound(err) to silence “not found”
errors) and therefore callers should not wrap those specific cleanup calls in
try/catch; remove the claim that deleteClusterCustomResource also swallows 404s
and instead note that KubernetesClient.deleteClusterCustomResource is not
implemented in the client so it should not be included in the “swallow 404s”
guidance.
🪄 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: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 66663232-ec3f-459e-98bc-542625f22f0a
📒 Files selected for processing (7)
.claude/migration-context.md.claude/skills/migrate-cypress/SKILL.mdfrontend/e2e/pages/cluster-settings-page.tsfrontend/e2e/pages/navigation.tsfrontend/e2e/pages/oauth-page.tsfrontend/e2e/pages/web-terminal-page.tsfrontend/e2e/tests/console/favorites/favorites.spec.ts
💤 Files with no reviewable changes (5)
- frontend/e2e/pages/web-terminal-page.ts
- frontend/e2e/pages/oauth-page.ts
- frontend/e2e/tests/console/favorites/favorites.spec.ts
- frontend/e2e/pages/navigation.ts
- frontend/e2e/pages/cluster-settings-page.ts
rhamilto
force-pushed
the
playwright-waitfor-cleanup
branch
from
be0795b to
0af4aa6
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
frontend/e2e/pages/details-page.ts (1)
27-31: ⚡ Quick winConsider removing redundant visibility wait for consistency.
Similar to
clickResourceRow, line 29 has an explicitwaitFor({ state: 'visible' })beforerobustClick, which is redundant sincerobustClickalready waits for visibility. For consistency with the PR's objective and the cleanup in other methods, consider removing this redundant wait.♻️ Suggested refactor
async clickKebabAction(actionId: string): Promise<void> { const action = this.page.locator(`[data-test-action="${actionId}"]`); - await action.waitFor({ state: 'visible', timeout: 10_000 }); - await this.robustClick(action); + await this.robustClick(action, { timeout: 10_000 }); }🤖 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/pages/details-page.ts` around lines 27 - 31, The clickKebabAction method redundantly calls await action.waitFor({ state: 'visible', timeout: 10_000 }) before calling this.robustClick(action); remove that explicit wait so clickKebabAction mirrors clickResourceRow and relies on robustClick to handle visibility/waiting. Update the method implementation (clickKebabAction) to call only this.robustClick(action) and keep any existing timeout/visibility behavior centralized inside robustClick.
🤖 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 `@frontend/e2e/pages/details-page.ts`:
- Around line 52-56: In clickResourceRow, remove the redundant explicit waitFor
call (the await row.waitFor... line) because BasePage.robustClick already waits
for visibility; instead call robustClick with the desired timeout if you need
the 30_000ms window (i.e., use this.robustClick(row, { timeout: 30_000 }) or the
existing robustClick API), updating clickResourceRow to obtain the locator via
getResourceRow and delegate waiting/clicking to robustClick; reference
functions: clickResourceRow, getResourceRow, robustClick, and
BasePage.robustClick.
---
Nitpick comments:
In `@frontend/e2e/pages/details-page.ts`:
- Around line 27-31: The clickKebabAction method redundantly calls await
action.waitFor({ state: 'visible', timeout: 10_000 }) before calling
this.robustClick(action); remove that explicit wait so clickKebabAction mirrors
clickResourceRow and relies on robustClick to handle visibility/waiting. Update
the method implementation (clickKebabAction) to call only
this.robustClick(action) and keep any existing timeout/visibility behavior
centralized inside robustClick.
🪄 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: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 1904c79d-caf2-425e-92a0-946c90ffdc08
📒 Files selected for processing (8)
.claude/migration-context.md.claude/skills/migrate-cypress/SKILL.mdfrontend/e2e/pages/cluster-settings-page.tsfrontend/e2e/pages/details-page.tsfrontend/e2e/pages/navigation.tsfrontend/e2e/pages/oauth-page.tsfrontend/e2e/pages/web-terminal-page.tsfrontend/e2e/tests/console/favorites/favorites.spec.ts
💤 Files with no reviewable changes (5)
- frontend/e2e/pages/navigation.ts
- frontend/e2e/pages/oauth-page.ts
- frontend/e2e/pages/cluster-settings-page.ts
- frontend/e2e/pages/web-terminal-page.ts
- frontend/e2e/tests/console/favorites/favorites.spec.ts
✅ Files skipped from review due to trivial changes (2)
- .claude/skills/migrate-cypress/SKILL.md
- .claude/migration-context.md
rhamilto
force-pushed
the
playwright-waitfor-cleanup
branch
from
0af4aa6 to
cac8b24
Compare
1 task
Member
Author
|
/retest |
1 similar comment
Member
Author
|
/retest |
rhamilto
force-pushed
the
playwright-waitfor-cleanup
branch
from
cac8b24 to
51bcada
Compare
openshift-ci
Bot
added
the
needs-rebase
fsgreco
reviewed
Jun 10, 2026
Comment on lines
+19
to
+20
| 'If this waitFor() is intentional (waiting for state without a subsequent action), ' + | ||
| 'add // eslint-disable-next-line no-restricted-syntax', |
Contributor
There was a problem hiding this comment.
We could add another alternative to the the eslint disable rule, which is to use web assertions (it's suggested also by Playwright documentation: either use web assertions or waitFor)
Suggested change
| 'If this waitFor() is intentional (waiting for state without a subsequent action), ' + | |
| 'add // eslint-disable-next-line no-restricted-syntax', | |
| 'If this waitFor() is intentional (waiting for state without a subsequent action), ' + | |
| 'either add // eslint-disable-next-line no-restricted-syntax', | |
| 'or use web assertions, e.g. `await expect(yourElement).toBeVisible()`' |
Member
Author
There was a problem hiding this comment.
fsgreco
reviewed
Jun 10, 2026
| await page.getByTestId('loading-indicator').waitFor({ state: 'detached' }); | ||
|
|
||
| // OK — confirming the editor loaded before reading its content (not an action on the element) | ||
| await page.getByTestId('code-editor').waitFor({ state: 'visible' }); |
Contributor
There was a problem hiding this comment.
We could also add another stronger alternative, which is to add it in conjunction with or(). Let's say we have a modal with either error or success (so there are two possible locators):
Suggested change
| await page.getByTestId('code-editor').waitFor({ state: 'visible' }); | |
| await page.getByTestId('code-editor').waitFor({ state: 'visible' }); | |
| const successMessage = page.getByTestId('.success-message'); | |
| const errorMessage = page.getByTestId('.error-message'); | |
| // Use the .or() combinator and wait for either one to appear | |
| await successMessage.or(errorMessage).waitFor({ state: 'visible' }); |
rhamilto
force-pushed
the
playwright-waitfor-cleanup
branch
from
51bcada to
41a9c22
Compare
openshift-ci
Bot
removed
the
needs-rebase
Member
Author
|
/retest |
fsgreco
suggested changes
Jun 10, 2026
rhamilto
added
the
jira/valid-bug
Member
Author
|
/retest |
rhamilto
force-pushed
the
playwright-waitfor-cleanup
branch
from
305a3a5 to
74e434b
Compare
openshift-ci
Bot
added
component/helm
rhamilto
force-pushed
the
playwright-waitfor-cleanup
branch
from
ed2fe29 to
acc58e7
Compare
openshift-ci
Bot
removed
the
needs-rebase
Member
Author
|
/test frontend |
rhamilto
force-pushed
the
playwright-waitfor-cleanup
branch
3 times, most recently
from
645641d to
4122e7c
Compare
openshift-ci
Bot
added
the
needs-rebase
Add a no-restricted-syntax ESLint rule that warns on all .waitFor() calls in Playwright e2e tests. Playwright actions (click, fill, check, clear) auto-wait for actionability, so waitFor() before an action is redundant. For intentional uses (waiting for state without a subsequent action, or inside error recovery), add an eslint-disable comment. Replace 22 waitFor() calls with idiomatic expect() web-first assertions (e.g. toBeVisible, not.toBeAttached) and remove 3 that were redundant before an action. The remaining 10 are all in try/catch or .catch() blocks where expect() cannot be used. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Remove the `perspective` useState that duplicated `activePerspective` — use `activePerspective` directly to eliminate the stale-state window. Guard the tour dispatch with `if (loaded)` to avoid dispatching initialize before user preferences have loaded. Gate tour rendering with an `initializedWithLoadedData` state flag that prevents the guided tour from flashing stale state during perspective switches. The flag resets when the perspective changes and is only set after the effect re-initializes the reducer with the correct completion state for the new perspective. Using state (not a ref) ensures the component re-renders after initialization, so the tour appears for uncompleted perspectives. Add a unit test verifying that completed tours do not re-appear when the completed state changes. Co-authored-by: Santiago Greco <sgreco@redhat.com> Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
rhamilto
force-pushed
the
playwright-waitfor-cleanup
branch
from
4122e7c to
df62a09
Compare
openshift-ci
Bot
removed
the
needs-rebase
Member
Author
|
/retest |
Contributor
|
@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. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
7 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Analysis / Root cause:
Playwright action methods (
fill(),click(),check(),clear()) androbustClick()auto-wait for element actionability (visible, enabled, stable). Several e2e page objects and specs had explicitwaitFor({ state: 'visible' })calls immediately before actions on the same element, which is redundant.Additionally, there was no lint enforcement to prevent new redundant
waitFor()calls from being introduced, and many legitimate state-waiting uses were written aswaitFor()instead of idiomatic Playwrightexpect()web-first assertions.The integration test user agent override (used to dismiss the getting started modal) was only set via
addInitScript+Object.defineProperty, which is fragile — it only overrides the JS-side value and can fail on some Chromium builds wherenavigator.userAgentis non-configurable.Solution description:
ESLint enforcement
no-restricted-syntaxrule toe2e/.eslintrc.cjsthat warns on all.waitFor()callseslint-disable-next-lineis appropriatewaitForat lint time — both for humans and AI migration toolsRedundant waitFor removal
DetailsPage.waitForPageLoad()method and its unused fields (skeletonLoader,resourceTitle)waitForLoadparameter fromclickResourceRow()(never called withfalse, androbustClickalready auto-waits)waitForbeforerobustClickinclickKebabAction()andclickResourceRow()waitFor()calls in page objects and specs where the next action on the same locator auto-waitsReplace waitFor with expect() assertions
Replaced 22
waitFor()calls with idiomatic Playwright web-first assertions:waitFor({ state: 'visible' })→expect(x).toBeVisible()waitFor({ state: 'detached' })→expect(x).not.toBeAttached()waitFor({ state: 'hidden' })→expect(x).toBeHidden()The 10 remaining
waitFor()calls are intry/catchretry loops or.catch()blocks whereexpect()cannot be used (it throws on failure rather than allowing graceful recovery).Harden integration test user agent
userAgentat the Playwright project config level for all test projects (not auth setup projects), ensuringnavigator.userAgentreturnsConsoleIntegrationTestEnvironmentnatively at the browser leveladdInitScriptfallback to also overrideNavigator.prototype.userAgentwithconfigurable: trueMigration docs
waitForguidance from migrate-cypress skill (the lint rule covers it)migration-context.mdto reference the ESLint ruledeleteClusterCustomResourcedoes not exist inKubernetesClientScreenshots / screen recording:
N/A - no visual changes
Test setup:
None required
Test cases:
All affected e2e tests pass against localhost:9000:
pseudolocalization.spec.ts(1 test)cluster-settings.spec.ts(3 tests)update-modal.spec.ts(1 test)console-link.spec.ts(2 tests)create-namespace.spec.ts(1 test)Browser conformance:
Additional info:
waitFor()remains appropriate when:try/catchor.catch()block (e.g., retry loops, optional elements)For all other state-waiting cases, prefer
expect()web-first assertions — they retry automatically and produce better error messages on failure.