From 002fc12e3b2902fd6323ca9ff08a8e6a7c91667b Mon Sep 17 00:00:00 2001
From: Dmitry Gozman <dgozman@gmail.com>
Date: Tue, 22 Oct 2024 02:02:13 -0700
Subject: [PATCH] fix: allow trace in showBrowser mode (#542)

---
 src/testModel.ts        | 23 +++++++----------------
 tests/run-tests.spec.ts | 24 ++++++++++++++++--------
 2 files changed, 23 insertions(+), 24 deletions(-)

diff --git a/src/testModel.ts b/src/testModel.ts
index 0cb9eef54..db0bb1181 100644
--- a/src/testModel.ts
+++ b/src/testModel.ts
@@ -531,25 +531,16 @@ export class TestModel extends DisposableBase {
     const externalOptions = await this._embedder.runHooks.onWillRunTests(this.config, false);
     const showBrowser = this._embedder.settingsModel.showBrowser.get() && !!externalOptions.connectWsEndpoint;
 
-    let trace: 'on' | 'off' | undefined;
-    let video: 'on' | 'off' | undefined;
-
-    if (this._embedder.settingsModel.showTrace.get())
-      trace = 'on';
-    // "Show browser" mode forces context reuse that survives over multiple test runs.
-    // Playwright Test sets up `tracesDir` inside the `test-results` folder, so it will be removed between runs.
-    // When context is reused, its ongoing tracing will fail with ENOENT because trace files
-    // were suddenly removed. So we disable tracing in this case.
-    if (this._embedder.settingsModel.showBrowser.get()) {
-      trace = 'off';
-      video = 'off';
-    }
-
     const options: PlaywrightTestRunOptions = {
       headed: showBrowser && !this._embedder.isUnderTest,
       workers: showBrowser ? 1 : undefined,
-      trace,
-      video,
+      // Note: we used to disable trace in "show browser" mode, but it's not necessary anymore:
+      // - output directory is not removed;
+      // - tracesDir is not respected when connecting to a reused browser.
+      trace: this._embedder.settingsModel.showTrace.get() ? 'on' : undefined,
+      // Disable video when reusing context, because video cannot be chunked between multiple
+      // tests that use a single context.
+      video: this._embedder.settingsModel.showBrowser.get() ? 'off' : undefined,
       reuseContext: showBrowser,
       connectWsEndpoint: showBrowser ? externalOptions.connectWsEndpoint : undefined,
     };
diff --git a/tests/run-tests.spec.ts b/tests/run-tests.spec.ts
index 0a60c2f21..fd48f2cc5 100644
--- a/tests/run-tests.spec.ts
+++ b/tests/run-tests.spec.ts
@@ -1206,22 +1206,30 @@ test('should produce output twice', async ({ activate, overridePlaywrightVersion
   `);
 });
 
-test('should disable tracing when reusing context', async ({ activate, showBrowser }) => {
-  test.skip(!showBrowser);
-
+test('should not crash with tracing no matter reusing context', async ({ activate }) => {
   const { testController } = await activate({
     'playwright.config.js': `module.exports = { testDir: 'tests', use: { trace: 'on' } }`,
-    'tests/test.spec.ts': `
+    'tests/test1.spec.ts': `
       import { test } from '@playwright/test';
       test('one', async ({ page }) => {});
     `,
+    'tests/test2.spec.ts': `
+      import { test } from '@playwright/test';
+      test('two', async ({ page }) => {});
+    `,
   });
 
-  const testItems = testController.findTestItems(/test.spec.ts/);
-  expect(testItems.length).toBe(1);
-  await testController.run(testItems);
+  const testItems1 = testController.findTestItems(/test1.spec.ts/);
+  expect(testItems1.length).toBe(1);
+  await testController.run(testItems1);
+  expect(fs.existsSync(test.info().outputPath('test-results', 'test1-one', 'trace.zip'))).toBe(true);
 
-  expect(fs.existsSync(test.info().outputPath('test-results', 'test-one', 'trace.zip'))).toBe(false);
+  const testItems2 = testController.findTestItems(/test2.spec.ts/);
+  expect(testItems2.length).toBe(1);
+  await testController.run(testItems2);
+  // We do not clear output dir, so both traces should be present.
+  expect(fs.existsSync(test.info().outputPath('test-results', 'test1-one', 'trace.zip'))).toBe(true);
+  expect(fs.existsSync(test.info().outputPath('test-results', 'test2-two', 'trace.zip'))).toBe(true);
 });
 
 test('should force workers=1 when reusing the browser', async ({ activate, showBrowser }) => {