feat: add path option to toMatchSnapshot (#9156)

docs/src/test-advanced-js.md

@@ -120,7 +120,7 @@ In addition to everything from the [`workerInfo`](#workerinfo), the following in
 - `timeout: number` - Test timeout.
 - `annotations` - [Annotations](./test-annotations.md) that were added to the test.
 - `snapshotSuffix: string` - Suffix used to locate snapshots for the test.
-- `snapshotPath(snapshotName: string)` - Function that returns the full path to a particular snapshot for the test.
+- `snapshotPath(...pathSegments: string[])` - Function that returns the full path to a particular snapshot for the test.
 - `outputDir: string` - Path to the output directory for this test run.
 - `outputPath(...pathSegments: string[])` - Function that returns the full path to a particular output artifact for the test.
 

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

@@ -193,12 +193,14 @@ test('example test', async ({}, testInfo) => {
 });
 ```
 
+> Note that `pathSegments` accepts path segments to the test output directory such as `testInfo.outputPath('relative', 'path', 'to', 'output')`.
+> However, this path must stay within the [`property: TestInfo.outputDir`] directory for each test (i.e. `test-results/a-test-title`), otherwise it will throw.
+
 ### param: TestInfo.outputPath.pathSegments
 - `pathSegments` <[string...]>
 
 Path segments to append at the end of the resulting path.
 
-
 ## property: TestInfo.project
 - type: <[TestProject]>
 
@@ -275,10 +277,15 @@ Optional description that will be reflected in a test report.
 ## method: TestInfo.snapshotPath
 - returns: <[string]>
 
-Returns a path to a snapshot file with the given `snapshotName`. Learn more about [snapshots](./test-snapshots.md).
+Returns a path to a snapshot file with the given `pathSegments`. Learn more about [snapshots](./test-snapshots.md).
 
-### param: TestInfo.snapshotPath.snapshotName
-- `snapshotName` <[string]> The name of the snapshot. Note that snapshots with the same name in the same test file are expected to be the same.
+> Note that `pathSegments` accepts path segments to the snapshot file such as `testInfo.snapshotPath('relative', 'path', 'to', 'snapshot.png')`.
+> However, this path must stay within the snapshots directory for each test file (i.e. `a.spec.js-snapshots`), otherwise it will throw.
+
+### param: TestInfo.snapshotPath.pathSegments
+- `pathSegments` <[string...]>
+
+The name of the snapshot or the path segments to define the snapshot file path. Snapshots with the same name in the same test file are expected to be the same.
 
 ## property: TestInfo.snapshotSuffix
 - type: <[string]>

docs/src/test-snapshots-js.md

@@ -55,7 +55,8 @@ Sometimes you need to update the reference screenshot, for example when the page
 npx playwright test --update-snapshots
 ```
 
-Note that `snapshotName` is *not a path* relative to the test file, so don't try to use it like `expect(value).toMatchSnapshot('../../test-snapshots/snapshot.png')`.
+> Note that `snapshotName` also accepts an array of path segments to the snapshot file such as `expect(value).toMatchSnapshot(['relative', 'path', 'to', 'snapshot.png'])`.
+> However, this path must stay within the snapshots directory for each test file (i.e. `a.spec.js-snapshots`), otherwise it will throw.
 
 Playwright Test uses the [pixelmatch](https://github.com/mapbox/pixelmatch) library. You can pass comparison `threshold` as an option.
 

src/test/matchers/golden.ts

@@ -22,7 +22,8 @@ import path from 'path';
 import jpeg from 'jpeg-js';
 import pixelmatch from 'pixelmatch';
 import { diff_match_patch, DIFF_INSERT, DIFF_DELETE, DIFF_EQUAL } from '../../third_party/diff_match_patch';
-import { UpdateSnapshots } from '../types';
+import { TestInfoImpl, UpdateSnapshots } from '../types';
+import { addSuffixToFilePath } from '../util';
 
 // Note: we require the pngjs version of pixelmatch to avoid version mismatches.
 const { PNG } = require(require.resolve('pngjs', { paths: [require.resolve('pixelmatch')] }));
@@ -82,18 +83,18 @@ function compareText(actual: Buffer | string, expectedBuffer: Buffer): { diff?:
 
 export function compare(
   actual: Buffer | string,
-  name: string,
-  snapshotPath: (name: string) => string,
-  outputPath: (name: string) => string,
+  pathSegments: string[],
+  snapshotPath: TestInfoImpl['snapshotPath'],
+  outputPath: TestInfoImpl['outputPath'],
   updateSnapshots: UpdateSnapshots,
   withNegateComparison: boolean,
   options?: { threshold?: number }
 ): { pass: boolean; message?: string; expectedPath?: string, actualPath?: string, diffPath?: string, mimeType?: string } {
-  const snapshotFile = snapshotPath(name);
-  const outputFile = outputPath(name);
-  const expectedPath = addSuffix(outputFile, '-expected');
-  const actualPath = addSuffix(outputFile, '-actual');
-  const diffPath = addSuffix(outputFile, '-diff');
+  const snapshotFile = snapshotPath(...pathSegments);
+  const outputFile = outputPath(...pathSegments);
+  const expectedPath = addSuffixToFilePath(outputFile, '-expected');
+  const actualPath = addSuffixToFilePath(outputFile, '-actual');
+  const diffPath = addSuffixToFilePath(outputFile, '-diff');
 
   if (!fs.existsSync(snapshotFile)) {
     const isWriteMissingMode = updateSnapshots === 'all' || updateSnapshots === 'missing';
@@ -104,6 +105,7 @@ export function compare(
     }
     if (isWriteMissingMode) {
       fs.mkdirSync(path.dirname(snapshotFile), { recursive: true });
+      fs.mkdirSync(path.dirname(actualPath), { recursive: true });
       fs.writeFileSync(snapshotFile, actual);
       fs.writeFileSync(actualPath, actual);
     }
@@ -159,6 +161,8 @@ export function compare(
     };
   }
 
+  fs.mkdirSync(path.dirname(expectedPath), { recursive: true });
+  fs.mkdirSync(path.dirname(actualPath), { recursive: true });
   fs.writeFileSync(expectedPath, expected);
   fs.writeFileSync(actualPath, actual);
   if (result.diff)
@@ -191,13 +195,6 @@ function indent(lines: string, tab: string) {
   return lines.replace(/^(?=.+$)/gm, tab);
 }
 
-function addSuffix(filePath: string, suffix: string, customExtension?: string): string {
-  const dirname = path.dirname(filePath);
-  const ext = path.extname(filePath);
-  const name = path.basename(filePath, ext);
-  return path.join(dirname, name + suffix + (customExtension || ext));
-}
-
 function diff_prettyTerminal(diffs: diff_match_patch.Diff[]) {
   const html = [];
   for (let x = 0; x < diffs.length; x++) {

src/test/matchers/toMatchSnapshot.ts

@@ -17,6 +17,7 @@
 import type { Expect } from '../types';
 import { currentTestInfo } from '../globals';
 import { compare } from './golden';
+import { addSuffixToFilePath } from '../util';
 
 // from expect/build/types
 type SyncExpectationResult = {
@@ -24,12 +25,13 @@ type SyncExpectationResult = {
   message: () => string;
 };
 
-export function toMatchSnapshot(this: ReturnType<Expect['getState']>, received: Buffer | string, nameOrOptions: string | { name: string, threshold?: number }, optOptions: { threshold?: number } = {}): SyncExpectationResult {
-  let options: { name: string, threshold?: number };
+type NameOrSegments = string | string[];
+export function toMatchSnapshot(this: ReturnType<Expect['getState']>, received: Buffer | string, nameOrOptions: NameOrSegments | { name: NameOrSegments, threshold?: number }, optOptions: { threshold?: number } = {}): SyncExpectationResult {
+  let options: { name: NameOrSegments, threshold?: number };
   const testInfo = currentTestInfo();
   if (!testInfo)
     throw new Error(`toMatchSnapshot() must be called during the test`);
-  if (typeof nameOrOptions === 'string')
+  if (Array.isArray(nameOrOptions) || typeof nameOrOptions === 'string')
     options = { name: nameOrOptions, ...optOptions };
   else
     options = { ...nameOrOptions };
@@ -40,10 +42,12 @@ export function toMatchSnapshot(this: ReturnType<Expect['getState']>, received:
   if (options.threshold === undefined && projectThreshold !== undefined)
     options.threshold = projectThreshold;
 
+  // sanitizes path if string
+  const pathSegments = Array.isArray(options.name) ? options.name : [addSuffixToFilePath(options.name, '', undefined, true)];
   const withNegateComparison = this.isNot;
   const { pass, message, expectedPath, actualPath, diffPath, mimeType } = compare(
       received,
-      options.name,
+      pathSegments,
       testInfo.snapshotPath,
       testInfo.outputPath,
       testInfo.retry < testInfo.project.retries ? 'none' : testInfo.config.updateSnapshots,

src/test/util.ts

@@ -144,4 +144,21 @@ export function sanitizeForFilePath(s: string) {
   return s.replace(/[\x00-\x2F\x3A-\x40\x5B-\x60\x7B-\x7F]+/g, '-');
 }
 
+export function addSuffixToFilePath(filePath: string, suffix: string, customExtension?: string, sanitize = false): string {
+  const dirname = path.dirname(filePath);
+  const ext = path.extname(filePath);
+  const name = path.basename(filePath, ext);
+  const base = path.join(dirname, name);
+  return (sanitize ? sanitizeForFilePath(base) : base) + suffix + (customExtension || ext);
+}
+
+/**
+ * Returns absolute path contained within parent directory.
+ */
+export function getContainedPath(parentPath: string, subPath: string = ''): string | null {
+  const resolvedPath = path.resolve(parentPath, subPath);
+  if (resolvedPath === parentPath || resolvedPath.startsWith(parentPath + path.sep)) return resolvedPath;
+  return null;
+}
+
 export const debugTest = debug('pw:test');

src/test/workerRunner.ts

@@ -20,7 +20,7 @@ import rimraf from 'rimraf';
 import util from 'util';
 import colors from 'colors/safe';
 import { EventEmitter } from 'events';
-import { monotonicTime, serializeError, sanitizeForFilePath } from './util';
+import { monotonicTime, serializeError, sanitizeForFilePath, getContainedPath, addSuffixToFilePath } from './util';
 import { TestBeginPayload, TestEndPayload, RunPayload, TestEntry, DonePayload, WorkerInitParams, StepBeginPayload, StepEndPayload } from './ipc';
 import { setCurrentTestInfo } from './globals';
 import { Loader } from './loader';
@@ -255,20 +255,25 @@ export class WorkerRunner extends EventEmitter {
       outputDir: baseOutputDir,
       outputPath: (...pathSegments: string[]): string => {
         fs.mkdirSync(baseOutputDir, { recursive: true });
-        return path.join(baseOutputDir, ...pathSegments);
+        const joinedPath = path.join(...pathSegments);
+        const outputPath = getContainedPath(baseOutputDir, joinedPath);
+        if (outputPath) return outputPath;
+        throw new Error(`The outputPath is not allowed outside of the parent directory. Please fix the defined path.\n\n\toutputPath: ${joinedPath}`);
+
       },
-      snapshotPath: (snapshotName: string): string => {
+      snapshotPath: (...pathSegments: string[]): string => {
         let suffix = '';
         if (this._projectNamePathSegment)
           suffix += '-' + this._projectNamePathSegment;
         if (testInfo.snapshotSuffix)
           suffix += '-' + testInfo.snapshotSuffix;
-        const ext = path.extname(snapshotName);
-        if (ext)
-          snapshotName = sanitizeForFilePath(snapshotName.substring(0, snapshotName.length - ext.length)) + suffix + ext;
-        else
-          snapshotName = sanitizeForFilePath(snapshotName) + suffix;
-        return path.join(test._requireFile + '-snapshots', snapshotName);
+
+        const baseSnapshotPath = test._requireFile + '-snapshots';
+        const subPath = addSuffixToFilePath(path.join(...pathSegments), suffix);
+        const snapshotPath =  getContainedPath(baseSnapshotPath, subPath);
+
+        if (snapshotPath) return snapshotPath;
+        throw new Error(`The snapshotPath is not allowed outside of the parent directory. Please fix the defined path.\n\n\tsnapshotPath: ${subPath}`);
       },
       skip: (...args: [arg?: any, description?: string]) => modifier(testInfo, 'skip', args),
       fixme: (...args: [arg?: any, description?: string]) => modifier(testInfo, 'fixme', args),

src/utils/utils.ts

@@ -427,7 +427,7 @@ export async function removeFolders(dirs: string[]): Promise<Array<Error|undefin
   return await Promise.all(dirs.map((dir: string) => {
     return new Promise<Error|undefined>(fulfill => {
       removeFolder(dir, { maxBusyTries: 10 }, error => {
-        fulfill(error);
+        fulfill(error ?? undefined);
       });
     });
   }));

tests/playwright-test/golden.spec.ts

@@ -456,12 +456,12 @@ test('should respect project threshold', async ({ runInlineTest }) => {
   expect(result.exitCode).toBe(0);
 });
 
-test('should sanitize snapshot name', async ({ runInlineTest }) => {
+test('should sanitize snapshot name when passed as string', async ({ runInlineTest }) => {
   const result = await runInlineTest({
     ...files,
     'a.spec.js-snapshots/-snapshot-.txt': `Hello world`,
     'a.spec.js': `
-      const { test } = require('./helper');
+      const { test } = require('./helper');;
       test('is a test', ({}) => {
         expect('Hello world').toMatchSnapshot('../../snapshot!.txt');
       });
@@ -474,7 +474,7 @@ test('should write missing expectations with sanitized snapshot name', async ({
   const result = await runInlineTest({
     ...files,
     'a.spec.js': `
-      const { test } = require('./helper');
+      const { test } = require('./helper');;
       test('is a test', ({}) => {
         expect('Hello world').toMatchSnapshot('../../snapshot!.txt');
       });
@@ -488,6 +488,77 @@ test('should write missing expectations with sanitized snapshot name', async ({
   expect(data.toString()).toBe('Hello world');
 });
 
+test('should join array of snapshot path segments without sanitizing ', async ({ runInlineTest }) => {
+  const result = await runInlineTest({
+    ...files,
+    'a.spec.js-snapshots/test/path/snapshot.txt': `Hello world`,
+    'a.spec.js': `
+      const { test } = require('./helper');;
+      test('is a test', ({}) => {
+        expect('Hello world').toMatchSnapshot(['test', 'path', 'snapshot.txt']);
+      });
+    `
+  });
+  expect(result.exitCode).toBe(0);
+});
+
+test('should update snapshot with array of path segments', async ({ runInlineTest }, testInfo) => {
+  const result = await runInlineTest({
+    ...files,
+    'a.spec.js': `
+      const { test } = require('./helper');
+      test('is a test', ({}) => {
+        expect('Hello world').toMatchSnapshot(['test', 'path', 'snapshot.txt']);
+      });
+    `
+  }, { 'update-snapshots': true });
+
+  expect(result.exitCode).toBe(0);
+  const snapshotOutputPath = testInfo.outputPath('a.spec.js-snapshots/test/path/snapshot.txt');
+  expect(result.output).toContain(`${snapshotOutputPath} is missing in snapshots, writing actual`);
+  const data = fs.readFileSync(snapshotOutputPath);
+  expect(data.toString()).toBe('Hello world');
+});
+
+test('should attach expected/actual/diff with snapshot path', async ({ runInlineTest }, testInfo) => {
+  const result = await runInlineTest({
+    ...files,
+    'a.spec.js-snapshots/test/path/snapshot.png':
+        Buffer.from('iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAYAAAAfFcSJAAAADUlEQVR42mNk+P+/HgAFhAJ/wlseKgAAAABJRU5ErkJggg==', 'base64'),
+    'a.spec.js': `
+      const { test } = require('./helper');
+      test.afterEach(async ({}, testInfo) => {
+        console.log('## ' + JSON.stringify(testInfo.attachments));
+      });
+      test('is a test', ({}) => {
+        expect(Buffer.from('iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAQAAAC1HAwCAAAAC0lEQVQYV2NgYAAAAAMAAWgmWQ0AAAAASUVORK5CYII==', 'base64')).toMatchSnapshot(['test', 'path', 'snapshot.png']);
+      });
+    `
+  });
+
+  const outputText = stripAscii(result.output);
+  const attachments = outputText.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(/\\/g, '/').replace(/.*test-results\//, '');
+  expect(attachments).toEqual([
+    {
+      name: 'expected',
+      contentType: 'image/png',
+      path: 'a-is-a-test/test/path/snapshot-expected.png'
+    },
+    {
+      name: 'actual',
+      contentType: 'image/png',
+      path: 'a-is-a-test/test/path/snapshot-actual.png'
+    },
+    {
+      name: 'diff',
+      contentType: 'image/png',
+      path: 'a-is-a-test/test/path/snapshot-diff.png'
+    }
+  ]);
+});
+
 test('should attach expected/actual/diff', async ({ runInlineTest }, testInfo) => {
   const result = await runInlineTest({
     ...files,

tests/playwright-test/test-output-dir.spec.ts

@@ -216,6 +216,94 @@ test('should include the project name', async ({ runInlineTest }) => {
   expect(result.output).toContain('my-test.spec.js-snapshots/bar-Bar-space--suffix.txt');
 });
 
+test('should include path option in snapshot', async ({ runInlineTest }) => {
+  const result = await runInlineTest({
+    'helper.ts': `
+      export const test = pwt.test.extend({
+        auto: [ async ({}, run, testInfo) => {
+          testInfo.snapshotSuffix = 'suffix';
+          await run();
+        }, { auto: true } ]
+      });
+    `,
+    'playwright.config.ts': `
+    module.exports = { projects: [
+      { name: 'foo' },
+    ] };
+    `,
+    'my-test.spec.js': `
+      const { test } = require('./helper');
+      test('test with path', async ({}, testInfo) => {
+        console.log(testInfo.snapshotPath('test', 'path', 'bar.txt').replace(/\\\\/g, '/'));
+      });
+    `,
+  });
+
+  expect(result.exitCode).toBe(0);
+  expect(result.results[0].status).toBe('passed');
+  expect(result.output).toContain('my-test.spec.js-snapshots/test/path/bar-foo-suffix.txt');
+});
+
+test('should error if snapshotPath is resolved to outside of parent', async ({ runInlineTest }) => {
+  const result = await runInlineTest({
+    'helper.ts': `
+      export const test = pwt.test.extend({
+        auto: [ async ({}, run, testInfo) => {
+          testInfo.snapshotSuffix = 'suffix';
+          await run();
+        }, { auto: true } ]
+      });
+    `,
+    'playwright.config.ts': `
+      module.exports = { projects: [
+        { name: 'foo' },
+      ] };
+    `,
+    'my-test.spec.js': `
+      const { test } = require('./helper');
+      test('test with parent path', async ({}, testInfo) => {
+        console.log(testInfo.snapshotPath('..', 'test', 'path', 'bar.txt').replace(/\\\\/g, '/'));
+      });
+    `,
+  });
+
+  expect(result.exitCode).toBe(1);
+  expect(result.results[0].status).toBe('failed');
+  expect(result.output).toContain('The snapshotPath is not allowed outside of the parent directory. Please fix the defined path.');
+  const badPath = path.join('..', 'test', 'path', 'bar-foo-suffix.txt');
+  expect(result.output).toContain(`snapshotPath: ${badPath}`);
+});
+
+test('should error if outputPath is resolved to outside of parent', async ({ runInlineTest }) => {
+  const result = await runInlineTest({
+    'helper.ts': `
+      export const test = pwt.test.extend({
+        auto: [ async ({}, run, testInfo) => {
+          testInfo.snapshotSuffix = 'suffix';
+          await run();
+        }, { auto: true } ]
+      });
+    `,
+    'playwright.config.ts': `
+      module.exports = { projects: [
+        { name: 'foo' },
+      ] };
+    `,
+    'my-test.spec.js': `
+      const { test } = require('./helper');
+      test('test with parent path', async ({}, testInfo) => {
+        console.log(testInfo.outputPath('..', 'test', 'path', 'bar-test').replace(/\\\\/g, '/'));
+      });
+    `,
+  });
+
+  expect(result.exitCode).toBe(1);
+  expect(result.results[0].status).toBe('failed');
+  expect(result.output).toContain('The outputPath is not allowed outside of the parent directory. Please fix the defined path.');
+  const badPath = path.join('..', 'test', 'path', 'bar-test');
+  expect(result.output).toContain(`outputPath: ${badPath}`);
+});
+
 test('should remove output dirs for projects run', async ({ runInlineTest }, testInfo) => {
   const paths: string[] = [];
   const files: string[] = [];

types/test.d.ts

@@ -1101,10 +1101,15 @@ export interface TestInfo {
    */
   outputDir: string;
   /**
-   * Returns a path to a snapshot file with the given `snapshotName`. Learn more about [snapshots](https://playwright.dev/docs/test-snapshots).
-   * @param snapshotName
+   * Returns a path to a snapshot file with the given `pathSegments`. Learn more about [snapshots](https://playwright.dev/docs/test-snapshots).
+   *
+   * > Note that `pathSegments` accepts path segments to the snapshot file such as `testInfo.snapshotPath('relative', 'path',
+   * 'to', 'snapshot.png')`.
+   * > However, this path must stay within the snapshots directory for each test file (i.e. `a.spec.js-snapshots`), otherwise
+   * it will throw.
+   * @param pathSegments The name of the snapshot or the path segments to define the snapshot file path. Snapshots with the same name in the same test file are expected to be the same.
    */
-  snapshotPath: (snapshotName: string) => string;
+  snapshotPath: (...pathSegments: string[]) => string;
   /**
    * Returns a path inside the [testInfo.outputDir](https://playwright.dev/docs/api/class-testinfo#test-info-output-dir)
    * where the test can safely put a temporary file. Guarantees that tests running in parallel will not interfere with each
@@ -1120,6 +1125,11 @@ export interface TestInfo {
    * });
    * ```
    *
+   * > Note that `pathSegments` accepts path segments to the test output directory such as `testInfo.outputPath('relative',
+   * 'path', 'to', 'output')`.
+   * > However, this path must stay within the
+   * [testInfo.outputDir](https://playwright.dev/docs/api/class-testinfo#test-info-output-dir) directory for each test (i.e.
+   * `test-results/a-test-title`), otherwise it will throw.
    * @param pathSegments Path segments to append at the end of the resulting path.
    */
   outputPath: (...pathSegments: string[]) => string;

utils/generate_types/overrides-test.d.ts

@@ -208,7 +208,7 @@ export interface TestInfo {
   stderr: (string | Buffer)[];
   snapshotSuffix: string;
   outputDir: string;
-  snapshotPath: (snapshotName: string) => string;
+  snapshotPath: (...pathSegments: string[]) => string;
   outputPath: (...pathSegments: string[]) => string;
 }