Skip to content

fix(e2e): remove hardcoded 5s timeout in verifyTextInLocator#4533

Open
jonkoops wants to merge 1 commit intoredhat-developer:mainfrom
jonkoops:fix/e2e-verify-text-timeout
Open

fix(e2e): remove hardcoded 5s timeout in verifyTextInLocator#4533
jonkoops wants to merge 1 commit intoredhat-developer:mainfrom
jonkoops:fix/e2e-verify-text-timeout

Conversation

@jonkoops
Copy link
Copy Markdown
Contributor

@jonkoops jonkoops commented Apr 2, 2026 •
edited
Loading

Removes the hardcoded timeout: 5000 from verifyTextInLocator in ui-helper.ts, restoring the original behavior where Playwright's configured actionTimeout (10-15s) applies. The 5s timeout was inadvertently introduced in #4020 and is shorter than the global config, causing flaky CI failures for verifyText and verifyRowsInTable callers (e.g. RBAC role creation toast, catalog table checks)

@jonkoops
fix(e2e): remove hardcoded 5s timeout in verifyTextInLocator
18b95dc 
The hardcoded `timeout: 5000` was inadvertently introduced in redhat-developer#4020 and
is shorter than Playwright's configured `actionTimeout` (10-15s),
causing flaky failures in CI for `verifyText` and `verifyRowsInTable`
callers (e.g. RBAC role creation, catalog table checks).

Removing it restores the original behavior where Playwright's global
`actionTimeout` applies.

Made-with: Cursor
@openshift-ci openshift-ci bot requested review from jrichter1 and kadel April 2, 2026 16:08
@rhdh-qodo-merge
Copy link
Copy Markdown

rhdh-qodo-merge bot commented Apr 2, 2026

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 1 🔵⚪⚪⚪⚪
🔒 No security concerns identified
⚡ Recommended focus areas for review

Wait Order

The code now waits for the element to be visible before waiting for it to be attached. In Playwright, an element typically must be attached before it can become visible; consider validating that the visible wait won’t fail or be redundant, or reorder waits to attach first (then visible) to better match the element lifecycle.

 
await elementLocator.waitFor({ state: "visible" });
await elementLocator.waitFor({ state: "attached" });
📄 References
  1. redhat-developer/rhdh/e2e-tests/playwright.config.ts [45-84]
  2. redhat-developer/rhdh/e2e-tests/playwright/e2e/plugins/rbac/rbac.spec.ts [22-967]
  3. redhat-developer/rhdh/e2e-tests/playwright/e2e/catalog-timestamp.spec.ts [1-103]
  4. redhat-developer/rhdh/e2e-tests/playwright/e2e/extensions.spec.ts [570-574]
  5. redhat-developer/rhdh/e2e-tests/playwright/e2e/plugins/orchestrator/orchestrator-rbac.spec.ts [12-1542]
  6. redhat-developer/rhdh/e2e-tests/playwright/e2e/github-happy-path.spec.ts [15-230]
  7. redhat-developer/rhdh/e2e-tests/playwright/e2e/plugins/scaffolder-backend-module-annotator/annotator.spec.ts [11-186]
  8. redhat-developer/rhdh/e2e-tests/playwright/e2e/plugins/adoption-insights/adoption-insights.spec.ts [10-286]

@rhdh-qodo-merge
Copy link
Copy Markdown

rhdh-qodo-merge bot commented Apr 2, 2026

PR Type

Bug fix

Description

  • Removes hardcoded 5s timeout from verifyTextInLocator method

  • Restores Playwright's configured actionTimeout (10-15s) behavior

  • Fixes flaky CI failures in RBAC and catalog table tests

File Walkthrough
Relevant files
Bug fix
ui-helper.ts
Remove hardcoded timeout from waitFor call                             

e2e-tests/playwright/utils/ui-helper.ts

  • Removed timeout: 5000 parameter from elementLocator.waitFor() call
  • Allows Playwright's global actionTimeout configuration to apply
  • Fixes flaky test failures caused by insufficient timeout duration
 +1/-1     

@sonarqubecloud
Copy link
Copy Markdown

sonarqubecloud bot commented Apr 2, 2026

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

@rhdh-qodo-merge
Copy link
Copy Markdown

rhdh-qodo-merge bot commented Apr 2, 2026

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
General
Simplify function by removing redundant waits

Simplify the verifyTextInLocator function by removing redundant waitFor and
scrollIntoViewIfNeeded calls, and relying solely on the
expect(elementLocator).toBeVisible() assertion which handles waiting
automatically.

e2e-tests/playwright/utils/ui-helper.ts [443-453]

-await elementLocator.waitFor({ state: "visible" });
-await elementLocator.waitFor({ state: "attached" });
-
-try {
-  await elementLocator.scrollIntoViewIfNeeded();
-} catch (error) {
-  console.warn(
-    `Warning: Could not scroll element into view. Error: ${error.message}`,
-  );
-}
 await expect(elementLocator).toBeVisible();
  • Apply / Chat
Suggestion importance[1-10]: 7

__

Why: The suggestion correctly identifies redundant waitFor calls and simplifies the code to use Playwright's auto-waiting assertion expect().toBeVisible(), which improves code quality and aligns with best practices.

Medium
  • More

@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Apr 2, 2026

Image was built and published successfully. It is available at:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jrichter1 jrichter1 Awaiting requested review from jrichter1

@kadel kadel Awaiting requested review from kadel

Assignees

No one assigned

Labels

Bug fix Review effort 1/5

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@jonkoops