Merge pull request #7358 from Shopify/psyw-0421-E2E-teardown-polish

E2E: automatic teardown polish

Author: phyllis-sy-wu

packages/e2e/helpers/browser-login.ts

@@ -1,3 +1,4 @@
+import {isVisibleWithin} from '../setup/browser.js'
 import {BROWSER_TIMEOUT} from '../setup/constants.js'
 import type {Page} from '@playwright/test'
 
@@ -47,12 +48,12 @@ export async function completeLogin(page: Page, loginUrl: string, email: string,
     ]).catch(() => {})
 
     // If passkey page shown, navigate to password login
-    if (await differentMethodBtn.isVisible({timeout: BROWSER_TIMEOUT.short}).catch(() => false)) {
+    if (await isVisibleWithin(differentMethodBtn, BROWSER_TIMEOUT.short)) {
       await differentMethodBtn.click()
       await page.waitForTimeout(BROWSER_TIMEOUT.short)
 
       const continueWithPassword = page.locator('text=Continue with password')
-      if (await continueWithPassword.isVisible({timeout: BROWSER_TIMEOUT.medium}).catch(() => false)) {
+      if (await isVisibleWithin(continueWithPassword, BROWSER_TIMEOUT.medium)) {
         await continueWithPassword.click()
         await page.waitForTimeout(BROWSER_TIMEOUT.short)
       }
@@ -67,7 +68,7 @@ export async function completeLogin(page: Page, loginUrl: string, email: string,
     await page.waitForTimeout(BROWSER_TIMEOUT.medium)
     try {
       const btn = page.locator('button[type="submit"]').first()
-      if (await btn.isVisible({timeout: BROWSER_TIMEOUT.long})) await btn.click()
+      if (await isVisibleWithin(btn, BROWSER_TIMEOUT.long)) await btn.click()
       // eslint-disable-next-line no-catch-all/no-catch-all
     } catch (_error) {
       // No confirmation page — expected

packages/e2e/setup/app.ts

@@ -1,6 +1,6 @@
 import {cliFixture} from './cli.js'
 import {BROWSER_TIMEOUT} from './constants.js'
-import {chromium, type Page} from '@playwright/test'
+import {chromium, type Locator, type Page} from '@playwright/test'
 import * as fs from 'fs'
 
 // ---------------------------------------------------------------------------
@@ -11,6 +11,33 @@ export interface BrowserContext {
   browserPage: Page
 }
 
+// ---------------------------------------------------------------------------
+// Main-frame response status tracking
+// ---------------------------------------------------------------------------
+
+/**
+ * Records the HTTP status of the most recent main-frame document response per
+ * page. Populated by a `response` listener attached when the page is created
+ * (see fixture below). Read via `getLastPageStatus(page)`.
+ *
+ * A WeakMap lets the entry be garbage-collected with the page — no manual
+ * cleanup required.
+ */
+const lastMainFrameStatus = new WeakMap<Page, number>()
+
+export function trackMainFrameStatus(page: Page): void {
+  page.on('response', (response) => {
+    if (response.frame() !== page.mainFrame()) return
+    if (response.request().resourceType() !== 'document') return
+    lastMainFrameStatus.set(page, response.status())
+  })
+}
+
+/** Get the HTTP status of the last main-frame document response on `page`. */
+export function getLastPageStatus(page: Page): number | undefined {
+  return lastMainFrameStatus.get(page)
+}
+
 // ---------------------------------------------------------------------------
 // Fixture
 // ---------------------------------------------------------------------------
@@ -40,6 +67,7 @@ export const browserFixture = cliFixture.extend<{}, {browserPage: Page}>({
       context.setDefaultTimeout(BROWSER_TIMEOUT.max)
       context.setDefaultNavigationTimeout(BROWSER_TIMEOUT.max)
       const page = await context.newPage()
+      trackMainFrameStatus(page)
       await use(page)
       await browser.close()
     },
@@ -52,13 +80,32 @@ export const browserFixture = cliFixture.extend<{}, {browserPage: Page}>({
 // ---------------------------------------------------------------------------
 
 /**
- * Check if the current page shows a server error (500, 502). If so, refresh and return true.
- * Call this in retry loops when a selector isn't found — the page might be an error page.
+ * Wait up to `timeoutMs` for `locator` to become visible. Returns `true` if it
+ * appears in time, `false` on timeout or any other locator error (detached
+ * element, closed context, etc.).
+ *
+ * Use this instead of `locator.isVisible({timeout})` — that API does not
+ * actually wait in modern Playwright; it returns the current visibility state.
+ * This helper uses `waitFor({state: 'visible', timeout})` for true polling.
+ */
+export async function isVisibleWithin(locator: Locator, timeoutMs: number): Promise<boolean> {
+  return locator
+    .waitFor({state: 'visible', timeout: timeoutMs})
+    .then(() => true)
+    .catch(() => false)
+}
+
+/**
+ * If the most recent main-frame response was a 5xx server error, reload the
+ * page and return true. Otherwise return false. Call this in retry loops when
+ * a selector isn't found — the page might be an error page.
+ *
+ * Uses the HTTP status captured by `trackMainFrameStatus` rather than scraping
+ * body text, so it works regardless of how an error page is rendered.
  */
 export async function refreshIfPageError(page: Page): Promise<boolean> {
-  const pageText = (await page.textContent('body')) ?? ''
-  if (!pageText.includes('Internal Server Error') && !pageText.includes('502 Bad Gateway')) return false
-  // if (process.env.DEBUG === '1') process.stdout.write('                   page refreshing...\n')
+  const status = getLastPageStatus(page)
+  if (status === undefined || status < 500) return false
   await page.reload({waitUntil: 'domcontentloaded'})
   await page.waitForTimeout(BROWSER_TIMEOUT.medium)
   return true
@@ -83,7 +130,7 @@ export async function navigateToDashboard(
   // Handle account picker (skip if email not provided)
   if (ctx.email) {
     const accountButton = browserPage.locator(`text=${ctx.email}`).first()
-    if (await accountButton.isVisible({timeout: BROWSER_TIMEOUT.medium}).catch(() => false)) {
+    if (await isVisibleWithin(accountButton, BROWSER_TIMEOUT.medium)) {
       await accountButton.click()
       await browserPage.waitForTimeout(BROWSER_TIMEOUT.medium)
     }

packages/e2e/setup/browser.ts

@@ -11,9 +11,19 @@ export interface ExecResult {
   exitCode: number
 }
 
+export interface WaitForOutputOptions {
+  timeoutMs?: number
+  /** Cancel the wait early — frees the timer and removes the waiter entry. */
+  signal?: AbortSignal
+}
+
 export interface SpawnedProcess {
-  /** Wait for a string to appear in the PTY output */
-  waitForOutput(text: string, timeoutMs?: number): Promise<void>
+  /**
+   * Wait for a string to appear in the PTY output.
+   * Pass a number for the legacy positional `timeoutMs` form, or an options
+   * object to also supply an `AbortSignal` for cancellation.
+   */
+  waitForOutput(text: string, opts?: number | WaitForOutputOptions): Promise<void>
   /** Send a single key to the PTY */
   sendKey(key: string): void
   /** Send a line of text followed by Enter */
@@ -132,13 +142,13 @@ export const cliFixture = envFixture.extend<{cli: CLIProcess}>({
             process.stdout.write(data)
           }
 
-          // Check if any waiters are satisfied (check both raw and stripped output)
+          // Check if any waiters are satisfied (check both raw and stripped
+          // output). resolve() removes the waiter from outputWaiters internally,
+          // so we iterate over a snapshot to avoid index shifting during the loop.
           const stripped = stripAnsi(output)
-          for (let idx = outputWaiters.length - 1; idx >= 0; idx--) {
-            const waiter = outputWaiters[idx]
-            if (waiter && (stripped.includes(waiter.text) || output.includes(waiter.text))) {
+          for (const waiter of [...outputWaiters]) {
+            if (stripped.includes(waiter.text) || output.includes(waiter.text)) {
               waiter.resolve()
-              outputWaiters.splice(idx, 1)
             }
           }
         })
@@ -151,26 +161,39 @@ export const cliFixture = envFixture.extend<{cli: CLIProcess}>({
           if (exitResolve) {
             exitResolve(code)
           }
-          // Reject any remaining output waiters
-          for (const waiter of outputWaiters) {
+          // Reject any remaining output waiters. reject() removes each waiter
+          // from outputWaiters, so iterate over a snapshot to avoid skipping.
+          for (const waiter of [...outputWaiters]) {
             waiter.reject(new Error(`Process exited (code ${code}) while waiting for output: "${waiter.text}"`))
           }
-          outputWaiters.length = 0
         })
 
         const spawned: SpawnedProcess = {
           ptyProcess,
 
-          waitForOutput(text: string, timeoutMs = CLI_TIMEOUT.medium) {
+          waitForOutput(text: string, opts: number | WaitForOutputOptions = {}) {
+            const {timeoutMs = CLI_TIMEOUT.medium, signal} =
+              typeof opts === 'number' ? {timeoutMs: opts, signal: undefined} : opts
+
             // Check if already in output (raw or stripped)
             if (stripAnsi(output).includes(text) || output.includes(text)) {
               return Promise.resolve()
             }
+            if (signal?.aborted) {
+              return Promise.reject(new Error(`Cancelled waiting for output: "${text}"`))
+            }
 
             return new Promise<void>((resolve, reject) => {
+              // eslint-disable-next-line prefer-const
+              let waiter: {text: string; resolve: () => void; reject: (err: Error) => void}
+
+              const removeWaiter = () => {
+                const idx = outputWaiters.indexOf(waiter)
+                if (idx >= 0) outputWaiters.splice(idx, 1)
+              }
+
               const timer = setTimeout(() => {
-                const waiterIdx = outputWaiters.findIndex((waiter) => waiter.text === text)
-                if (waiterIdx >= 0) outputWaiters.splice(waiterIdx, 1)
+                removeWaiter()
                 reject(
                   new Error(
                     `Timed out after ${timeoutMs}ms waiting for output: "${text}"\n\nCaptured output:\n${stripAnsi(
@@ -180,17 +203,32 @@ export const cliFixture = envFixture.extend<{cli: CLIProcess}>({
                 )
               }, timeoutMs)
 
-              outputWaiters.push({
+              waiter = {
                 text,
                 resolve: () => {
                   clearTimeout(timer)
+                  removeWaiter()
                   resolve()
                 },
                 reject: (err) => {
                   clearTimeout(timer)
+                  removeWaiter()
                   reject(err)
                 },
-              })
+              }
+              outputWaiters.push(waiter)
+
+              if (signal) {
+                signal.addEventListener(
+                  'abort',
+                  () => {
+                    clearTimeout(timer)
+                    removeWaiter()
+                    reject(new Error(`Cancelled waiting for output: "${text}"`))
+                  },
+                  {once: true},
+                )
+              }
             })
           },
 

packages/e2e/setup/cli.ts

@@ -6,6 +6,7 @@
  */
 
 /* eslint-disable no-restricted-imports */
+import {isVisibleWithin} from './browser.js'
 import {directories, executables, globalLog} from './env.js'
 import {CLI_TIMEOUT, BROWSER_TIMEOUT} from './constants.js'
 import {stripAnsi} from '../helpers/strip-ansi.js'
@@ -154,7 +155,7 @@ async function visitAndHandleAccountPicker(page: Page, url: string, email: strin
   await page.waitForTimeout(BROWSER_TIMEOUT.medium)
   if (isAccountsShopifyUrl(page.url())) {
     const accountButton = page.locator(`text=${email}`).first()
-    if (await accountButton.isVisible({timeout: BROWSER_TIMEOUT.long}).catch(() => false)) {
+    if (await isVisibleWithin(accountButton, BROWSER_TIMEOUT.long)) {
       await accountButton.click()
       await page.waitForTimeout(BROWSER_TIMEOUT.medium)
     }