chore: make _setupArtifacts a worker-scoped fixture (#22739)

This should unblock having separate test-fixture scopes for hooks and
test.

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<void>;
+  didFinishTestFunction(testInfo: TestInfoImpl): Promise<void>;
+  didFinishTest(testInfo: TestInfoImpl): Promise<void>;
+}
+let testInstrumentation: TestInstrumentation | undefined;
+export function setCurrentTestInstrumentation(instrumentation: TestInstrumentation | undefined) {
+  testInstrumentation = instrumentation;
+}
+export function currentTestInstrumentation() {
+  return testInstrumentation;
+}

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<BrowserContext>;
 };
 type WorkerFixtures = PlaywrightWorkerArgs & PlaywrightWorkerOptions & {
   _browserOptions: LaunchOptions;
   _artifactsDir: () => string;
+  _setupArtifacts: void;
   _snapshotSuffix: string;
 };
 
@@ -264,15 +265,15 @@ const playwrightFixtures: Fixtures<TestFixtures, WorkerFixtures> = ({
     }
   }, { auto: 'all-hooks-included',  _title: 'context configuration' } as any],
 
-  _setupArtifacts: [async ({ playwright, _artifactsDir, trace, screenshot }, use, testInfo) => {
+  _setupArtifacts: [async ({ playwright, _artifactsDir, trace, screenshot }, use) => {
-    const artifactsRecorder = new ArtifactsRecorder(playwright, _artifactsDir(), trace, screenshot);
+    let artifactsRecorder: ArtifactsRecorder | undefined;
-    const testInfoImpl = testInfo as TestInfoImpl;
 
     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<TestFixtures, WorkerFixtures> = ({
         step?.complete({ error });
       },
       onWillPause: () => {
-        testInfo.setTimeout(0);
+        currentTestInfo()?.setTimeout(0);
       },
       onDidCreateBrowserContext: async (context: BrowserContext) => {
-        await artifactsRecorder.didCreateBrowserContext(context);
+        await artifactsRecorder?.didCreateBrowserContext(context);
-        attachConnectedHeaderIfNeeded(testInfo, context.browser());
+        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 willStartTest = async (testInfo: TestInfoImpl) => {
-    const instrumentation = (playwright as any)._instrumentation as ClientInstrumentation;
+      artifactsRecorder = new ArtifactsRecorder(playwright, _artifactsDir(), trace, screenshot);
-    instrumentation.addListener(csiListener);
+      await artifactsRecorder.willStartTest(testInfo);
-    await artifactsRecorder.testStarted(testInfoImpl);
+    };
+
+    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.
+    // 3. Teardown for the last test in the worker.
-    await artifactsRecorder.testFinished();
+    {
-    instrumentation.removeListener(csiListener);
+      const lastTestInfo = currentTestInfo();
-  }, { auto: 'all-hooks-included',  _title: 'trace recording' } as any],
+      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<void>;
 
   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<APIRequestContext>);
       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<APIRequestContext>);
-    this._testInfo._onTestFailureImmediateCallbacks.delete(this._screenshotOnTestFailureBound);
 
     // Collect traces/screenshots for remaining contexts.
     await Promise.all(leftoverContexts.map(async context => {

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<void>, string>(); // fn -> title
   _didTimeout = false;
   _wasInterrupted = false;
   _lastStepId = 0;

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.
+        // Run "immediately upon test function finish" callback.
-        if (testInfo._isFailure()) {
+        debugTest(`on-test-function-finish callback started`);
-          const onFailureError = await testInfo._runAndFailOnError(async () => {
+        const didFinishTestFunctionError = await testInfo._runAndFailOnError(async () => await currentTestInstrumentation()?.didFinishTestFunction(testInfo));
-            testInfo._timeoutManager.setCurrentRunnable({ type: 'test', slot: afterHooksSlot });
+        firstAfterHooksError = firstAfterHooksError || didFinishTestFunctionError;
-            for (const [fn, title] of testInfo._onTestFailureImmediateCallbacks) {
+        debugTest(`on-test-function-finish callback finished`);
-              debugTest(`on-failure callback started`);
-              await testInfo._runAsStep({ category: 'hook', title }, fn);
-              debugTest(`on-failure callback finished`);
-            }
-          });
-          firstAfterHooksError = firstAfterHooksError || onFailureError;
-        }
 
         // 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 });
     });

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\"}]}`,
   ]);
 });
 

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',
+        },
       ],
     },
   ]);