fix(playwright): teardown when global quit force terminates browser (v…

…ercel#59548)
Gand0r · Dec 12, 2023 · 61889f8 · 61889f8
1 parent ed12b55
commit 61889f8
Show file tree

Hide file tree

Showing 2 changed files with 18 additions and 10 deletions.
diff --git a/run-tests.js b/run-tests.js
@@ -555,7 +555,8 @@ ${ENDGROUP}`)
         // If environment is CI and if this test execution is failed after retry, preserve test traces
         // to upload into github actions artifacts for debugging purpose
         const shouldPreserveTracesOutput =
-          process.env.CI && isRetry && isChildExitWithNonZero
+          (process.env.CI && isRetry && isChildExitWithNonZero) ||
+          process.env.PRESERVE_TRACES_OUTPUT
         if (!shouldPreserveTracesOutput) {
           await fsp
             .rm(

diff --git a/test/lib/browsers/playwright.ts b/test/lib/browsers/playwright.ts
@@ -21,13 +21,25 @@ let websocketFrames: Array<{ payload: string | Buffer }> = []
 
 const tracePlaywright = process.env.TRACE_PLAYWRIGHT
 
+// loose global to register teardown functions before quitting the browser instance.
+// This is due to `quit` can be called anytime outside of BrowserInterface's lifecycle,
+// which can create corrupted state by terminating the context.
+// [TODO] global `quit` might need to be removed, instead should introduce per-instance teardown
+const pendingTeardown = []
 export async function quit() {
+  await Promise.all(pendingTeardown.map((fn) => fn()))
   await context?.close()
   await browser?.close()
   context = undefined
   browser = undefined
 }
 
+async function teardown(tearDownFn: () => Promise<void>) {
+  pendingTeardown.push(tearDownFn)
+  await tearDownFn()
+  pendingTeardown.splice(pendingTeardown.indexOf(tearDownFn), 1)
+}
+
 interface ElementHandleExt extends ElementHandle {
   getComputedCss(prop: string): Promise<string>
   text(): Promise<string>
@@ -38,7 +50,6 @@ export class Playwright extends BrowserInterface {
   private eventCallbacks: Record<Event, Set<(...args: any[]) => void>> = {
     request: new Set(),
   }
-
   private async initContextTracing(
     url: string,
     page: Page,
@@ -50,7 +61,7 @@ export class Playwright extends BrowserInterface {
 
     try {
       // Clean up if any previous traces are still active
-      await this.teardownTracing()
+      await teardown(this.teardownTracing.bind(this))
 
       await context.tracing.start({
         screenshots: true,
@@ -60,7 +71,7 @@ export class Playwright extends BrowserInterface {
       this.activeTrace = encodeURIComponent(url)
 
       page.on('close', async () => {
-        await this.teardownTracing()
+        await teardown(this.teardownTracing.bind(this))
       })
     } catch (e) {
       this.activeTrace = undefined
@@ -87,6 +98,7 @@ export class Playwright extends BrowserInterface {
         path: traceOutputPath,
       })
     } catch (e) {
+      require('console').warn('Failed to teardown playwright tracing', e)
     } finally {
       this.activeTrace = undefined
     }
@@ -128,16 +140,14 @@ export class Playwright extends BrowserInterface {
     if (browser) {
       if (contextHasJSEnabled !== javaScriptEnabled) {
         // If we have switched from having JS enable/disabled we need to recreate the context.
+        await teardown(this.teardownTracing.bind(this))
         await context?.close()
         context = await browser.newContext({
           locale,
           javaScriptEnabled,
           ignoreHTTPSErrors,
           ...device,
         })
-        context.once('close', async () => {
-          await this.teardownTracing()
-        })
         contextHasJSEnabled = javaScriptEnabled
       }
       return
@@ -150,9 +160,6 @@ export class Playwright extends BrowserInterface {
       ignoreHTTPSErrors,
       ...device,
     })
-    context.once('close', async () => {
-      await this.teardownTracing()
-    })
     contextHasJSEnabled = javaScriptEnabled
   }