Fix flaky E2E tests #4755
Fix flaky E2E tests #4755
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR refactors ACH checkout tests to improve reliability by addressing timing issues with Stripe iframe interactions. The changes introduce helper functions for handling asynchronous operations and update test flows to be more explicit and robust.
Key Changes:
- Added
waitForStripeReady()helper to handle iframe loading race conditions - Added
retryWithBackoff()helper for retry logic with exponential backoff - Refactored
fillACHBankDetails()to use a more explicit step-by-step approach - Increased timeout values in Playwright config to accommodate slower Stripe API interactions
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| tests/e2e/utils/payments.js | Added retry and iframe-ready helpers; refactored ACH setup and bank detail filling with explicit visibility checks and retry logic |
| tests/e2e/tests/checkout/shortcode/lpms/ach.spec.js | Inlined fillACHBankDetails in one test with modified step order; added blank lines for readability |
| tests/e2e/tests/checkout/blocks/lpms/ach.spec.js | Added blank lines for improved readability |
| tests/e2e/config/playwright.config.js | Increased test timeout to 120s, expect timeout to 30s, and added 15s action timeout |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
diegocurbelo
requested review from
a team,
daledupreez and
malithsen
and removed request for
a team
daledupreez
left a comment
daledupreez
left a comment
There was a problem hiding this comment.
Everything is looking good to me, with one minor area where I am not clear on what we're doing.
tests/e2e/utils/payments.js
Outdated
| for ( const indicator of loadingIndicators ) { | ||
| const loader = stripeFrame.locator( indicator ); | ||
| if ( await loader.isVisible().catch( () => false ) ) { | ||
| await expect( loader ).toBeHidden( { timeout: 10000 } ); |
There was a problem hiding this comment.
Two thoughts:
- It's not clear to me whether we need to check the selectors in sequence, or whether we can shift to something like
Promise.all()that allows us to perform these checks in parallel. We should either add a comment about why we're doing this sequentially, or we should look at handling this in parallel. - Not blocking: should this timeout be configurable in some way?
There was a problem hiding this comment.
Updated to check the selector in parallel in f04f9bc
tests/e2e/utils/payments.js
Outdated
| await emptyCart( page ); | ||
| await setupCart( page ); | ||
|
|
||
| let iframeSelector = 'iframe[src*="elements-inner-payment"]'; |
There was a problem hiding this comment.
Nit: rather than overwriting the iframeSelector variable, it may be clearer to use a separate variable name for this initial/inner selector, and then use a separate variable for the full iframe selector.
| let iframeSelector = 'iframe[src*="elements-inner-payment"]'; | |
| const rawIframeSelector = 'iframe[src*="elements-inner-payment"]'; |
malithsen
left a comment
malithsen
left a comment
There was a problem hiding this comment.
Thank you for making these improvements. This looks good to me.
daledupreez
left a comment
daledupreez
left a comment
There was a problem hiding this comment.
The changes look good to me! 
🙏
Fixes STRIPE-<linear_issue_id>
Fixes #<github_issue_id>
Changes proposed in this Pull Request:
This PR fixes the flakiness of the ACH E2E tests.
It updates the selectors for the ACH Stripe iframe modal and it adds two new helpers:
Wait for Stripe iframe to be fully loaded and ready for interaction (to address race conditions with Stripe Elements rendering).
Retry an async function with exponential backoff (for iframe interactions).
Testing instructions
Verify that all PR checks pass, particularly the two E2E suites:
Changelog entry
Changelog Entry Comment
This PR only fixes E2E tests, it has no customer facing changes.
Post merge