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(tests): upgrade playwright #20441

Merged
merged 6 commits into from
Feb 20, 2024
Merged

fix(tests): upgrade playwright #20441

merged 6 commits into from
Feb 20, 2024

Conversation

thmsobrmlr
Copy link
Contributor

@thmsobrmlr thmsobrmlr commented Feb 20, 2024

Problem

A visual regression test is flip-flopping dc7de36. It's always the first test of the insight stories and looks like it's flip-flopping the width of the scrollbar. There are a couple of bug reports in playwright (1, 2, 3, 4) that might be related to this (might also be something else) with various workarounds, but a fix might also be in the latest version.

Changes

Upgrades playwright to the latest version.

How did you test this code?

CI run

@posthog-bot
Copy link
Contributor

📸 UI snapshots have been updated

123 snapshot changes in total. 0 added, 123 modified, 0 deleted:

Triggered by this commit.

👉 Review this PR's diff of snapshots.

Copy link
Contributor

github-actions bot commented Feb 20, 2024

Size Change: 0 B

Total Size: 864 kB

ℹ️ View Unchanged
Filename Size
frontend/dist/toolbar.js 864 kB

compressed-size-action

@@ -54,7 +54,7 @@ jobs:
runs-on: ubuntu-latest
timeout-minutes: 30
container:
image: mcr.microsoft.com/playwright:v1.32.2 # Same as @storybook/[email protected]'s dependency
image: mcr.microsoft.com/playwright:v1.41.2
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@daibhin I saw that you added the initial comment regarding the version lock to 1.32.2 here. The dependency is still locked to the same version via @storybook/[email protected] -> [email protected] -> [email protected]. Since the CI run is successful I'm assuming everything works with a newer version, but let me know if there's an issue that you know about and that isn't obvious to me.

Copy link
Contributor

Choose a reason for hiding this comment

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

Don't recall the exact context of the comment but I'd imagine it's safe to upgrade. Most likely my CI wasn't passing without it but I think we've even upgraded Storybook a couple of times since so perhaps they loosened their dependencies somewhat

@thmsobrmlr thmsobrmlr marked this pull request as ready for review February 20, 2024 10:48
@thmsobrmlr thmsobrmlr requested a review from a team February 20, 2024 10:48
@@ -54,7 +54,7 @@ jobs:
runs-on: ubuntu-latest
timeout-minutes: 30
container:
image: mcr.microsoft.com/playwright:v1.32.2 # Same as @storybook/[email protected]'s dependency
image: mcr.microsoft.com/playwright:v1.41.2
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't recall the exact context of the comment but I'd imagine it's safe to upgrade. Most likely my CI wasn't passing without it but I think we've even upgraded Storybook a couple of times since so perhaps they loosened their dependencies somewhat

Copy link
Contributor

Choose a reason for hiding this comment

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

Wild how many snapshots (especially in one area) are being updated by one change. Seems unrelated to anything you've done @thmsobrmlr but maybe we need to reduce the tolerance slightly on snapshots

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I think that's because font rendering got changed slightly and they now go above the threshold (they have been outdated since we've increased the contrast). Will keep a look on how the test behave.

# Conflicts:
#	frontend/__snapshots__/scenes-app-insights--trends-line--light--webkit.png
#	frontend/__snapshots__/scenes-app-insights--trends-table-breakdown-edit--light--webkit.png
#	frontend/__snapshots__/scenes-app-insights-error-empty-states--estimated-query-execution-time-too-long--light.png
@thmsobrmlr thmsobrmlr enabled auto-merge (squash) February 20, 2024 11:31
@thmsobrmlr thmsobrmlr merged commit 588055a into master Feb 20, 2024
79 checks passed
@thmsobrmlr thmsobrmlr deleted the upgrade-playwright branch February 20, 2024 12:06
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.

3 participants