Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(merge-reports): only change test ids when needed #31061

Merged
merged 4 commits into from
May 30, 2024

Conversation

muhqu
Copy link
Contributor

@muhqu muhqu commented May 29, 2024

When merging blob reports test ids are patched to make sure there is no collision when merging reports that might have overlapping test ids. However, even if you were merging reports that had no overlapping ids, all test ids will be modified, which is an undesirable side effect.

This PR only modify test ids when the same test id has already been used in a previous blob report.


This change is also part of #30962

This comment has been minimized.

@dgozman dgozman changed the title fix(merge-reports) only change test ids when needed fix(merge-reports): only change test ids when needed May 29, 2024
Copy link
Contributor

@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! Looks good overall, see inline comments.

tests/playwright-test/reporter-blob.spec.ts Outdated Show resolved Hide resolved
packages/playwright/src/reporters/merge.ts Outdated Show resolved Hide resolved

This comment has been minimized.

This comment has been minimized.

@muhqu muhqu requested a review from dgozman May 30, 2024 08:49

This comment has been minimized.

@dgozman
Copy link
Contributor

dgozman commented May 30, 2024

Thank you for the PR, merging in.

@dgozman dgozman merged commit 5708148 into microsoft:main May 30, 2024
30 checks passed
Copy link
Contributor

Test results for "tests 1"

2 flaky ⚠️ [playwright-test] › ui-mode-test-watch.spec.ts:223:5 › should queue watches
⚠️ [webkit-library] › library/browsercontext-clearcookies.spec.ts:116:3 › should remove cookies by path

27093 passed, 608 skipped
✔️✔️✔️

Merge workflow run.

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