chore: bring back per test artifacts (#23153)

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) {

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<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,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<BrowserContext>;
 };
 type WorkerFixtures = PlaywrightWorkerArgs & PlaywrightWorkerOptions & {
   _browserOptions: LaunchOptions;
   _artifactsDir: () => string;
-  _setupArtifacts: void;
   _snapshotSuffix: string;
 };
 
@@ -269,9 +269,9 @@ const playwrightFixtures: Fixtures<TestFixtures, WorkerFixtures> = ({
     }
   }, { auto: 'all-hooks-included',  _title: 'context configuration' } as any],
 
-  _setupArtifacts: [async ({ playwright, _artifactsDir, trace, screenshot }, use) => {
+  _setupArtifacts: [async ({ playwright, _artifactsDir, trace, screenshot }, use, testInfo) => {
-    let artifactsRecorder: ArtifactsRecorder | undefined;
+    const artifactsRecorder = new ArtifactsRecorder(playwright, _artifactsDir(), trace, screenshot);
-
+    await artifactsRecorder.willStartTest(testInfo as TestInfoImpl);
     const csiListener: ClientInstrumentationListener = {
       onApiCallBegin: (apiName: string, params: Record<string, any>, stackTrace: ParsedStackTrace | null, wallTime: number, userData: any) => {
         const testInfo = currentTestInfo();
@@ -312,46 +312,15 @@ const playwrightFixtures: Fixtures<TestFixtures, WorkerFixtures> = ({
       },
     };
 
-    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)

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<void>) | undefined;
 
   // ------------ TestInfo fields ------------
   readonly testId: string;

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));

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));

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',

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);
 });

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);

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+\/one.html[\d.]+m?s/,
-    /page.gotohttp:\/\/localhost:\d+\/two.html/,
+    /page.gotohttp:\/\/localhost:\d+\/two.html[\d.]+m?s/,
     /After Hooks[\d.]+m?s/,
     /fixture: page[\d.]+m?s/,
     /fixture: context[\d.]+m?s/,

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