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: Fix perf issue with command log #21007

Closed

Conversation

andrewmtam
Copy link

@andrewmtam andrewmtam commented Apr 8, 2022

User facing changelog

Added a new configuration option, numCommandsToShow, which can be used to yield performance improvements for long running tests.

This will limit the number to commands that show in the Command Log (the left sidebar) of a cypress test, instead of allowing it to grow indefinitely.

Additional details

We already have a parameter, CYPRESS_NO_COMMAND_LOG, which hides the Command Log altogether. However, this is not sufficient for both debugging and performance. numCommandsToShow seeks to provide a solution that satisfies both performance and debugging concerns.

While this does restrict the user from clicking on snapshots past the specified numCommandsToShow value (and only if they opted into this), I don't think this is typically needed (when debugging, we only need the last couple of commands around when the test failed)

Related docs PR:
cypress-io/cypress-documentation#4431

How has the user experience changed?

I believe this simple change / configuration can yield huge huge performance gains for users, with little to no risk of regression at all.

I ran a cypress test w/o this change, and repeated the same series of steps in a test 1000 times (I just took screenshots because the video was too big, and I also didn't bother waiting to get to 1000).

The test was to:

  • Go to a public todo page
  • Add a todo item
  • Delete the todo item
  • Repeat

From the screenshots, we can see that this step started off taking 400ms, and very quickly we found ourselves at the 1200ms mark, a 3x increment from when the test started, and after only 100 iterations.

Before applying this PR

Screen Shot 2022-04-08 at 6 09 32 PM

Screen Shot 2022-04-08 at 6 10 44 PM

After applying this PR (using a numCommandsToShow of 25)

Next, I applied this PR, and adjusted the new configuration option, numCommandsToShow, to 25.

For the duration of the test, the duration of the same sequence of actions did not linearly increase with the number of commands getting run. Also, note that at the 100 iteration mark, this test took /50%/ of the time as the previous test, 42s vs 87s.

Screen Shot 2022-04-08 at 6 13 36 PM

Screen Shot 2022-04-08 at 6 14 07 PM

PR Tasks

  • Have tests been added/updated?
  • Has the original issue (or this PR, if no issue exists) been tagged with a release in ZenHub? (user-facing changes only)
  • Has a PR for user-facing changes been opened in cypress-documentation?
  • Have API changes been updated in the type definitions?
  • Have new configuration options been added to the cypress.schema.json?

@cypress-bot
Copy link
Contributor

cypress-bot bot commented Apr 8, 2022

Thanks for taking the time to open a PR!

@CLAassistant
Copy link

CLAassistant commented Apr 8, 2022

CLA assistant check
All committers have signed the CLA.

@andrewmtam andrewmtam marked this pull request as ready for review April 8, 2022 22:52
@andrewmtam andrewmtam requested a review from a team as a code owner April 8, 2022 22:52
@andrewmtam andrewmtam requested review from jennifer-shehane and removed request for a team April 8, 2022 22:52
cli/schema/cypress.schema.json Outdated Show resolved Hide resolved
cli/types/cypress.d.ts Outdated Show resolved Hide resolved
@jennifer-shehane
Copy link
Member

@andrewmtam We appreciate the work put into this PR, however exposing a configuration option that would limit the number of commands shown is not a feature that we want to add.

We would prefer a solution that addresses the performance issues without degrading the experience of using the entirety of the Cypress Test Runner at the time it is run. Not being able to see x commands degrades the experience of the runner significantly as information is essentially lost from that run.

Ideally a rendering solution that would show the relevant commands and relevant data upon scroll. So far our investigations into a rendering solution like this that would improve performance have not given us any clear solutions.

@andrewmtam
Copy link
Author

Thanks @jennifer-shehane , appreciate the feedback! I've got two follow up questions if you've got a moment:

  1. As we already have a CYPRESS_NO_COMMAND_LOG={1|0} environment variable which does something similar, how do you feel about adding a CYPRESS_LIMIT_COMMAND_LOG={number} environment variable (instead of it being a configuration option). I believe this environment variable could provide more benefits over the one that hides the command log altogether, as it still surfaces information surrounding failures, /and/ it speeds up performance. I know this just shifts your argument about:

We would prefer a solution that addresses the performance issues without degrading the experience of using the entirety of the Cypress Test Runner at the time it is run

out of a configuration option and into an environment variable, but seeing that we already have an environment variable that hides the command log altogether, I just wanted to double check whether or not doing the same thing for limiting the command log is also off the table. My main push for this point is that, if consumers set CYPRESS_NO_COMMAND_LOG and find that there is a significant performance boost, there is not really a next step for them (while it fixes their perf issues, it cannot be run in production because it does not surface any error messaging), and so providing an alternate CYPRESS_LIMIT_COMMAND_LOG may be a way to bridge that gap (some requests here regarding that).

  1. If that doesn't sound good, another alternative could be to look into virtualization of some sort (since we know the command log can grow indefinitely, it sounds like we'd want to curb that, as opposed to continuing to render everything, but faster). If that's an approach you are aligned with, I'd be happy to poke around there next time I have a few cycles

@andrewmtam
Copy link
Author

Ooo one more idea came to mind:

  1. By default, limit the command log to show X items (lets say 25), but surface "Show more" and/or "Show all" button at the top of the command list for that section that a user can interact with if they need to see more commands. This sounds like it could be a good happy medium:
  • It provides a nice performance boost out of the box
  • If users need to see more commands, they can opt-in by clicking "Show more" within the same instance of the test run

Something kinda like this:
Screen Shot 2022-04-11 at 11 25 11 AM

Clicking "Show More" each time could increase the amount shown by 25. So on first click, 50 commands would show, then 75, 100, etc etc. But if the user really does want to see everything, they just click "Show All", and it'll behave as-if th command log was never limited

@andrewmtam andrewmtam mentioned this pull request Apr 12, 2022
5 tasks
@andrewmtam
Copy link
Author

Hey @jennifer-shehane , I wanted to give one more go at this and put up a PR here:

#21036

This does not implement the ideal solution you hinted at, but it does solve this problem:

Not being able to see x commands degrades the experience of the runner significantly as information is essentially lost from that run.

by providing a way to surface the entire command log list and allow users to click through snapshots from the whole test run. Curious to hear your thoughts on this approach if you've got a couple minutes!

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