cypress-testing-library icon indicating copy to clipboard operation
cypress-testing-library copied to clipboard

Chained cypress-testing-library commands don't work as expected

Open dsumer opened this issue 5 years ago • 14 comments

  • cypress-testing-library version: 6.0.0
  • node version: 12.14.1
  • npm (or yarn) version: yarn 1.21.1

Relevant code or config

cy.findAllByTestId('test-id').findByText('mytext').parents('.parent');

What you did: I want to chain commands from cypress-testing-library, so that they are executed in the same context. In the example above I want to retrieve the element which contains the text "mytext" but only for elements which have a parent (somewhere in the upper DOM) with a data-testid attribute with the value 'test-id'.

What happened: The command doesn't find an element with the given text, even if there would be such an element (proof is being provided in the Hint below).

image

Hint:

If I run the same code as above, but with a contains instead of the findByText everything works as expected:

cy.findAllByTestId('test-id').contains('mytext').parents('.parent');

image

Thanks for this library and the whole testing-library family in general! Cheers!

dsumer avatar Mar 18 '20 07:03 dsumer

Thanks for filing an issue!

Do you have a sample HTML that the commands don't work against?

Just a guess, but perhaps the command in this library is failing to handle an array of elements. This library delegates to DOM testing library and probably isn't iterating over all the elements found by findAllByTestId (which returns an array) and passing each to DOM testing library.

Would you care to take a crack at a PR?

NicholasBoll avatar Mar 21 '20 16:03 NicholasBoll

I'm not sure your specific DOM and wasn't able to reproduce the problem, but I think the following is related:

  <section>
    <h2>Multiple elements</h3>
    <div class="multiple-matches">
      <p>Text 1</p>
    </div>
    <div class="multiple-matches">
      <p>Text 2</p>
    </div>
  </section>
  // passes
  it('findByText should find element if given multiple elements and match is in first given element ', () => {
    cy.get('.multiple-matches')
      .findByText('Text 1')
      .should('exist')
  })

  // fails. Same error you have
  it('findByText should find element if given multiple elements and match is in second given element ', () => {
    cy.get('.multiple-matches')
      .findByText('Text 2')
      .should('exist')
  })

  // fails.
  it('findAllByText should handle multiple parent elements and return multiple results', () => {
    cy.get('.multiple-matches')
      .findAllByText(/Text \d/)
      .should('have.length', 2)
  })

This tells me that only the first element returned by the cy.get is being used as a subject for finding more things. That is definitely a bug.

NicholasBoll avatar Mar 21 '20 16:03 NicholasBoll

I see DOM testing library is being called with only a single element: https://github.com/testing-library/cypress-testing-library/blob/ee75c1495bb3963a3becb5bfe3c05b8e901313de/src/index.js#L47-L50

https://github.com/testing-library/cypress-testing-library/blob/ee75c1495bb3963a3becb5bfe3c05b8e901313de/src/utils.js#L1-L14

This logic will have to change to iterate over an array of subjects.

NicholasBoll avatar Mar 21 '20 17:03 NicholasBoll

Not 100% sure if it's related to this. I updated downshift dependencies, this one to 6.0.0 and cypress to 4.3.0.

Apparently all findByTestId queries that are chained after a click or a type fail saying that they cannot find the element. I did not dig deeper into it than what I just posted.

Screenshot 2020-04-07 at 9 46 36 PM

silviuaavram avatar Apr 07 '20 18:04 silviuaavram

Hi @silviuaavram,

I'm guessing your issue is related to the breaking change documented here: https://github.com/testing-library/cypress-testing-library/releases/tag/v6.0.0

Should be a reasonably easy update for you :)

kentcdodds avatar Apr 07 '20 23:04 kentcdodds

thank you @kentcdodds ! sorry that I pointed out something that was obvious. I am trying to update the downshift dependencies and I am going from one failure to another. but I hope this one is the last piece! thanks again for pointing it out!

silviuaavram avatar Apr 08 '20 08:04 silviuaavram

I see DOM testing library is being called with only a single element:

https://github.com/testing-library/cypress-testing-library/blob/ee75c1495bb3963a3becb5bfe3c05b8e901313de/src/index.js#L47-L50

https://github.com/testing-library/cypress-testing-library/blob/ee75c1495bb3963a3becb5bfe3c05b8e901313de/src/utils.js#L1-L14

This logic will have to change to iterate over an array of subjects.

I was running into the same issue as the OP with a test I wrote a long time ago like this: cy.findAllByRole("heading").findByText("Add Device")

But what I really wanted to do is: cy.findAllByRole("heading", { name: "Add Device" })

That being said, is there ever a case where we want to be able to query off a collection of elements?

I tended to think I could do this, similar to Cypress/jQuery: cy.get("parent").find("children") or cy.get("parent").filter("parentResult")

samtsai avatar May 13 '20 18:05 samtsai

Hi, @kentcdodds. Regarding the breaking change here https://github.com/testing-library/cypress-testing-library/releases/tag/v6.0.0

cy.findByText('Foo').click()
-   .findByText('Bar').click() // Element with 'Bar' text is not a child of an element with 'Foo' text
+ cy.findByText('Bar').click()

why the chaining behavior was removed? It seemed to work just fine previously

Jhonatangiraldo avatar Jul 28 '20 15:07 Jhonatangiraldo

