chore(blob): zip output directory by default (#24536)

Changed the logic to add attachment to the zip in onEnd rather than
onTestEnd because attachment files can be deleted if e.g. preserveOutput
option is specified. Instead we add files once all workers have been
shut down. On a simple run with 1000 tests each adding 1Mb attachment
the overall time difference is 49s (streaming attachments) v 1m9s
(attachments added in the end).

packages/playwright-test/src/reporters/blob.ts

@@ -37,13 +37,10 @@ export type BlobReportMetadata = {
 };
 
 export class BlobReporter extends TeleReporterEmitter {
-  private _messages: JsonEvent[] = [];
-  private _options: BlobReporterOptions;
-  private _salt: string;
-  private _copyFilePromises = new Set<Promise<void>>();
-
-  private _outputDir!: Promise<string>;
-  private _reportName!: string;
+  private readonly _messages: JsonEvent[] = [];
+  private readonly _attachments: { originalPath: string, zipEntryPath: string }[] = [];
+  private readonly _options: BlobReporterOptions;
+  private readonly _salt: string;
 
   constructor(options: BlobReporterOptions) {
     super(message => this._messages.push(message), false);
@@ -52,10 +49,6 @@ export class BlobReporter extends TeleReporterEmitter {
   }
 
   override onConfigure(config: FullConfig) {
-    const outputDir = resolveReporterOutputPath('blob-report', this._options.configDir, this._options.outputDir);
-    const removePromise = process.env.PWTEST_BLOB_DO_NOT_REMOVE ? Promise.resolve() : removeFolders([outputDir]);
-    this._outputDir = removePromise.then(() => fs.promises.mkdir(path.join(outputDir, 'resources'), { recursive: true })).then(() => outputDir);
-    this._reportName = `report-${createGuid()}`;
     const metadata: BlobReportMetadata = {
       projectSuffix: process.env.PWTEST_BLOB_SUFFIX,
       shard: config.shard ? config.shard : undefined,
@@ -64,55 +57,59 @@ export class BlobReporter extends TeleReporterEmitter {
       method: 'onBlobReportMetadata',
       params: metadata
     });
+
     super.onConfigure(config);
   }
 
   override async onEnd(result: FullResult): Promise<void> {
     await super.onEnd(result);
-    const outputDir = await this._outputDir;
-    const lines = this._messages.map(m => JSON.stringify(m) + '\n');
-    const content = Readable.from(lines);
+
+    const outputDir = resolveReporterOutputPath('blob-report', this._options.configDir, this._options.outputDir);
+    if (!process.env.PWTEST_BLOB_DO_NOT_REMOVE)
+      await removeFolders([outputDir]);
+    await fs.promises.mkdir(outputDir, { recursive: true });
+
+    const reportName = `report-${createGuid()}`;
 
     const zipFile = new yazl.ZipFile();
     const zipFinishPromise = new ManualPromise<undefined>();
+    const finishPromise = zipFinishPromise.catch(e => {
+      throw new Error(`Failed to write report ${reportName + '.zip'}: ` + e.message);
+    });
+
     (zipFile as any as EventEmitter).on('error', error => zipFinishPromise.reject(error));
-    const zipFileName = path.join(outputDir, this._reportName + '.zip');
+    const zipFileName = path.join(outputDir, reportName + '.zip');
     zipFile.outputStream.pipe(fs.createWriteStream(zipFileName)).on('close', () => {
       zipFinishPromise.resolve(undefined);
     }).on('error', error => zipFinishPromise.reject(error));
-    zipFile.addReadStream(content, this._reportName + '.jsonl');
+
+    for (const { originalPath, zipEntryPath } of this._attachments) {
+      if (!fs.statSync(originalPath, { throwIfNoEntry: false })?.isFile())
+        continue;
+      zipFile.addFile(originalPath, zipEntryPath);
+    }
+
+    const lines = this._messages.map(m => JSON.stringify(m) + '\n');
+    const content = Readable.from(lines);
+    zipFile.addReadStream(content, reportName + '.jsonl');
     zipFile.end();
 
-    await Promise.all([
-      ...this._copyFilePromises,
-      // Requires Node v14.18.0+
-      zipFinishPromise.catch(e => {
-        throw new Error(`Failed to write report ${zipFileName}: ` + e.message);
-      }),
-    ]);
+    await finishPromise;
   }
 
   override _serializeAttachments(attachments: TestResult['attachments']): JsonAttachment[] {
     return super._serializeAttachments(attachments).map(attachment => {
-      if (!attachment.path || !fs.statSync(attachment.path, { throwIfNoEntry: false })?.isFile())
+      if (!attachment.path)
         return attachment;
       // Add run guid to avoid clashes between shards.
       const sha1 = calculateSha1(attachment.path + this._salt);
       const extension = mime.getExtension(attachment.contentType) || 'dat';
       const newPath = `resources/${sha1}.${extension}`;
-      this._startCopyingFile(attachment.path, newPath);
+      this._attachments.push({ originalPath: attachment.path, zipEntryPath: newPath });
       return {
         ...attachment,
         path: newPath,
       };
     });
   }
-
-  private _startCopyingFile(from: string, to: string) {
-    const copyPromise: Promise<void> = this._outputDir
-        .then(dir => fs.promises.copyFile(from, path.join(dir, to)))
-        .catch(e => { console.error(`Failed to copy file from "${from}" to "${to}": ${e}`); })
-        .then(() => { this._copyFilePromises.delete(copyPromise); });
-    this._copyFilePromises.add(copyPromise);
-  }
 }

packages/playwright-test/src/reporters/merge.ts

@@ -57,18 +57,28 @@ function parseEvents(reportJsonl: Buffer): JsonEvent[] {
   return reportJsonl.toString().split('\n').filter(line => line.length).map(line => JSON.parse(line)) as JsonEvent[];
 }
 
-async function extractReportFromZip(file: string): Promise<Buffer> {
-  const zipFile = new ZipFile(file);
-  const entryNames = await zipFile.entries();
-  try {
+async function extractAndParseReports(dir: string, shardFiles: string[]): Promise<{ metadata: BlobReportMetadata, parsedEvents: JsonEvent[] }[]> {
+  const shardEvents = [];
+  await fs.promises.mkdir(path.join(dir, 'resources'), { recursive: true });
+  for (const file of shardFiles) {
+    const zipFile = new ZipFile(path.join(dir, file));
+    const entryNames = await zipFile.entries();
     for (const entryName of entryNames) {
-      if (entryName.endsWith('.jsonl'))
-        return await zipFile.read(entryName);
+      const content = await zipFile.read(entryName);
+      if (entryName.endsWith('.jsonl')) {
+        const parsedEvents = parseEvents(content);
+        shardEvents.push({
+          metadata: findMetadata(parsedEvents, file),
+          parsedEvents
+        });
+      } else {
+        const fileName = path.join(dir, entryName);
+        await fs.promises.writeFile(fileName, content);
+      }
     }
-  } finally {
     zipFile.close();
   }
-  throw new Error(`Cannot find *.jsonl file in ${file}`);
+  return shardEvents;
 }
 
 function findMetadata(events: JsonEvent[], file: string): BlobReportMetadata {
@@ -82,15 +92,7 @@ async function mergeEvents(dir: string, shardReportFiles: string[]) {
   const configureEvents: JsonEvent[] = [];
   const beginEvents: JsonEvent[] = [];
   const endEvents: JsonEvent[] = [];
-  const shardEvents: { metadata: BlobReportMetadata, parsedEvents: JsonEvent[] }[] = [];
-  for (const reportFile of shardReportFiles) {
-    const reportJsonl = await extractReportFromZip(path.join(dir, reportFile));
-    const parsedEvents = parseEvents(reportJsonl);
-    shardEvents.push({
-      metadata: findMetadata(parsedEvents, reportFile),
-      parsedEvents
-    });
-  }
+  const shardEvents = await extractAndParseReports(dir, shardReportFiles);
   shardEvents.sort((a, b) => {
     const shardA = a.metadata.shard?.current ?? 0;
     const shardB = b.metadata.shard?.current ?? 0;

packages/playwright-test/src/reporters/teleEmitter.ts

@@ -17,9 +17,9 @@
 import path from 'path';
 import { createGuid } from 'playwright-core/lib/utils';
 import type { SuitePrivate } from '../../types/reporterPrivate';
-import type { FullConfig, FullResult, Location, TestError, TestResult, TestStep } from '../../types/testReporter';
+import type { FullConfig, FullResult, Location, TestCase, TestError, TestResult, TestStep } from '../../types/testReporter';
 import { FullConfigInternal, FullProjectInternal } from '../common/config';
-import type { Suite, TestCase } from '../common/test';
+import type { Suite } from '../common/test';
 import type { JsonAttachment, JsonConfig, JsonEvent, JsonProject, JsonStdIOType, JsonSuite, JsonTestCase, JsonTestEnd, JsonTestResultEnd, JsonTestResultStart, JsonTestStepEnd, JsonTestStepStart } from '../isomorphic/teleReceiver';
 import { serializeRegexPatterns } from '../isomorphic/teleReceiver';
 import type { ReporterV2 } from './reporterV2';

tests/playwright-test/reporter-blob.spec.ts

@@ -130,7 +130,7 @@ test('should call methods in right order', async ({ runInlineTest, mergeReports
   await runInlineTest(files, { shard: `3/3` }, { PWTEST_BLOB_DO_NOT_REMOVE: '1' });
   const reportFiles = await fs.promises.readdir(reportDir);
   reportFiles.sort();
-  expect(reportFiles).toEqual([expect.stringMatching(/report-.*.zip/), expect.stringMatching(/report-.*.zip/), 'resources']);
+  expect(reportFiles).toEqual([expect.stringMatching(/report-.*.zip/), expect.stringMatching(/report-.*.zip/)]);
   const { exitCode, output } = await mergeReports(reportDir, {}, { additionalArgs: ['--reporter', test.info().outputPath('echo-reporter.js')] });
   expect(exitCode).toBe(0);
   const lines = output.split('\n').filter(l => l.trim().length);
@@ -204,7 +204,7 @@ test('should merge into html with dependencies', async ({ runInlineTest, mergeRe
     await runInlineTest(files, { shard: `${i + 1}/${totalShards}` }, { PWTEST_BLOB_DO_NOT_REMOVE: '1' });
   const reportFiles = await fs.promises.readdir(reportDir);
   reportFiles.sort();
-  expect(reportFiles).toEqual([expect.stringMatching(/report-.*.zip/), expect.stringMatching(/report-.*.zip/), expect.stringMatching(/report-.*.zip/), 'resources']);
+  expect(reportFiles).toEqual([expect.stringMatching(/report-.*.zip/), expect.stringMatching(/report-.*.zip/), expect.stringMatching(/report-.*.zip/)]);
   const { exitCode, output } = await mergeReports(reportDir, { 'PW_TEST_HTML_REPORT_OPEN': 'never' }, { additionalArgs: ['--reporter', 'html'] });
   expect(exitCode).toBe(0);
 
@@ -274,7 +274,7 @@ test('be able to merge incomplete shards', async ({ runInlineTest, mergeReports,
 
   const reportFiles = await fs.promises.readdir(reportDir);
   reportFiles.sort();
-  expect(reportFiles).toEqual([expect.stringMatching(/report-.*.zip/), expect.stringMatching(/report-.*.zip/), 'resources']);
+  expect(reportFiles).toEqual([expect.stringMatching(/report-.*.zip/), expect.stringMatching(/report-.*.zip/)]);
   const { exitCode } = await mergeReports(reportDir, { 'PW_TEST_HTML_REPORT_OPEN': 'never' }, { additionalArgs: ['--reporter', 'html'] });
   expect(exitCode).toBe(0);
 
@@ -380,7 +380,7 @@ test('merge into list report by default', async ({ runInlineTest, mergeReports }
     await runInlineTest(files, { shard: `${i + 1}/${totalShards}` }, { PWTEST_BLOB_DO_NOT_REMOVE: '1' });
   const reportFiles = await fs.promises.readdir(reportDir);
   reportFiles.sort();
-  expect(reportFiles).toEqual([expect.stringMatching(/report-.*.zip/), expect.stringMatching(/report-.*.zip/), expect.stringMatching(/report-.*.zip/), 'resources']);
+  expect(reportFiles).toEqual([expect.stringMatching(/report-.*.zip/), expect.stringMatching(/report-.*.zip/), expect.stringMatching(/report-.*.zip/)]);
   const { exitCode, output } = await mergeReports(reportDir, { PW_TEST_DEBUG_REPORTERS: '1', PW_TEST_DEBUG_REPORTERS_PRINT_STEPS: '1', PWTEST_TTY_WIDTH: '80' }, { additionalArgs: ['--reporter', 'list'] });
   expect(exitCode).toBe(0);
 
@@ -451,7 +451,7 @@ test('preserve attachments', async ({ runInlineTest, mergeReports, showReport, p
 
   const reportFiles = await fs.promises.readdir(reportDir);
   reportFiles.sort();
-  expect(reportFiles).toEqual([expect.stringMatching(/report-.*.zip/), 'resources']);
+  expect(reportFiles).toEqual([expect.stringMatching(/report-.*.zip/)]);
   const { exitCode } = await mergeReports(reportDir, { 'PW_TEST_HTML_REPORT_OPEN': 'never' }, { additionalArgs: ['--reporter', 'html'] });
   expect(exitCode).toBe(0);
 
@@ -514,7 +514,7 @@ test('generate html with attachment urls', async ({ runInlineTest, mergeReports,
 
   const reportFiles = await fs.promises.readdir(reportDir);
   reportFiles.sort();
-  expect(reportFiles).toEqual([expect.stringMatching(/report-.*.zip/), 'resources']);
+  expect(reportFiles).toEqual([expect.stringMatching(/report-.*.zip/)]);
   const { exitCode } = await mergeReports(reportDir, { 'PW_TEST_HTML_REPORT_OPEN': 'never' }, { additionalArgs: ['--reporter', 'html'] });
   expect(exitCode).toBe(0);
 
@@ -588,7 +588,7 @@ test('resource names should not clash between runs', async ({ runInlineTest, sho
 
   const reportFiles = await fs.promises.readdir(reportDir);
   reportFiles.sort();
-  expect(reportFiles).toEqual([expect.stringMatching(/report-.*.zip/), expect.stringMatching(/report-.*.zip/), 'resources']);
+  expect(reportFiles).toEqual([expect.stringMatching(/report-.*.zip/), expect.stringMatching(/report-.*.zip/)]);
 
   const { exitCode } = await mergeReports(reportDir, {}, { additionalArgs: ['--reporter', 'html'] });
   expect(exitCode).toBe(0);
@@ -663,7 +663,7 @@ test('multiple output reports', async ({ runInlineTest, mergeReports, showReport
 
   const reportFiles = await fs.promises.readdir(reportDir);
   reportFiles.sort();
-  expect(reportFiles).toEqual([expect.stringMatching(/report-.*.zip/), 'resources']);
+  expect(reportFiles).toEqual([expect.stringMatching(/report-.*.zip/)]);
   const { exitCode, output } = await mergeReports(reportDir, { 'PW_TEST_HTML_REPORT_OPEN': 'never' }, { additionalArgs: ['--reporter', 'html,line'] });
   expect(exitCode).toBe(0);
 
@@ -724,7 +724,7 @@ test('multiple output reports based on config', async ({ runInlineTest, mergeRep
 
   const reportFiles = await fs.promises.readdir(reportDir);
   reportFiles.sort();
-  expect(reportFiles).toEqual([expect.stringMatching(/report-.*.zip/), expect.stringMatching(/report-.*.zip/), 'resources']);
+  expect(reportFiles).toEqual([expect.stringMatching(/report-.*.zip/), expect.stringMatching(/report-.*.zip/)]);
   const { exitCode, output } = await mergeReports(reportDir, undefined, { additionalArgs: ['--config', test.info().outputPath('merged/playwright.config.ts')] });
   expect(exitCode).toBe(0);
 
@@ -739,7 +739,7 @@ test('multiple output reports based on config', async ({ runInlineTest, mergeRep
 
   // Check report presence.
   const mergedBlobReportFiles = await fs.promises.readdir(test.info().outputPath('merged/merged-blob'));
-  expect(mergedBlobReportFiles).toEqual([expect.stringMatching(/report.*.zip/), 'resources']);
+  expect(mergedBlobReportFiles).toEqual([expect.stringMatching(/report.*.zip/)]);
 });
 
 test('onError in the report', async ({ runInlineTest, mergeReports, showReport, page }) => {
@@ -870,7 +870,7 @@ test('preserve config fields', async ({ runInlineTest, mergeReports }) => {
 
   const reportFiles = await fs.promises.readdir(reportDir);
   reportFiles.sort();
-  expect(reportFiles).toEqual([expect.stringMatching(/report-.*.zip/), expect.stringMatching(/report-.*.zip/), 'resources']);
+  expect(reportFiles).toEqual([expect.stringMatching(/report-.*.zip/), expect.stringMatching(/report-.*.zip/)]);
   const { exitCode } = await mergeReports(reportDir, {}, { additionalArgs: ['--reporter', test.info().outputPath('echo-reporter.js'), '-c', test.info().outputPath('merge.config.ts')] });
   expect(exitCode).toBe(0);
   const json = JSON.parse(fs.readFileSync(test.info().outputPath('config.json')).toString());
@@ -1027,7 +1027,7 @@ test('preserve steps in html report', async ({ runInlineTest, mergeReports, show
   await runInlineTest(files);
   const reportFiles = await fs.promises.readdir(reportDir);
   reportFiles.sort();
-  expect(reportFiles).toEqual([expect.stringMatching(/report-.*.zip/), 'resources']);
+  expect(reportFiles).toEqual([expect.stringMatching(/report-.*.zip/)]);
   // Run merger in a different directory to make sure relative paths will not be resolved
   // relative to the current directory.
   const mergeCwd = test.info().outputPath('foo');