fix: No unnecessary snapshotting

[BlueWinds]

• Closes Fast memory leak/creep when cy.get fails to find the element, and eventually crashes #18549

User facing changelog

Bugfix: Prevent unnecessary snapshotting when running default assertions that would unnecessarily increase CPU use in cypress open mode and lead to out of memory crashes on certain browsers.

Additional details

Ensure that we don't snapshot while verifying implicit assertions, the same way that we don't snapshot when verifying upcoming explicit assertions

How has the user experience changed?

Improved performance, reducing both CPU and memory usage while waiting for assertions to pass. In certain versions of Chromium / Chrome / Electron, this also fixes a serious memory leak for failing tests.

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)
• [N/A] Has a PR for user-facing changes been opened in cypress-documentation?
• [N/A] Have API changes been updated in the type definitions?
• [N/A] Have new configuration options been added to the cypress.schema.json?

[cypress-bot]

Thanks for taking the time to open a PR!

• Create a Draft Pull Request if your PR is not ready for review. Mark the PR as Ready for Review when you're ready for a Cypress team member to review the PR.
• Become familiar with the Code Review Checklist for guidelines on coding standards and what needs to be done before a PR can be merged.

[cypress]

---

Test summary

18909 • 0 • 218 • 0 • 2

---

Run details

Project	cypress
Status	Passed
Commit	30b5f49
Started	Dec 20, 2021 7:49 PM
Ended	Dec 20, 2021 8:01 PM
Duration	11:28 💡
OS	Linux Debian - 10.10
Browser	Multiple

View run in Cypress Dashboard ➡️

---

Flakiness

	commands/net_stubbing_spec.ts		1
1	network stubbing > waiting and aliasing > can timeout waiting on a single request using "alias.request"
	cypress/source_map_utils_spec.js		1
1	driver/src/cypress/source_map_utils > .extractSourceMap > is performant with multiple source map comments in file

---

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

[brian-mann]

you can work off of the event that cypress fires when it retries in order to count the number of retries that happened

• on the first retry, spy on cy.createSnapshot
• and then make sure that after it fails, there was only a single call to cy.createSnapshot
• and that the retry count was >1 times

that would tell you it took a snapshot on the final “error” state and not during the interstitial retries, and that it did go through the retry loop multiple times

its like... cy.on('retry', ...) or cy.on('command:retry', ...) I can't remember but there are other tests that use this event to count things.

that’ll give you a consistent and deterministic way to ensure that it both retried AND only created 1 snapshot and that test should obviously fail if we reverted the fix

[BlueWinds]

Reworked this into a more generic utility listening to cypress events, and added an event for snapshoting since we didn't already have one.

I also split the test into two pieces, one which precisely pinpoints the problem (a failing DOM assertion is taking three snapshots outside the context of retries, two of them because of ensureElExistence), and one which demonstrates it more dramatically (these calls because of ensureElExistence repeat with every retry, resulting in hundreds of snapshots).

[emilyrohrbough]

Could be a bit off here on the flow so if my comments seem way off, sorry in advance!
I have a few questions....

1. Mind adding a few more details on how override the assertion method is fixing this? It's not 100% apparent.

2. Ensure that we don't snapshot for assertions that fail our retry logic already takes a snapshot at the end when a test actually fails.

Is this true when retries are set to 0 in the configuration? Does your fix handle this scenario?

2. Did you consider updating the retries logic here to account for this given the finishAssertions callback provided to the retries class which uses the assertion class' finishAssertion method which also takes the snapshot?

      // snapshot the DOM since we are bailing so the user can see the state we're in
      // when we fail
-      if (log) {
-        log.snapshot()
-      }

      const assertions = options.assertions

      if (assertions) {
        finishAssertions(assertions)
      }
+    else if (log) {
+        log.snapshot()
+    }

4. Inverse of the previous thought (prob better to keep implementation details separate), could we update the assertion class' finishAssertion method to determine when it should take a snaphot? (i.e. when retries === false?)

