-
Notifications
You must be signed in to change notification settings - Fork 22
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
#8990: fix non-awaited screenshot promise #9004
Conversation
// Set the viewport to null because we rely on the real inner window width to detect when the sidepanel is open | ||
// See: https://github.com/microsoft/playwright/issues/11465 and https://github.com/pixiebrix/pixiebrix-extension/blob/b4b0a2efde2c3ac5e634220b555532a2875fe5da/src/contentScript/sidebarController.tsx#L78 | ||
viewport: null, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should no longer need this work-around since we removed this bit of code that detects if the sidebar is open based on the window inner width.
{ platform }: BrickOptions, | ||
): Promise<unknown> { | ||
): Promise<{ data: string }> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Side note: this should probably correspond to the output schema directly
No loom links were found in the first post. Please add one there if you'd like to it to appear on Slack. Do not edit this comment manually. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #9004 +/- ##
==========================================
+ Coverage 74.24% 74.39% +0.14%
==========================================
Files 1332 1353 +21
Lines 40817 41898 +1081
Branches 7634 7828 +194
==========================================
+ Hits 30306 31168 +862
- Misses 10511 10730 +219 ☔ View full report in Codecov by Sentry. |
Playwright test resultsDetails Open report ↗︎ Flaky testschrome › tests/pageEditor/addStarterBrick.spec.ts › Add starter brick to mod Skipped testschrome › tests/regressions/doNotCloseSidebarOnPageEditorSave.spec.ts › #8104: Do not automatically close the sidebar when saving in the Page Editor |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Didn't know playwright had screenshot testing built in by default. That required a 3rd party plugin in Cypress. Nice!
Looks like screenshot tests might have cross-browser compatibility issues? That's not surprising, though I thought Chrome/Edge would be similar enough. We might want to limit those to just Chrome unless you can create screenshots for both browsers? |
We can create screenshots for both browsers by adding the browserName to the snapshot, but for now I'd rather defer trying to handle this more generally and just check to see if there is any rendered image where expected (the actual visual look of this particular image isn't really that relevant) |
What does this PR do?
Discussion
This PR introduces screenshot testing in our e2e playwright tests. Right now the max diff is set to 0.1 (so if the image is more than 10% different, it will fail the test).Checklist