fix(test runner): make TestTracing responsible for trace management (#29181)

... instead of a fixture.

Fixes #29133, fixes #28733, fixes #28476.

packages/playwright/src/index.ts

@@ -19,13 +19,12 @@ import * as path from 'path';
 import type { APIRequestContext, BrowserContext, Browser, BrowserContextOptions, LaunchOptions, Page, Tracing, Video } from 'playwright-core';
 import * as playwrightLibrary from 'playwright-core';
 import { createGuid, debugMode, addInternalStackPrefix, isString, asLocator, jsonStringifyForceASCII } from 'playwright-core/lib/utils';
-import type { Fixtures, PlaywrightTestArgs, PlaywrightTestOptions, PlaywrightWorkerArgs, PlaywrightWorkerOptions, ScreenshotMode, TestInfo, TestType, TraceMode, VideoMode } from '../types/test';
+import type { Fixtures, PlaywrightTestArgs, PlaywrightTestOptions, PlaywrightWorkerArgs, PlaywrightWorkerOptions, ScreenshotMode, TestInfo, TestType, VideoMode } from '../types/test';
 import type { TestInfoImpl } from './worker/testInfo';
 import { rootTestType } from './common/testType';
 import type { ContextReuseMode } from './common/config';
 import type { ClientInstrumentation, ClientInstrumentationListener } from '../../playwright-core/src/client/clientInstrumentation';
 import { currentTestInfo } from './common/globals';
-import { mergeTraceFiles } from './worker/testTracing';
 export { expect } from './matchers/expect';
 export { store as _store } from './store';
 export const _baseTest: TestType<{}, {}> = rootTestType.test;
@@ -54,7 +53,6 @@ type TestFixtures = PlaywrightTestArgs & PlaywrightTestOptions & {
 };
 type WorkerFixtures = PlaywrightWorkerArgs & PlaywrightWorkerOptions & {
   _browserOptions: LaunchOptions;
-  _artifactsDir: string;
 };
 
 const playwrightFixtures: Fixtures<TestFixtures, WorkerFixtures> = ({
@@ -73,11 +71,7 @@ const playwrightFixtures: Fixtures<TestFixtures, WorkerFixtures> = ({
   video: ['off', { scope: 'worker', option: true }],
   trace: ['off', { scope: 'worker', option: true }],
 
-  _artifactsDir: [async ({}, use) => {
+  _browserOptions: [async ({ playwright, headless, channel, launchOptions }, use) => {
-    await use(process.env.TEST_ARTIFACTS_DIR!);
-  }, { scope: 'worker', _title: 'playwright configuration' } as any],
-
-  _browserOptions: [async ({ playwright, headless, channel, launchOptions, _artifactsDir }, use) => {
     const options: LaunchOptions = {
       handleSIGINT: false,
       ...launchOptions,
@@ -86,7 +80,7 @@ const playwrightFixtures: Fixtures<TestFixtures, WorkerFixtures> = ({
       options.headless = headless;
     if (channel !== undefined)
       options.channel = channel;
-    options.tracesDir = path.join(_artifactsDir, 'traces');
+    options.tracesDir = tracing().tracesDir();
 
     for (const browserType of [playwright.chromium, playwright.firefox, playwright.webkit])
       (browserType as any)._defaultLaunchOptions = options;
@@ -225,7 +219,7 @@ const playwrightFixtures: Fixtures<TestFixtures, WorkerFixtures> = ({
     });
   },
 
-  _setupContextOptions: [async ({ playwright, _combinedContextOptions, _artifactsDir, actionTimeout, navigationTimeout, testIdAttribute }, use, testInfo) => {
+  _setupContextOptions: [async ({ playwright, _combinedContextOptions, actionTimeout, navigationTimeout, testIdAttribute }, use, testInfo) => {
     if (testIdAttribute)
       playwrightLibrary.selectors.setTestIdAttribute(testIdAttribute);
     testInfo.snapshotSuffix = process.platform;
@@ -237,7 +231,7 @@ const playwrightFixtures: Fixtures<TestFixtures, WorkerFixtures> = ({
       (browserType as any)._defaultContextNavigationTimeout = navigationTimeout || 0;
     }
     (playwright.request as any)._defaultContextOptions = { ..._combinedContextOptions };
-    (playwright.request as any)._defaultContextOptions.tracesDir = path.join(_artifactsDir, 'traces');
+    (playwright.request as any)._defaultContextOptions.tracesDir = tracing().tracesDir();
     (playwright.request as any)._defaultContextOptions.timeout = actionTimeout || 0;
     await use();
     (playwright.request as any)._defaultContextOptions = undefined;
@@ -248,8 +242,8 @@ 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, screenshot }, use, testInfo) => {
-    const artifactsRecorder = new ArtifactsRecorder(playwright, _artifactsDir, trace, screenshot);
+    const artifactsRecorder = new ArtifactsRecorder(playwright, tracing().artifactsDir(), screenshot);
     await artifactsRecorder.willStartTest(testInfo as TestInfoImpl);
     const csiListener: ClientInstrumentationListener = {
       onApiCallBegin: (apiName: string, params: Record<string, any>, frames: StackFrame[], wallTime: number, userData: any) => {
@@ -297,11 +291,11 @@ const playwrightFixtures: Fixtures<TestFixtures, WorkerFixtures> = ({
     await use();
 
     clientInstrumentation.removeListener(csiListener);
-    await artifactsRecorder?.didFinishTest();
+    await artifactsRecorder.didFinishTest();
 
   }, { auto: 'all-hooks-included',  _title: 'trace recording' } as any],
 
-  _contextFactory: [async ({ browser, video, _artifactsDir, _reuseContext }, use, testInfo) => {
+  _contextFactory: [async ({ browser, video, _reuseContext }, use, testInfo) => {
     const testInfoImpl = testInfo as TestInfoImpl;
     const videoMode = normalizeVideoMode(video);
     const captureVideo = shouldCaptureVideo(videoMode, testInfo) && !_reuseContext;
@@ -318,7 +312,7 @@ const playwrightFixtures: Fixtures<TestFixtures, WorkerFixtures> = ({
       }
       const videoOptions: BrowserContextOptions = captureVideo ? {
         recordVideo: {
-          dir: _artifactsDir,
+          dir: tracing().artifactsDir(),
           size: typeof video === 'string' ? undefined : video.size,
         }
       } : {};
@@ -333,7 +327,6 @@ const playwrightFixtures: Fixtures<TestFixtures, WorkerFixtures> = ({
     let counter = 0;
     const closeReason = testInfo.status === 'timedOut' ? 'Test timeout of ' + testInfo.timeout + 'ms exceeded.' : 'Test ended.';
     await Promise.all([...contexts.keys()].map(async context => {
-      (context as any)[kStartedContextTearDown] = true;
       await (context as any)._wrapApiCall(async () => {
         await context.close({ reason: closeReason });
       }, true);
@@ -395,7 +388,6 @@ const playwrightFixtures: Fixtures<TestFixtures, WorkerFixtures> = ({
   request: async ({ playwright }, use) => {
     const request = await playwright.request.newContext();
     await use(request);
-    (request as any)[kStartedContextTearDown] = true;
     await request.dispose();
   },
 });
@@ -414,7 +406,6 @@ type StackFrame = {
 };
 
 type ScreenshotOption = PlaywrightWorkerOptions['screenshot'] | undefined;
-type TraceOption =  PlaywrightWorkerOptions['trace'] | undefined;
 type Playwright = PlaywrightWorkerArgs['playwright'];
 
 function normalizeVideoMode(video: VideoMode | 'retry-with-video' | { mode: VideoMode } | undefined): VideoMode {
@@ -430,19 +421,6 @@ function shouldCaptureVideo(videoMode: VideoMode, testInfo: TestInfo) {
   return (videoMode === 'on' || videoMode === 'retain-on-failure' || (videoMode === 'on-first-retry' && testInfo.retry === 1));
 }
 
-function normalizeTraceMode(trace: TraceOption): TraceMode {
-  if (!trace)
-    return 'off';
-  let traceMode = typeof trace === 'string' ? trace : trace.mode;
-  if (traceMode === 'retry-with-trace')
-    traceMode = 'on-first-retry';
-  return traceMode;
-}
-
-function shouldCaptureTrace(traceMode: TraceMode, testInfo: TestInfo) {
-  return traceMode === 'on' || traceMode === 'retain-on-failure' || (traceMode === 'on-first-retry' && testInfo.retry === 1) || (traceMode === 'on-all-retries' && testInfo.retry > 0);
-}
-
 function normalizeScreenshotMode(screenshot: ScreenshotOption): ScreenshotMode {
   if (!screenshot)
     return 'off';
@@ -467,8 +445,6 @@ function attachConnectedHeaderIfNeeded(testInfo: TestInfo, browser: Browser | nu
 
 const kTracingStarted = Symbol('kTracingStarted');
 const kIsReusedContext = Symbol('kReusedContext');
-const kStartedContextTearDown = Symbol('kStartedContextTearDown');
-let traceOrdinal = 0;
 
 function connectOptionsFromEnv() {
   const wsEndpoint = process.env.PW_TEST_CONNECT_WS_ENDPOINT;
@@ -487,11 +463,7 @@ class ArtifactsRecorder {
   private _playwright: Playwright;
   private _artifactsDir: string;
   private _screenshotMode: ScreenshotMode;
-  private _traceMode: TraceMode;
-  private _captureTrace = false;
   private _screenshotOptions: { mode: ScreenshotMode } & Pick<playwrightLibrary.PageScreenshotOptions, 'fullPage' | 'omitBackground'> | undefined;
-  private _traceOptions: { screenshots: boolean, snapshots: boolean, sources: boolean, attachments: boolean, _live: boolean, mode?: TraceMode };
-  private _temporaryTraceFiles: string[] = [];
   private _temporaryScreenshots: string[] = [];
   private _temporaryArtifacts: string[] = [];
   private _reusedContexts = new Set<BrowserContext>();
@@ -499,14 +471,11 @@ class ArtifactsRecorder {
   private _screenshottedSymbol: symbol;
   private _startedCollectingArtifacts: symbol;
 
-  constructor(playwright: Playwright, artifactsDir: string, trace: TraceOption, screenshot: ScreenshotOption) {
+  constructor(playwright: Playwright, artifactsDir: string, screenshot: ScreenshotOption) {
     this._playwright = playwright;
     this._artifactsDir = artifactsDir;
     this._screenshotMode = normalizeScreenshotMode(screenshot);
     this._screenshotOptions = typeof screenshot === 'string' ? undefined : screenshot;
-    this._traceMode = normalizeTraceMode(trace);
-    const defaultTraceOptions = { screenshots: true, snapshots: true, sources: true, attachments: true, _live: false };
-    this._traceOptions = typeof trace === 'string' ? defaultTraceOptions : { ...defaultTraceOptions, ...trace, mode: undefined };
     this._screenshottedSymbol = Symbol('screenshotted');
     this._startedCollectingArtifacts = Symbol('startedCollectingArtifacts');
   }
@@ -520,9 +489,6 @@ 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;
-    if (this._captureTrace)
-      this._testInfo._tracing.start(this._createTemporaryArtifact('traces', `${this._testInfo.testId}-test.trace`), this._traceOptions);
 
     // Since beforeAll(s), test and afterAll(s) reuse the same TestInfo, make sure we do not
     // overwrite previous screenshots.
@@ -555,7 +521,7 @@ class ArtifactsRecorder {
     // Do not record empty traces and useless screenshots for them.
     if (this._reusedContexts.has(context))
       return;
-    await this._stopTracing(context.tracing, (context as any)[kStartedContextTearDown]);
+    await this._stopTracing(context.tracing);
     if (this._screenshotMode === 'on' || this._screenshotMode === 'only-on-failure') {
       // Capture screenshot for now. We'll know whether we have to preserve them
       // after the test finishes.
@@ -570,7 +536,7 @@ class ArtifactsRecorder {
 
   async willCloseRequestContext(context: APIRequestContext) {
     const tracing = (context as any)._tracing as Tracing;
-    await this._stopTracing(tracing, (context as any)[kStartedContextTearDown]);
+    await this._stopTracing(tracing);
   }
 
   async didFinishTestFunction() {
@@ -591,10 +557,10 @@ class ArtifactsRecorder {
 
     // Collect traces/screenshots for remaining contexts.
     await Promise.all(leftoverContexts.map(async context => {
-      await this._stopTracing(context.tracing, true);
+      await this._stopTracing(context.tracing);
     }).concat(leftoverApiRequests.map(async context => {
       const tracing = (context as any)._tracing as Tracing;
-      await this._stopTracing(tracing, true);
+      await this._stopTracing(tracing);
     })));
 
     // Attach temporary screenshots for contexts closed before collecting the test trace.
@@ -608,32 +574,6 @@ class ArtifactsRecorder {
         }
       }
     }
-
-    // Collect test trace.
-    if (this._preserveTrace()) {
-      const tracePath = this._createTemporaryArtifact(createGuid() + '.zip');
-      this._temporaryTraceFiles.push(tracePath);
-      await this._testInfo._tracing.stop(tracePath);
-    }
-
-    // Either remove or attach temporary traces for contexts closed 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);
-      // Do not add attachment twice.
-      if (!beforeHooksHadTrace)
-        this._testInfo.attachments.push({ name: 'trace', path: tracePath, contentType: 'application/zip' });
-    }
-
-    for (const file of this._temporaryArtifacts)
-      await fs.promises.unlink(file).catch(() => {});
   }
 
   private _createScreenshotAttachmentPath() {
@@ -675,15 +615,12 @@ class ArtifactsRecorder {
   }
 
   private async _startTraceChunkOnContextCreation(tracing: Tracing) {
-    if (this._captureTrace) {
+    const options = this._testInfo._tracing.traceOptions();
-      const title = [path.relative(this._testInfo.project.testDir, this._testInfo.file) + ':' + this._testInfo.line, ...this._testInfo.titlePath.slice(1)].join(' › ');
+    if (options) {
-      const ordinalSuffix = traceOrdinal ? `-context${traceOrdinal}` : '';
+      const title = this._testInfo._tracing.traceTitle();
-      ++traceOrdinal;
+      const name = this._testInfo._tracing.generateNextTraceRecordingName();
-      const retrySuffix = this._testInfo.retry ? `-retry${this._testInfo.retry}` : '';
-      // Note that trace name must start with testId for live tracing to work.
-      const name = `${this._testInfo.testId}${retrySuffix}${ordinalSuffix}`;
       if (!(tracing as any)[kTracingStarted]) {
-        await tracing.start({ ...this._traceOptions, title, name });
+        await tracing.start({ ...options, title, name });
         (tracing as any)[kTracingStarted] = true;
       } else {
         await tracing.startChunk({ title, name });
@@ -696,26 +633,12 @@ class ArtifactsRecorder {
     }
   }
 
-  private _preserveTrace() {
+  private async _stopTracing(tracing: Tracing) {
-    const testFailed = this._testInfo.status !== this._testInfo.expectedStatus;
-    return this._captureTrace && (this._traceMode === 'on' || (testFailed && this._traceMode === 'retain-on-failure') || (this._traceMode === 'on-first-retry' && this._testInfo.retry === 1) || (this._traceMode === 'on-all-retries' && this._testInfo.retry > 0));
-  }
-
-  private async _stopTracing(tracing: Tracing, contextTearDownStarted: boolean) {
     if ((tracing as any)[this._startedCollectingArtifacts])
       return;
     (tracing as any)[this._startedCollectingArtifacts] = true;
-    if (this._captureTrace) {
+    if (this._testInfo._tracing.traceOptions())
-      let tracePath;
+      await tracing.stopChunk({ path: this._testInfo._tracing.generateNextTraceRecordingPath() });
-      // Create a trace file if we know that:
-      // - it is's going to be used due to the config setting and the test status or
-      // - we are inside a test or afterEach and the user manually closed the context.
-      if (this._preserveTrace() || !contextTearDownStarted) {
-        tracePath = this._createTemporaryArtifact(createGuid() + '.zip');
-        this._temporaryTraceFiles.push(tracePath);
-      }
-      await tracing.stopChunk({ path: tracePath });
-    }
   }
 }
 
@@ -743,6 +666,10 @@ function renderApiCall(apiName: string, params: any) {
   return apiName + paramsText;
 }
 
+function tracing() {
+  return (test.info() as TestInfoImpl)._tracing;
+}
+
 export const test = _baseTest.extend<TestFixtures, WorkerFixtures>(playwrightFixtures);
 
 export { defineConfig } from './common/configLoader';

packages/playwright/src/worker/testInfo.ts

@@ -57,7 +57,7 @@ export class TestInfoImpl implements TestInfo {
   readonly _startTime: number;
   readonly _startWallTime: number;
   private _hasHardError: boolean = false;
-  readonly _tracing = new TestTracing();
+  readonly _tracing: TestTracing;
 
   _didTimeout = false;
   _wasInterrupted = false;
@@ -187,6 +187,8 @@ export class TestInfoImpl implements TestInfo {
         this._attach(a.name, a);
       return this.attachments.length;
     };
+
+    this._tracing = new TestTracing(this, workerParams.artifactsDir);
   }
 
   private _modifier(type: 'skip' | 'fail' | 'fixme' | 'slow', modifierArgs: [arg?: any, description?: string]) {
@@ -223,6 +225,7 @@ export class TestInfoImpl implements TestInfo {
     if (!this._wasInterrupted && timeoutError && !this._didTimeout) {
       this._didTimeout = true;
       this.errors.push(timeoutError);
+      this._tracing.appendForError(timeoutError);
       // Do not overwrite existing failure upon hook/teardown timeout.
       if (this.status === 'passed' || this.status === 'skipped')
         this.status = 'timedOut';
@@ -359,6 +362,7 @@ export class TestInfoImpl implements TestInfo {
     if (step && step.boxedStack)
       serialized.stack = `${error.name}: ${error.message}\n${stringifyStackFrames(step.boxedStack).join('\n')}`;
     this.errors.push(serialized);
+    this._tracing.appendForError(serialized);
   }
 
   async _runAsStepWithRunnable<T>(

packages/playwright/src/worker/testTracing.ts

@@ -19,31 +19,104 @@ import type * as trace from '@trace/trace';
 import type EventEmitter from 'events';
 import fs from 'fs';
 import path from 'path';
-import { ManualPromise, calculateSha1, monotonicTime } from 'playwright-core/lib/utils';
+import { ManualPromise, calculateSha1, monotonicTime, createGuid } from 'playwright-core/lib/utils';
 import { yauzl, yazl } from 'playwright-core/lib/zipBundle';
 import type { TestInfo, TestInfoError } from '../../types/test';
 import { filteredStackTrace } from '../util';
-
+import type { TraceMode, PlaywrightWorkerOptions } from '../../types/test';
+import type { TestInfoImpl } from './testInfo';
 
 export type Attachment = TestInfo['attachments'][0];
 export const testTraceEntryName = 'test.trace';
+let traceOrdinal = 0;
+
+type TraceFixtureValue =  PlaywrightWorkerOptions['trace'] | undefined;
+type TraceOptions = { screenshots: boolean, snapshots: boolean, sources: boolean, attachments: boolean, _live: boolean, mode: TraceMode };
 
 export class TestTracing {
+  private _testInfo: TestInfoImpl;
+  private _options: TraceOptions | undefined;
   private _liveTraceFile: string | undefined;
   private _traceEvents: trace.TraceEvent[] = [];
-  private _options: { sources: boolean; attachments: boolean; _live: boolean; } | undefined;
+  private _temporaryTraceFiles: string[] = [];
+  private _artifactsDir: string;
+  private _tracesDir: string;
 
-  start(liveFileName: string, options: { sources: boolean, attachments: boolean, _live: boolean }) {
+  constructor(testInfo: TestInfoImpl, artifactsDir: string) {
-    this._options = options;
+    this._testInfo = testInfo;
-    if (!this._liveTraceFile && options._live) {
+    this._artifactsDir = artifactsDir;
-      this._liveTraceFile = liveFileName;
+    this._tracesDir = path.join(this._artifactsDir, 'traces');
-      fs.mkdirSync(path.dirname(this._liveTraceFile), { recursive: true });
+  }
+
+  async startIfNeeded(value: TraceFixtureValue) {
+    const defaultTraceOptions: TraceOptions = { screenshots: true, snapshots: true, sources: true, attachments: true, _live: false, mode: 'off' };
+    if (!value) {
+      this._options = defaultTraceOptions;
+    } else if (typeof value === 'string') {
+      this._options = { ...defaultTraceOptions, mode: value === 'retry-with-trace' ? 'on-first-retry' : value as TraceMode };
+    } else {
+      const mode = value.mode || 'off';
+      this._options = { ...defaultTraceOptions, ...value, mode: (mode as string) === 'retry-with-trace' ? 'on-first-retry' : mode };
+    }
+
+    let shouldCaptureTrace = this._options.mode === 'on' || this._options.mode === 'retain-on-failure' || (this._options.mode === 'on-first-retry' && this._testInfo.retry === 1) || (this._options.mode === 'on-all-retries' && this._testInfo.retry > 0);
+    shouldCaptureTrace = shouldCaptureTrace && !process.env.PW_TEST_DISABLE_TRACING;
+    if (!shouldCaptureTrace) {
+      this._options = undefined;
+      return;
+    }
+
+    if (!this._liveTraceFile && this._options._live) {
+      // Note that trace name must start with testId for live tracing to work.
+      this._liveTraceFile = path.join(this._tracesDir, `${this._testInfo.testId}-test.trace`);
+      await fs.promises.mkdir(path.dirname(this._liveTraceFile), { recursive: true });
       const data = this._traceEvents.map(e => JSON.stringify(e)).join('\n') + '\n';
-      fs.writeFileSync(this._liveTraceFile, data);
+      await fs.promises.writeFile(this._liveTraceFile, data);
     }
   }
 
-  async stop(fileName: string) {
+  artifactsDir() {
+    return this._artifactsDir;
+  }
+
+  tracesDir() {
+    return this._tracesDir;
+  }
+
+  traceTitle() {
+    return [path.relative(this._testInfo.project.testDir, this._testInfo.file) + ':' + this._testInfo.line, ...this._testInfo.titlePath.slice(1)].join(' › ');
+  }
+
+  generateNextTraceRecordingName() {
+    const ordinalSuffix = traceOrdinal ? `-recording${traceOrdinal}` : '';
+    ++traceOrdinal;
+    const retrySuffix = this._testInfo.retry ? `-retry${this._testInfo.retry}` : '';
+    // Note that trace name must start with testId for live tracing to work.
+    return `${this._testInfo.testId}${retrySuffix}${ordinalSuffix}`;
+  }
+
+  generateNextTraceRecordingPath() {
+    const file = path.join(this._artifactsDir, createGuid() + '.zip');
+    this._temporaryTraceFiles.push(file);
+    return file;
+  }
+
+  traceOptions() {
+    return this._options;
+  }
+
+  async stopIfNeeded() {
+    if (!this._options)
+      return;
+
+    const testFailed = this._testInfo.status !== this._testInfo.expectedStatus;
+    const shouldAbandonTrace = !testFailed && this._options.mode === 'retain-on-failure';
+    if (shouldAbandonTrace) {
+      for (const file of this._temporaryTraceFiles)
+        await fs.promises.unlink(file).catch(() => {});
+      return;
+    }
+
     const zipFile = new yazl.ZipFile();
 
     if (!this._options?.attachments) {
@@ -97,9 +170,13 @@ export class TestTracing {
 
     await new Promise(f => {
       zipFile.end(undefined, () => {
-        zipFile.outputStream.pipe(fs.createWriteStream(fileName)).on('close', f);
+        zipFile.outputStream.pipe(fs.createWriteStream(this.generateNextTraceRecordingPath())).on('close', f);
       });
     });
+
+    const tracePath = this._testInfo.outputPath('trace.zip');
+    await mergeTraceFiles(tracePath, this._temporaryTraceFiles);
+    this._testInfo.attachments.push({ name: 'trace', path: tracePath, contentType: 'application/zip' });
   }
 
   appendForError(error: TestInfoError) {
@@ -185,7 +262,7 @@ function generatePreview(value: any, visited = new Set<any>()): string {
   return String(value);
 }
 
-export async function mergeTraceFiles(fileName: string, temporaryTraceFiles: string[]) {
+async function mergeTraceFiles(fileName: string, temporaryTraceFiles: string[]) {
   if (temporaryTraceFiles.length === 1) {
     await fs.promises.rename(temporaryTraceFiles[0], fileName);
     return;

packages/playwright/src/worker/workerMain.ts

@@ -62,7 +62,6 @@ export class WorkerMain extends ProcessRunner {
     super();
     process.env.TEST_WORKER_INDEX = String(params.workerIndex);
     process.env.TEST_PARALLEL_INDEX = String(params.parallelIndex);
-    process.env.TEST_ARTIFACTS_DIR = params.artifactsDir;
     setIsWorkerProcess();
 
     this._params = params;
@@ -313,6 +312,21 @@ export class WorkerMain extends ProcessRunner {
     let shouldRunAfterEachHooks = false;
 
     await testInfo._runWithTimeout(async () => {
+      const traceError = await testInfo._runAndFailOnError(async () => {
+        // Ideally, "trace" would be an config-level option belonging to the
+        // test runner instead of a fixture belonging to Playwright.
+        // However, for backwards compatibility, we have to read it from a fixture today.
+        // We decided to not introduce the config-level option just yet.
+        const traceFixtureRegistration = test._pool!.registrations.get('trace');
+        if (!traceFixtureRegistration)
+          return;
+        if (typeof traceFixtureRegistration.fn === 'function')
+          throw new Error(`"trace" option cannot be a function`);
+        await testInfo._tracing.startIfNeeded(traceFixtureRegistration.fn);
+      });
+      if (traceError)
+        return;
+
       if (this._isStopped || isSkipped) {
         // Two reasons to get here:
         // - Last test is skipped, so we should not run the test, but run the cleanup.
@@ -379,9 +393,6 @@ export class WorkerMain extends ProcessRunner {
         await fn(testFunctionParams, testInfo);
         debugTest(`test function finished`);
       }, 'allowSkips');
-
-      for (const error of testInfo.errors)
-        testInfo._tracing.appendForError(error);
     });
 
     if (didFailBeforeAllForSuite) {
@@ -474,6 +485,10 @@ export class WorkerMain extends ProcessRunner {
         step.complete({ error: firstAfterHooksError });
     });
 
+    await testInfo._runAndFailOnError(async () => {
+      await testInfo._tracing.stopIfNeeded();
+    });
+
     this._currentTest = null;
     setCurrentTestInfo(null);
     this.dispatchEvent('testEnd', buildTestEndPayload(testInfo));

tests/playwright-test/playwright.trace.spec.ts

@@ -326,13 +326,13 @@ test('should not override trace file in afterAll', async ({ runInlineTest, serve
     'After Hooks',
     '  fixture: page',
     '  fixture: context',
-    '  attach \"trace\"',
     '  afterAll hook',
     '    fixture: request',
     '      apiRequest.newContext',
     '    apiRequestContext.get',
     '  fixture: request',
     '    apiRequestContext.dispose',
+    '  fixture: browser',
   ]);
   expect(trace1.errors).toEqual([`'oh no!'`]);
 
@@ -407,18 +407,16 @@ for (const mode of ['off', 'retain-on-failure', 'on-first-retry', 'on-all-retrie
       'a.spec.ts': `
         import { test as base, expect } from '@playwright/test';
         import fs from 'fs';
-        const test = base.extend<{
+        let artifactsDir;
-          locale: string | undefined,
+        const test = base.extend({
-          _artifactsDir: () => string,
+          workerAuto: [async ({}, use) => {
-        }>({
+            await use();
-          // Override locale fixture to check in teardown that no temporary trace zip was created.
+            const entries =  fs.readdirSync(artifactsDir);
-          locale: [async ({ locale, _artifactsDir }, use) => {
-            await use(locale);
-            const entries =  fs.readdirSync(_artifactsDir);
             expect(entries.filter(e => e.endsWith('.zip'))).toEqual([]);
-          }, { option: true }],
+          }, { scope: 'worker', auto: true }],
         });
         test('passing test', async ({ page }) => {
+          artifactsDir = test.info()._tracing.artifactsDir();
           await page.goto('about:blank');
         });
       `,
@@ -432,18 +430,16 @@ for (const mode of ['off', 'retain-on-failure', 'on-first-retry', 'on-all-retrie
       'a.spec.ts': `
         import { test as base, expect } from '@playwright/test';
         import fs from 'fs';
-        const test = base.extend<{
+        let artifactsDir;
-          locale: string | undefined,
+        const test = base.extend({
-          _artifactsDir: () => string,
+          workerAuto: [async ({}, use) => {
-        }>({
+            await use();
-          // Override locale fixture to check in teardown that no temporary trace zip was created.
+            const entries =  fs.readdirSync(artifactsDir);
-          locale: [async ({ locale, _artifactsDir }, use) => {
-            await use(locale);
-            const entries =  fs.readdirSync(_artifactsDir);
             expect(entries.filter(e => e.endsWith('.zip'))).toEqual([]);
-          }, { option: true }],
+          }, { scope: 'worker', auto: true }],
         });
         test('passing test', async ({ request }) => {
+          artifactsDir = test.info()._tracing.artifactsDir();
           expect(await request.get('${server.EMPTY_PAGE}')).toBeOK();
         });
       `,
@@ -671,10 +667,51 @@ test('should show non-expect error in trace', async ({ runInlineTest }, testInfo
     'After Hooks',
     '  fixture: page',
     '  fixture: context',
+    '  fixture: browser',
   ]);
   expect(trace.errors).toEqual(['ReferenceError: undefinedVariable1 is not defined']);
 });
 
+test('should show error from beforeAll in trace', async ({ runInlineTest }, testInfo) => {
+  const result = await runInlineTest({
+    'playwright.config.ts': `
+      module.exports = { use: { trace: { mode: 'on' } } };
+    `,
+    'a.spec.ts': `
+      import { test, expect } from '@playwright/test';
+      test.beforeAll(async () => {
+        throw new Error('Oh my!');
+      })
+      test('fail', async ({ page }) => {
+      });
+    `,
+  }, { workers: 1 });
+
+  expect(result.exitCode).toBe(1);
+  expect(result.failed).toBe(1);
+  const trace = await parseTrace(testInfo.outputPath('test-results', 'a-fail', 'trace.zip'));
+  expect(trace.errors).toEqual(['Error: Oh my!']);
+});
+
+test('should throw when trace fixture is a function', async ({ runInlineTest }, testInfo) => {
+  const result = await runInlineTest({
+    'a.spec.ts': `
+      import { test, expect } from '@playwright/test';
+      test.use({
+        trace: async ({}, use) => {
+          await use('on');
+        },
+      });
+      test('skipped', async ({ page }) => {
+      });
+    `,
+  }, { workers: 1 });
+
+  expect(result.exitCode).toBe(1);
+  expect(result.failed).toBe(1);
+  expect(result.output).toContain('Error: "trace" option cannot be a function');
+});
+
 test('should not throw when attachment is missing', async ({ runInlineTest }, testInfo) => {
   const result = await runInlineTest({
     'playwright.config.ts': `

tests/playwright-test/ui-mode-test-progress.spec.ts

@@ -288,3 +288,63 @@ test('should show live trace for serial', async ({ runUITest, server, createLatc
     /expect.not.toBeCheckedlocator\('input'\)[\d.]/,
   ]);
 });
+
+test('should show live trace from hooks', async ({ runUITest, createLatch }) => {
+  const latch1 = createLatch();
+  const latch2 = createLatch();
+
+  const { page } = await runUITest({
+    'a.test.ts': `
+      import { test, expect } from '@playwright/test';
+      test.beforeAll(async ({ browser }) => {
+        const page = await browser.newPage();
+        ${latch1.blockingCode}
+        await page.close();
+      });
+      test.beforeEach(async ({ browser }) => {
+        const page = await browser.newPage();
+        ${latch2.blockingCode}
+        await page.close();
+      });
+      test('test one', async ({ page }) => {
+        await page.setContent('Page content');
+      });
+    `,
+  });
+
+  await expect.poll(dumpTestTree(page)).toBe(`
+    ▼ ◯ a.test.ts
+        ◯ test one
+  `);
+  await page.getByText('test one').dblclick();
+
+  const listItem = page.getByTestId('actions-tree').getByRole('listitem');
+  await expect(
+      listItem,
+      'action list'
+  ).toHaveText([
+    /Before Hooks/,
+    /beforeAll hook/,
+    /fixture: browser/,
+    /browser.newPage/,
+  ]);
+  latch1.open();
+  await expect(
+      listItem,
+      'action list'
+  ).toHaveText([
+    /Before Hooks/,
+    /beforeAll hook/,
+    /beforeEach hook/,
+    /browser.newPage/,
+  ]);
+  latch2.open();
+  await expect(
+      listItem,
+      'action list'
+  ).toHaveText([
+    /Before Hooks/,
+    /page.setContent/,
+    /After Hooks/,
+  ]);
+});