fix: No unnecessary snapshotting

packages/driver/cypress/integration/commands/assertions_spec.js

@@ -1,6 +1,39 @@
 const { assertLogLength } = require('../../support/utils')
 const { $, _ } = Cypress
 
+const captureCommands = () => {
+  const commands = []
+
+  let current
+
+  cy.on('command:start', (command) => {
+    current = command
+    commands.push({
+      name: command.attributes.name,
+      snapshots: 0,
+      retries: 0,
+    })
+  })
+
+  cy.on('command:retry', () => {
+    commands[commands.length - 1].retries++
+  })
+
+  cy.on('snapshot', () => {
+    // Snapshots can occur outside the context of a command - for example, `expect(foo).to.exist` without any wrapping cy command.
+    // So we keep track of the current command when one starts, and if we're not inside that, create an 'empty' command
+    // for the snapshot to belong to
+    if (!commands.length || current !== cy.state('current')) {
+      current = null
+      commands.push({ name: null, snapshots: 0, retries: 0 })
+    }
+
+    commands[commands.length - 1].snapshots++
+  })
+
+  return commands
+}
+
 describe('src/cy/commands/assertions', () => {
   before(() => {
     cy
@@ -10,10 +43,14 @@ describe('src/cy/commands/assertions', () => {
     })
   })
 
+  let testCommands
+
   beforeEach(function () {
     const doc = cy.state('document')
 
     $(doc.body).empty().html(this.body)
+
+    testCommands = captureCommands()
   })
 
   context('#should', () => {
@@ -32,6 +69,15 @@ describe('src/cy/commands/assertions', () => {
       cy.noop({ foo: 'bar' }).should('deep.eq', { foo: 'bar' }).then((obj) => {
         expect(obj).to.deep.eq({ foo: 'bar' })
       })
+
+      cy.then(() => {
+        expect(testCommands).to.eql([
+          { name: 'noop', snapshots: 0, retries: 0 },
+          { name: 'should', snapshots: 1, retries: 0 },
+          { name: 'then', snapshots: 1, retries: 0 },
+          { name: 'then', snapshots: 0, retries: 0 },
+        ])
+      })
     })
 
     it('can use negation', () => {
@@ -1475,7 +1521,7 @@ describe('src/cy/commands/assertions', () => {
           done()
         })
 
-        cy.get('div').should(($divs) => {
+        cy.get('div', { timeout: 100 }).should(($divs) => {
           expect($divs, 'Filter should have 1 items').to.have.length(1)
         })
       })
@@ -2927,4 +2973,34 @@ describe('src/cy/commands/assertions', () => {
       cy.get('.foo').should('not.exist')
     })
   })
+
+  context('implicit assertions', () => {
+    // https://github.com/cypress-io/cypress/issues/18549
+    // A targeted test for the above issue - in the absence of retries, only a single snapshot
+    // should be taken.
+    it('only snapshots once when failing to find DOM elements', (done) => {
+      cy.on('fail', (err) => {
+        expect(testCommands[0].snapshots).to.eq(1)
+        done()
+      })
+
+      cy.get('.badId', { timeout: 0 })
+    })
+
+    // https://github.com/cypress-io/cypress/issues/18549
+    // This issue was also causing two DOM snapshots to be taken every 50ms
+    // while waiting for an element to exist. The first test is sufficient to
+    // prevent regressions of the specific issue, but this one is intended to
+    // more generally assert that retries do not trigger multiple snapshots.
+    it('only snapshots once when retrying assertions', (done) => {
+      cy.on('fail', (err) => {
+        const totalSnapshots = _.sum(_.map(testCommands, 'snapshots'))
+
+        expect(totalSnapshots).to.eq(1)
+        done()
+      })
+
+      cy.get('.badId', { timeout: 1000 })
+    })
+  })
 })

packages/driver/src/cy/assertions.ts

@@ -143,9 +143,19 @@ export const create = (Cypress, cy) => {
       obj.$el = $dom.wrap(value)
     }
 
-    // if we are simply verifying the upcoming
-    // assertions then do not immediately end or snapshot
-    // else do
+    // `verifying` represents whether we're deciding whether or not to resolve
+    // a command (true) or of we're actually performing a user-facing assertion
+    // (false).
+
+    // If we're verifying upcoming assertions (implicit or explicit),
+    // then we don't need to take a DOM snapshot - one will be taken later when
+    // retries time out or the command otherwise entirely fails or passes.
+    // We save the error on _error because we may use it to construct the
+    // timeout error which we eventually do display to the user.
+
+    // If we're actually performing an assertion which will be displayed to the
+    // user though, then we want to take a DOM snapshot and display this error
+    // (if any) in the log message on screen.
     if (verifying) {
       obj._error = error
     } else {
@@ -267,6 +277,7 @@ export const create = (Cypress, cy) => {
       }
 
       const onPassFn = () => {
+        cy.state('overrideAssert', undefined)
         if (_.isFunction(callbacks.onPass)) {
           return callbacks.onPass.call(this, cmds, options.assertions)
         }
@@ -289,6 +300,7 @@ export const create = (Cypress, cy) => {
           err = e2
         }
 
+        cy.state('overrideAssert', undefined)
         err.isDefaultAssertionErr = isDefaultAssertionErr
 
         options.error = err
@@ -324,6 +336,24 @@ export const create = (Cypress, cy) => {
       // bail if we have no assertions and apply
       // the default assertions if applicable
       if (!cmds.length) {
+        // In general in cypress, when assertions fail we want to take a DOM
+        // snapshot to display to the user. In this case though, when we invoke
+        // ensureExistence, we're not going to display the error (if there is
+        // one) to the user - we're only deciding whether to resolve this current
+        // command (assertions pass) or fail (and probably retry). A DOM snapshot
+        // isn't necessary in either case - one will be taken later as part of the
+        // command (if they pass) or when we time out retrying.
+
+        // Chai assertions have a signature of (passed, message, value, actual,
+        // expected, error). Our assertFn, defined earlier in the file, adds
+        // on a 7th arg, "verifying", which defaults to false. We here override
+        // the assert function with our own, which just invokes the old one
+        // with verifying = true. This override is cleaned up immediately
+        // afterwards, in either onPassFn or onFailFn.
+        cy.state('overrideAssert', function (...args) {
+          return assertFn.apply(this, args.concat(true))
+        })
+
         return Promise
         .try(ensureExistence)
         .then(onPassFn)
@@ -477,8 +507,6 @@ export const create = (Cypress, cy) => {
         return cy.state('overrideAssert', undefined)
       }
 
-      // store this in case our test ends early
-      // and we reset between tests
       cy.state('overrideAssert', overrideAssert)
 
       return Promise

packages/driver/src/cy/snapshots.ts

@@ -132,6 +132,7 @@ export const create = ($$, state) => {
   }
 
   const createSnapshot = (name, $elToHighlight) => {
+    Cypress.action('cy:snapshot', name)
     // create a unique selector for this el
     // but only IF the subject is truly an element. For example
     // we might be wrapping a primitive like "$([1, 2]).first()"

packages/driver/src/cypress.ts

@@ -515,6 +515,9 @@ class $Cypress {
       case 'cy:scrolled':
         return this.emit('scrolled', ...args)
 
+      case 'cy:snapshot':
+        return this.emit('snapshot', ...args)
+
       case 'app:uncaught:exception':
         return this.emitMap('uncaught:exception', ...args)