fix(test runner): TestInfo.snapshotPath() should sanitize (#35709)

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;
       }

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');

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]) {

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({