It's a bit unclear to me whether the chaining behaviour was removed on purpose (as I read @kentcdodds reply), or if it's a bug (@NicholasBoll's reply).

Personally, I think the chaining is useful. For example, say that in a table I want to verify that a particular cell (selected by the 'rowheader' role), has sibling cells with a certain expected content.

If you can't chain I don't see how I could gradually scope the selection of elements. For a practical example, see the screenshot below:

Screenshot 2021-02-24 at 11 45 53

Let's say I want to ensure that Job 1 (rowheader) is rendering with the expected type 'Credentials expiry alert'. I'd expect to be able to do:

    cy.findByRole('rowheader', { name: 'Job 1' })
        .siblings()
        .findByRole('cell', { name: 'Credentials expiry alert' })
        .should('exist')

Yes, I could do this (and that works):

    cy.findByRole('cell, { name: 'Credentials expiry alert' }).should('exist')

But that doesn't guarantee that the expected cell is rendered in the row that contains the Job 1 rowheader.

ismay avatar Feb 24 '21 10:02 ismay

I also find the current behaviour really confusing and conter-intuitive.

cy.findByRole('rowheader', { name: 'Job 1' })
  .findByRole('cell', { name: 'Credentials expiry alert' })
// ^^^^^^^^^^
// Uses subject from 'rowheader'

cy.get('#myContainer').within(() => {
  cy.findByRole('rowheader', { name: 'Job 1' })
    .findByRole('cell', { name: 'Credentials expiry alert' })
//   ^^^^^^^^^^
//   Uses subject from '#myContainer'
})

Additionally, this makes the container option a no-op when used within a within callback

cy.get('#myContainer').within(() => {
  cy.findByRole('rowheader', { name: 'Job 1', container: myContainer })
//                                            ^^^^^^^^^^^^^^^^^^^^^^
//                                            Does nothing...
})

Xiphe avatar Apr 30 '21 16:04 Xiphe

@ismay @Xiphe Chaining wasn't removed, but Cypress Testing Library commands were updated to behave like native Cypress commands. Previously container was used to align with React Testing Library, but that's not how Cypress commands work. Cypress commands take the previous subject and further query off of that.

Before v6, each command would start at the root document (unless container was specified). This isn't what normal Cypress commands do.

I find excessive chaining confusing, especially if chaining doesn't care about previous results:

// before
cy
  .findByLabelText('username')
  .type('User')
  .findByLabelText('password')
  .type('Password123')
  .findByRole('button', { name: 'Submit' })
  .click()

// Reads like a run-on sentence:
// Find element with a label of "username" and type "User" and then find element with a label of "password" and type "Password123" and then find a button with name "Submit" and click on it.

// after
cy.findByLabelText('username').type('User')
cy.findByLabelText('password').type('Password123')
cy.findByRole('button', { name: 'Submit' })

// Reads like independent sentences organized as steps
// * Find element with a label of "username" and type "User"
// * Find element with a label of "password" and type "Password123".
// * Find a button with name "Submit" and click on it.

Cypress's subject chaining system allows for subjects to be refined. The cell example is a good one:

cy.findByRole('rowheader', { name: 'Job 1' })
  .findByRole('cell', { name: 'Credentials expiry alert' })
// ^^^^^^^^^^
// Uses subject from 'rowheader'. This scopes the query to only cells within the row header

@ismay, you wouldn't do siblings, you'd do:

    cy.findByRole('rowheader', { name: 'Job 1' })
        .findByRole('cell', { name: 'Credentials expiry alert' })
        .should('exist')
// Back to the sentence:
// Find a row labelled "Job 1" and within find a cell labelled "Credentials expiry alert" and assert it exists.

If you don't want to scope, don't chain. Starting a new chain starts the subject over to the document - a fresh, document-wide query. Scoping allows you to get more specific.

Tests are a lot easier to read if chains follow a sentence structure.

@Xiphe The following is a bug:

cy.get('#myContainer').within(() => {
  cy.findByRole('rowheader', { name: 'Job 1' })
    .findByRole('cell', { name: 'Credentials expiry alert' })
//   ^^^^^^^^^^
//   Uses subject from '#myContainer'
})

The subject in findByRole should be rowheader and not #myContainer. within is supposed to scope top-level queries and not scoped queries.

NicholasBoll avatar Jun 02 '21 18:06 NicholasBoll

@ismay, you wouldn't do siblings, you'd do:

cy.findByRole('rowheader', { name: 'Job 1' })
    .findByRole('cell', { name: 'Credentials expiry alert' })
    .should('exist')

The rowheader in my example is a cell, it seems you're interpreting it as a row. That's why I'm using siblings.

I was trying to do exactly what you're mentioning, creating a chain that drills down and could potentially be read as a single sentence. I preferred that over the alternative I listed, as it's more specific and tests what I want to test more precisely. It confused me that this didn't work, which is why I commented in this issue, as I thought it related to what was being discussed. Let me know if it doesn't.

ismay avatar Jun 03 '21 05:06 ismay

@NicholasBoll thanks for taking the time to give background and reasoning. Really appreciated! Should I open a separate issue for the bug or is it fine to keep it with this one?

Xiphe avatar Jun 21 '21 14:06 Xiphe

from https://github.com/testing-library/cypress-testing-library/issues/132#issuecomment-784988617

But that doesn't guarantee that the expected cell is rendered in the row that contains the Job 1 rowheader.

I needed to find a cell that was a sibling of another on a row, and this is the approach I found that works pretty well

cy.findByRole('cell', { name: 'Job 1' })
  .parent() // The row
  .within(() => { 
    // now `cy` is scoped to the row containing 'Job 1'
    cy.findByRole('cell', { name: 'Credentials expiry alert' }).should('exist')
  });

IanVS avatar Oct 04 '21 15:10 IanVS