feat(testinfo): add name to attachment output name (#18440)

Per discussion in #12950, adding sanitized name to the output filename
prefix. This can make debugging easier, and the filename structure more
human friendly.

docs/src/test-api/class-testinfo.md

@@ -104,7 +104,8 @@ after awaiting the attach call.
 * since: v1.10
 - `name` <[string]>
 
-Attachment name.
+Attachment name. The name will also be sanitized and used as the prefix of file name
+when saving to disk.
 
 ### option: TestInfo.attach.body
 * since: v1.10

packages/playwright-test/src/util.ts

@@ -21,7 +21,7 @@ import path from 'path';
 import url from 'url';
 import { colors, debug, minimatch } from 'playwright-core/lib/utilsBundle';
 import type { TestError, Location } from './types';
-import { calculateSha1, isRegExp } from 'playwright-core/lib/utils';
+import { calculateSha1, isRegExp, isString } from 'playwright-core/lib/utils';
 import { isInternalFileName } from 'playwright-core/lib/utils/stackTrace';
 import { currentTestInfo } from './globals';
 import type { ParsedStackTrace } from 'playwright-core/lib/utils/stackTrace';
@@ -287,7 +287,12 @@ export async function normalizeAndSaveAttachment(outputPath: string, name: strin
     throw new Error(`Exactly one of "path" and "body" must be specified`);
   if (options.path !== undefined) {
     const hash = calculateSha1(options.path);
-    const dest = path.join(outputPath, 'attachments', hash + path.extname(options.path));
+
+    if (!isString(name))
+      throw new Error('"name" should be string.');
+
+    const sanitizedNamePrefix = sanitizeForFilePath(name) + '-';
+    const dest = path.join(outputPath, 'attachments', sanitizedNamePrefix + hash + path.extname(options.path));
     await fs.promises.mkdir(path.dirname(dest), { recursive: true });
     await fs.promises.copyFile(options.path, dest);
     const contentType = options.contentType ?? (mime.getType(path.basename(options.path)) || 'application/octet-stream');

packages/playwright-test/types/test.d.ts

@@ -1458,7 +1458,7 @@ export interface TestInfo {
    * > NOTE: [testInfo.attach(name[, options])](https://playwright.dev/docs/api/class-testinfo#test-info-attach)
    * automatically takes care of copying attached files to a location that is accessible to reporters. You can safely remove
    * the attachment after awaiting the attach call.
-   * @param name Attachment name.
+   * @param name Attachment name. The name will also be sanitized and used as the prefix of file name when saving to disk.
    * @param options
    */
   attach(name: string, options?: {

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

@@ -14,6 +14,7 @@
  * limitations under the License.
  */
 
+import path from 'path';
 import { test, expect, stripAnsi } from './playwright-test-fixtures';
 
 test('render text attachment', async ({ runInlineTest }) => {
@@ -186,3 +187,94 @@ test(`testInfo.attach allow empty buffer body`, async ({ runInlineTest }) => {
   expect(result.failed).toBe(1);
   expect(stripAnsi(result.output)).toMatch(/^.*attachment #1: name \(text\/plain\).*\n.*\n.*------/gm);
 });
+
+test(`testInfo.attach use name as prefix`, async ({ runInlineTest }) => {
+  const result = await runInlineTest({
+    'a.test.js': `
+      const test = pwt.test.extend({
+        fixture: async ({}, use, testInfo) => {
+          const filePath = testInfo.outputPath('foo.txt');
+          require('fs').writeFileSync(filePath, 'hello');
+          await use();
+          await testInfo.attach('some random string', { path: filePath });
+        },
+      });
+      test('success', async ({ fixture }) => {
+        expect(true).toBe(false);
+      });
+    `,
+  });
+  expect(result.exitCode).toBe(1);
+  expect(result.failed).toBe(1);
+
+  expect(stripAnsi(result.output)).toContain('attachment #1: some random string (text/plain)');
+  expect(stripAnsi(result.output)).toContain('some-random-string-');
+});
+
+test(`testInfo.attach name should be sanitized`, async ({ runInlineTest }) => {
+  const result = await runInlineTest({
+    'a.test.js': `
+      const test = pwt.test.extend({
+        fixture: async ({}, use, testInfo) => {
+          const filePath = testInfo.outputPath('foo.txt');
+          require('fs').writeFileSync(filePath, 'hello');
+          await use();
+          await testInfo.attach('../../../test', { path: filePath });
+        },
+      });
+      test('success', async ({ fixture }) => {
+        expect(true).toBe(false);
+      });
+    `,
+  });
+  expect(result.exitCode).toBe(1);
+  expect(result.failed).toBe(1);
+
+  expect(stripAnsi(result.output)).toContain('attachment #1: ../../../test (text/plain)');
+  expect(stripAnsi(result.output)).toContain(`attachments${path.sep}-test`);
+});
+
+test(`testInfo.attach name can be empty string`, async ({ runInlineTest }) => {
+  const result = await runInlineTest({
+    'a.test.js': `
+      const test = pwt.test.extend({
+        fixture: async ({}, use, testInfo) => {
+          const filePath = testInfo.outputPath('foo.txt');
+          require('fs').writeFileSync(filePath, 'hello');
+          await use();
+          await testInfo.attach('', { path: filePath });
+        },
+      });
+      test('success', async ({ fixture }) => {
+        expect(true).toBe(false);
+      });
+    `,
+  });
+  expect(result.exitCode).toBe(1);
+  expect(result.failed).toBe(1);
+
+  expect(stripAnsi(result.output)).toContain('attachment #1:  (text/plain)');
+  expect(stripAnsi(result.output)).toContain(`attachments${path.sep}-`);
+});
+
+test(`testInfo.attach throw if name is not string`, async ({ runInlineTest }) => {
+  const result = await runInlineTest({
+    'a.test.js': `
+      const test = pwt.test.extend({
+        fixture: async ({}, use, testInfo) => {
+          const filePath = testInfo.outputPath('foo.txt');
+          require('fs').writeFileSync(filePath, 'hello');
+          await use();
+          await testInfo.attach(false, { path: filePath });
+        },
+      });
+      test('success', async ({ fixture }) => {
+        expect(true).toBe(true);
+      });
+    `,
+  });
+  expect(result.exitCode).toBe(1);
+  expect(result.failed).toBe(1);
+
+  expect(stripAnsi(result.output)).toContain('"name" should be string.');
+});

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

@@ -155,7 +155,7 @@ test(`testInfo.attach should save attachments via path`, async ({ runInlineTest
     expect(result.attachments[0].name).toBe('foo');
     expect(result.attachments[0].contentType).toBe('application/json');
     const p = result.attachments[0].path;
-    expect(p).toMatch(/[/\\]attachments[/\\][0-9a-f]+\.json$/);
+    expect(p).toMatch(/[/\\]attachments[/\\]foo-[0-9a-f]+\.json$/);
     const contents = fs.readFileSync(p);
     expect(contents.toString()).toBe('We <3 Playwright!');
   }
@@ -164,7 +164,7 @@ test(`testInfo.attach should save attachments via path`, async ({ runInlineTest
     expect(result.attachments[0].name).toBe('foo');
     expect(result.attachments[0].contentType).toBe('image/png');
     const p = result.attachments[0].path;
-    expect(p).toMatch(/[/\\]attachments[/\\][0-9a-f]+\.json$/);
+    expect(p).toMatch(/[/\\]attachments[/\\]foo-[0-9a-f]+\.json$/);
     const contents = fs.readFileSync(p);
     expect(contents.toString()).toBe('We <3 Playwright!');
   }
@@ -173,7 +173,7 @@ test(`testInfo.attach should save attachments via path`, async ({ runInlineTest
     expect(result.attachments[0].name).toBe('example.png');
     expect(result.attachments[0].contentType).toBe('x-playwright/custom');
     const p = result.attachments[0].path;
-    expect(p).toMatch(/[/\\]attachments[/\\][0-9a-f]+\.json$/);
+    expect(p).toMatch(/[/\\]attachments[/\\]example-png-[0-9a-f]+\.json$/);
     const contents = fs.readFileSync(p);
     expect(contents.toString()).toBe('We <3 Playwright!');
   }
@@ -182,7 +182,7 @@ test(`testInfo.attach should save attachments via path`, async ({ runInlineTest
     expect(result.attachments[0].name).toBe('foo');
     expect(result.attachments[0].contentType).toBe('application/octet-stream');
     const p = result.attachments[0].path;
-    expect(p).toMatch(/[/\\]attachments[/\\][0-9a-f]+\.this-extension-better-not-map-to-an-actual-mimetype$/);
+    expect(p).toMatch(/[/\\]attachments[/\\]foo-[0-9a-f]+\.this-extension-better-not-map-to-an-actual-mimetype$/);
     const contents = fs.readFileSync(p);
     expect(contents.toString()).toBe('We <3 Playwright!');
   }