Skip to content

chore: change generated filename for toMatchAriaSnapshot from .yml to .snapshot.yml - #34931

Merged
Simon Knott (Skn0tt) merged 7 commits into
microsoft:mainfrom
Skn0tt:snapshot-matcher-file-ext
Feb 26, 2025
Merged

chore: change generated filename for toMatchAriaSnapshot from .yml to .snapshot.yml#34931
Simon Knott (Skn0tt) merged 7 commits into
microsoft:mainfrom
Skn0tt:snapshot-matcher-file-ext

Conversation

@Skn0tt

Copy link
Copy Markdown
Contributor

No description provided.

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.

Comment threadtests/playwright-test/aria-snapshot-file.spec.ts Outdated
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Signed-off-by: Simon Knott <info@simonknott.de>
@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

Comment threaddocs/src/aria-snapshots.md Outdated
_resolveSnapshotPath(template: string | undefined, defaultTemplate: string, pathSegments: string[]) {
const subPath = path.join(...pathSegments);
const parsedSubPath = path.parse(subPath);
const parsedSubPath = parsePathMultiExt(subPath);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Why do we need these changes? Could you please provide an example of what goes wrong without the change in this line?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

as discussed offline, this is needed because path.parse turns foo.snapshot.yml into { name: 'foo.snapshot', ext: 'yml' }, while our usecase needs { name: 'foo', ext: 'snapshot.yml' }. This is a subtle breaking change though, because it'll also impact extensions like .foo.txt. So we decided to make it a special case for .snapshot.yml instead.

Co-authored-by: Dmitry Gozman <dgozman@gmail.com>
Signed-off-by: Simon Knott <info@simonknott.de>
Comment threadpackages/playwright/src/util.ts Outdated

export function sanitizeFilePathBeforeExtension(filePath: string): string {
const ext = path.extname(filePath);
export function sanitizeFilePathBeforeExtension(filePath: string, ext = path.extname(filePath)): string {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

For some reason, I feel uncomfortable with non-constant default values. I'd rather see any code evaluated during the function call inside the function body.

@github-actions

This comment has been minimized.

@github-actions

Copy link
Copy Markdown
Contributor

Test results for "tests 1"

3 flaky⚠️ [firefox-page] › tests/page/page-evaluate.spec.ts:403:3 › should throw for too deep reference chain @firefox-ubuntu-22.04-node18
⚠️ [playwright-test] › tests/ui-mode-trace.spec.ts:341:5 › should work behind reverse proxy @macos-latest-node18-1
⚠️ [webkit-page] › tests/page/page-screenshot.spec.ts:345:5 › page screenshot › should work while navigating @webkit-ubuntu-22.04-node18

38058 passed, 650 skipped
✔️✔️✔️

Merge workflow run.

Sign up for freeto 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.

3 participants

@Skn0tt@dgozman