Chains of query commands do not retry anything before an assertion

[bahmutov]

Current behavior

Notice in v12 (using 12.1.0 right now) that if there are assertions in the chain of queries, the query commands before an assertion are NOT retried, even if the assertion should pass.

Desired behavior

Cypress should go all the way to the start of the chain of queries and retry it, even if there are assertions in the mix.

Test code to reproduce

const double = (n) => n * 2

// @ts-ignore
Cypress.Commands.addQuery('apply', (fn) => (n) => fn(n))

it('fails to retry the cy.its', () => {
  const list = []
  cy.wrap(list)
    .its(0) // first item
    // this assertion breaks the query chain retries
    // it never "sees" the new number 5
    // because it never retries cy.its above
    .should('be.a', 'number')
    .apply(double)
    .should('equal', 10)

  setTimeout(() => {
    list[0] = 1
  }, 1000)

  setTimeout(() => {
    list[0] = 5
  }, 2000)
})

it('retries queries with assertions', () => {
  const list = []
  cy.wrap(list)
    // several queries (without assertion)
    .its(0) // first item
    .apply(double)
    .should('equal', 10)

  setTimeout(() => {
    list[0] = 1
  }, 1000)

  setTimeout(() => {
    list[0] = 5
  }, 2000)
})

https://github.com/bahmutov/cypress-map/blob/main/cypress/e2e/chain-with-assertions.cy.js

Cypress Version

v12

Node version

16

Operating System

mac

Debug Logs

No response

Other

No response

[BlueWinds]

Hm, you're right, it's definitely intended to go through as you're describing. Thank you for the extremely precise repro - looking into this now.

[BlueWinds]

Putting out some notes from our meeting on the subject.

tl;dr: We will be fixing this in the next feat release. Command log improvements will follow at a later date.

---

This was originally an intentional decision in Cypress 12, which we were forced into because of a few rules.

1. Assertions can change the subject.

  cy.get('button')                     // [undefined, get()]
    .should('have.css', 'font-family') // Subject now must resolve to 'sans-serif', so either [undefined, get(), should()] or ['san-serif']
    .then(sub => { ... })

Therefore, assertions must modify the subject chain somehow.

2. We don't want to retry assertions inside aliases. Eg:

  cy.get('button')             // [undefined, get()]
    .should('not.have.focus')  //
    .as('btn')                 // Can't have subject of [undefined, get(), should()], because otherwise...

  cy.get('@btn')               //
    .focus()                   //

  cy.get('@btn')               // ...this will fail because the element is now focused, and we have no subject.

Therefore, assertions must not be queries, because they cannot be appended / retried.

3. Thus, assertions are currently non-query commands. They replace the subject chain, rather than append to it, meaning:

  cy.get('button')   // [undefined, get()]
    .should('exist') // [button]
    .click()         // ^ can cause detached dom errors

is an unsafe operation, susceptible to detached dom errors.

4. Additional complication:

Whatever is returned in the function is ignored. Cypress always forces the command to yield the value from the previous cy command's yield (which in the example below is )

  cy.get('button')
    .should(($button) => {
      expect({ foo: 'bar' }).to.deep.eq({ foo: 'bar' })

      return { foo: 'bar' } // return is ignored, .should() yields <button>
    })
    .then(($button) => {
      // do anything we want with <button>
    })

That is to say: only builtin chainers can change the subject, user supplied callbacks can't. Weird inconsistency in the current rules.

---

After discussing several solutions, we decided to drop rule 2 above. Aliases will now include assertions in their subject chain, and retry them when queried.

  cy.get('button')             // [undefined, get()]
    .should('not.have.focus')  // [undefined, get(), should()]
    .as('btn')                 // Querying the alias later could throw an error, if the button now has focus.

This has the advantage that assertions now follow the same rules as all other queries. The rules are very clear and straightforward.

Downside: When they fail, you don't get the same visual patterns (eg, command log shows green -> red) as when you first were doing it. Mitigation:

• We have to make errors very clear in the error log when things fail about why and where.
• We can also do command grouping, nesting retried queries inside the .get(). Collapses if everything passes. The command log might look like this:

    .get('@btn')
      > .get('button')
      > .should('not.be.focused')

Then, once .get() resolves, we collapse the children before adding the next command:

    .get('@btn') (two hidden children)
      .find('Login')

A test that will fail under the new rules:

      cy.get('modal')
        .should('be.visible')
        .as('m')

      cy.closeModal()

      cy.get('@m')
        .should('not.be.visible')

The command log would look like:

    .get('@m')
      > .get('modal')
      > .should('be.visible') ----- in red, failed

.get() never finished resolving, so it remains uncollapsed and shows what happened as we tried to fetch the alias (what was contained inside the query chain).

We will release the behavior change (assertions becoming full-fledged queries) in a feat release, alongside the {type: 'value'} flag for aliases (see #25251), so that users can go back to the previous behavior if they want.

The command log updates (collapsing groups when querying aliases) can follow at a later date, it's not required immediately.

[BlueWinds]

@emilyrohrbough - This is the issue we discussed last Thursday in our meeting with Brian. Notes posted just above.

[BlueWinds]

Just a bit of an update - I've been out sick a fair bit recently, but this is what I'm focusing on at the moment. It's a fair bit larger and more complicated than I'd hoped, but will be a huge gain in simplicity of Cypress' internal code once I finish working out all the kinks in the pull draft request.

[samtsai]

Just a bit of an update - I've been out sick a fair bit recently, but this is what I'm focusing on at the moment. It's a fair bit larger and more complicated than I'd hoped, but will be a huge gain in simplicity of Cypress' internal code once I finish working out all the kinks in the pull draft request.

Your health first! Thanks for the update.

[BlueWinds]

Some further updates - some prerequisite work for this is being done in #25595, which should be merged into the 13.0.0 release soon.

#25296 is mostly ready, just pending the above changes.

[cypress-app-bot]

This issue has not had any activity in 180 days. Cypress evolves quickly and the reported behavior should be tested on the latest version of Cypress to verify the behavior is still occurring. It will be closed in 14 days if no updates are provided.

[cjcrandall]

Seems like this shouldn't be closed until Cypress 13 is released? Also, following the chain of PR references it's hard to tell if what was originally PR #25296 (then #25738) even made it into the Cypress 13 branch. The former says it was replaced by the latter, but the latter shows closed, not merged.

[bahmutov]

Just got burned by this again. Please do some work and fix this issue since it makes better practices (add more assertions) break the retry-ability in a really spectacular ways that is super hard for people to debug.

[cypress-app-bot]

This issue has not had any activity in 180 days. Cypress evolves quickly and the reported behavior should be tested on the latest version of Cypress to verify the behavior is still occurring. It will be closed in 14 days if no updates are provided.

[cjcrandall]

We still keep having to work around this.