From 79fd15b5307e521a57bfca7c4141864545065cc1 Mon Sep 17 00:00:00 2001 From: Dmitry Gozman Date: Thu, 24 Apr 2025 09:36:38 +0000 Subject: [PATCH] fix(test runner): TestInfo.snapshotPath() should sanitize (#35709) --- .../src/matchers/toMatchAriaSnapshot.ts | 12 +++---- .../src/matchers/toMatchSnapshot.ts | 20 ++++++++---- packages/playwright/src/worker/testInfo.ts | 14 ++++++--- .../to-have-screenshot.spec.ts | 31 +++++++++++++++++++ 4 files changed, 61 insertions(+), 16 deletions(-) diff --git a/packages/playwright/src/matchers/toMatchAriaSnapshot.ts b/packages/playwright/src/matchers/toMatchAriaSnapshot.ts index ea38793882..003b92ab83 100644 --- a/packages/playwright/src/matchers/toMatchAriaSnapshot.ts +++ b/packages/playwright/src/matchers/toMatchAriaSnapshot.ts @@ -18,11 +18,11 @@ import fs from 'fs'; import path from 'path'; -import { escapeTemplateString, isString, sanitizeForFilePath } from 'playwright-core/lib/utils'; +import { escapeTemplateString, isString } from 'playwright-core/lib/utils'; import { kNoElementsFoundError, matcherHint } from './matcherHint'; import { EXPECTED_COLOR } from '../common/expectBundle'; -import { callLogText, fileExistsAsync, sanitizeFilePathBeforeExtension, trimLongString } from '../util'; +import { callLogText, fileExistsAsync, trimLongString } from '../util'; import { printReceivedStringContainExpectedSubstring } from './expect'; import { currentTestInfo } from '../common/globals'; @@ -70,8 +70,8 @@ export async function toMatchAriaSnapshot( timeout = options.timeout ?? this.timeout; } else { if (expectedParam?.name) { - const ext = expectedParam.name!.endsWith('.aria.yml') ? '.aria.yml' : undefined; - expectedPath = testInfo._resolveSnapshotPath(pathTemplate, defaultTemplate, [sanitizeFilePathBeforeExtension(expectedParam.name, ext)]); + const ext = expectedParam.name.endsWith('.aria.yml') ? '.aria.yml' : undefined; + expectedPath = testInfo._resolveSnapshotPath(pathTemplate, defaultTemplate, [expectedParam.name], true, ext); } else { let snapshotNames = (testInfo as any)[snapshotNamesSymbol] as SnapshotNames; if (!snapshotNames) { @@ -79,11 +79,11 @@ export async function toMatchAriaSnapshot( (testInfo as any)[snapshotNamesSymbol] = snapshotNames; } const fullTitleWithoutSpec = [...testInfo.titlePath.slice(1), ++snapshotNames.anonymousSnapshotIndex].join(' '); - expectedPath = testInfo._resolveSnapshotPath(pathTemplate, defaultTemplate, [sanitizeForFilePath(trimLongString(fullTitleWithoutSpec))], '.aria.yml'); + expectedPath = testInfo._resolveSnapshotPath(pathTemplate, defaultTemplate, [trimLongString(fullTitleWithoutSpec) + '.aria.yml'], true, '.aria.yml'); // in 1.51, we changed the default template to use .aria.yml extension // for backwards compatibility, we check for the legacy .yml extension if (!(await fileExistsAsync(expectedPath))) { - const legacyPath = testInfo._resolveSnapshotPath(pathTemplate, defaultTemplate, [sanitizeForFilePath(trimLongString(fullTitleWithoutSpec))], '.yml'); + const legacyPath = testInfo._resolveSnapshotPath(pathTemplate, defaultTemplate, [trimLongString(fullTitleWithoutSpec) + '.yml'], true, '.yml'); if (await fileExistsAsync(legacyPath)) expectedPath = legacyPath; } diff --git a/packages/playwright/src/matchers/toMatchSnapshot.ts b/packages/playwright/src/matchers/toMatchSnapshot.ts index 6cefb48f1e..fb05e4ce38 100644 --- a/packages/playwright/src/matchers/toMatchSnapshot.ts +++ b/packages/playwright/src/matchers/toMatchSnapshot.ts @@ -122,6 +122,7 @@ class SnapshotHelper { } let expectedPathSegments: string[]; + let sanitizeFilePath = true; let outputBasePath: string; if (!name) { // Consider the use case below. We should save actual to different paths. @@ -135,17 +136,24 @@ class SnapshotHelper { ++snapshotNames.anonymousSnapshotIndex, ].join(' '); // Note: expected path must not ever change for backwards compatibility. - expectedPathSegments = [sanitizeForFilePath(trimLongString(fullTitleWithoutSpec)) + '.' + anonymousSnapshotExtension]; + expectedPathSegments = [trimLongString(fullTitleWithoutSpec) + '.' + anonymousSnapshotExtension]; // Trim the output file paths more aggressively to avoid hitting Windows filesystem limits. 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, 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)); + let joinedName: string; + if (Array.isArray(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. + sanitizeFilePath = false; + expectedPathSegments = name; + joinedName = name.join(path.sep); + } else { + expectedPathSegments = [name]; + joinedName = sanitizeFilePathBeforeExtension(trimLongString(name, windowsFilesystemFriendlyLength)); + } snapshotNames.namedSnapshotIndex[joinedName] = (snapshotNames.namedSnapshotIndex[joinedName] || 0) + 1; const index = snapshotNames.namedSnapshotIndex[joinedName]; const sanitizedName = index > 1 ? addSuffixToFilePath(joinedName, `-${index - 1}`) : joinedName; @@ -153,7 +161,7 @@ class SnapshotHelper { this.attachmentBaseName = sanitizedName; } const defaultTemplate = '{snapshotDir}/{testFileDir}/{testFileName}-snapshots/{arg}{-projectName}{-snapshotSuffix}{ext}'; - this.expectedPath = testInfo._resolveSnapshotPath(configOptions.pathTemplate, defaultTemplate, expectedPathSegments); + this.expectedPath = testInfo._resolveSnapshotPath(configOptions.pathTemplate, defaultTemplate, expectedPathSegments, sanitizeFilePath); this.legacyExpectedPath = addSuffixToFilePath(outputBasePath, '-expected'); this.previousPath = addSuffixToFilePath(outputBasePath, '-previous'); this.actualPath = addSuffixToFilePath(outputBasePath, '-actual'); diff --git a/packages/playwright/src/worker/testInfo.ts b/packages/playwright/src/worker/testInfo.ts index c1d1bcb116..f14a483a35 100644 --- a/packages/playwright/src/worker/testInfo.ts +++ b/packages/playwright/src/worker/testInfo.ts @@ -20,7 +20,7 @@ import path from 'path'; import { captureRawStack, monotonicTime, sanitizeForFilePath, stringifyStackFrames, currentZone } from 'playwright-core/lib/utils'; import { TimeoutManager, TimeoutManagerError, kMaxDeadline } from './timeoutManager'; -import { filteredStackTrace, getContainedPath, normalizeAndSaveAttachment, trimLongString, windowsFilesystemFriendlyLength } from '../util'; +import { filteredStackTrace, getContainedPath, normalizeAndSaveAttachment, sanitizeFilePathBeforeExtension, trimLongString, windowsFilesystemFriendlyLength } from '../util'; import { TestTracing } from './testTracing'; import { testInfoError } from './util'; @@ -448,8 +448,11 @@ export class TestInfoImpl implements TestInfo { return sanitizeForFilePath(trimLongString(fullTitleWithoutSpec)); } - _resolveSnapshotPath(template: string | undefined, defaultTemplate: string, pathSegments: string[], extension?: string) { - const subPath = path.join(...pathSegments); + _resolveSnapshotPath(template: string | undefined, defaultTemplate: string, pathSegments: string[], sanitizeFilePath: boolean, extension?: string) { + let subPath = path.join(...pathSegments); + if (sanitizeFilePath) + subPath = sanitizeFilePathBeforeExtension(subPath, extension); + const dir = path.dirname(subPath); const ext = extension ?? path.extname(subPath); const name = path.basename(subPath, ext); @@ -475,8 +478,11 @@ export class TestInfoImpl implements TestInfo { } snapshotPath(...pathSegments: string[]) { + // Assume a single path segment corresponds to `toHaveScreenshot(name)` and sanitize it, + // like we do in SnapshotHelper. See https://github.com/microsoft/playwright/pull/9156 for history. + const sanitizeFilePath = pathSegments.length === 1; const legacyTemplate = '{snapshotDir}/{testFileDir}/{testFileName}-snapshots/{arg}{-projectName}{-snapshotSuffix}{ext}'; - return this._resolveSnapshotPath(undefined, legacyTemplate, pathSegments); + return this._resolveSnapshotPath(undefined, legacyTemplate, pathSegments, sanitizeFilePath); } skip(...args: [arg?: any, description?: string]) { diff --git a/tests/playwright-test/to-have-screenshot.spec.ts b/tests/playwright-test/to-have-screenshot.spec.ts index a2f3cfc728..aedfb477e9 100644 --- a/tests/playwright-test/to-have-screenshot.spec.ts +++ b/tests/playwright-test/to-have-screenshot.spec.ts @@ -742,6 +742,37 @@ test('should update snapshot with the update-snapshots flag', async ({ runInline expect(comparePNGs(fs.readFileSync(snapshotOutputPath), whiteImage)).toBe(null); }); +test('should respect config.snapshotPathTemplate and sanitize the name', { + annotation: { type: 'issue', description: 'https://github.com/microsoft/playwright/issues/35669' }, +}, async ({ runInlineTest }) => { + const result = await runInlineTest({ + ...playwrightConfig({ + snapshotPathTemplate: '__screenshots__/{testFilePath}/{arg}{ext}', + }), + '__screenshots__/a.spec.js/my-name.png': whiteImage, + '__screenshots__/a.spec.js/my_name/bar.png': whiteImage, + 'a.spec.js': ` + const path = require('path'); + const { test, expect } = require('@playwright/test'); + test('is a test', async ({ page }) => { + const testDir = test.info().project.testDir; + + // A single name is sanitized to a valid file name. + const expectedPath1 = path.join(testDir, '__screenshots__/a.spec.js/my-name.png'); + expect(test.info().snapshotPath('my_name.png')).toBe(expectedPath1); + await expect(page).toHaveScreenshot('my_name.png'); + + // An array is not sanitized - see https://github.com/microsoft/playwright/pull/9156. + const expectedPath2 = path.join(testDir, '__screenshots__/a.spec.js/my_name/bar.png'); + expect(test.info().snapshotPath('my_name', 'bar.png')).toBe(expectedPath2); + await expect(page).toHaveScreenshot(['my_name', 'bar.png']); + }); + ` + }); + expect(result.exitCode).toBe(0); + expect(result.passed).toBe(1); +}); + test('should respect config.expect.toHaveScreenshot.pathTemplate', async ({ runInlineTest }, testInfo) => { const result = await runInlineTest({ ...playwrightConfig({