From b814e8a5f190b7adde65d95863233de763fb35e6 Mon Sep 17 00:00:00 2001 From: Pavel Feldman Date: Tue, 23 May 2023 09:36:35 -0700 Subject: [PATCH] chore: bring back per test artifacts (#23153) --- .../playwright-core/src/utils/traceUtils.ts | 2 +- .../playwright-test/src/common/globals.ts | 13 ----- packages/playwright-test/src/index.ts | 58 ++++++------------- .../playwright-test/src/worker/testInfo.ts | 1 + .../playwright-test/src/worker/workerMain.ts | 8 +-- packages/trace-viewer/src/traceModel.ts | 14 +++++ .../playwright.artifacts.spec.ts | 3 + .../playwright-test/playwright.reuse.spec.ts | 2 - .../playwright-test/playwright.trace.spec.ts | 4 -- .../ui-mode-test-progress.spec.ts | 4 +- tests/playwright-test/ui-mode-trace.spec.ts | 3 +- 11 files changed, 42 insertions(+), 70 deletions(-) diff --git a/packages/playwright-core/src/utils/traceUtils.ts b/packages/playwright-core/src/utils/traceUtils.ts index e6ca9de7b7..791744f01f 100644 --- a/packages/playwright-core/src/utils/traceUtils.ts +++ b/packages/playwright-core/src/utils/traceUtils.ts @@ -67,7 +67,7 @@ export async function mergeTraceFiles(fileName: string, temporaryTraceFiles: str let pendingEntries = inZipFile.entryCount; inZipFile.on('entry', entry => { let entryName = entry.fileName; - if (entry.fileName.startsWith('trace.')) + if (entry.fileName.match(/[\d-]*trace./)) entryName = i + '-' + entry.fileName; inZipFile.openReadStream(entry, (err, readStream) => { if (err) { diff --git a/packages/playwright-test/src/common/globals.ts b/packages/playwright-test/src/common/globals.ts index f988dc073f..9443db25d3 100644 --- a/packages/playwright-test/src/common/globals.ts +++ b/packages/playwright-test/src/common/globals.ts @@ -69,16 +69,3 @@ export function setCurrentConfig(config: FullConfigInternal | null) { export function currentConfig(): FullConfigInternal | null { return currentConfigValue; } - -export interface TestInstrumentation { - willStartTest(testInfo: TestInfoImpl): Promise; - didFinishTestFunction(testInfo: TestInfoImpl): Promise; - didFinishTest(testInfo: TestInfoImpl): Promise; -} -let testInstrumentation: TestInstrumentation | undefined; -export function setCurrentTestInstrumentation(instrumentation: TestInstrumentation | undefined) { - testInstrumentation = instrumentation; -} -export function currentTestInstrumentation() { - return testInstrumentation; -} diff --git a/packages/playwright-test/src/index.ts b/packages/playwright-test/src/index.ts index c719e21f84..d064b3678b 100644 --- a/packages/playwright-test/src/index.ts +++ b/packages/playwright-test/src/index.ts @@ -26,7 +26,7 @@ import { type ContextReuseMode } from './common/config'; import { artifactsFolderName } from './isomorphic/folders'; import type { ClientInstrumentation, ClientInstrumentationListener } from '../../playwright-core/src/client/clientInstrumentation'; import type { ParsedStackTrace } from '../../playwright-core/src/utils/stackTrace'; -import { currentTestInfo, setCurrentTestInstrumentation } from './common/globals'; +import { currentTestInfo } from './common/globals'; export { expect } from './matchers/expect'; export { store as _store } from './store'; export const _baseTest: TestType<{}, {}> = rootTestType.test; @@ -50,12 +50,12 @@ type TestFixtures = PlaywrightTestArgs & PlaywrightTestOptions & { _contextReuseMode: ContextReuseMode, _reuseContext: boolean, _setupContextOptions: void; + _setupArtifacts: void; _contextFactory: (options?: BrowserContextOptions) => Promise; }; type WorkerFixtures = PlaywrightWorkerArgs & PlaywrightWorkerOptions & { _browserOptions: LaunchOptions; _artifactsDir: () => string; - _setupArtifacts: void; _snapshotSuffix: string; }; @@ -269,9 +269,9 @@ const playwrightFixtures: Fixtures = ({ } }, { auto: 'all-hooks-included', _title: 'context configuration' } as any], - _setupArtifacts: [async ({ playwright, _artifactsDir, trace, screenshot }, use) => { - let artifactsRecorder: ArtifactsRecorder | undefined; - + _setupArtifacts: [async ({ playwright, _artifactsDir, trace, screenshot }, use, testInfo) => { + const artifactsRecorder = new ArtifactsRecorder(playwright, _artifactsDir(), trace, screenshot); + await artifactsRecorder.willStartTest(testInfo as TestInfoImpl); const csiListener: ClientInstrumentationListener = { onApiCallBegin: (apiName: string, params: Record, stackTrace: ParsedStackTrace | null, wallTime: number, userData: any) => { const testInfo = currentTestInfo(); @@ -312,46 +312,15 @@ const playwrightFixtures: Fixtures = ({ }, }; - const willStartTest = async (testInfo: TestInfoImpl) => { - artifactsRecorder = new ArtifactsRecorder(playwright, _artifactsDir(), trace, screenshot); - await artifactsRecorder.willStartTest(testInfo); - }; - - const didFinishTestFunction = async (testInfo: TestInfoImpl) => { - await artifactsRecorder?.didFinishTestFunction(); - }; - - const didFinishTest = async (testInfo: TestInfoImpl) => { - await artifactsRecorder?.didFinishTest(); - artifactsRecorder = undefined; - }; - - // 1. Setup instrumentation. const clientInstrumentation = (playwright as any)._instrumentation as ClientInstrumentation; clientInstrumentation.addListener(csiListener); - setCurrentTestInstrumentation({ willStartTest, didFinishTestFunction, didFinishTest }); - // 2. Setup for the first test in the worker. - { - const firstTestInfo = currentTestInfo(); - if (firstTestInfo) - await willStartTest(firstTestInfo); - } - - // 2. Run the test. await use(); - // 3. Teardown for the last test in the worker. - { - const lastTestInfo = currentTestInfo(); - if (lastTestInfo) - await didFinishTest(lastTestInfo); - } - - // 4. Cleanup instrumentation. - setCurrentTestInstrumentation(undefined); clientInstrumentation.removeListener(csiListener); - }, { scope: 'worker', auto: 'all-hooks-included', _title: 'trace recording' } as any], + await artifactsRecorder?.didFinishTest(); + + }, { auto: 'all-hooks-included', _title: 'trace recording' } as any], _contextFactory: [async ({ browser, video, _artifactsDir, _reuseContext }, use, testInfo) => { const testInfoImpl = testInfo as TestInfoImpl; @@ -580,6 +549,7 @@ class ArtifactsRecorder { async willStartTest(testInfo: TestInfoImpl) { this._testInfo = testInfo; + testInfo._onDidFinishTestFunction = () => this.didFinishTestFunction(); this._captureTrace = shouldCaptureTrace(this._traceMode, testInfo) && !process.env.PW_TEST_DISABLE_TRACING; // Process existing contexts. @@ -677,8 +647,16 @@ class ArtifactsRecorder { // before the test has finished. if (this._preserveTrace() && this._temporaryTraceFiles.length) { const tracePath = this._testInfo.outputPath(`trace.zip`); + // This could be: beforeHooks, or beforeHooks + test, etc. + const beforeHooksHadTrace = fs.existsSync(tracePath); + if (beforeHooksHadTrace) { + await fs.promises.rename(tracePath, tracePath + '.tmp'); + this._temporaryTraceFiles.unshift(tracePath + '.tmp'); + } await mergeTraceFiles(tracePath, this._temporaryTraceFiles); - this._testInfo.attachments.push({ name: 'trace', path: tracePath, contentType: 'application/zip' }); + // Do not add attachment twice. + if (!beforeHooksHadTrace) + this._testInfo.attachments.push({ name: 'trace', path: tracePath, contentType: 'application/zip' }); } await Promise.all(this._temporaryScreenshots.map(async file => { if (captureScreenshots) diff --git a/packages/playwright-test/src/worker/testInfo.ts b/packages/playwright-test/src/worker/testInfo.ts index 7e4364d526..97e094c122 100644 --- a/packages/playwright-test/src/worker/testInfo.ts +++ b/packages/playwright-test/src/worker/testInfo.ts @@ -58,6 +58,7 @@ export class TestInfoImpl implements TestInfo { readonly _steps: TestStepInternal[] = []; _beforeHooksStep: TestStepInternal | undefined; _afterHooksStep: TestStepInternal | undefined; + _onDidFinishTestFunction: (() => Promise) | undefined; // ------------ TestInfo fields ------------ readonly testId: string; diff --git a/packages/playwright-test/src/worker/workerMain.ts b/packages/playwright-test/src/worker/workerMain.ts index 6dbdf2388a..5338596ff8 100644 --- a/packages/playwright-test/src/worker/workerMain.ts +++ b/packages/playwright-test/src/worker/workerMain.ts @@ -18,7 +18,7 @@ import { colors, rimraf } from 'playwright-core/lib/utilsBundle'; import util from 'util'; import { debugTest, formatLocation, relativeFilePath, serializeError } from '../util'; import type { TestBeginPayload, TestEndPayload, RunPayload, DonePayload, WorkerInitParams, TeardownErrorsPayload, TestOutputPayload } from '../common/ipc'; -import { setCurrentTestInfo, setIsWorkerProcess, currentTestInstrumentation } from '../common/globals'; +import { setCurrentTestInfo, setIsWorkerProcess } from '../common/globals'; import { ConfigLoader } from '../common/configLoader'; import type { Suite, TestCase } from '../common/test'; import type { Annotation, FullConfigInternal, FullProjectInternal } from '../common/config'; @@ -326,8 +326,6 @@ export class WorkerMain extends ProcessRunner { // Note: wrap all preparation steps together, because failure/skip in any of them // prevents further setup and/or test from running. const beforeHooksError = await testInfo._runAndFailOnError(async () => { - await currentTestInstrumentation()?.willStartTest(testInfo); - // Run "beforeAll" modifiers on parent suites, unless already run during previous tests. for (const suite of suites) { if (this._extraSuiteAnnotations.has(suite)) @@ -401,7 +399,7 @@ export class WorkerMain extends ProcessRunner { // Run "immediately upon test function finish" callback. debugTest(`on-test-function-finish callback started`); - const didFinishTestFunctionError = await testInfo._runAndFailOnError(async () => await currentTestInstrumentation()?.didFinishTestFunction(testInfo)); + const didFinishTestFunctionError = await testInfo._runAndFailOnError(async () => testInfo._onDidFinishTestFunction?.()); firstAfterHooksError = firstAfterHooksError || didFinishTestFunctionError; debugTest(`on-test-function-finish callback finished`); @@ -468,8 +466,6 @@ export class WorkerMain extends ProcessRunner { step.complete({ error: firstAfterHooksError }); }); - await testInfo._runAndFailOnError(async () => await currentTestInstrumentation()?.didFinishTest(testInfo)); - this._currentTest = null; setCurrentTestInfo(null); this.dispatchEvent('testEnd', buildTestEndPayload(testInfo)); diff --git a/packages/trace-viewer/src/traceModel.ts b/packages/trace-viewer/src/traceModel.ts index 005f625492..3518a3ab80 100644 --- a/packages/trace-viewer/src/traceModel.ts +++ b/packages/trace-viewer/src/traceModel.ts @@ -76,6 +76,20 @@ export class TraceModel { unzipProgress(++done, total); contextEntry.actions = [...actionMap.values()].sort((a1, a2) => a1.startTime - a2.startTime); + if (!backend.isLive()) { + // Terminate actions w/o after event gracefully. + // This would close after hooks event that has not been closed because + // the trace is usually saved before after hooks complete. + for (const action of contextEntry.actions.slice().reverse()) { + if (!action.endTime && !action.error) { + for (const a of contextEntry.actions) { + if (a.parentId === action.callId && action.endTime < a.endTime) + action.endTime = a.endTime; + } + } + } + } + const stacks = await this._backend.readText(ordinal + '.stacks'); if (stacks) { const callMetadata = parseClientSideCallMetadata(JSON.parse(stacks)); diff --git a/tests/playwright-test/playwright.artifacts.spec.ts b/tests/playwright-test/playwright.artifacts.spec.ts index 7490532010..62a2dcf0d2 100644 --- a/tests/playwright-test/playwright.artifacts.spec.ts +++ b/tests/playwright-test/playwright.artifacts.spec.ts @@ -151,8 +151,10 @@ test('should work with screenshot: on', async ({ runInlineTest }, testInfo) => { ' test-finished-1.png', 'artifacts-shared-shared-failing', ' test-failed-1.png', + ' test-failed-2.png', 'artifacts-shared-shared-passing', ' test-finished-1.png', + ' test-finished-2.png', 'artifacts-two-contexts', ' test-finished-1.png', ' test-finished-2.png', @@ -182,6 +184,7 @@ test('should work with screenshot: only-on-failure', async ({ runInlineTest }, t ' test-failed-1.png', 'artifacts-shared-shared-failing', ' test-failed-1.png', + ' test-failed-2.png', 'artifacts-two-contexts-failing', ' test-failed-1.png', ' test-failed-2.png', diff --git a/tests/playwright-test/playwright.reuse.spec.ts b/tests/playwright-test/playwright.reuse.spec.ts index 0bf77686ae..99b3811af0 100644 --- a/tests/playwright-test/playwright.reuse.spec.ts +++ b/tests/playwright-test/playwright.reuse.spec.ts @@ -156,7 +156,6 @@ test('should reuse context with trace if mode=when-possible', async ({ runInline 'After Hooks', 'fixture: page', 'fixture: context', - 'tracing.stopChunk', ]); expect(trace1.traceModel.storage().snapshotsForTest().length).toBeGreaterThan(0); expect(fs.existsSync(testInfo.outputPath('test-results', 'reuse-one', 'trace-1.zip'))).toBe(false); @@ -173,7 +172,6 @@ test('should reuse context with trace if mode=when-possible', async ({ runInline 'After Hooks', 'fixture: page', 'fixture: context', - 'tracing.stopChunk', ]); expect(trace2.traceModel.storage().snapshotsForTest().length).toBeGreaterThan(0); }); diff --git a/tests/playwright-test/playwright.trace.spec.ts b/tests/playwright-test/playwright.trace.spec.ts index b049e4327d..418795d681 100644 --- a/tests/playwright-test/playwright.trace.spec.ts +++ b/tests/playwright-test/playwright.trace.spec.ts @@ -115,12 +115,10 @@ test('should record api trace', async ({ runInlineTest, server }, testInfo) => { 'tracing.start', 'apiRequestContext.get', 'After Hooks', - 'tracing.stopChunk', ]); const trace3 = await parseTrace(testInfo.outputPath('test-results', 'a-fail', 'trace.zip')); expect(trace3.apiNames).toEqual([ 'Before Hooks', - 'tracing.startChunk', 'fixture: request', 'apiRequest.newContext', 'tracing.start', @@ -138,7 +136,6 @@ test('should record api trace', async ({ runInlineTest, server }, testInfo) => { 'fixture: request', 'tracing.stopChunk', 'apiRequestContext.dispose', - 'tracing.stopChunk', ]); }); @@ -345,7 +342,6 @@ test('should not override trace file in afterAll', async ({ runInlineTest, serve 'fixture: request', 'tracing.stopChunk', 'apiRequestContext.dispose', - 'fixture: browser', ]); const error = await parseTrace(testInfo.outputPath('test-results', 'a-test-2', 'trace.zip')).catch(e => e); diff --git a/tests/playwright-test/ui-mode-test-progress.spec.ts b/tests/playwright-test/ui-mode-test-progress.spec.ts index 01c91bf476..482f831bf4 100644 --- a/tests/playwright-test/ui-mode-test-progress.spec.ts +++ b/tests/playwright-test/ui-mode-test-progress.spec.ts @@ -106,8 +106,8 @@ test('should update trace live', async ({ runUITest, server }) => { await expect(listItem).toHaveText([ /Before Hooks[\d.]+m?s/, - /page.gotohttp:\/\/localhost:\d+\/one.html/, - /page.gotohttp:\/\/localhost:\d+\/two.html/, + /page.gotohttp:\/\/localhost:\d+\/one.html[\d.]+m?s/, + /page.gotohttp:\/\/localhost:\d+\/two.html[\d.]+m?s/, /After Hooks[\d.]+m?s/, /fixture: page[\d.]+m?s/, /fixture: context[\d.]+m?s/, diff --git a/tests/playwright-test/ui-mode-trace.spec.ts b/tests/playwright-test/ui-mode-trace.spec.ts index d24956ca09..ede1fabae2 100644 --- a/tests/playwright-test/ui-mode-trace.spec.ts +++ b/tests/playwright-test/ui-mode-trace.spec.ts @@ -101,10 +101,9 @@ test('should merge screenshot assertions', async ({ runUITest }, testInfo) => { /Before Hooks[\d.]+m?s/, /page.setContent[\d.]+m?s/, /expect.toHaveScreenshot[\d.]+m?s/, - /After Hooks-/, + /After Hooks[\d.]+m?s/, /fixture: page[\d.]+m?s/, /fixture: context[\d.]+m?s/, - /fixture: browser[\d.]+m?s/, ]); });