From b126a5685b0e047b19bbf54ec7bc753cba1efa8d Mon Sep 17 00:00:00 2001 From: Nick Partridge Date: Fri, 1 Oct 2021 11:15:44 -0500 Subject: [PATCH] feat: add path option to `toMatchSnapshot` (#9156) --- docs/src/test-advanced-js.md | 2 +- docs/src/test-api/class-testinfo.md | 15 +++- docs/src/test-snapshots-js.md | 3 +- src/test/matchers/golden.ts | 29 +++--- src/test/matchers/toMatchSnapshot.ts | 12 ++- src/test/util.ts | 17 ++++ src/test/workerRunner.ts | 23 +++-- src/utils/utils.ts | 2 +- tests/playwright-test/golden.spec.ts | 77 +++++++++++++++- tests/playwright-test/test-output-dir.spec.ts | 88 +++++++++++++++++++ types/test.d.ts | 16 +++- utils/generate_types/overrides-test.d.ts | 2 +- 12 files changed, 243 insertions(+), 43 deletions(-) diff --git a/docs/src/test-advanced-js.md b/docs/src/test-advanced-js.md index 5d606d91f4..1c668d98d4 100644 --- a/docs/src/test-advanced-js.md +++ b/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. diff --git a/docs/src/test-api/class-testinfo.md b/docs/src/test-api/class-testinfo.md index c0cc611ebb..c8ca2e6ad3 100644 --- a/docs/src/test-api/class-testinfo.md +++ b/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]> diff --git a/docs/src/test-snapshots-js.md b/docs/src/test-snapshots-js.md index ea804e2daf..5c67d2c7e6 100644 --- a/docs/src/test-snapshots-js.md +++ b/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. diff --git a/src/test/matchers/golden.ts b/src/test/matchers/golden.ts index 14de9904ab..d1a04c36ff 100644 --- a/src/test/matchers/golden.ts +++ b/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++) { diff --git a/src/test/matchers/toMatchSnapshot.ts b/src/test/matchers/toMatchSnapshot.ts index f1d9b7b600..64802f0be9 100644 --- a/src/test/matchers/toMatchSnapshot.ts +++ b/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, 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, 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, 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, diff --git a/src/test/util.ts b/src/test/util.ts index 3977a76477..a7c84ebdfc 100644 --- a/src/test/util.ts +++ b/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'); diff --git a/src/test/workerRunner.ts b/src/test/workerRunner.ts index cfc2d983db..6555c449c8 100644 --- a/src/test/workerRunner.ts +++ b/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), diff --git a/src/utils/utils.ts b/src/utils/utils.ts index 3663186184..9f0030232c 100644 --- a/src/utils/utils.ts +++ b/src/utils/utils.ts @@ -427,7 +427,7 @@ export async function removeFolders(dirs: string[]): Promise { return new Promise(fulfill => { removeFolder(dir, { maxBusyTries: 10 }, error => { - fulfill(error); + fulfill(error ?? undefined); }); }); })); diff --git a/tests/playwright-test/golden.spec.ts b/tests/playwright-test/golden.spec.ts index c3cd866a70..5e77f2d066 100644 --- a/tests/playwright-test/golden.spec.ts +++ b/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, diff --git a/tests/playwright-test/test-output-dir.spec.ts b/tests/playwright-test/test-output-dir.spec.ts index efe8753859..a66940df4f 100644 --- a/tests/playwright-test/test-output-dir.spec.ts +++ b/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[] = []; diff --git a/types/test.d.ts b/types/test.d.ts index ae8fbc8bd2..fd11bb62e5 100644 --- a/types/test.d.ts +++ b/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; diff --git a/utils/generate_types/overrides-test.d.ts b/utils/generate_types/overrides-test.d.ts index 48f9c0074c..e078a27fa3 100644 --- a/utils/generate_types/overrides-test.d.ts +++ b/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; }