+ // by default the retries handle will take the snapshot for us
+  const finishAssertions = (assertions, shouldTakeSnapshot = false) => {
-  const finishAssertions = (assertions) => {
-     return _.each(assertions, (log) => {
+     if (shouldTakeSnapshot) {
      log.snapshot()

Then updating this case to pass in "yes should take screenshot"

[emilyrohrbough]

Do we not manage / track the number of snapshots taken during a test? I assumed this data was reported to the dashboard?

[BlueWinds]

These DOM snapshots are the ones visible when you hover over test steps and it displays how the application looked at that point in time. They're very ephemeral - we only take them in open mode, and they live in the browser running tests, not even sent to the cypress server. We discard the snapshots regularly (if the number of tests run is greater than numTestsKeptInMemory, whenever we rerun tests, etc).

We didn't even have an internal event for counting them until I added it in this PR (Cypress.action('cy:snapshot', name)) so this utility could listen for it.

[BlueWinds]

Could be a bit off here on the flow so if my comments seem way off, sorry in advance! I have a few questions....

1. Mind adding a few more details on how override the assertion method is fixing this? It's not 100% apparent.

Yeah, the key here is that I'm not doing something new, I'm covering another case and doing the same thing we were already doing in the other location.

verifyUpcomingAssertions has two exit points. It can return on either line 328 if there are no upcoming commands:

if (!cmds.length) {
        return Promise
        .try(ensureExistence)
        .then(onPassFn)
        .catch(onFailFn)
}

or it can return on line 490:

     cy.state('overrideAssert', overrideAssert)

      return Promise
      .reduce(fns, assertions, [subject])
      .then(() => {
        restore()
        ...
      }).catch(() => {
        restore()
        ...
     })

When there are upcoming commands in the queue - when we're verifying assertions like ".should('have.class', 'foo')", then we override the assert function in a way that passes in verifying: true - line 442 inside overrideAssert. This PR makes the same thing happen for implicit assertions.

So to try and put it in English a bit better: In general, when an assertion fails, we want to take a snapshot, so we can show the users what went wrong. But when we're when we're in the middle of cy.get() checking to see if we should move on to following commands or sit here retrying cy.get() - that is to say we're verifyingUpcomingAssertions - then we execute the assertion code every 50ms, but we're only doing to to see if we're ready to move on - not in order to pass or fail the assertion itself!

That's why what verifying = false | true on line 128 in assertFn represents. If it's false, we are actually executing an assertion - we do things like put an error on the log message and take a snapshot. If we're only verifying upcoming assertions, then we still execute the assertion code - but verifying = true tells us not to do things that we only want to do once at the very end (eg, take a snapshot to display to the user).

2. > Ensure that we don't snapshot for assertions that fail our retry logic already takes a snapshot at the end when a test actually fails.

Is this true when retries are set to 0 in the configuration? Does your fix handle this scenario?

I'll update the PR description here - I wrote it based on an earlier misunderstanding of the bug. It's what the first test I added, with timeout: 0 is demonstrating - we were taking extra snapshots unrelated to retries.

Retries are not the cause of the bug, they just make it way more obvious. Setting {timeout: 0} in that first test ensures that no retries occur - but if you run that test against develop (or comment out the code additions in this branch), you'll see that three snapshots were occurring even though there were 0 retries.

2. Did you consider updating [the retries logic here](https://github.com/cypress-io/cypress/blob/issue-18549-no-unnecessary-snapshotting/packages/driver/src/cy/retries.ts?rgh-link-date=2021-12-16T21%3A42%3A37Z#L57-L65) to account for this given the [finishAssertions callback](https://github.com/cypress-io/cypress/blob/issue-18549-no-unnecessary-snapshotting/packages/driver/src/cypress/cy.ts?rgh-link-date=2021-12-16T21%3A42%3A37Z#L250-L254) provided to the retries class which uses the [assertion class' finishAssertion method which also takes the snapshot](https://github.com/cypress-io/cypress/blob/issue-18549-no-unnecessary-snapshotting/packages/driver/src/cy/assertions.ts?rgh-link-date=2021-12-16T21%3A42%3A37Z#L205)?

Yeah, my initial attempt to fix this was in the retries logic - but that's why the first test is so important. It demonstrates the problem outside the context of retries. If we're triple snapshoting without retries, then making sure we only do three snapshots is still not the correct solution - there should only be one!

I really should have updated the description before sending this over.

[mjhenkes]

Succinct solution to a deep problem. Thanks for the detailed description in the comment, without that i'd have had no idea what this was doing.

[emilyrohrbough]

maybe out of scope, but thoughts on renaming verifying to something a little more self-explanatory.... idea might be isVerifyingUpcomingAssertions (the terminology you emphasized in your log description).

[BlueWinds]

I think I'd like to save renaming things for a later date. My real goal would be to make a flowchart of the logic and consider a refactor of this area of code - Brian spoke to some regrets that we didn't just re-implement parts of Chai, rather than weaving in and out of 3rd party code and keeping track of state internally to make it behave as we want.

[brian-mann]

• call a method on testCommands which returns an immutable command state to assert on in order to prevent those assertions from mutating the command state
• try out chai-match-pattern to create more flexibility in the assertions of the command state

[BlueWinds]

• call a method on testCommands which returns an immutable command state to assert on in order to prevent those assertions from mutating the command state

  • try out chai-match-pattern to create more flexibility in the assertions of the command state

Both done in latest commit. Turns out we already have chai-subset which serves the same purpose (but neither of us could remember the name while on the call).

[emilyrohrbough]

Mind expanding the User Facing Changelog a bit to expand on what you mean by "churn"?

[BlueWinds]

Mind expanding the User Facing Changelog a bit to expand on what you mean by "churn"?

Tweaked the wording slightly, but not sure how to make it clearer: taking fewer snapshots means less CPU usage. Creating a copy of the DOM in memory is expensive, doing it less frequently is cheaper.

The difference is very noticeable - running the test

it('is slow', () => {
  cy.visit('https://duckduckgo.com/?t=ffab&q=how+big+is+the+empire+state+building&ia=web')
  cy.get('.doesNotExist', { timeout: 10000 })
})

revs up the fans on my laptop on develop, and does so much less with this PR branch.

[emilyrohrbough]

@BlueWinds Nice job on the changelog update. 👍🏻 I think it is cleared to an end-user what this change means for them.

Dismissing Brian's review since he paired with Blue on this.