Tests: Fix failure to dismiss guided tour when running locally#2118
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughA new auto-enabled Playwright fixture ChangesGuided Tour Dismissal Fixture
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
[APPROVALNOTIFIER] This PR is APPROVED Approval requirements bypassed by manually added approval. This pull-request has been approved by: 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 |
1 similar comment
|
[APPROVALNOTIFIER] This PR is APPROVED Approval requirements bypassed by manually added approval. This pull-request has been approved by: 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 |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/support/fixtures.ts (1)
254-256: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winConsider handling specific error cases rather than swallowing all exceptions.
The empty catch block silently swallows all errors, including unexpected failures like click errors, incorrect selectors, or cases where the tour button is present but doesn't hide after clicking. This could mask legitimate test failures.
Consider using a more targeted approach:
♻️ Alternative implementation with explicit presence check
export const dismissGuidedTour = async (page: Page): Promise<void> => { const skipTourBtn = page.locator('[data-test="tour-step-footer-secondary"]'); - try { - await expect(skipTourBtn).toBeVisible({ timeout: 3_000 }); + const isVisible = await skipTourBtn.isVisible({ timeout: 3_000 }).catch(() => false); + if (isVisible) { await skipTourBtn.click(); await expect(skipTourBtn).toBeHidden(); - } catch { - // Tour not present } };This way, only the "tour not present" case is handled silently, while genuine failures (e.g., button doesn't hide after click) will still throw and fail the test.
🤖 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 `@tests/support/fixtures.ts` around lines 254 - 256, The empty catch block is silently swallowing all exceptions, masking legitimate test failures like click errors or selector issues. Instead of using a bare try-catch, first explicitly check if the tour element or button exists before attempting any operations on it (use a targeted selector check or element presence verification). Only handle the specific case where the element is not found silently, and allow any genuine failures to propagate and fail the test.
🤖 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 `@tests/support/fixtures.ts`:
- Around line 254-256: The empty catch block is silently swallowing all
exceptions, masking legitimate test failures like click errors or selector
issues. Instead of using a bare try-catch, first explicitly check if the tour
element or button exists before attempting any operations on it (use a targeted
selector check or element presence verification). Only handle the specific case
where the element is not found silently, and allow any genuine failures to
propagate and fail the test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: cc62dca1-9003-4670-b8a0-b2717dc98979
📒 Files selected for processing (2)
tests/support/fixtures.tstests/tests/lightspeed.spec.ts
bcb3bdd to
757fb85
Compare
|
/cherry-pick main |
|
@kyoto: once the present PR merges, I will cherry-pick it on top of 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. |
|
/lgtm |
21942b7
into
openshift:release-4.19
|
@kyoto: new pull request created: #2123 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. |
Fix is just for running locally with the
SKIP_OLS_SETUP=trueoptionSummary by CodeRabbit