fix(trace): do not save trace on failure after success in test+hooks (#34724)

packages/playwright/src/index.ts

@@ -726,7 +726,7 @@ class ArtifactsRecorder {
       return;
     (tracing as any)[this._startedCollectingArtifacts] = true;
     if (this._testInfo._tracing.traceOptions() && (tracing as any)[kTracingStarted])
-      await tracing.stopChunk({ path: this._testInfo._tracing.generateNextTraceRecordingPath() });
+      await tracing.stopChunk({ path: this._testInfo._tracing.maybeGenerateNextTraceRecordingPath() });
   }
 }
 

packages/playwright/src/worker/testTracing.ts

@@ -46,6 +46,7 @@ export class TestTracing {
   private _artifactsDir: string;
   private _tracesDir: string;
   private _contextCreatedEvent: trace.ContextCreatedTraceEvent;
+  private _didFinishTestFunctionAndAfterEachHooks = false;
 
   constructor(testInfo: TestInfoImpl, artifactsDir: string) {
     this._testInfo = testInfo;
@@ -113,6 +114,10 @@ export class TestTracing {
     }
   }
 
+  didFinishTestFunctionAndAfterEachHooks() {
+    this._didFinishTestFunctionAndAfterEachHooks = true;
+  }
+
   artifactsDir() {
     return this._artifactsDir;
   }
@@ -133,7 +138,7 @@ export class TestTracing {
     return `${this._testInfo.testId}${retrySuffix}${ordinalSuffix}`;
   }
 
-  generateNextTraceRecordingPath() {
+  private _generateNextTraceRecordingPath() {
     const file = path.join(this._artifactsDir, createGuid() + '.zip');
     this._temporaryTraceFiles.push(file);
     return file;
@@ -143,6 +148,22 @@ export class TestTracing {
     return this._options;
   }
 
+  maybeGenerateNextTraceRecordingPath() {
+    // Forget about traces that should be saved on failure, when no failure happened
+    // during the test and beforeEach/afterEach hooks.
+    // This avoids downloading traces over the wire when not really needed.
+    if (this._didFinishTestFunctionAndAfterEachHooks && this._shouldAbandonTrace())
+      return;
+    return this._generateNextTraceRecordingPath();
+  }
+
+  private _shouldAbandonTrace() {
+    if (!this._options)
+      return true;
+    const testFailed = this._testInfo.status !== this._testInfo.expectedStatus;
+    return !testFailed && (this._options.mode === 'retain-on-failure' || this._options.mode === 'retain-on-first-failure');
+  }
+
   async stopIfNeeded() {
     if (!this._options)
       return;
@@ -151,10 +172,7 @@ export class TestTracing {
     if (error)
       throw error;
 
-    const testFailed = this._testInfo.status !== this._testInfo.expectedStatus;
-    const shouldAbandonTrace = !testFailed && (this._options.mode === 'retain-on-failure' || this._options.mode === 'retain-on-first-failure');
-
-    if (shouldAbandonTrace) {
+    if (this._shouldAbandonTrace()) {
       for (const file of this._temporaryTraceFiles)
         await fs.promises.unlink(file).catch(() => {});
       return;
@@ -213,7 +231,7 @@ export class TestTracing {
 
     await new Promise(f => {
       zipFile.end(undefined, () => {
-        zipFile.outputStream.pipe(fs.createWriteStream(this.generateNextTraceRecordingPath())).on('close', f);
+        zipFile.outputStream.pipe(fs.createWriteStream(this._generateNextTraceRecordingPath())).on('close', f);
       });
     });
 

packages/playwright/src/worker/workerMain.ts

@@ -399,6 +399,8 @@ export class WorkerMain extends ProcessRunner {
         firstAfterHooksError = firstAfterHooksError ?? error;
       }
 
+      testInfo._tracing.didFinishTestFunctionAndAfterEachHooks();
+
       try {
         // Teardown test-scoped fixtures. Attribute to 'test' so that users understand
         // they should probably increase the test timeout to fix this issue.

tests/playwright-test/playwright.connect.spec.ts

@@ -14,7 +14,9 @@
  * limitations under the License.
  */
 
-import { test, expect } from './playwright-test-fixtures';
+import { test, expect, countTimes } from './playwright-test-fixtures';
+import { parseTrace } from '../config/utils';
+import fs from 'fs';
 
 test('should work with connectOptions', async ({ runInlineTest }) => {
   const result = await runInlineTest({
@@ -167,3 +169,62 @@ test('should print debug log when failed to connect', async ({ runInlineTest })
   expect(result.output).toContain('b-debug-log-string');
   expect(result.results[0].attachments).toEqual([]);
 });
+
+test('should record trace', async ({ runInlineTest }) => {
+  const result = await runInlineTest({
+    'playwright.config.js': `
+      module.exports = {
+        globalSetup: './global-setup',
+        use: {
+          connectOptions: {
+            wsEndpoint: process.env.CONNECT_WS_ENDPOINT,
+          },
+          trace: 'retain-on-failure',
+        },
+      };
+    `,
+    'global-setup.ts': `
+      import { chromium } from '@playwright/test';
+      module.exports = async () => {
+        const server = await chromium.launchServer();
+        process.env.CONNECT_WS_ENDPOINT = server.wsEndpoint();
+        process.env.DEBUG = 'pw:channel';
+        return () => server.close();
+      };
+    `,
+    'a.test.ts': `
+      import { test, expect } from '@playwright/test';
+      test('pass', async ({ page }) => {
+        expect(1).toBe(1);
+      });
+      test('fail', async ({ page }) => {
+        expect(1).toBe(2);
+      });
+    `,
+  });
+  expect(result.exitCode).toBe(1);
+  expect(result.passed).toBe(1);
+  expect(result.failed).toBe(1);
+
+  // A single tracing artifact should be created. We see it in the logs twice:
+  // as a regular message and wrapped inside a jsonPipe.
+  expect(countTimes(result.output, `"type":"Artifact","initializer"`)).toBe(2);
+
+  expect(fs.existsSync(test.info().outputPath('test-results', 'a-pass', 'trace.zip'))).toBe(false);
+
+  const trace = await parseTrace(test.info().outputPath('test-results', 'a-fail', 'trace.zip'));
+  expect(trace.apiNames).toEqual([
+    'Before Hooks',
+    'fixture: context',
+    'browser.newContext',
+    'fixture: page',
+    'browserContext.newPage',
+    'expect.toBe',
+    'After Hooks',
+    'locator.ariaSnapshot',
+    'fixture: page',
+    'fixture: context',
+    'Worker Cleanup',
+    'fixture: browser',
+  ]);
+});