Conversation
…correctly Because: Stage sits behind Fastly Next-Gen WAF, which serves a JavaScript "Client Challenge" interstitial to plain fetch() calls. The pair spec gate read the content server with fetch(), failed to match the fxa-config meta tag, and returned false. Backbone pair specs then ran against stage (which serves React) and failed, while the React specs skipped. This commit: - Rewrites isPairRoutesReact to take (browser, target) and reuse the existing ConfigPage helper, which opens a real Playwright page so the WAF challenge JS executes before the meta tag is read. - Applies the WAF bypass header (CI_WAF_TOKEN) to the context it creates so the config read works in CI the same way fixture-created contexts do. - Moves the gate from beforeEach to beforeAll in all four pair specs since the rollout is env-stable for the lifetime of a worker, avoiding a WAF challenge on every test. - Captures the result in a worker-scoped variable in pairingFlowiOS so both inline call sites reuse the single lookup.
There was a problem hiding this comment.
Pull request overview
Updates pairing E2E spec gating so React vs Backbone pair-route suites are skipped based on config loaded via a real Playwright page (to allow Fastly WAF challenge JS to run before reading the fxa-config meta tag).
Changes:
- Rewrites
isPairRoutesReactto take(browser, target)and read config viaConfigPagein a real Playwright context. - Switches pairing specs’ gating hooks from
beforeEachtobeforeAll(runs once per worker). - Aligns suite-level skipping logic for React vs Backbone pairing specs based on
showReactApp.pairRoutes.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/functional-tests/lib/pairing-helpers.ts | Reworks pair-route detection to use Playwright + ConfigPage and CI WAF headers. |
| packages/functional-tests/tests/pairing/pairingFlow.spec.ts | React pairing suite gating moved to beforeAll using new helper signature. |
| packages/functional-tests/tests/pairing/pairingFlowNegative.spec.ts | React negative-path suite gating moved to beforeAll. |
| packages/functional-tests/tests/pairing/pairingFlowBackbone.spec.ts | Backbone pairing suite gating moved to beforeAll. |
| packages/functional-tests/tests/pairing/pairingFlowBackboneNegative.spec.ts | Backbone negative-path suite gating moved to beforeAll. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const config = await configPage.getConfig(); | ||
| return config?.showReactApp?.pairRoutes === true; |
There was a problem hiding this comment.
ConfigPage.getConfig() opens its own new tab via this.page.context().newPage() and closes it, so the page created here is only used as a context handle and stays open until context.close(). Consider avoiding the extra unused page (e.g., update ConfigPage.getConfig to use the provided page, or close the placeholder page immediately after getConfig()), to reduce resource usage and noise in traces.
| const config = await configPage.getConfig(); | |
| return config?.showReactApp?.pairRoutes === true; | |
| try { | |
| const config = await configPage.getConfig(); | |
| return config?.showReactApp?.pairRoutes === true; | |
| } finally { | |
| if (!page.isClosed()) { | |
| await page.close(); | |
| } | |
| } |
There was a problem hiding this comment.
This is a good suggestion, but what's proposed in this PR follows the existing usage. Not a priority to fix here since it would expand scope.
| return false; | ||
| const configPage = new ConfigPage(page, target); | ||
| const config = await configPage.getConfig(); | ||
| return config?.showReactApp?.pairRoutes === true; |
There was a problem hiding this comment.
This helper used to swallow failures and return false, but now any navigation/timeout/parsing error from configPage.getConfig() will fail the entire suite in beforeAll. If that’s not intentional, add explicit error handling here (either preserve the prior false fallback, or throw a clearer error that points to missing/invalid CI_WAF_TOKEN/WAF interstitial issues) so failures are actionable and less flaky.
| return config?.showReactApp?.pairRoutes === true; | |
| return config?.showReactApp?.pairRoutes === true; | |
| } catch (err) { | |
| const message = err instanceof Error ? err.message : String(err); | |
| console.warn( | |
| `Unable to determine whether pair routes are React-enabled; ` + | |
| `defaulting to false. This can happen when CI_WAF_TOKEN is missing or invalid, ` + | |
| `or when a WAF interstitial prevented config page loading/parsing. ` + | |
| `Original error: ${message}` | |
| ); | |
| return false; |
There was a problem hiding this comment.
It may be nice to improve the error messaging here (not blocking), but I don't think we want to fallback to false/backbone config since we're moving to React 100%
Because
This pull request
isPairRoutesReactinpairing-helpers.tsto take(browser, target)and reuse the existingConfigPagehelper, which loads the page in a real Playwright context so the WAF challenge JS executes before the meta tag is read.Issue that this pull request solves
Closes: https://mozilla-hub.atlassian.net/browse/FXA-13493
Checklist
Other information
Verified against stage (
CI=1 CI_WAF_TOKEN=... yarn test-stage):