From 4044f4a8ff6c0fec482dc70ac031c1e3722e827b Mon Sep 17 00:00:00 2001 From: nickofthyme Date: Sun, 26 Sep 2021 11:23:40 -0500 Subject: [PATCH 01/17] feat: add path option to toMatchSnapshot adds optional path to toMathchSnapshot assertion. --- src/test/matchers/golden.ts | 6 +-- src/test/util.ts | 10 ++++ src/test/workerRunner.ts | 9 ++-- tests/playwright-test/golden.spec.ts | 48 +++++++++++++++++++ tests/playwright-test/test-output-dir.spec.ts | 33 +++++++++++++ types/test.d.ts | 2 +- utils/generate_types/overrides-test.d.ts | 2 +- 7 files changed, 102 insertions(+), 8 deletions(-) diff --git a/src/test/matchers/golden.ts b/src/test/matchers/golden.ts index 14de9904ab697..e3700ff49b2f0 100644 --- a/src/test/matchers/golden.ts +++ b/src/test/matchers/golden.ts @@ -83,13 +83,13 @@ function compareText(actual: Buffer | string, expectedBuffer: Buffer): { diff?: export function compare( actual: Buffer | string, name: string, - snapshotPath: (name: string) => string, + snapshotPath: (name: string, path?: string) => string, outputPath: (name: string) => string, updateSnapshots: UpdateSnapshots, withNegateComparison: boolean, - options?: { threshold?: number } + options?: { threshold?: number, path?: string } ): { pass: boolean; message?: string; expectedPath?: string, actualPath?: string, diffPath?: string, mimeType?: string } { - const snapshotFile = snapshotPath(name); + const snapshotFile = snapshotPath(name, options?.path); const outputFile = outputPath(name); const expectedPath = addSuffix(outputFile, '-expected'); const actualPath = addSuffix(outputFile, '-actual'); diff --git a/src/test/util.ts b/src/test/util.ts index 3977a76477e7d..cd400843b2ec8 100644 --- a/src/test/util.ts +++ b/src/test/util.ts @@ -144,4 +144,14 @@ export function sanitizeForFilePath(s: string) { return s.replace(/[\x00-\x2F\x3A-\x40\x5B-\x60\x7B-\x7F]+/g, '-'); } +/** + * prevent parent directory traversal + */ +export function sanitizeForParentPath(parentPath: string, p?: string): string { + if (!p) return ''; + + const joinedPath = path.join(parentPath, path.normalize(p)); + return /\.{2,}\//.test(path.relative(parentPath, joinedPath)) ? '' : path.normalize(p); +} + export const debugTest = debug('pw:test'); diff --git a/src/test/workerRunner.ts b/src/test/workerRunner.ts index fe300a810104a..f74b78e13120a 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, sanitizeForParentPath } from './util'; import { TestBeginPayload, TestEndPayload, RunPayload, TestEntry, DonePayload, WorkerInitParams, StepBeginPayload, StepEndPayload } from './ipc'; import { setCurrentTestInfo } from './globals'; import { Loader } from './loader'; @@ -244,7 +244,7 @@ export class WorkerRunner extends EventEmitter { fs.mkdirSync(baseOutputDir, { recursive: true }); return path.join(baseOutputDir, ...pathSegments); }, - snapshotPath: (snapshotName: string): string => { + snapshotPath: (snapshotName: string, snapshotPath?: string): string => { let suffix = ''; if (this._projectNamePathSegment) suffix += '-' + this._projectNamePathSegment; @@ -255,7 +255,10 @@ export class WorkerRunner extends EventEmitter { 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 = sanitizeForParentPath(test._requireFile, snapshotPath); + + return path.join(baseSnapshotPath, subPath, snapshotName); }, skip: (...args: [arg?: any, description?: string]) => modifier(testInfo, 'skip', args), fixme: (...args: [arg?: any, description?: string]) => modifier(testInfo, 'fixme', args), diff --git a/tests/playwright-test/golden.spec.ts b/tests/playwright-test/golden.spec.ts index b43866edb5325..f6ec2af744406 100644 --- a/tests/playwright-test/golden.spec.ts +++ b/tests/playwright-test/golden.spec.ts @@ -488,6 +488,54 @@ test('should write missing expectations with sanitized snapshot name', async ({r expect(data.toString()).toBe('Hello world'); }); +test('should lookup snapshot with path', 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('snapshot.txt', { path: 'test/path' }); + }); + ` + }); + + expect(result.exitCode).toBe(0); +}); + +test('should set snapshot with path', async ({ runInlineTest }, testInfo) => { + const result = await runInlineTest({ + ...files, + 'a.spec.js': ` + const { test } = require('./helper'); + test('is a test', ({}) => { + expect('Hello world').toMatchSnapshot('snapshot.txt', { path: 'test/path' }); + }); + ` + }, { '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 fail to write snapshot with parent path', async ({ runInlineTest }) => { + const result = await runInlineTest({ + ...files, + 'test/snapshot.txt': `Hello world`, + 'a.spec.js': ` + const { test } = require('./helper'); + test('is a test', ({}) => { + expect('Hello world').toMatchSnapshot('snapshot.txt', { path: "../test" }); + }); + ` + }); + + expect(result.exitCode).toBe(1); +}); + 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 2f5d571426845..716801b8a216d 100644 --- a/tests/playwright-test/test-output-dir.spec.ts +++ b/tests/playwright-test/test-output-dir.spec.ts @@ -216,6 +216,39 @@ 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('bar.txt', 'test/path').replace(/\\\\/g, '/')); + }); + test('test with parent path', async ({}, testInfo) => { + console.log(testInfo.snapshotPath('bar.txt', '../test/path').replace(/\\\\/g, '/')); + }); + `, + }); + expect(result.exitCode).toBe(0); + expect(result.results[0].status).toBe('passed'); + expect(result.results[1].status).toBe('passed'); // reverts to base snapshot path + + expect(result.output).toContain('my-test.spec.js-snapshots/test/path/bar-foo-suffix.txt'); + expect(result.output).toContain('my-test.spec.js-snapshots/bar-foo-suffix.txt'); +}); + 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 8375c4f8e3077..da9ae1d4fba83 100644 --- a/types/test.d.ts +++ b/types/test.d.ts @@ -1104,7 +1104,7 @@ export interface TestInfo { * Returns a path to a snapshot file with the given `snapshotName`. Learn more about [snapshots](https://playwright.dev/docs/test-snapshots). * @param snapshotName */ - snapshotPath: (snapshotName: string) => string; + snapshotPath: (snapshotName: string, path?: 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 diff --git a/utils/generate_types/overrides-test.d.ts b/utils/generate_types/overrides-test.d.ts index 0dc3004bd10d0..42bbfa3196041 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: (snapshotName: string, path?: string) => string; outputPath: (...pathSegments: string[]) => string; } From 0d0510afe0d687afc14249cb543e57462bb487de Mon Sep 17 00:00:00 2001 From: nickofthyme Date: Mon, 27 Sep 2021 17:12:22 -0500 Subject: [PATCH 02/17] refactor: validate contained path using full path --- src/test/util.ts | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/src/test/util.ts b/src/test/util.ts index cd400843b2ec8..b7e528e4fa071 100644 --- a/src/test/util.ts +++ b/src/test/util.ts @@ -145,13 +145,12 @@ export function sanitizeForFilePath(s: string) { } /** - * prevent parent directory traversal + * Get absolute path contained within parent directory, otherwise returns parent path */ -export function sanitizeForParentPath(parentPath: string, p?: string): string { - if (!p) return ''; - - const joinedPath = path.join(parentPath, path.normalize(p)); - return /\.{2,}\//.test(path.relative(parentPath, joinedPath)) ? '' : path.normalize(p); +export function getContainedPath(parentPath: string, subPath?: string): string { + if (!subPath) return parentPath; + const resolvedPath = path.resolve(parentPath, subPath); + return resolvedPath.startsWith(parentPath) ? resolvedPath : path.join(parentPath, path.basename(subPath)); } export const debugTest = debug('pw:test'); From 9b9b677ea3b30b6d80180d1233e0e9bf51da272a Mon Sep 17 00:00:00 2001 From: nickofthyme Date: Mon, 27 Sep 2021 17:44:19 -0500 Subject: [PATCH 03/17] refactor: revert path sanitization on path and add contained check --- docs/src/test-snapshots-js.md | 3 +- src/test/matchers/golden.ts | 8 ++- src/test/workerRunner.ts | 22 ++++---- tests/playwright-test/golden.spec.ts | 55 ++++++++++++++----- tests/playwright-test/test-output-dir.spec.ts | 5 +- types/test.d.ts | 2 +- utils/generate_types/overrides-test.d.ts | 2 +- 7 files changed, 63 insertions(+), 34 deletions(-) diff --git a/docs/src/test-snapshots-js.md b/docs/src/test-snapshots-js.md index ea804e2daf75b..4d1e9665a61e1 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 a relative path to the snapshot file such as `expect(value).toMatchSnapshot('test-snapshots/snapshot.png')`. +> However, this path is enfored to be within the snapshots directory for each test file (i.e. `a.spec.js-snapshots`) and will be ignored otherwise. 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 e3700ff49b2f0..2f382bc72fcb3 100644 --- a/src/test/matchers/golden.ts +++ b/src/test/matchers/golden.ts @@ -83,13 +83,13 @@ function compareText(actual: Buffer | string, expectedBuffer: Buffer): { diff?: export function compare( actual: Buffer | string, name: string, - snapshotPath: (name: string, path?: string) => string, + snapshotPath: (name: string) => string, outputPath: (name: string) => string, updateSnapshots: UpdateSnapshots, withNegateComparison: boolean, - options?: { threshold?: number, path?: string } + options?: { threshold?: number } ): { pass: boolean; message?: string; expectedPath?: string, actualPath?: string, diffPath?: string, mimeType?: string } { - const snapshotFile = snapshotPath(name, options?.path); + const snapshotFile = snapshotPath(name); const outputFile = outputPath(name); const expectedPath = addSuffix(outputFile, '-expected'); const actualPath = addSuffix(outputFile, '-actual'); @@ -104,6 +104,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 +160,7 @@ export function compare( }; } + fs.mkdirSync(path.dirname(expectedPath), { recursive: true }); fs.writeFileSync(expectedPath, expected); fs.writeFileSync(actualPath, actual); if (result.diff) diff --git a/src/test/workerRunner.ts b/src/test/workerRunner.ts index f74b78e13120a..17e8ee26559d4 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, sanitizeForParentPath } from './util'; +import { monotonicTime, serializeError, sanitizeForFilePath, getContainedPath } from './util'; import { TestBeginPayload, TestEndPayload, RunPayload, TestEntry, DonePayload, WorkerInitParams, StepBeginPayload, StepEndPayload } from './ipc'; import { setCurrentTestInfo } from './globals'; import { Loader } from './loader'; @@ -242,23 +242,23 @@ export class WorkerRunner extends EventEmitter { outputDir: baseOutputDir, outputPath: (...pathSegments: string[]): string => { fs.mkdirSync(baseOutputDir, { recursive: true }); - return path.join(baseOutputDir, ...pathSegments); + return getContainedPath(baseOutputDir, path.join(...pathSegments)); }, - snapshotPath: (snapshotName: string, snapshotPath?: string): string => { + snapshotPath: (snapshotName: 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; + if (suffix) { + const ext = path.extname(snapshotName); + if (ext) + snapshotName = snapshotName.substring(0, snapshotName.length - ext.length) + suffix + ext; + else + snapshotName += suffix; + } const baseSnapshotPath = test._requireFile + '-snapshots'; - const subPath = sanitizeForParentPath(test._requireFile, snapshotPath); - - return path.join(baseSnapshotPath, subPath, snapshotName); + return getContainedPath(baseSnapshotPath, snapshotName); }, skip: (...args: [arg?: any, description?: string]) => modifier(testInfo, 'skip', args), fixme: (...args: [arg?: any, description?: string]) => modifier(testInfo, 'fixme', args), diff --git a/tests/playwright-test/golden.spec.ts b/tests/playwright-test/golden.spec.ts index f6ec2af744406..4c5ae7e792d06 100644 --- a/tests/playwright-test/golden.spec.ts +++ b/tests/playwright-test/golden.spec.ts @@ -456,33 +456,34 @@ test('should respect project threshold', async ({runInlineTest}) => { expect(result.exitCode).toBe(0); }); -test('should sanitize snapshot name', async ({runInlineTest}) => { +test('should contain snapshot name/path to snapshot directory', async ({ runInlineTest }) => { const result = await runInlineTest({ ...files, - 'a.spec.js-snapshots/-snapshot-.txt': `Hello world`, + 'a.spec.js-snapshots/snapshot.txt': `Hello world`, 'a.spec.js': ` const { test } = require('./helper'); test('is a test', ({}) => { - expect('Hello world').toMatchSnapshot('../../snapshot!.txt'); + expect('Hello world').toMatchSnapshot('../test/snapshot.txt'); }); ` }); + expect(result.exitCode).toBe(0); }); -test('should write missing expectations with sanitized snapshot name', async ({runInlineTest}, testInfo) => { +test('should write missing expectations to contained outputPath using basename', async ({runInlineTest}, testInfo) => { const result = await runInlineTest({ ...files, 'a.spec.js': ` const { test } = require('./helper'); test('is a test', ({}) => { - expect('Hello world').toMatchSnapshot('../../snapshot!.txt'); + expect('Hello world').toMatchSnapshot('../test/snapshot.txt'); }); ` - }, {}, { CI: '' }); + }); expect(result.exitCode).toBe(1); - const snapshotOutputPath = testInfo.outputPath('a.spec.js-snapshots/-snapshot-.txt'); + const snapshotOutputPath = testInfo.outputPath('a.spec.js-snapshots/snapshot.txt'); expect(result.output).toContain(`${snapshotOutputPath} is missing in snapshots, writing actual`); const data = fs.readFileSync(snapshotOutputPath); expect(data.toString()).toBe('Hello world'); @@ -495,7 +496,7 @@ test('should lookup snapshot with path', async ({ runInlineTest }) => { 'a.spec.js': ` const { test } = require('./helper'); test('is a test', ({}) => { - expect('Hello world').toMatchSnapshot('snapshot.txt', { path: 'test/path' }); + expect('Hello world').toMatchSnapshot('test/path/snapshot.txt'); }); ` }); @@ -503,13 +504,13 @@ test('should lookup snapshot with path', async ({ runInlineTest }) => { expect(result.exitCode).toBe(0); }); -test('should set snapshot with path', async ({ runInlineTest }, testInfo) => { +test('should update snapshot with path', async ({ runInlineTest }, testInfo) => { const result = await runInlineTest({ ...files, 'a.spec.js': ` const { test } = require('./helper'); test('is a test', ({}) => { - expect('Hello world').toMatchSnapshot('snapshot.txt', { path: 'test/path' }); + expect('Hello world').toMatchSnapshot('test/path/snapshot.txt'); }); ` }, { 'update-snapshots': true }); @@ -521,22 +522,46 @@ test('should set snapshot with path', async ({ runInlineTest }, testInfo) => { expect(data.toString()).toBe('Hello world'); }); -test('should fail to write snapshot with parent path', async ({ runInlineTest }) => { +test('should attach expected/actual/diff with snapshot path', async ({runInlineTest}, testInfo) => { const result = await runInlineTest({ ...files, - 'test/snapshot.txt': `Hello world`, + '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('Hello world').toMatchSnapshot('snapshot.txt', { path: "../test" }); + expect(Buffer.from('iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAQAAAC1HAwCAAAAC0lEQVQYV2NgYAAAAAMAAWgmWQ0AAAAASUVORK5CYII==', 'base64')).toMatchSnapshot('test/path/snapshot.png'); }); ` }); - expect(result.exitCode).toBe(1); + 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) => { +test('should attach expected/actual/diff', async ({ runInlineTest }, testInfo) => { const result = await runInlineTest({ ...files, 'a.spec.js-snapshots/snapshot.png': diff --git a/tests/playwright-test/test-output-dir.spec.ts b/tests/playwright-test/test-output-dir.spec.ts index 716801b8a216d..713be33df70c1 100644 --- a/tests/playwright-test/test-output-dir.spec.ts +++ b/tests/playwright-test/test-output-dir.spec.ts @@ -234,13 +234,14 @@ test('should include path option in snapshot', async ({ runInlineTest }) => { 'my-test.spec.js': ` const { test } = require('./helper'); test('test with path', async ({}, testInfo) => { - console.log(testInfo.snapshotPath('bar.txt', 'test/path').replace(/\\\\/g, '/')); + console.log(testInfo.snapshotPath('test/path/bar.txt').replace(/\\\\/g, '/')); }); test('test with parent path', async ({}, testInfo) => { - console.log(testInfo.snapshotPath('bar.txt', '../test/path').replace(/\\\\/g, '/')); + console.log(testInfo.snapshotPath('../test/path/bar.txt').replace(/\\\\/g, '/')); }); `, }); + expect(result.exitCode).toBe(0); expect(result.results[0].status).toBe('passed'); expect(result.results[1].status).toBe('passed'); // reverts to base snapshot path diff --git a/types/test.d.ts b/types/test.d.ts index da9ae1d4fba83..8375c4f8e3077 100644 --- a/types/test.d.ts +++ b/types/test.d.ts @@ -1104,7 +1104,7 @@ export interface TestInfo { * Returns a path to a snapshot file with the given `snapshotName`. Learn more about [snapshots](https://playwright.dev/docs/test-snapshots). * @param snapshotName */ - snapshotPath: (snapshotName: string, path?: string) => string; + snapshotPath: (snapshotName: 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 diff --git a/utils/generate_types/overrides-test.d.ts b/utils/generate_types/overrides-test.d.ts index 42bbfa3196041..0dc3004bd10d0 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, path?: string) => string; + snapshotPath: (snapshotName: string) => string; outputPath: (...pathSegments: string[]) => string; } From e8998577873cacb13dc687db038c15bc43e38a77 Mon Sep 17 00:00:00 2001 From: nickofthyme Date: Tue, 28 Sep 2021 20:27:25 -0500 Subject: [PATCH 04/17] docs: update docs with path segment logic --- docs/src/test-api/class-testinfo.md | 15 +++++++++++---- docs/src/test-snapshots-js.md | 4 ++-- types/test.d.ts | 11 ++++++++--- 3 files changed, 21 insertions(+), 9 deletions(-) diff --git a/docs/src/test-api/class-testinfo.md b/docs/src/test-api/class-testinfo.md index c0cc611ebb162..6ebdddd3a8773 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 an array of path segments to the test output directory such as `testInfo.outputPath('relative', 'path', 'to', 'output')`. +> However, this path must stay within the snapshots directory for each test file (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). + +> Note that `pathSegments` accepts an array of 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...]> -### 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. +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 4d1e9665a61e1..8f94c82517336 100644 --- a/docs/src/test-snapshots-js.md +++ b/docs/src/test-snapshots-js.md @@ -55,8 +55,8 @@ Sometimes you need to update the reference screenshot, for example when the page npx playwright test --update-snapshots ``` -> Note that `snapshotName` also accepts a relative path to the snapshot file such as `expect(value).toMatchSnapshot('test-snapshots/snapshot.png')`. -> However, this path is enfored to be within the snapshots directory for each test file (i.e. `a.spec.js-snapshots`) and will be ignored otherwise. +> 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/types/test.d.ts b/types/test.d.ts index ae8fbc8bd2587..4290f3530c180 100644 --- a/types/test.d.ts +++ b/types/test.d.ts @@ -1101,10 +1101,13 @@ 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 an array of 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 file 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 +1123,8 @@ export interface TestInfo { * }); * ``` * + * > Note that `pathSegments` accepts an array of path segments to the test output directory such as `testInfo.outputPath('relative', 'path', 'to', 'output')`. + * > However, this path must stay within the snapshots directory for each test file (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; From df1e5787c02c1d687ce2faa4f55561878b2199d9 Mon Sep 17 00:00:00 2001 From: nickofthyme Date: Tue, 28 Sep 2021 20:09:32 -0500 Subject: [PATCH 05/17] chore: add error message for contained path method --- src/test/util.ts | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/src/test/util.ts b/src/test/util.ts index b7e528e4fa071..b1b7739b51745 100644 --- a/src/test/util.ts +++ b/src/test/util.ts @@ -145,12 +145,17 @@ export function sanitizeForFilePath(s: string) { } /** - * Get absolute path contained within parent directory, otherwise returns parent path + * Returns absolute path contained within parent directory. */ -export function getContainedPath(parentPath: string, subPath?: string): string { - if (!subPath) return parentPath; +export function getContainedPath(parentPath: string, subPath: string = ''): string { const resolvedPath = path.resolve(parentPath, subPath); - return resolvedPath.startsWith(parentPath) ? resolvedPath : path.join(parentPath, path.basename(subPath)); + + if (resolvedPath.startsWith(parentPath)) return resolvedPath; + + throw new Error(`Path defined outside of parent directory + path: ${subPath} + resolved path: ${resolvedPath} + parent: ${parentPath}`); } export const debugTest = debug('pw:test'); From 93d9632b3d43b0b3860b4e45f96a86e30f126ec4 Mon Sep 17 00:00:00 2001 From: nickofthyme Date: Tue, 28 Sep 2021 20:24:59 -0500 Subject: [PATCH 06/17] refactor: use name and pathSegments union type, allow toMatchSnapshot array arg --- src/test/matchers/golden.ts | 21 ++++++--------- src/test/matchers/toMatchSnapshot.ts | 11 +++++--- src/test/util.ts | 8 ++++++ src/test/workerRunner.ts | 17 +++++------- tests/playwright-test/golden.spec.ts | 39 ++++++++++++---------------- 5 files changed, 47 insertions(+), 49 deletions(-) diff --git a/src/test/matchers/golden.ts b/src/test/matchers/golden.ts index 2f382bc72fcb3..af856b86fbc97 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 { addSuffix } 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,15 +83,15 @@ 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 snapshotFile = snapshotPath(...pathSegments); + const outputFile = outputPath(...pathSegments); const expectedPath = addSuffix(outputFile, '-expected'); const actualPath = addSuffix(outputFile, '-actual'); const diffPath = addSuffix(outputFile, '-diff'); @@ -161,6 +162,7 @@ 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) @@ -193,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 c987b032a080f..1bc979ce66d8c 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 { addSuffix } 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,11 @@ export function toMatchSnapshot(this: ReturnType, received: if (options.threshold === undefined && projectThreshold !== undefined) options.threshold = projectThreshold; + const pathSegments = Array.isArray(options.name) ? options.name : [addSuffix(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.config.updateSnapshots, diff --git a/src/test/util.ts b/src/test/util.ts index b1b7739b51745..9fc47a046c6bb 100644 --- a/src/test/util.ts +++ b/src/test/util.ts @@ -144,6 +144,14 @@ export function sanitizeForFilePath(s: string) { return s.replace(/[\x00-\x2F\x3A-\x40\x5B-\x60\x7B-\x7F]+/g, '-'); } +export function addSuffix(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. */ diff --git a/src/test/workerRunner.ts b/src/test/workerRunner.ts index 17e8ee26559d4..415a911ffcd61 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, getContainedPath } from './util'; +import { monotonicTime, serializeError, sanitizeForFilePath, getContainedPath, addSuffix } from './util'; import { TestBeginPayload, TestEndPayload, RunPayload, TestEntry, DonePayload, WorkerInitParams, StepBeginPayload, StepEndPayload } from './ipc'; import { setCurrentTestInfo } from './globals'; import { Loader } from './loader'; @@ -244,21 +244,18 @@ export class WorkerRunner extends EventEmitter { fs.mkdirSync(baseOutputDir, { recursive: true }); return getContainedPath(baseOutputDir, path.join(...pathSegments)); }, - snapshotPath: (snapshotName: string): string => { + snapshotPath: (...pathSegments: string[]): string => { let suffix = ''; if (this._projectNamePathSegment) suffix += '-' + this._projectNamePathSegment; if (testInfo.snapshotSuffix) suffix += '-' + testInfo.snapshotSuffix; - if (suffix) { - const ext = path.extname(snapshotName); - if (ext) - snapshotName = snapshotName.substring(0, snapshotName.length - ext.length) + suffix + ext; - else - snapshotName += suffix; - } + + const snapshotPath = path.join(...pathSegments.map((segment, i, { length }) => + (i < length - 1) ? segment : addSuffix(segment, suffix), + )); const baseSnapshotPath = test._requireFile + '-snapshots'; - return getContainedPath(baseSnapshotPath, snapshotName); + return getContainedPath(baseSnapshotPath, snapshotPath); }, skip: (...args: [arg?: any, description?: string]) => modifier(testInfo, 'skip', args), fixme: (...args: [arg?: any, description?: string]) => modifier(testInfo, 'fixme', args), diff --git a/tests/playwright-test/golden.spec.ts b/tests/playwright-test/golden.spec.ts index a93ab973de512..6ec01973859dc 100644 --- a/tests/playwright-test/golden.spec.ts +++ b/tests/playwright-test/golden.spec.ts @@ -456,61 +456,56 @@ 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-snapshots/-snapshot--darwin.txt': `Hello world`, 'a.spec.js': ` - const { test } = require('./helper'); + const { test } = pwt; test('is a test', ({}) => { - expect('Hello world').toMatchSnapshot('../test/snapshot.txt'); + expect('Hello world').toMatchSnapshot('../../snapshot!.txt'); }); ` }); - expect(result.exitCode).toBe(0); }); -test('should write missing expectations to contained outputPath using basename', async ({ runInlineTest }, testInfo) => { +test('should write missing expectations with sanitized snapshot name', async ({ runInlineTest }, testInfo) => { const result = await runInlineTest({ - ...files, 'a.spec.js': ` - const { test } = require('./helper'); + const { test } = pwt; test('is a test', ({}) => { - expect('Hello world').toMatchSnapshot('../test/snapshot.txt'); + expect('Hello world').toMatchSnapshot('../../snapshot!.txt'); }); ` - }); + }, {}, { CI: '' }); expect(result.exitCode).toBe(1); - const snapshotOutputPath = testInfo.outputPath('a.spec.js-snapshots/snapshot.txt'); + const snapshotOutputPath = testInfo.outputPath('a.spec.js-snapshots/-snapshot--darwin.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 lookup snapshot with path', async ({ runInlineTest }) => { +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-snapshots/test/path/snapshot-darwin.txt': `Hello world`, 'a.spec.js': ` - const { test } = require('./helper'); + const { test } = pwt; test('is a test', ({}) => { - expect('Hello world').toMatchSnapshot('test/path/snapshot.txt'); + expect('Hello world').toMatchSnapshot(['test', 'path', 'snapshot.txt']); }); ` }); - expect(result.exitCode).toBe(0); }); -test('should update snapshot with path', async ({ runInlineTest }, testInfo) => { +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'); + expect('Hello world').toMatchSnapshot(['test', 'path', 'snapshot.txt']); }); ` }, { 'update-snapshots': true }); @@ -522,7 +517,7 @@ test('should update snapshot with path', async ({ runInlineTest }, testInfo) => expect(data.toString()).toBe('Hello world'); }); -test('should attach expected/actual/diff with snapshot path', async ({ runInlineTest }, testInfo) => { +test.only('should attach expected/actual/diff with snapshot path', async ({ runInlineTest }, testInfo) => { const result = await runInlineTest({ ...files, 'a.spec.js-snapshots/test/path/snapshot.png': @@ -533,7 +528,7 @@ test('should attach expected/actual/diff with snapshot path', async ({ runInline console.log('## ' + JSON.stringify(testInfo.attachments)); }); test('is a test', ({}) => { - expect(Buffer.from('iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAQAAAC1HAwCAAAAC0lEQVQYV2NgYAAAAAMAAWgmWQ0AAAAASUVORK5CYII==', 'base64')).toMatchSnapshot('test/path/snapshot.png'); + expect(Buffer.from('iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAQAAAC1HAwCAAAAC0lEQVQYV2NgYAAAAAMAAWgmWQ0AAAAASUVORK5CYII==', 'base64')).toMatchSnapshot(['test', 'path', 'snapshot.png']); }); ` }); From 45f532935a0157fab8f592a5cef88919e64ab287 Mon Sep 17 00:00:00 2001 From: Nick Partridge Date: Wed, 29 Sep 2021 20:50:21 -0500 Subject: [PATCH 07/17] chore: apply suggestions from code review Co-authored-by: Dmitry Gozman --- docs/src/test-api/class-testinfo.md | 2 +- docs/src/test-snapshots-js.md | 4 ++-- src/test/util.ts | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/docs/src/test-api/class-testinfo.md b/docs/src/test-api/class-testinfo.md index 6ebdddd3a8773..99f96d42c02eb 100644 --- a/docs/src/test-api/class-testinfo.md +++ b/docs/src/test-api/class-testinfo.md @@ -194,7 +194,7 @@ test('example test', async ({}, testInfo) => { ``` > Note that `pathSegments` accepts an array of path segments to the test output directory such as `testInfo.outputPath('relative', 'path', 'to', 'output')`. -> However, this path must stay within the snapshots directory for each test file (i.e. `test-results/a-test-title`), otherwise it will throw. +> 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...]> diff --git a/docs/src/test-snapshots-js.md b/docs/src/test-snapshots-js.md index 8f94c82517336..5c67d2c7e64b6 100644 --- a/docs/src/test-snapshots-js.md +++ b/docs/src/test-snapshots-js.md @@ -55,8 +55,8 @@ Sometimes you need to update the reference screenshot, for example when the page npx playwright test --update-snapshots ``` -> 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. +> 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/util.ts b/src/test/util.ts index 9fc47a046c6bb..297889c52c9c4 100644 --- a/src/test/util.ts +++ b/src/test/util.ts @@ -158,7 +158,7 @@ export function addSuffix(filePath: string, suffix: string, customExtension?: st export function getContainedPath(parentPath: string, subPath: string = ''): string { const resolvedPath = path.resolve(parentPath, subPath); - if (resolvedPath.startsWith(parentPath)) return resolvedPath; + if (resolvedPath.startsWith(parentPath + path.sep)) return resolvedPath; throw new Error(`Path defined outside of parent directory path: ${subPath} From 311fa94da6bd8a815020f3483d78de129ded57f2 Mon Sep 17 00:00:00 2001 From: nickofthyme Date: Wed, 29 Sep 2021 21:37:49 -0500 Subject: [PATCH 08/17] refactor: types, renaming and test cleanup per pr comments --- docs/src/test-advanced-js.md | 2 +- docs/src/test-api/class-testinfo.md | 2 +- src/test/matchers/golden.ts | 10 ++--- src/test/matchers/toMatchSnapshot.ts | 5 ++- src/test/util.ts | 2 +- src/test/workerRunner.ts | 8 ++-- tests/playwright-test/golden.spec.ts | 7 +-- tests/playwright-test/test-output-dir.spec.ts | 45 ++++++++++++++----- types/test.d.ts | 17 ++++--- utils/generate_types/overrides-test.d.ts | 2 +- 10 files changed, 63 insertions(+), 37 deletions(-) diff --git a/docs/src/test-advanced-js.md b/docs/src/test-advanced-js.md index 5d606d91f4cc3..78c506167a7b8 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 99f96d42c02eb..a00ec7806b371 100644 --- a/docs/src/test-api/class-testinfo.md +++ b/docs/src/test-api/class-testinfo.md @@ -283,7 +283,7 @@ Returns a path to a snapshot file with the given `pathSegments`. Learn more abou > 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...]> +- `pathSegments` <[Array]<[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. diff --git a/src/test/matchers/golden.ts b/src/test/matchers/golden.ts index af856b86fbc97..a3eba465a55e0 100644 --- a/src/test/matchers/golden.ts +++ b/src/test/matchers/golden.ts @@ -23,7 +23,7 @@ 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 { TestInfoImpl, UpdateSnapshots } from '../types'; -import { addSuffix } from '../util'; +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')] })); @@ -90,11 +90,11 @@ export function compare( withNegateComparison: boolean, options?: { threshold?: number } ): { pass: boolean; message?: string; expectedPath?: string, actualPath?: string, diffPath?: string, mimeType?: string } { - const snapshotFile = snapshotPath(...pathSegments); + const snapshotFile = snapshotPath(pathSegments); const outputFile = outputPath(...pathSegments); - const expectedPath = addSuffix(outputFile, '-expected'); - const actualPath = addSuffix(outputFile, '-actual'); - const diffPath = addSuffix(outputFile, '-diff'); + 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'; diff --git a/src/test/matchers/toMatchSnapshot.ts b/src/test/matchers/toMatchSnapshot.ts index 1bc979ce66d8c..38035d627be9c 100644 --- a/src/test/matchers/toMatchSnapshot.ts +++ b/src/test/matchers/toMatchSnapshot.ts @@ -17,7 +17,7 @@ import type { Expect } from '../types'; import { currentTestInfo } from '../globals'; import { compare } from './golden'; -import { addSuffix } from '../util'; +import { addSuffixToFilePath } from '../util'; // from expect/build/types type SyncExpectationResult = { @@ -42,7 +42,8 @@ export function toMatchSnapshot(this: ReturnType, received: if (options.threshold === undefined && projectThreshold !== undefined) options.threshold = projectThreshold; - const pathSegments = Array.isArray(options.name) ? options.name : [addSuffix(options.name, '', undefined, true)]; + // 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, diff --git a/src/test/util.ts b/src/test/util.ts index 297889c52c9c4..09442e5854319 100644 --- a/src/test/util.ts +++ b/src/test/util.ts @@ -144,7 +144,7 @@ export function sanitizeForFilePath(s: string) { return s.replace(/[\x00-\x2F\x3A-\x40\x5B-\x60\x7B-\x7F]+/g, '-'); } -export function addSuffix(filePath: string, suffix: string, customExtension?: string, sanitize = false): string { +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); diff --git a/src/test/workerRunner.ts b/src/test/workerRunner.ts index 6e8fe755d25ff..222fc1b210a1e 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, getContainedPath, addSuffix } 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'; @@ -257,17 +257,15 @@ export class WorkerRunner extends EventEmitter { fs.mkdirSync(baseOutputDir, { recursive: true }); return getContainedPath(baseOutputDir, path.join(...pathSegments)); }, - snapshotPath: (...pathSegments: string[]): string => { + snapshotPath: (pathSegments: string[]): string => { let suffix = ''; if (this._projectNamePathSegment) suffix += '-' + this._projectNamePathSegment; if (testInfo.snapshotSuffix) suffix += '-' + testInfo.snapshotSuffix; - const snapshotPath = path.join(...pathSegments.map((segment, i, { length }) => - (i < length - 1) ? segment : addSuffix(segment, suffix), - )); const baseSnapshotPath = test._requireFile + '-snapshots'; + const snapshotPath = addSuffixToFilePath(path.join(...pathSegments), suffix); return getContainedPath(baseSnapshotPath, snapshotPath); }, skip: (...args: [arg?: any, description?: string]) => modifier(testInfo, 'skip', args), diff --git a/tests/playwright-test/golden.spec.ts b/tests/playwright-test/golden.spec.ts index 6ec01973859dc..bf062d6579fd4 100644 --- a/tests/playwright-test/golden.spec.ts +++ b/tests/playwright-test/golden.spec.ts @@ -458,9 +458,10 @@ test('should respect project threshold', async ({ runInlineTest }) => { test('should sanitize snapshot name when passed as string', async ({ runInlineTest }) => { const result = await runInlineTest({ - 'a.spec.js-snapshots/-snapshot--darwin.txt': `Hello world`, + ...files, + 'a.spec.js-snapshots/-snapshot-.txt': `Hello world`, 'a.spec.js': ` - const { test } = pwt; + const { test } = require('./helper');; test('is a test', ({}) => { expect('Hello world').toMatchSnapshot('../../snapshot!.txt'); }); @@ -517,7 +518,7 @@ test('should update snapshot with array of path segments', async ({ runInlineTes expect(data.toString()).toBe('Hello world'); }); -test.only('should attach expected/actual/diff with snapshot path', async ({ runInlineTest }, testInfo) => { +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': diff --git a/tests/playwright-test/test-output-dir.spec.ts b/tests/playwright-test/test-output-dir.spec.ts index 16fb293fd3cfc..fece6bf67c69f 100644 --- a/tests/playwright-test/test-output-dir.spec.ts +++ b/tests/playwright-test/test-output-dir.spec.ts @@ -161,13 +161,13 @@ test('should include the project name', async ({ runInlineTest }) => { const { test, test2 } = require('./helper'); test('test 1', async ({}, testInfo) => { console.log(testInfo.outputPath('bar.txt').replace(/\\\\/g, '/')); - console.log(testInfo.snapshotPath('bar.txt').replace(/\\\\/g, '/')); + console.log(testInfo.snapshotPath(['bar.txt']).replace(/\\\\/g, '/')); if (testInfo.retry !== 1) throw new Error('Give me a retry'); }); test2('test 2', async ({}, testInfo) => { console.log(testInfo.outputPath('bar.txt').replace(/\\\\/g, '/')); - console.log(testInfo.snapshotPath('bar.txt').replace(/\\\\/g, '/')); + console.log(testInfo.snapshotPath(['bar.txt']).replace(/\\\\/g, '/')); }); `, }, { retries: 1 }); @@ -227,27 +227,48 @@ test('should include path option in snapshot', async ({ runInlineTest }) => { }); `, 'playwright.config.ts': ` - module.exports = { projects: [ - { name: 'foo' }, - ] }; + 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, '/')); - }); - test('test with parent path', async ({}, testInfo) => { - console.log(testInfo.snapshotPath('../test/path/bar.txt').replace(/\\\\/g, '/')); + console.log(testInfo.snapshotPath(['test', 'path', 'bar.txt']).replace(/\\\\/g, '/')); }); `, }); expect(result.exitCode).toBe(0); expect(result.results[0].status).toBe('passed'); - expect(result.results[1].status).toBe('passed'); // reverts to base snapshot path - expect(result.output).toContain('my-test.spec.js-snapshots/test/path/bar-foo-suffix.txt'); - expect(result.output).toContain('my-test.spec.js-snapshots/bar-foo-suffix.txt'); +}); + +test('should error if path 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'); }); test('should remove output dirs for projects run', async ({ runInlineTest }, testInfo) => { diff --git a/types/test.d.ts b/types/test.d.ts index 4290f3530c180..76379df107cb1 100644 --- a/types/test.d.ts +++ b/types/test.d.ts @@ -1103,11 +1103,13 @@ export interface TestInfo { /** * 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 an array of 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 file 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. + * > Note that `pathSegments` accepts an array of 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: (...pathSegments: 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 @@ -1123,8 +1125,11 @@ export interface TestInfo { * }); * ``` * - * > Note that `pathSegments` accepts an array of path segments to the test output directory such as `testInfo.outputPath('relative', 'path', 'to', 'output')`. - * > However, this path must stay within the snapshots directory for each test file (i.e. `test-results/a-test-title`), otherwise it will throw. + * > Note that `pathSegments` accepts an array of 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 48f9c0074c2d7..de48a200dab38 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; } From 07567bccb62807f1c88dd4d3a66e0ae22743c260 Mon Sep 17 00:00:00 2001 From: nickofthyme Date: Wed, 29 Sep 2021 21:58:43 -0500 Subject: [PATCH 09/17] chore: add better error messages for escaping parent path --- src/test/util.ts | 7 ++----- src/test/workerRunner.ts | 13 ++++++++++--- 2 files changed, 12 insertions(+), 8 deletions(-) diff --git a/src/test/util.ts b/src/test/util.ts index 09442e5854319..0191b56395da1 100644 --- a/src/test/util.ts +++ b/src/test/util.ts @@ -155,15 +155,12 @@ export function addSuffixToFilePath(filePath: string, suffix: string, customExte /** * Returns absolute path contained within parent directory. */ -export function getContainedPath(parentPath: string, subPath: string = ''): string { +export function getContainedPath(parentPath: string, subPath: string = ''): string | null { const resolvedPath = path.resolve(parentPath, subPath); if (resolvedPath.startsWith(parentPath + path.sep)) return resolvedPath; - throw new Error(`Path defined outside of parent directory - path: ${subPath} - resolved path: ${resolvedPath} - parent: ${parentPath}`); + return null; } export const debugTest = debug('pw:test'); diff --git a/src/test/workerRunner.ts b/src/test/workerRunner.ts index 222fc1b210a1e..8ab604bf23c05 100644 --- a/src/test/workerRunner.ts +++ b/src/test/workerRunner.ts @@ -255,7 +255,11 @@ export class WorkerRunner extends EventEmitter { outputDir: baseOutputDir, outputPath: (...pathSegments: string[]): string => { fs.mkdirSync(baseOutputDir, { recursive: true }); - return getContainedPath(baseOutputDir, path.join(...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: ${outputPath}`); + }, snapshotPath: (pathSegments: string[]): string => { let suffix = ''; @@ -265,8 +269,11 @@ export class WorkerRunner extends EventEmitter { suffix += '-' + testInfo.snapshotSuffix; const baseSnapshotPath = test._requireFile + '-snapshots'; - const snapshotPath = addSuffixToFilePath(path.join(...pathSegments), suffix); - return getContainedPath(baseSnapshotPath, snapshotPath); + 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\snapshotPath: ${snapshotPath}`); }, skip: (...args: [arg?: any, description?: string]) => modifier(testInfo, 'skip', args), fixme: (...args: [arg?: any, description?: string]) => modifier(testInfo, 'fixme', args), From 76efbc2ea5ddd8458b5643f085ed3e53f8690137 Mon Sep 17 00:00:00 2001 From: nickofthyme Date: Thu, 30 Sep 2021 18:38:39 -0500 Subject: [PATCH 10/17] fix: path segments args and test cleanup --- docs/src/test-advanced-js.md | 2 +- docs/src/test-api/class-testinfo.md | 6 +-- src/test/matchers/golden.ts | 2 +- src/test/workerRunner.ts | 6 +-- tests/playwright-test/golden.spec.ts | 10 +++-- tests/playwright-test/test-output-dir.spec.ts | 43 ++++++++++++++++--- types/test.d.ts | 10 ++--- 7 files changed, 57 insertions(+), 22 deletions(-) diff --git a/docs/src/test-advanced-js.md b/docs/src/test-advanced-js.md index 78c506167a7b8..1c668d98d4022 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(pathSegments: 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 a00ec7806b371..c8ca2e6ad3b05 100644 --- a/docs/src/test-api/class-testinfo.md +++ b/docs/src/test-api/class-testinfo.md @@ -193,7 +193,7 @@ test('example test', async ({}, testInfo) => { }); ``` -> Note that `pathSegments` accepts an array of path segments to the test output directory such as `testInfo.outputPath('relative', 'path', 'to', 'output')`. +> 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 @@ -279,11 +279,11 @@ Optional description that will be reflected in a test report. Returns a path to a snapshot file with the given `pathSegments`. Learn more about [snapshots](./test-snapshots.md). -> Note that `pathSegments` accepts an array of path segments to the snapshot file such as `testInfo.snapshotPath('relative', 'path', 'to', 'snapshot.png')`. +> 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` <[Array]<[string]>> +- `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. diff --git a/src/test/matchers/golden.ts b/src/test/matchers/golden.ts index a3eba465a55e0..d1a04c36ff0f2 100644 --- a/src/test/matchers/golden.ts +++ b/src/test/matchers/golden.ts @@ -90,7 +90,7 @@ export function compare( withNegateComparison: boolean, options?: { threshold?: number } ): { pass: boolean; message?: string; expectedPath?: string, actualPath?: string, diffPath?: string, mimeType?: string } { - const snapshotFile = snapshotPath(pathSegments); + const snapshotFile = snapshotPath(...pathSegments); const outputFile = outputPath(...pathSegments); const expectedPath = addSuffixToFilePath(outputFile, '-expected'); const actualPath = addSuffixToFilePath(outputFile, '-actual'); diff --git a/src/test/workerRunner.ts b/src/test/workerRunner.ts index 8ab604bf23c05..6555c449c8e0a 100644 --- a/src/test/workerRunner.ts +++ b/src/test/workerRunner.ts @@ -258,10 +258,10 @@ export class WorkerRunner extends EventEmitter { 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: ${outputPath}`); + throw new Error(`The outputPath is not allowed outside of the parent directory. Please fix the defined path.\n\n\toutputPath: ${joinedPath}`); }, - snapshotPath: (pathSegments: string[]): string => { + snapshotPath: (...pathSegments: string[]): string => { let suffix = ''; if (this._projectNamePathSegment) suffix += '-' + this._projectNamePathSegment; @@ -273,7 +273,7 @@ export class WorkerRunner extends EventEmitter { 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\snapshotPath: ${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/tests/playwright-test/golden.spec.ts b/tests/playwright-test/golden.spec.ts index bf062d6579fd4..8c6e256b52cc8 100644 --- a/tests/playwright-test/golden.spec.ts +++ b/tests/playwright-test/golden.spec.ts @@ -472,8 +472,9 @@ test('should sanitize snapshot name when passed as string', async ({ runInlineTe test('should write missing expectations with sanitized snapshot name', async ({ runInlineTest }, testInfo) => { const result = await runInlineTest({ + ...files, 'a.spec.js': ` - const { test } = pwt; + const { test } = require('./helper');; test('is a test', ({}) => { expect('Hello world').toMatchSnapshot('../../snapshot!.txt'); }); @@ -481,7 +482,7 @@ test('should write missing expectations with sanitized snapshot name', async ({ }, {}, { CI: '' }); expect(result.exitCode).toBe(1); - const snapshotOutputPath = testInfo.outputPath('a.spec.js-snapshots/-snapshot--darwin.txt'); + const snapshotOutputPath = testInfo.outputPath('a.spec.js-snapshots/-snapshot-.txt'); expect(result.output).toContain(`${snapshotOutputPath} is missing in snapshots, writing actual`); const data = fs.readFileSync(snapshotOutputPath); expect(data.toString()).toBe('Hello world'); @@ -489,9 +490,10 @@ test('should write missing expectations with sanitized snapshot name', async ({ test('should join array of snapshot path segments without sanitizing ', async ({ runInlineTest }) => { const result = await runInlineTest({ - 'a.spec.js-snapshots/test/path/snapshot-darwin.txt': `Hello world`, + ...files, + 'a.spec.js-snapshots/test/path/snapshot.txt': `Hello world`, 'a.spec.js': ` - const { test } = pwt; + const { test } = require('./helper');; test('is a test', ({}) => { expect('Hello world').toMatchSnapshot(['test', 'path', 'snapshot.txt']); }); diff --git a/tests/playwright-test/test-output-dir.spec.ts b/tests/playwright-test/test-output-dir.spec.ts index fece6bf67c69f..66dd05f95d037 100644 --- a/tests/playwright-test/test-output-dir.spec.ts +++ b/tests/playwright-test/test-output-dir.spec.ts @@ -161,13 +161,13 @@ test('should include the project name', async ({ runInlineTest }) => { const { test, test2 } = require('./helper'); test('test 1', async ({}, testInfo) => { console.log(testInfo.outputPath('bar.txt').replace(/\\\\/g, '/')); - console.log(testInfo.snapshotPath(['bar.txt']).replace(/\\\\/g, '/')); + console.log(testInfo.snapshotPath('bar.txt').replace(/\\\\/g, '/')); if (testInfo.retry !== 1) throw new Error('Give me a retry'); }); test2('test 2', async ({}, testInfo) => { console.log(testInfo.outputPath('bar.txt').replace(/\\\\/g, '/')); - console.log(testInfo.snapshotPath(['bar.txt']).replace(/\\\\/g, '/')); + console.log(testInfo.snapshotPath(bar.txt').replace(/\\\\/g, '')); }); `, }, { retries: 1 }); @@ -234,7 +234,7 @@ test('should include path option in snapshot', async ({ runInlineTest }) => { 'my-test.spec.js': ` const { test } = require('./helper'); test('test with path', async ({}, testInfo) => { - console.log(testInfo.snapshotPath(['test', 'path', 'bar.txt']).replace(/\\\\/g, '/')); + console.log(testInfo.snapshotPath('test', 'path', 'bar.txt').replace(/\\\\/g, '/')); }); `, }); @@ -244,7 +244,7 @@ test('should include path option in snapshot', async ({ runInlineTest }) => { expect(result.output).toContain('my-test.spec.js-snapshots/test/path/bar-foo-suffix.txt'); }); -test('should error if path is resolved to outside of parent', async ({ runInlineTest }) => { +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({ @@ -262,13 +262,46 @@ test('should error if path is resolved to outside of parent', async ({ runInline '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, '/')); + 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) => { diff --git a/types/test.d.ts b/types/test.d.ts index 76379df107cb1..fd11bb62e5117 100644 --- a/types/test.d.ts +++ b/types/test.d.ts @@ -1103,13 +1103,13 @@ export interface TestInfo { /** * 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 an array of path segments to the snapshot file such as - * `testInfo.snapshotPath('relative', 'path', 'to', 'snapshot.png')`. + * > 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: (pathSegments: 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 @@ -1125,8 +1125,8 @@ export interface TestInfo { * }); * ``` * - * > Note that `pathSegments` accepts an array of path segments to the test output directory such as - * `testInfo.outputPath('relative', 'path', 'to', 'output')`. + * > 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. From df06c4b167e562d12f6adb910bcfb58dcef3d799 Mon Sep 17 00:00:00 2001 From: nickofthyme Date: Thu, 30 Sep 2021 19:09:44 -0500 Subject: [PATCH 11/17] fix: bad null error type --- src/utils/utils.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/utils/utils.ts b/src/utils/utils.ts index 9fae9a87edc9a..345d40afd492c 100644 --- a/src/utils/utils.ts +++ b/src/utils/utils.ts @@ -356,7 +356,7 @@ export async function removeFolders(dirs: string[]): Promise { return new Promise(fulfill => { removeFolder(dir, { maxBusyTries: 10 }, error => { - fulfill(error); + fulfill(error ?? undefined); }); }); })); From ac578b1ca21460418d63e7d6cfd94f62ccd673c0 Mon Sep 17 00:00:00 2001 From: nickofthyme Date: Thu, 30 Sep 2021 19:10:17 -0500 Subject: [PATCH 12/17] fix: fix pathSegements type in overrides-test --- utils/generate_types/overrides-test.d.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/utils/generate_types/overrides-test.d.ts b/utils/generate_types/overrides-test.d.ts index de48a200dab38..e078a27fa3efb 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: (pathSegments: string[]) => string; + snapshotPath: (...pathSegments: string[]) => string; outputPath: (...pathSegments: string[]) => string; } From b705b26d806ff880b40967db9f3d9492e2054302 Mon Sep 17 00:00:00 2001 From: nickofthyme Date: Thu, 30 Sep 2021 20:42:04 -0500 Subject: [PATCH 13/17] fix: remove path sep from parent directory check --- src/test/util.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/test/util.ts b/src/test/util.ts index 0191b56395da1..8dc76676f346d 100644 --- a/src/test/util.ts +++ b/src/test/util.ts @@ -158,7 +158,7 @@ export function addSuffixToFilePath(filePath: string, suffix: string, customExte export function getContainedPath(parentPath: string, subPath: string = ''): string | null { const resolvedPath = path.resolve(parentPath, subPath); - if (resolvedPath.startsWith(parentPath + path.sep)) return resolvedPath; + if (resolvedPath.startsWith(parentPath)) return resolvedPath; return null; } From 5785cdb6712c44f1379cdd4003549adfd73bcccb Mon Sep 17 00:00:00 2001 From: nickofthyme Date: Thu, 30 Sep 2021 20:55:59 -0500 Subject: [PATCH 14/17] fix: missing quote in test --- tests/playwright-test/test-output-dir.spec.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/playwright-test/test-output-dir.spec.ts b/tests/playwright-test/test-output-dir.spec.ts index 66dd05f95d037..4a02e03d408e2 100644 --- a/tests/playwright-test/test-output-dir.spec.ts +++ b/tests/playwright-test/test-output-dir.spec.ts @@ -18,7 +18,7 @@ import * as fs from 'fs'; import * as path from 'path'; import { test, expect } from './playwright-test-fixtures'; -test('should work and remove non-failures', async ({ runInlineTest }, testInfo) => { +test.only('should work and remove non-failures', async ({ runInlineTest }, testInfo) => { const result = await runInlineTest({ 'playwright.config.ts': ` module.exports = { @@ -167,7 +167,7 @@ test('should include the project name', async ({ runInlineTest }) => { }); test2('test 2', async ({}, testInfo) => { console.log(testInfo.outputPath('bar.txt').replace(/\\\\/g, '/')); - console.log(testInfo.snapshotPath(bar.txt').replace(/\\\\/g, '')); + console.log(testInfo.snapshotPath('bar.txt').replace(/\\\\/g, '')); }); `, }, { retries: 1 }); From a6931ca5067af8d63b4622cb47f5bde01e140774 Mon Sep 17 00:00:00 2001 From: nickofthyme Date: Thu, 30 Sep 2021 21:05:27 -0500 Subject: [PATCH 15/17] fix: remote test.only --- tests/playwright-test/test-output-dir.spec.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/playwright-test/test-output-dir.spec.ts b/tests/playwright-test/test-output-dir.spec.ts index 4a02e03d408e2..6c4273a377141 100644 --- a/tests/playwright-test/test-output-dir.spec.ts +++ b/tests/playwright-test/test-output-dir.spec.ts @@ -18,7 +18,7 @@ import * as fs from 'fs'; import * as path from 'path'; import { test, expect } from './playwright-test-fixtures'; -test.only('should work and remove non-failures', async ({ runInlineTest }, testInfo) => { +test('should work and remove non-failures', async ({ runInlineTest }, testInfo) => { const result = await runInlineTest({ 'playwright.config.ts': ` module.exports = { From 9bc8041053970e4b2ff3ef26e1a1dcdfc6a5ef29 Mon Sep 17 00:00:00 2001 From: nickofthyme Date: Thu, 30 Sep 2021 21:30:42 -0500 Subject: [PATCH 16/17] fix: add exact check for parent path --- src/test/util.ts | 4 +--- tests/page/page-request-fulfill.spec.ts | 2 +- 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/src/test/util.ts b/src/test/util.ts index 8dc76676f346d..a7c84ebdfcacb 100644 --- a/src/test/util.ts +++ b/src/test/util.ts @@ -157,9 +157,7 @@ export function addSuffixToFilePath(filePath: string, suffix: string, customExte */ export function getContainedPath(parentPath: string, subPath: string = ''): string | null { const resolvedPath = path.resolve(parentPath, subPath); - - if (resolvedPath.startsWith(parentPath)) return resolvedPath; - + if (resolvedPath === parentPath || resolvedPath.startsWith(parentPath + path.sep)) return resolvedPath; return null; } diff --git a/tests/page/page-request-fulfill.spec.ts b/tests/page/page-request-fulfill.spec.ts index 2f201ce47d700..dbf5dff9ddbd1 100644 --- a/tests/page/page-request-fulfill.spec.ts +++ b/tests/page/page-request-fulfill.spec.ts @@ -217,7 +217,7 @@ it('should fulfill with fetch result', async ({ page, server, isElectron }) => { expect(await response.json()).toEqual({ 'foo': 'bar' }); }); -it('should fulfill with fetch result and overrides', async ({ page, server, isElectron }) => { +it.only('should fulfill with fetch result and overrides', async ({ page, server, isElectron }) => { it.fixme(isElectron, 'error: Browser context management is not supported.'); await page.route('**/*', async route => { const response = await page._request.get(server.PREFIX + '/simple.json'); From c17469e638a9cb53b1a7953d8e18179ec0d8b72b Mon Sep 17 00:00:00 2001 From: Nick Partridge Date: Fri, 1 Oct 2021 09:43:55 -0500 Subject: [PATCH 17/17] test: fix failing Co-authored-by: Dmitry Gozman --- tests/page/page-request-fulfill.spec.ts | 2 +- tests/playwright-test/test-output-dir.spec.ts | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/page/page-request-fulfill.spec.ts b/tests/page/page-request-fulfill.spec.ts index dbf5dff9ddbd1..2f201ce47d700 100644 --- a/tests/page/page-request-fulfill.spec.ts +++ b/tests/page/page-request-fulfill.spec.ts @@ -217,7 +217,7 @@ it('should fulfill with fetch result', async ({ page, server, isElectron }) => { expect(await response.json()).toEqual({ 'foo': 'bar' }); }); -it.only('should fulfill with fetch result and overrides', async ({ page, server, isElectron }) => { +it('should fulfill with fetch result and overrides', async ({ page, server, isElectron }) => { it.fixme(isElectron, 'error: Browser context management is not supported.'); await page.route('**/*', async route => { const response = await page._request.get(server.PREFIX + '/simple.json'); diff --git a/tests/playwright-test/test-output-dir.spec.ts b/tests/playwright-test/test-output-dir.spec.ts index 6c4273a377141..a66940df4fc2b 100644 --- a/tests/playwright-test/test-output-dir.spec.ts +++ b/tests/playwright-test/test-output-dir.spec.ts @@ -167,7 +167,7 @@ test('should include the project name', async ({ runInlineTest }) => { }); test2('test 2', async ({}, testInfo) => { console.log(testInfo.outputPath('bar.txt').replace(/\\\\/g, '/')); - console.log(testInfo.snapshotPath('bar.txt').replace(/\\\\/g, '')); + console.log(testInfo.snapshotPath('bar.txt').replace(/\\\\/g, '/')); }); `, }, { retries: 1 });