From f4399f7f062315034a52c2ae395e00979b7cc54d Mon Sep 17 00:00:00 2001 From: Dmitry Gozman Date: Wed, 17 Jul 2024 07:08:43 -0700 Subject: [PATCH] fix(toHaveScreenshot): sanitize attachment names and paths (#31712) ... unless an array of file-system-friendly parts is provided. Motivation: attachment name is used as a file system path when downloading attachments, so we keep them fs-friendly. References #30693. --- .../src/matchers/toMatchSnapshot.ts | 108 +++++++++--------- tests/playwright-test/golden.spec.ts | 8 +- .../to-have-screenshot.spec.ts | 46 ++++++++ 3 files changed, 105 insertions(+), 57 deletions(-) diff --git a/packages/playwright/src/matchers/toMatchSnapshot.ts b/packages/playwright/src/matchers/toMatchSnapshot.ts index 7a3242100e..a533f13b48 100644 --- a/packages/playwright/src/matchers/toMatchSnapshot.ts +++ b/packages/playwright/src/matchers/toMatchSnapshot.ts @@ -75,10 +75,10 @@ const NonConfigProperties: (keyof ToHaveScreenshotOptions)[] = [ class SnapshotHelper { readonly testInfo: TestInfoImpl; - readonly outputBaseName: string; + readonly attachmentBaseName: string; readonly legacyExpectedPath: string; readonly previousPath: string; - readonly snapshotPath: string; + readonly expectedPath: string; readonly actualPath: string; readonly diffPath: string; readonly mimeType: string; @@ -117,40 +117,42 @@ class SnapshotHelper { (testInfo as any)[snapshotNamesSymbol] = snapshotNames; } - // Consider the use case below. We should save actual to different paths. - // - // expect.toMatchSnapshot('a.png') - // // noop - // expect.toMatchSnapshot('a.png') - - let inputPathSegments: string[]; + let expectedPathSegments: string[]; + let outputBasePath: string; if (!name) { + // Consider the use case below. We should save actual to different paths. + // Therefore we auto-increment |anonymousSnapshotIndex|. + // + // expect.toMatchSnapshot('a.png') + // // noop + // expect.toMatchSnapshot('a.png') const fullTitleWithoutSpec = [ ...testInfo.titlePath.slice(1), ++snapshotNames.anonymousSnapshotIndex, ].join(' '); - inputPathSegments = [sanitizeForFilePath(trimLongString(fullTitleWithoutSpec)) + '.' + anonymousSnapshotExtension]; + // Note: expected path must not ever change for backwards compatibility. + expectedPathSegments = [sanitizeForFilePath(trimLongString(fullTitleWithoutSpec)) + '.' + anonymousSnapshotExtension]; // Trim the output file paths more aggressively to avoid hitting Windows filesystem limits. - this.outputBaseName = sanitizeForFilePath(trimLongString(fullTitleWithoutSpec, windowsFilesystemFriendlyLength)) + '.' + anonymousSnapshotExtension; + const sanitizedName = sanitizeForFilePath(trimLongString(fullTitleWithoutSpec, windowsFilesystemFriendlyLength)) + '.' + anonymousSnapshotExtension; + outputBasePath = testInfo._getOutputPath(sanitizedName); + this.attachmentBaseName = sanitizedName; } else { - // We intentionally do not sanitize user-provided array of segments, but for backwards - // compatibility we do sanitize the name if it is a single string. - // See https://github.com/microsoft/playwright/pull/9156 - inputPathSegments = Array.isArray(name) ? name : [sanitizeFilePathBeforeExtension(name)]; - const joinedName = Array.isArray(name) ? name.join(path.sep) : name; + // We intentionally do not sanitize user-provided array of segments, assuming + // it is a file system path. See https://github.com/microsoft/playwright/pull/9156. + // Note: expected path must not ever change for backwards compatibility. + expectedPathSegments = Array.isArray(name) ? name : [sanitizeFilePathBeforeExtension(name)]; + const joinedName = Array.isArray(name) ? name.join(path.sep) : sanitizeFilePathBeforeExtension(trimLongString(name, windowsFilesystemFriendlyLength)); snapshotNames.namedSnapshotIndex[joinedName] = (snapshotNames.namedSnapshotIndex[joinedName] || 0) + 1; const index = snapshotNames.namedSnapshotIndex[joinedName]; - if (index > 1) - this.outputBaseName = addSuffixToFilePath(joinedName, `-${index - 1}`); - else - this.outputBaseName = joinedName; + const sanitizedName = index > 1 ? addSuffixToFilePath(joinedName, `-${index - 1}`) : joinedName; + outputBasePath = testInfo._getOutputPath(sanitizedName); + this.attachmentBaseName = sanitizedName; } - this.snapshotPath = testInfo.snapshotPath(...inputPathSegments); - const outputFile = testInfo._getOutputPath(sanitizeFilePathBeforeExtension(this.outputBaseName)); - this.legacyExpectedPath = addSuffixToFilePath(outputFile, '-expected'); - this.previousPath = addSuffixToFilePath(outputFile, '-previous'); - this.actualPath = addSuffixToFilePath(outputFile, '-actual'); - this.diffPath = addSuffixToFilePath(outputFile, '-diff'); + this.expectedPath = testInfo.snapshotPath(...expectedPathSegments); + this.legacyExpectedPath = addSuffixToFilePath(outputBasePath, '-expected'); + this.previousPath = addSuffixToFilePath(outputBasePath, '-previous'); + this.actualPath = addSuffixToFilePath(outputBasePath, '-actual'); + this.diffPath = addSuffixToFilePath(outputBasePath, '-diff'); const filteredConfigOptions = { ...configOptions }; for (const prop of NonConfigProperties) @@ -176,7 +178,7 @@ class SnapshotHelper { this.locator = locator; this.updateSnapshots = testInfo.config.updateSnapshots; - this.mimeType = mime.getType(path.basename(this.snapshotPath)) ?? 'application/octet-string'; + this.mimeType = mime.getType(path.basename(this.expectedPath)) ?? 'application/octet-string'; this.comparator = getComparator(this.mimeType); this.testInfo = testInfo; @@ -186,7 +188,7 @@ class SnapshotHelper { createMatcherResult(message: string, pass: boolean, log?: string[]): ImageMatcherResult { const unfiltered: ImageMatcherResult = { name: this.matcherName, - expected: this.snapshotPath, + expected: this.expectedPath, actual: this.actualPath, diff: this.diffPath, pass, @@ -198,7 +200,7 @@ class SnapshotHelper { handleMissingNegated(): ImageMatcherResult { const isWriteMissingMode = this.updateSnapshots === 'all' || this.updateSnapshots === 'missing'; - const message = `A snapshot doesn't exist at ${this.snapshotPath}${isWriteMissingMode ? ', matchers using ".not" won\'t write them automatically.' : '.'}`; + const message = `A snapshot doesn't exist at ${this.expectedPath}${isWriteMissingMode ? ', matchers using ".not" won\'t write them automatically.' : '.'}`; // NOTE: 'isNot' matcher implies inversed value. return this.createMatcherResult(message, true); } @@ -221,11 +223,11 @@ class SnapshotHelper { handleMissing(actual: Buffer | string): ImageMatcherResult { const isWriteMissingMode = this.updateSnapshots === 'all' || this.updateSnapshots === 'missing'; if (isWriteMissingMode) { - writeFileSync(this.snapshotPath, actual); + writeFileSync(this.expectedPath, actual); writeFileSync(this.actualPath, actual); - this.testInfo.attachments.push({ name: addSuffixToFilePath(this.outputBaseName, '-actual'), contentType: this.mimeType, path: this.actualPath }); + this.testInfo.attachments.push({ name: addSuffixToFilePath(this.attachmentBaseName, '-actual'), contentType: this.mimeType, path: this.actualPath }); } - const message = `A snapshot doesn't exist at ${this.snapshotPath}${isWriteMissingMode ? ', writing actual.' : '.'}`; + const message = `A snapshot doesn't exist at ${this.expectedPath}${isWriteMissingMode ? ', writing actual.' : '.'}`; if (this.updateSnapshots === 'all') { /* eslint-disable no-console */ console.log(message); @@ -258,22 +260,22 @@ class SnapshotHelper { // Copy the expectation inside the `test-results/` folder for backwards compatibility, // so that one can upload `test-results/` directory and have all the data inside. writeFileSync(this.legacyExpectedPath, expected); - this.testInfo.attachments.push({ name: addSuffixToFilePath(this.outputBaseName, '-expected'), contentType: this.mimeType, path: this.snapshotPath }); - output.push(`\nExpected: ${colors.yellow(this.snapshotPath)}`); + this.testInfo.attachments.push({ name: addSuffixToFilePath(this.attachmentBaseName, '-expected'), contentType: this.mimeType, path: this.expectedPath }); + output.push(`\nExpected: ${colors.yellow(this.expectedPath)}`); } if (previous !== undefined) { writeFileSync(this.previousPath, previous); - this.testInfo.attachments.push({ name: addSuffixToFilePath(this.outputBaseName, '-previous'), contentType: this.mimeType, path: this.previousPath }); + this.testInfo.attachments.push({ name: addSuffixToFilePath(this.attachmentBaseName, '-previous'), contentType: this.mimeType, path: this.previousPath }); output.push(`Previous: ${colors.yellow(this.previousPath)}`); } if (actual !== undefined) { writeFileSync(this.actualPath, actual); - this.testInfo.attachments.push({ name: addSuffixToFilePath(this.outputBaseName, '-actual'), contentType: this.mimeType, path: this.actualPath }); + this.testInfo.attachments.push({ name: addSuffixToFilePath(this.attachmentBaseName, '-actual'), contentType: this.mimeType, path: this.actualPath }); output.push(`Received: ${colors.yellow(this.actualPath)}`); } if (diff !== undefined) { writeFileSync(this.diffPath, diff); - this.testInfo.attachments.push({ name: addSuffixToFilePath(this.outputBaseName, '-diff'), contentType: this.mimeType, path: this.diffPath }); + this.testInfo.attachments.push({ name: addSuffixToFilePath(this.attachmentBaseName, '-diff'), contentType: this.mimeType, path: this.diffPath }); output.push(` Diff: ${colors.yellow(this.diffPath)}`); } @@ -311,25 +313,25 @@ export function toMatchSnapshot( configOptions, nameOrOptions, optOptions); if (this.isNot) { - if (!fs.existsSync(helper.snapshotPath)) + if (!fs.existsSync(helper.expectedPath)) return helper.handleMissingNegated(); - const isDifferent = !!helper.comparator(received, fs.readFileSync(helper.snapshotPath), helper.options); + const isDifferent = !!helper.comparator(received, fs.readFileSync(helper.expectedPath), helper.options); return isDifferent ? helper.handleDifferentNegated() : helper.handleMatchingNegated(); } - if (!fs.existsSync(helper.snapshotPath)) + if (!fs.existsSync(helper.expectedPath)) return helper.handleMissing(received); - const expected = fs.readFileSync(helper.snapshotPath); + const expected = fs.readFileSync(helper.expectedPath); const result = helper.comparator(received, expected, helper.options); if (!result) return helper.handleMatching(); if (helper.updateSnapshots === 'all') { - writeFileSync(helper.snapshotPath, received); + writeFileSync(helper.expectedPath, received); /* eslint-disable no-console */ - console.log(helper.snapshotPath + ' does not match, writing actual.'); - return helper.createMatcherResult(helper.snapshotPath + ' running with --update-snapshots, writing actual.', true); + console.log(helper.expectedPath + ' does not match, writing actual.'); + return helper.createMatcherResult(helper.expectedPath + ' running with --update-snapshots, writing actual.', true); } return helper.handleDifferent(received, expected, undefined, result.diff, result.errorMessage, undefined); @@ -364,8 +366,8 @@ export async function toHaveScreenshot( const [page, locator] = pageOrLocator.constructor.name === 'Page' ? [(pageOrLocator as PageEx), undefined] : [(pageOrLocator as Locator).page() as PageEx, pageOrLocator as Locator]; const configOptions = testInfo._projectInternal.expect?.toHaveScreenshot || {}; const helper = new SnapshotHelper(testInfo, 'toHaveScreenshot', locator, 'png', configOptions, nameOrOptions, optOptions); - if (!helper.snapshotPath.toLowerCase().endsWith('.png')) - throw new Error(`Screenshot name "${path.basename(helper.snapshotPath)}" must have '.png' extension`); + if (!helper.expectedPath.toLowerCase().endsWith('.png')) + throw new Error(`Screenshot name "${path.basename(helper.expectedPath)}" must have '.png' extension`); expectTypes(pageOrLocator, ['Page', 'Locator'], 'toHaveScreenshot'); const style = await loadScreenshotStyles(helper.options.stylePath); const expectScreenshotOptions: ExpectScreenshotOptions = { @@ -387,7 +389,7 @@ export async function toHaveScreenshot( threshold: helper.options.threshold, }; - const hasSnapshot = fs.existsSync(helper.snapshotPath); + const hasSnapshot = fs.existsSync(helper.expectedPath); if (this.isNot) { if (!hasSnapshot) return helper.handleMissingNegated(); @@ -395,14 +397,14 @@ export async function toHaveScreenshot( // Having `errorMessage` means we timed out while waiting // for screenshots not to match, so screenshots // are actually the same in the end. - expectScreenshotOptions.expected = await fs.promises.readFile(helper.snapshotPath); + expectScreenshotOptions.expected = await fs.promises.readFile(helper.expectedPath); const isDifferent = !(await page._expectScreenshot(expectScreenshotOptions)).errorMessage; return isDifferent ? helper.handleDifferentNegated() : helper.handleMatchingNegated(); } // Fast path: there's no screenshot and we don't intend to update it. if (helper.updateSnapshots === 'none' && !hasSnapshot) - return helper.createMatcherResult(`A snapshot doesn't exist at ${helper.snapshotPath}.`, false); + return helper.createMatcherResult(`A snapshot doesn't exist at ${helper.expectedPath}.`, false); if (!hasSnapshot) { // Regenerate a new screenshot by waiting until two screenshots are the same. @@ -420,18 +422,18 @@ export async function toHaveScreenshot( // - snapshot exists // - regular matcher (i.e. not a `.not`) // - perhaps an 'all' flag to update non-matching screenshots - expectScreenshotOptions.expected = await fs.promises.readFile(helper.snapshotPath); + expectScreenshotOptions.expected = await fs.promises.readFile(helper.expectedPath); const { actual, diff, errorMessage, log } = await page._expectScreenshot(expectScreenshotOptions); if (!errorMessage) return helper.handleMatching(); if (helper.updateSnapshots === 'all') { - writeFileSync(helper.snapshotPath, actual!); + writeFileSync(helper.expectedPath, actual!); writeFileSync(helper.actualPath, actual!); /* eslint-disable no-console */ - console.log(helper.snapshotPath + ' is re-generated, writing actual.'); - return helper.createMatcherResult(helper.snapshotPath + ' running with --update-snapshots, writing actual.', true); + console.log(helper.expectedPath + ' is re-generated, writing actual.'); + return helper.createMatcherResult(helper.expectedPath + ' running with --update-snapshots, writing actual.', true); } return helper.handleDifferent(actual, expectScreenshotOptions.expected, undefined, diff, errorMessage, log); diff --git a/tests/playwright-test/golden.spec.ts b/tests/playwright-test/golden.spec.ts index 41cdb9da5a..205eeee7f0 100644 --- a/tests/playwright-test/golden.spec.ts +++ b/tests/playwright-test/golden.spec.ts @@ -145,7 +145,7 @@ test('should generate separate actual results for repeating names', async ({ run { 'name': 'bar/baz-actual.txt', 'contentType': 'text/plain', - 'path': 'test-results/a-is-a-test/bar-baz-actual.txt' + 'path': 'test-results/a-is-a-test/bar/baz-actual.txt' }, { 'name': 'bar/baz-1-expected.txt', @@ -155,7 +155,7 @@ test('should generate separate actual results for repeating names', async ({ run { 'name': 'bar/baz-1-actual.txt', 'contentType': 'text/plain', - 'path': 'test-results/a-is-a-test/bar-baz-1-actual.txt' + 'path': 'test-results/a-is-a-test/bar/baz-1-actual.txt' } ]); }); @@ -977,12 +977,12 @@ test('should attach expected/actual/diff with snapshot path', async ({ runInline { name: 'test/path/snapshot-actual.png', contentType: 'image/png', - path: 'a-is-a-test/test-path-snapshot-actual.png' + path: 'a-is-a-test/test/path/snapshot-actual.png' }, { name: 'test/path/snapshot-diff.png', contentType: 'image/png', - path: 'a-is-a-test/test-path-snapshot-diff.png' + path: 'a-is-a-test/test/path/snapshot-diff.png' } ]); }); diff --git a/tests/playwright-test/to-have-screenshot.spec.ts b/tests/playwright-test/to-have-screenshot.spec.ts index 07fb590375..39044e460a 100644 --- a/tests/playwright-test/to-have-screenshot.spec.ts +++ b/tests/playwright-test/to-have-screenshot.spec.ts @@ -1320,3 +1320,49 @@ function playwrightConfig(obj: any) { `, }; } + +test('should trim+sanitize attachment names and paths', async ({ runInlineTest }, testInfo) => { + const result = await runInlineTest({ + ...playwrightConfig({ + snapshotPathTemplate: '__screenshots__/{testFilePath}/{arg}{ext}', + }), + 'a.spec.js': ` + const { test, expect } = require('@playwright/test'); + test.afterEach(async ({}, testInfo) => { + console.log('## ' + JSON.stringify(testInfo.attachments)); + }); + const title = 'long '.repeat(30) + 'title'; + test(title, async ({ page }) => { + await expect.soft(page).toHaveScreenshot(); + const name = 'long '.repeat(30) + 'name.png'; + await expect.soft(page).toHaveScreenshot(name); + await expect.soft(page).toHaveScreenshot(['dir', name]); + }); + ` + }); + + expect(result.exitCode).toBe(1); + const attachments = result.output.split('\n').filter(l => l.startsWith('## ')).map(l => l.substring(3)).map(l => JSON.parse(l))[0]; + for (const attachment of attachments) { + attachment.path = attachment.path.replace(testInfo.outputDir, '').substring(1).replace(/\\/g, '/'); + attachment.name = attachment.name.replace(/\\/g, '/'); + } + expect(attachments).toEqual([ + { + name: 'long-long-long-long-long-l-852e1-long-long-long-long-title-1-actual.png', + contentType: 'image/png', + path: 'test-results/a-long-long-long-long-long-abd51-g-long-long-long-long-title/long-long-long-long-long-l-852e1-long-long-long-long-title-1-actual.png', + }, + { + name: 'long-long-long-long-long-l-6bf1e-ong-long-long-long-name-actual.png', + contentType: 'image/png', + path: 'test-results/a-long-long-long-long-long-abd51-g-long-long-long-long-title/long-long-long-long-long-l-6bf1e-ong-long-long-long-name-actual.png', + }, + { + name: 'dir/long long long long long long long long long long long long long long long long long long long long long long long long long long long long long long name-actual.png', + contentType: 'image/png', + path: 'test-results/a-long-long-long-long-long-abd51-g-long-long-long-long-title/dir/long long long long long long long long long long long long long long long long long long long long long long long long long long long long long long name-actual.png', + }, + ]); +}); +