-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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: test runner reporter performance #17243
Conversation
Thanks for taking the time to open a PR!
|
Test summaryRun details
View run in Cypress Dashboard ➡️ This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Dashboard |
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.
- I can see that this improves performance for Tests slow down over time when performing several XHR requests calls in single test #6783 but doesn't completely account for all slowdown. The test still runs progressively slower towards the end of the test than it did at the beginning, although the original example is a bit extreme. The type command is about ~400ms slower towards the end compared to the first type command instead of ~3000ms slower (before this change)! So this is a big improvement.
- I don't feel super great about not displaying a scrollbar for scrollable content. This is poor usability. Anything we can do about this?
- The performance seems to be slightly worse in Firefox after this change? 😬
- It would be 💯 if we had a way to test this. I know this is the hardest part though. There's kind of nothing stopping someone in the future from adding some slight change and just blowing out the performance again.
I'd like to explore points 2 and 3, but overall I still think this may be an improvement to merge in even with those downsides. Would love someone else on the team to review as well.
Chromium browsers
Before
Finished in 223 seconds
Using cypress-timings to show the type
command duration of first type versus last type in test.
After
Finished in 107 seconds
Using cypress-timings to show the type
command duration of first type versus last type in test.
Firefox browser
Before
Finished in 128 seconds
Using cypress-timings to show the type
command duration of first type versus last type in test.
After
Finished in 151 seconds
Using cypress-timings to show the type
command duration of first type versus last type in test.
@jennifer-shehane Thanks for the detailed review!
We could display the normal scrollbar unstyled (screenshot), or enable the scrollbar if you're not auto-scrolling? I didn't know if we wanted to keep the current styling to keep it consistent with how it is now, so I opted to disable it entirely while the specs are running and show it when you're done and are more likely to find it useful. If we virtualize (see point below), we'll probably want to disable it entirely as well.
Interesting about Firefox, will need to take a look and see if it's the CSS or the runnable renderings that causes this regression
Yes, didn't anticipate this would be the only solution - this was just the simplest first pass, also looking into another approach of virtualizing the runner content while it's executing, which I believe should have a pretty massive improvement - but the implementation remains to be seen.
We could probably create a bounds of what we roughly expect to be correct timings and run this in its own isolated e2e spec. Might be flaky but I think should be doable 🤔 |
@tgriesser I feel like the original intent for styling the scrollbar years ago was to make it slimmer than the native one and standardize across OS. I think it'd probably be ok to just remove the styling altogether. It doesn't even work in Firefox. |
@tgriesser Is this ready for rereview? I'm a little unclear. |
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.
Um, yah this is a lot faster with the build-prod
than I even realized. 💯 💯 💯 💯 Thanks @tgriesser
…ui/add-context-code-review * unified-desktop-gui: feat: Structuring context & schema so it can be used on the client (#17489) fix: vue 3 types, beta suffix & component name (#17508) fix: last test skipped with top navigation and already run suite (#17498) chore: Update Chrome (beta) to 93.0.4577.18 (#17536) fix: use process.geteuid and catch uid errors in file util (#17488) build: make vite build strict (#17509) release 8.1.0 fix(server): correctly include projectRoot when adding a CI project from GUI (#17514) fix(types): update cy.shadow docs url (#17506) chore(npm/webpack-preprocessor): fix CI pipeline (#17515) fix: test runner reporter performance (#17243) chore: release @cypress/vue-v3.0.0-beta.4 chore: release @cypress/vite-dev-server-v2.0.3 fix: make vite re-run on supportFile change (#17485)
User facing changelog
Improved performance with larger numbers of tests.
Additional details
Did some digging into rendering bottlenecks in the reporter, found a number of things needing addressing, the lowest hanging fruit are the lazy initial rendering of runnables, and the custom styling of the reporter scrollbar. The PR removes the custom styling for the scrollbar & renders all runnables from the start.
Uses this test case, updating the
COUNT
as seen in the table screenshot below:Recorded benchmarks for removing the progressive rendering of the runnable list, as well as the style fixes - and then both combined. My hunch here is that the reason we needed to add the incremental node rendering has been addressed by more recent React versions.
Also manually confirmed that it fixes the experience in: #6783
How has the user experience changed?
PR Tasks