From 2af3f486c4f141400d57912ae54785c5455757c0 Mon Sep 17 00:00:00 2001 From: Yury Semikhatsky Date: Mon, 10 Apr 2023 13:29:55 -0700 Subject: [PATCH] fix: do not load trace data for passing tests (#22311) Fixes #22122 Fixes #22120 --- packages/playwright-test/src/index.ts | 20 ++++--- .../playwright-test/playwright.trace.spec.ts | 52 +++++++++++++++++++ 2 files changed, 64 insertions(+), 8 deletions(-) diff --git a/packages/playwright-test/src/index.ts b/packages/playwright-test/src/index.ts index c4de2b927f..24f7ea2ffe 100644 --- a/packages/playwright-test/src/index.ts +++ b/packages/playwright-test/src/index.ts @@ -319,16 +319,22 @@ const playwrightFixtures: Fixtures = ({ (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 = ({ // 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 = ({ 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 = ({ // 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' }); diff --git a/tests/playwright-test/playwright.trace.spec.ts b/tests/playwright-test/playwright.trace.spec.ts index f3fdfa570d..bf22c26818 100644 --- a/tests/playwright-test/playwright.trace.spec.ts +++ b/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); + }); +} \ No newline at end of file