From 8eb816b363fac41d1c1b17edc725dd5142ac2877 Mon Sep 17 00:00:00 2001 From: Dmitry Gozman Date: Wed, 12 Feb 2025 15:41:16 +0000 Subject: [PATCH] fix(trace): do not save trace on failure after success in test+hooks (#34724) --- packages/playwright/src/index.ts | 2 +- packages/playwright/src/worker/testTracing.ts | 30 +++++++-- packages/playwright/src/worker/workerMain.ts | 2 + .../playwright.connect.spec.ts | 63 ++++++++++++++++++- 4 files changed, 89 insertions(+), 8 deletions(-) diff --git a/packages/playwright/src/index.ts b/packages/playwright/src/index.ts index 9b0fb456ab..a8d0cc0387 100644 --- a/packages/playwright/src/index.ts +++ b/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() }); } } diff --git a/packages/playwright/src/worker/testTracing.ts b/packages/playwright/src/worker/testTracing.ts index a90c006616..ba215c8792 100644 --- a/packages/playwright/src/worker/testTracing.ts +++ b/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); }); }); diff --git a/packages/playwright/src/worker/workerMain.ts b/packages/playwright/src/worker/workerMain.ts index 5188614271..30b59c36e7 100644 --- a/packages/playwright/src/worker/workerMain.ts +++ b/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. diff --git a/tests/playwright-test/playwright.connect.spec.ts b/tests/playwright-test/playwright.connect.spec.ts index 083a01ac5e..e86e7370ca 100644 --- a/tests/playwright-test/playwright.connect.spec.ts +++ b/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', + ]); +});