-
Notifications
You must be signed in to change notification settings - Fork 4.7k
Switch performance tests to Playwright #52022
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
Merged
Merged
Changes from 1 commit
Commits
Show all changes
46 commits
Select commit
Hold shift + click to select a range
a88918c
Run with Playwright in CI
WunderBart 4ce6571
Print stdout on error
WunderBart f6d9851
Run build:packages to use e2e utils
WunderBart 6fde09d
Extend timeout for the canvas spinner
WunderBart 6f28c60
Add missing front end perf report
WunderBart b9b0440
Remove obsolete step
WunderBart ea6891f
Make the typing testing consistent
WunderBart aeae2ec
Improve post editor perf testing consistency
WunderBart 309b9a2
Do not report slow tests
WunderBart b47d2b7
Use the same methodology for measuring post and site editors metrics
WunderBart 0427a37
Make the results table transformer more readable
WunderBart 542456e
Streamline the results processing p.1
WunderBart c06ab19
Improve visual step separation a tad :)
WunderBart df1216e
First step to separate metrics from the runner
WunderBart 2e13186
Define the results file suffix
WunderBart ed1e0cc
Merge remote-tracking branch 'origin' into refactor/playwright-perfor…
WunderBart fecc1a0
cosmetic
WunderBart 4a394d8
Fix matching pattern typo
WunderBart 647ea57
Make performance.js comparison runner results-agnostic
WunderBart 1a18006
Fix results handling
WunderBart f03e671
Use Playwright's attachments API to handle results
WunderBart 5db75c3
Upload raw results as well
WunderBart da43808
Be consistent with calc utils
WunderBart 00f641d
Streamline results handling
WunderBart 2b9a5ac
Use the same sampling convention for front-end metrics
WunderBart 1f8ecd1
Fix the metrics output in perf runner
WunderBart d649aac
tmp: compare performance with base branch
WunderBart ae40542
tmp: compare performance with base branch (run 2)
WunderBart 18fe132
tmp: compare performance with base branch (run 3)
WunderBart 8f15066
Revert "tmp: compare performance with base branch"
WunderBart b1d7f83
Remove unused code and do the switch
WunderBart e37cbc2
Merge remote-tracking branch 'origin' into refactor/playwright-perfor…
WunderBart 2c305da
Fix site editor typing spec
WunderBart c9c625e
Merge remote-tracking branch 'origin' into refactor/playwright-perfor…
WunderBart bbae249
Add inline comment
WunderBart 894896b
Expain why we need larger timeout for site editor visit util
WunderBart 7ac254e
Use dirent for getting files from dir
WunderBart 84bb262
Do not use the browser.newPage shortcut
WunderBart 182fbc1
Merge remote-tracking branch 'origin' into refactor/playwright-perfor…
WunderBart 1cf298a
Use trace buffer instead of tmp file
WunderBart f787f1e
Merge remote-tracking branch 'origin' into refactor/playwright-perfor…
WunderBart 5a68bbc
Fix 'unterminated string' error
WunderBart d0d81fb
Fix 'unterminated string' error p.2
WunderBart 37ef946
Make sure sanitized branch name is used for reading results json
WunderBart 127b21e
Merge remote-tracking branch 'origin' into refactor/playwright-perfor…
WunderBart 568d9d7
Prevent logging in via test.use() instead of creating new context
WunderBart File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Expain why we need larger timeout for site editor visit util
- Loading branch information
commit 894896bb3ac81ac4ed0f43b762708e0b87b69a4d
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Why do we need this here? If it applies to all the tests then maybe we should add a comment above explaining why 😅 .
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.
This was needed because, for performance tests, we load an enormous post fixture to test against, which often takes more than the default 10 seconds to load.
I'm not sure the rationale for using a post big enough to freeze the browser for a bit, but it's probably something to be discussed in a separate PR? /cc @youknowriad
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.
when I've tested with the large post in my browser it opens reasonably fast, but if I open the profiling tools I've seen it freeze for more than a minute.
I'm inferring that Chrome is spending a lot of time instrumenting the thousands of listeners and hooks when we turn of tracing, which probably happens here as well.
so this may not be realistic from a user perspective, but the wait may still occur when testing and profiling
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.
Yeah, profiling adds a lot of overhead. We could assume that the overhead added shares averagely across all code and can be seen as a fair emulated slowdown of the CPU, but I don't know if that's true 😅 .
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.
it's weird. I don't understand it, but it doesn't seem to appear in the metrics. I suspect that the profiling mechanisms don't start until after this initialization. the page isn't interactive while it's starting up, but then once it finishes things are suddenly fast and fine.
so it may not be the case that Chrome reports this big delay, but it still delays the tests. based on some recent profiling work I'm wondering if we might have code paths that are to blame. there are so many listeners involved and it doesn't seem reasonable that we should have so many. maybe Chrome's profiler simply chokes at tens of thousands of listeners (or maybe this is a normal number 🤷♂️ I really don't know).