Skip to content

feat: add path option to toMatchSnapshot#9156

Merged
dgozman merged 20 commits into
microsoft:masterfrom
nickofthyme:feat/snapshot-path
Oct 1, 2021
Merged

feat: add path option to toMatchSnapshot#9156
dgozman merged 20 commits into
microsoft:masterfrom
nickofthyme:feat/snapshot-path

Conversation

@nickofthyme
Copy link
Copy Markdown
Contributor

@nickofthyme nickofthyme commented Sep 26, 2021

allow snapshotName to accept an array of path segments in toMathchSnapshot.

// example.spec.ts
import { test, expect } from '@playwright/test';

test('example test', async ({ page }) => {
  await page.goto('https://playwright.dev');
  expect(await page.screenshot()).toMatchSnapshot(['test', 'path', 'landing.png']);
});

This would set and lookup the screenshot from example.spec.ts-snapshots/test/path/landing-<suffix>.png.

The path segment is contained to within the base snapshot directory for the given test file (e.g. example.spec.ts-snapshots/). Any path resolving to outside of this directory is ignored.

The current functionality of snapshotName as a 'string' is preserved. This string will continue to be sanitized for file path as is currently done.

// example.spec.ts
import { test, expect } from '@playwright/test';

test('example test', async ({ page }) => {
  await page.goto('https://playwright.dev');
  expect(await page.screenshot()).toMatchSnapshot('test/path/landing.png');
});

This would set and lookup the screenshot from example.spec.ts-snapshots/test-path-landing-<suffix>.png.

Related to #7792

adds optional path to toMathchSnapshot assertion.
@ghost
Copy link
Copy Markdown

ghost commented Sep 26, 2021

CLA assistant check
All CLA requirements met.

Comment thread src/test/util.ts
Copy link
Copy Markdown
Collaborator

@dgozman dgozman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the PR! See comments inline.

Comment thread tests/playwright-test/golden.spec.ts Outdated
Comment thread src/test/util.ts
@nickofthyme
Copy link
Copy Markdown
Contributor Author

nickofthyme commented Sep 27, 2021

@dgozman I'm curious what your thoughts are on adding a config option much like outputDir for snapshotDir that would be the base directory for all snapshots seeing as this was the main request from #7792. For example...

// playwright.config.ts
import { PlaywrightTestConfig } from '@playwright/test';

const config: PlaywrightTestConfig = {
  snapshotDir: 'snapshots',
};

export default config;
playwright.config.ts
tests/
└── foo.spec.ts
snapshots/
└── foo.spec.ts-snapshots/
     ├── basic-test-Chrome-linux.png
     └── basic-test-Safari-linux.png

Would there be any issue with this? I have started on these changes but think I'll put them in a follow up PR to this one.

@dgozman
Copy link
Copy Markdown
Collaborator

dgozman commented Sep 27, 2021

@dgozman I'm curious what your thoughts are on adding a config option much like outputDir for snapshotDir that would be the base directory for all snapshots seeing as this would the main request for #7792.

Sounds good to me!

Copy link
Copy Markdown
Collaborator

@dgozman dgozman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am sorry that this PR turns into a lot of work, but it is going to be a great feature!

Comment thread docs/src/test-snapshots-js.md Outdated
Comment thread src/test/util.ts Outdated
Comment thread src/test/util.ts Outdated
Comment thread src/test/workerRunner.ts Outdated
Comment thread docs/src/test-snapshots-js.md Outdated
@nickofthyme
Copy link
Copy Markdown
Contributor Author

I am sorry that this PR turns into a lot of work, but it is going to be a great feature!

All good! Thanks for the quick review. I agree with all your comments, I'll address them tomorrow. 👍🏼

@nickofthyme
Copy link
Copy Markdown
Contributor Author

I wasn't sure about the naming in the docs for toMatchSnapshot and changing it from snapshotName to pathSegments. Might be best to keep it as is with snapshotName with the note about using it as pathSegments. Also looks like I might need to fix some tests once you take another look.

@nickofthyme nickofthyme requested a review from dgozman September 29, 2021 23:52
Copy link
Copy Markdown
Collaborator

@dgozman dgozman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this works nicely, just a few code comments.

BTW do I need to manually update docs/src/test-api/class-testinfo.md and docs/src/test-snapshots-js.md from types/test.d.ts or is there some auto generation script I am missing?

Files in docs/src are the source of truth, types/test.d.ts is generated from them by npm run watch.

I wasn't sure about the naming in the docs for toMatchSnapshot and changing it from snapshotName to pathSegments. Might be best to keep it as is with snapshotName with the note about using it as pathSegments

I agree that it's better to keep snapshotName.

Comment thread docs/src/test-api/class-testinfo.md Outdated
Comment thread docs/src/test-snapshots-js.md Outdated
Comment thread docs/src/test-snapshots-js.md Outdated
Comment thread src/test/matchers/golden.ts
Comment thread src/test/util.ts Outdated
Comment thread src/test/matchers/toMatchSnapshot.ts Outdated
Comment thread src/test/workerRunner.ts Outdated
Comment thread tests/playwright-test/golden.spec.ts Outdated
Comment thread tests/playwright-test/golden.spec.ts Outdated
Comment thread tests/playwright-test/test-output-dir.spec.ts Outdated
@nickofthyme nickofthyme requested a review from dgozman September 30, 2021 13:03
Copy link
Copy Markdown
Collaborator

@dgozman dgozman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apart from the spread and some failing tests, this looks perfect!

Comment thread tests/playwright-test/golden.spec.ts Outdated
Comment thread tests/playwright-test/golden.spec.ts Outdated
Comment thread tests/playwright-test/test-output-dir.spec.ts
Comment thread docs/src/test-advanced-js.md Outdated
Comment thread docs/src/test-advanced-js.md
@nickofthyme nickofthyme requested a review from dgozman October 1, 2021 00:23
Comment thread src/test/util.ts Outdated
Copy link
Copy Markdown
Collaborator

@dgozman dgozman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two suggestions to fix failing tests, the rest is great!

Comment thread tests/page/page-request-fulfill.spec.ts Outdated
Comment thread tests/playwright-test/test-output-dir.spec.ts Outdated
Co-authored-by: Dmitry Gozman <dgozman@gmail.com>
@nickofthyme nickofthyme requested a review from dgozman October 1, 2021 15:24
@dgozman dgozman merged commit b126a56 into microsoft:master Oct 1, 2021
@nickofthyme nickofthyme deleted the feat/snapshot-path branch October 1, 2021 16:19
sidharthv96 added a commit to sidharthv96/playwright that referenced this pull request Oct 2, 2021
…tionWithBaseChange

* upstream/master:
  chore: roll Electron to 12.2.1 (microsoft#9271)
  test: use separate Playwright instance to automate inspector (microsoft#9270)
  feat(inspector): use chrome/msedge when chromium is not available (microsoft#9269)
  fix(webkit): deduce response mime type from content-type (microsoft#9264)
  fix(fetch): use data, form and multipart for different post data (microsoft#9248)
  chore: split ContextRecorder from inspector (microsoft#9250)
  feat: add path option to `toMatchSnapshot` (microsoft#9156)
  feat(cli): Support trace file URLs (microsoft#9030)
  browser(webkit): roll to 23/09/21 (microsoft#9107)
  feat(chromium): roll to r926934 (microsoft#9259)
  browser(chromium): roll to r926934 (microsoft#9255)
  test: fix 'should fulfill with fetch result and overrides' test (microsoft#9252)
  feat(test-runner): add reuse context mode to share a single context between tests (microsoft#9115)
  fix(docs): test-runner location column type (microsoft#9222)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants