From 9338355e472ce703e936cc05f6c8ea9a43363f09 Mon Sep 17 00:00:00 2001 From: Xiaoxing Ye Date: Fri, 4 Nov 2022 04:54:51 +0800 Subject: [PATCH] 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 | 3 +- packages/playwright-test/src/util.ts | 9 +- packages/playwright-test/types/test.d.ts | 2 +- .../reporter-attachment.spec.ts | 92 +++++++++++++++++++ tests/playwright-test/reporter-raw.spec.ts | 8 +- 5 files changed, 106 insertions(+), 8 deletions(-) diff --git a/docs/src/test-api/class-testinfo.md b/docs/src/test-api/class-testinfo.md index c9a9727dc8..4ff1fac892 100644 --- a/docs/src/test-api/class-testinfo.md +++ b/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 diff --git a/packages/playwright-test/src/util.ts b/packages/playwright-test/src/util.ts index b21e1c2585..d67620b5d5 100644 --- a/packages/playwright-test/src/util.ts +++ b/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'); diff --git a/packages/playwright-test/types/test.d.ts b/packages/playwright-test/types/test.d.ts index 57a9bcbf2f..db7912006a 100644 --- a/packages/playwright-test/types/test.d.ts +++ b/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?: { diff --git a/tests/playwright-test/reporter-attachment.spec.ts b/tests/playwright-test/reporter-attachment.spec.ts index bfc12005e8..17689111ee 100644 --- a/tests/playwright-test/reporter-attachment.spec.ts +++ b/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.'); +}); diff --git a/tests/playwright-test/reporter-raw.spec.ts b/tests/playwright-test/reporter-raw.spec.ts index d8c21c6344..0a1612d3c6 100644 --- a/tests/playwright-test/reporter-raw.spec.ts +++ b/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!'); }