From fcd966c4e593516094810ea05c8e4c41ed7bf67d Mon Sep 17 00:00:00 2001 From: Dmitry Gozman Date: Mon, 1 May 2023 13:53:15 -0700 Subject: [PATCH] chore: make `_setupArtifacts` a worker-scoped fixture (#22739) This should unblock having separate test-fixture scopes for hooks and test. --- .../playwright-test/src/common/globals.ts | 13 +++ packages/playwright-test/src/index.ts | 87 ++++++++++++------- .../playwright-test/src/worker/testInfo.ts | 1 - .../playwright-test/src/worker/workerMain.ts | 25 +++--- tests/playwright-test/reporter.spec.ts | 4 +- tests/playwright-test/test-step.spec.ts | 4 + 6 files changed, 90 insertions(+), 44 deletions(-) diff --git a/packages/playwright-test/src/common/globals.ts b/packages/playwright-test/src/common/globals.ts index 9443db25d3..f988dc073f 100644 --- a/packages/playwright-test/src/common/globals.ts +++ b/packages/playwright-test/src/common/globals.ts @@ -69,3 +69,16 @@ 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 ff0b0fdddd..95891edcad 100644 --- a/packages/playwright-test/src/index.ts +++ b/packages/playwright-test/src/index.ts @@ -26,6 +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'; export { expect } from './matchers/expect'; export { store as _store } from './store'; export const _baseTest: TestType<{}, {}> = rootTestType.test; @@ -49,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; }; @@ -264,15 +265,15 @@ const playwrightFixtures: Fixtures = ({ } }, { auto: 'all-hooks-included', _title: 'context configuration' } as any], - _setupArtifacts: [async ({ playwright, _artifactsDir, trace, screenshot }, use, testInfo) => { - const artifactsRecorder = new ArtifactsRecorder(playwright, _artifactsDir(), trace, screenshot); - const testInfoImpl = testInfo as TestInfoImpl; + _setupArtifacts: [async ({ playwright, _artifactsDir, trace, screenshot }, use) => { + let artifactsRecorder: ArtifactsRecorder | undefined; const csiListener: ClientInstrumentationListener = { onApiCallBegin: (apiCall: string, stackTrace: ParsedStackTrace | null, wallTime: number, userData: any) => { - if (apiCall.startsWith('expect.')) + const testInfo = currentTestInfo(); + if (!testInfo || apiCall.startsWith('expect.') || apiCall.includes('setTestIdAttribute')) return { userObject: null }; - const step = testInfoImpl._addStep({ + const step = testInfo._addStep({ location: stackTrace?.frames[0] as any, category: 'pw:api', title: apiCall, @@ -285,35 +286,65 @@ const playwrightFixtures: Fixtures = ({ step?.complete({ error }); }, onWillPause: () => { - testInfo.setTimeout(0); + currentTestInfo()?.setTimeout(0); }, onDidCreateBrowserContext: async (context: BrowserContext) => { - await artifactsRecorder.didCreateBrowserContext(context); - attachConnectedHeaderIfNeeded(testInfo, context.browser()); + await artifactsRecorder?.didCreateBrowserContext(context); + const testInfo = currentTestInfo(); + if (testInfo) + attachConnectedHeaderIfNeeded(testInfo, context.browser()); }, onDidCreateRequestContext: async (context: APIRequestContext) => { - await artifactsRecorder.didCreateRequestContext(context); + await artifactsRecorder?.didCreateRequestContext(context); }, onWillCloseBrowserContext: async (context: BrowserContext) => { - await artifactsRecorder.willCloseBrowserContext(context); + await artifactsRecorder?.willCloseBrowserContext(context); }, onWillCloseRequestContext: async (context: APIRequestContext) => { - await artifactsRecorder.willCloseRequestContext(context); + await artifactsRecorder?.willCloseRequestContext(context); }, }; - // 1. Setup instrumentation and process existing contexts. - const instrumentation = (playwright as any)._instrumentation as ClientInstrumentation; - instrumentation.addListener(csiListener); - await artifactsRecorder.testStarted(testInfoImpl); + 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. Cleanup instrumentation. - await artifactsRecorder.testFinished(); - instrumentation.removeListener(csiListener); - }, { auto: 'all-hooks-included', _title: 'trace recording' } as any], + // 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], _contextFactory: [async ({ browser, video, _artifactsDir, _reuseContext }, use, testInfo) => { const testInfoImpl = testInfo as TestInfoImpl; @@ -526,7 +557,6 @@ class ArtifactsRecorder { private _traceOrdinal = 0; private _screenshottedSymbol: symbol; private _startedCollectingArtifacts: symbol; - private _screenshotOnTestFailureBound: () => Promise; constructor(playwright: Playwright, artifactsDir: string, trace: TraceOption, screenshot: ScreenshotOption) { this._playwright = playwright; @@ -538,10 +568,9 @@ class ArtifactsRecorder { this._traceOptions = typeof trace === 'string' ? defaultTraceOptions : { ...defaultTraceOptions, ...trace, mode: undefined }; this._screenshottedSymbol = Symbol('screenshotted'); this._startedCollectingArtifacts = Symbol('startedCollectingArtifacts'); - this._screenshotOnTestFailureBound = this._screenshotOnTestFailure.bind(this); } - async testStarted(testInfo: TestInfoImpl) { + async willStartTest(testInfo: TestInfoImpl) { this._testInfo = testInfo; this._captureTrace = shouldCaptureTrace(this._traceMode, testInfo) && !process.env.PW_TEST_DISABLE_TRACING; @@ -561,10 +590,6 @@ class ArtifactsRecorder { const existingApiRequests: APIRequestContext[] = Array.from((this._playwright.request as any)._contexts as Set); await Promise.all(existingApiRequests.map(c => this.didCreateRequestContext(c))); } - - // Setup "screenshot on failure" callback. - if (this._screenshotMode === 'on' || this._screenshotMode === 'only-on-failure') - testInfo._onTestFailureImmediateCallbacks.set(this._screenshotOnTestFailureBound, 'Screenshot on failure'); } async didCreateBrowserContext(context: BrowserContext) { @@ -594,14 +619,18 @@ class ArtifactsRecorder { await this._stopTracing(tracing, (context as any)[kStartedContextTearDown]); } - async testFinished() { + async didFinishTestFunction() { + if (this._testInfo._isFailure() && (this._screenshotMode === 'on' || this._screenshotMode === 'only-on-failure')) + await this._screenshotOnTestFailure(); + } + + async didFinishTest() { const captureScreenshots = this._screenshotMode === 'on' || (this._screenshotMode === 'only-on-failure' && this._testInfo.status !== this._testInfo.expectedStatus); const leftoverContexts: BrowserContext[] = []; for (const browserType of [this._playwright.chromium, this._playwright.firefox, this._playwright.webkit]) leftoverContexts.push(...(browserType as any)._contexts); const leftoverApiRequests: APIRequestContext[] = Array.from((this._playwright.request as any)._contexts as Set); - this._testInfo._onTestFailureImmediateCallbacks.delete(this._screenshotOnTestFailureBound); // Collect traces/screenshots for remaining contexts. await Promise.all(leftoverContexts.map(async context => { diff --git a/packages/playwright-test/src/worker/testInfo.ts b/packages/playwright-test/src/worker/testInfo.ts index 51f29eb790..786055cfbb 100644 --- a/packages/playwright-test/src/worker/testInfo.ts +++ b/packages/playwright-test/src/worker/testInfo.ts @@ -44,7 +44,6 @@ export class TestInfoImpl implements TestInfo { readonly _startWallTime: number; private _hasHardError: boolean = false; readonly _traceEvents: trace.TraceEvent[] = []; - readonly _onTestFailureImmediateCallbacks = new Map<() => Promise, string>(); // fn -> title _didTimeout = false; _wasInterrupted = false; _lastStepId = 0; diff --git a/packages/playwright-test/src/worker/workerMain.ts b/packages/playwright-test/src/worker/workerMain.ts index d7083a72ae..ce7d9bce55 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 } from '../common/globals'; +import { setCurrentTestInfo, setIsWorkerProcess, currentTestInstrumentation } from '../common/globals'; import { ConfigLoader } from '../common/configLoader'; import type { Suite, TestCase } from '../common/test'; import type { Annotation, FullConfigInternal, FullProjectInternal } from '../common/config'; @@ -325,6 +325,8 @@ 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)) @@ -395,18 +397,11 @@ export class WorkerMain extends ProcessRunner { // Note: do not wrap all teardown steps together, because failure in any of them // does not prevent further teardown steps from running. - // Run "immediately upon test failure" callbacks. - if (testInfo._isFailure()) { - const onFailureError = await testInfo._runAndFailOnError(async () => { - testInfo._timeoutManager.setCurrentRunnable({ type: 'test', slot: afterHooksSlot }); - for (const [fn, title] of testInfo._onTestFailureImmediateCallbacks) { - debugTest(`on-failure callback started`); - await testInfo._runAsStep({ category: 'hook', title }, fn); - debugTest(`on-failure callback finished`); - } - }); - firstAfterHooksError = firstAfterHooksError || onFailureError; - } + // Run "immediately upon test function finish" callback. + debugTest(`on-test-function-finish callback started`); + const didFinishTestFunctionError = await testInfo._runAndFailOnError(async () => await currentTestInstrumentation()?.didFinishTestFunction(testInfo)); + firstAfterHooksError = firstAfterHooksError || didFinishTestFunctionError; + debugTest(`on-test-function-finish callback finished`); // Run "afterEach" hooks, unless we failed at beforeAll stage. if (shouldRunAfterEachHooks) { @@ -463,6 +458,10 @@ export class WorkerMain extends ProcessRunner { firstAfterHooksError = firstAfterHooksError || workerScopeError; }); } + + const didRunTestError = await testInfo._runAndFailOnError(async () => await currentTestInstrumentation()?.didFinishTest(testInfo)); + firstAfterHooksError = firstAfterHooksError || didRunTestError; + if (firstAfterHooksError) step.complete({ error: firstAfterHooksError }); }); diff --git a/tests/playwright-test/reporter.spec.ts b/tests/playwright-test/reporter.spec.ts index 58f6374751..e7260d3c0e 100644 --- a/tests/playwright-test/reporter.spec.ts +++ b/tests/playwright-test/reporter.spec.ts @@ -424,7 +424,9 @@ test('should report api step failure', async ({ runInlineTest }) => { `begin {\"title\":\"After Hooks\",\"category\":\"hook\"}`, `begin {\"title\":\"browserContext.close\",\"category\":\"pw:api\"}`, `end {\"title\":\"browserContext.close\",\"category\":\"pw:api\"}`, - `end {\"title\":\"After Hooks\",\"category\":\"hook\",\"steps\":[{\"title\":\"browserContext.close\",\"category\":\"pw:api\"}]}`, + `begin {\"title\":\"browser.close\",\"category\":\"pw:api\"}`, + `end {\"title\":\"browser.close\",\"category\":\"pw:api\"}`, + `end {\"title\":\"After Hooks\",\"category\":\"hook\",\"steps\":[{\"title\":\"browserContext.close\",\"category\":\"pw:api\"},{\"title\":\"browser.close\",\"category\":\"pw:api\"}]}`, ]); }); diff --git a/tests/playwright-test/test-step.spec.ts b/tests/playwright-test/test-step.spec.ts index fef4925516..4552862c09 100644 --- a/tests/playwright-test/test-step.spec.ts +++ b/tests/playwright-test/test-step.spec.ts @@ -281,6 +281,10 @@ test('should not report nested after hooks', async ({ runInlineTest }) => { category: 'pw:api', title: 'browserContext.close', }, + { + category: 'pw:api', + title: 'browser.close', + }, ], }, ]);