fix: do not load trace data for passing tests (#22311)

Fixes #22122
Fixes #22120

packages/playwright-test/src/index.ts

@@ -319,16 +319,22 @@ const playwrightFixtures: Fixtures<TestFixtures, WorkerFixtures> = ({
       (context as any)._instrumentation.addListener(createInstrumentationListener());
     };
 
+    const preserveTrace = () => {
+      const testFailed = testInfo.status !== testInfo.expectedStatus;
+      return captureTrace && (traceMode === 'on' || (testFailed && traceMode === 'retain-on-failure') || (traceMode === 'on-first-retry' && testInfo.retry === 1) || (traceMode === 'on-all-retries' && testInfo.retry > 0));
+    };
+
     const startedCollectingArtifacts = Symbol('startedCollectingArtifacts');
     const stopTracing = async (tracing: Tracing) => {
       if ((tracing as any)[startedCollectingArtifacts])
         return;
       (tracing as any)[startedCollectingArtifacts] = true;
       if (captureTrace) {
-        // Export trace for now. We'll know whether we have to preserve it
-        // after the test finishes.
-        const tracePath = path.join(_artifactsDir(), createGuid() + '.zip');
-        temporaryTraceFiles.push(tracePath);
+        let tracePath;
+        if (preserveTrace()) {
+          tracePath = path.join(_artifactsDir(), createGuid() + '.zip');
+          temporaryTraceFiles.push(tracePath);
+        }
         await tracing.stopChunk({ path: tracePath });
       }
     };
@@ -400,7 +406,6 @@ const playwrightFixtures: Fixtures<TestFixtures, WorkerFixtures> = ({
 
     // 3. Determine whether we need the artifacts.
     const testFailed = testInfo.status !== testInfo.expectedStatus;
-    const preserveTrace = captureTrace && (traceMode === 'on' || (testFailed && traceMode === 'retain-on-failure') || (traceMode === 'on-first-retry' && testInfo.retry === 1) || (traceMode === 'on-all-retries' && testInfo.retry > 0));
     const captureScreenshots = screenshotMode === 'on' || (screenshotMode === 'only-on-failure' && testFailed);
 
     const screenshotAttachments: string[] = [];
@@ -445,9 +450,8 @@ const playwrightFixtures: Fixtures<TestFixtures, WorkerFixtures> = ({
       await stopTracing(tracing);
     })));
 
-
     // 6. Save test trace.
-    if (preserveTrace) {
+    if (preserveTrace()) {
       const events = (testInfo as any)._traceEvents;
       if (events.length) {
         const tracePath = path.join(_artifactsDir(), createGuid() + '.zip');
@@ -458,7 +462,7 @@ const playwrightFixtures: Fixtures<TestFixtures, WorkerFixtures> = ({
 
     // 7. Either remove or attach temporary traces and screenshots for contexts closed
     // before the test has finished.
-    if (preserveTrace && temporaryTraceFiles.length) {
+    if (preserveTrace() && temporaryTraceFiles.length) {
       const tracePath = testInfo.outputPath(`trace.zip`);
       await mergeTraceFiles(tracePath, temporaryTraceFiles);
       testInfo.attachments.push({ name: 'trace', path: tracePath, contentType: 'application/zip' });

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

@@ -340,3 +340,55 @@ test('should respect PW_TEST_DISABLE_TRACING', async ({ runInlineTest }, testInf
   expect(result.passed).toBe(1);
   expect(fs.existsSync(testInfo.outputPath('test-results', 'a-test-1', 'trace.zip'))).toBe(false);
 });
+
+for (const mode of ['off', 'retain-on-failure', 'on-first-retry', 'on-all-retries']) {
+  test(`trace:${mode} should not create trace zip artifact if page test passed`, async ({ runInlineTest }) => {
+    const result = await runInlineTest({
+      'a.spec.ts': `
+        import { test as base, expect } from '@playwright/test';
+        import fs from 'fs';
+        const test = base.extend<{
+          locale: string | undefined,
+          _artifactsDir: () => string,
+        }>({
+          // Override locale fixture to check in teardown that no temporary trace zip was created.
+          locale: [async ({ locale, _artifactsDir }, use) => {
+            await use(locale);
+            const entries =  fs.readdirSync(_artifactsDir());
+            expect(entries.filter(e => e.endsWith('.zip'))).toEqual([]);
+          }, { option: true }],
+        });
+        test('passing test', async ({ page }) => {
+          await page.goto('about:blank');
+        });
+      `,
+    }, { trace: 'retain-on-failure' });
+    expect(result.exitCode).toBe(0);
+    expect(result.passed).toBe(1);
+  });
+
+  test(`trace:${mode} should not create trace zip artifact if APIRequestContext test passed`, async ({ runInlineTest, server }) => {
+    const result = await runInlineTest({
+      'a.spec.ts': `
+        import { test as base, expect } from '@playwright/test';
+        import fs from 'fs';
+        const test = base.extend<{
+          locale: string | undefined,
+          _artifactsDir: () => string,
+        }>({
+          // Override locale fixture to check in teardown that no temporary trace zip was created.
+          locale: [async ({ locale, _artifactsDir }, use) => {
+            await use(locale);
+            const entries =  fs.readdirSync(_artifactsDir());
+            expect(entries.filter(e => e.endsWith('.zip'))).toEqual([]);
+          }, { option: true }],
+        });
+        test('passing test', async ({ request }) => {
+          expect(await request.get('${server.EMPTY_PAGE}')).toBeOK();
+        });
+      `,
+    }, { trace: 'retain-on-failure' });
+    expect(result.exitCode).toBe(0);
+    expect(result.passed).toBe(1);
+  });
+}