testing-library / cypress-testing-library

🐅 Simple and complete custom Cypress commands and utilities that encourage good testing practices.
http://npm.im/@testing-library/cypress
MIT License
1.8k stars 153 forks source link

TypeError: container.querySelectorAll is not a function #109

Closed fredbliss closed 4 years ago

fredbliss commented 4 years ago

Relevant code or config


describe('anonymous calculator', () => {
  it('can make calculations', () => {
    cy.visit('/')
      .findByText(/^1$/)
      .click()
      .findByText(/^\+$/)
      .click()
      .findByText(/^2$/)
      .click()
      .findByText(/^=$/)
      .click()
      .findByTestId('total')
      .should('have.text', '3')
  })
})

What you did: Attempted to run tjs/cypress-04 branch of https://github.com/kentcdodds/jest-cypress-react-babel-webpack/tree/tjs/cypress-04

What happened:

Screen Shot 2020-01-30 at 1 52 55 PM

Reproduction repository: https://github.com/kentcdodds/jest-cypress-react-babel-webpack/tree/tjs/cypress-04

Problem description:

TypeError: container.querySelectorAll is not a function

Suggested solution:

--

kentcdodds commented 4 years ago

I think this is the same issue that was brought up here: https://github.com/testing-library/cypress-testing-library/pull/100#issuecomment-580095440

Thank you for opening it so we can discuss this. cc @tlrobinson and @twgraham.

When I merged that other PR I didn't realize it was going to break this use case. I personally like this style quite a bit and would like it very much if we could get the benefits that #100 was going for without losing the ability to write tests in this way.

If we cannot, then I think we should revert #100. Anyone have ideas?

kentcdodds commented 4 years ago

I'd also love feedback from @NicholasBoll on this as well.

fredbliss commented 4 years ago

Thanks for the quick response on this @kentcdodds!

kentcdodds commented 4 years ago

Just to be clear, the justification for the style I prefer is based on how get works:

describe('anonymous calculator', () => {
  it('can make calculations', () => {
    cy.visit('/')
      .get('.one')
      .click()
      .get('.plus')
      .click()
      .get('.two')
      .click()
      .get('.equals')
      .click()
      .get('.total')
      .should('have.text', '3')
  })
})

I really like that style and want to make sure we preserve that style for our queries. I think that is least surprising for people.

tlrobinson commented 4 years ago

However, this isn't how contains, find and most other cypress commands work, which I'd argue are more similar to the testing-library commands. get is kind of a special case.

The way Cypress recommends "reseting" to the root element is with end() (but "you can always start a new chain of commands off of cy" is the more common way of doing it anyway): https://docs.cypress.io/api/commands/end.html#Examples

IMHO the ability to chain commands is more important than supporting the above style, which isn't really idiomatic Cypress. IMHO this is much more readable:

    cy.visit('/')
    cy.get('.one').click()
    cy.get('.plus').click()
    cy.get('.two').click()
    cy.get('.equals').click()
    cy.get('.total').should('have.text', '3')
twgraham commented 4 years ago

@kentcdodds I think the comment i made in #100 would only apply to 5.1.0, this is 5.0.2 so it could be a different issue? 🤷‍♂️

kentcdodds commented 4 years ago

Oh, that's true. Maybe it is a different issue. Anyone want to dig in and figure out where the problem lies?

twgraham commented 4 years ago

Scratch that... It could be resolving the next minor version (5.1.0) because the package.json requires ^5.0.2. Probably the same issue 😅

fredbliss commented 4 years ago

Oh, Crumbs. No, I was on 5.1.1, I'm sorry @kentcdodds @twgraham!

twgraham commented 4 years ago

When I merged that other PR I didn't realize it was going to break this use case. I personally like this style quite a bit and would like it very much if we could get the benefits that #100 was going for without losing the ability to write tests in this way.

Your call - I can see the benefits of sticking to the old way, and the new way which @tlrobinson has provided. I don't think you can have it both ways though (from a usability perspective). It would become confusing (magic?) how the command resolves elements. I would also suggest that adding it as an option is a bad idea e.g. findByText(..., { usePrevious: true })

Pick a method and stick with it 👍

kentcdodds commented 4 years ago

Ok, I think that in order to address the breaking change I'm going to revert #100 right now, and then we can talk about the merits of making a breaking change to support it in a new major version bump.

kentcdodds commented 4 years ago

5.1.2 has been published to revert that change. I apologize for not recognizing the implications of the change in the first place. Sorry for the bump.

kentcdodds commented 4 years ago

I've opened a new issue to discuss adding #100 as a breaking change.

Please comment if you have opinions: #110

NicholasBoll commented 4 years ago

I'll look up this issue and see if it is related to #100 and if there is any way to introduce #100 as a non-breaking change

kentcdodds commented 4 years ago

:tada: This issue has been resolved in version 5.2.0 :tada:

The release is available on:

Your semantic-release bot :package::rocket:

ayush000 commented 4 years ago

For people who came across this issue, from what I understand, starting from @testing-library/cypress@6.0.0, they are no longer supporting chaining of multiple find* commands. So, you'll have to write it as

    cy.visit('/')
    cy.findByText(/^1$/).click()
    cy.findByText(/^\+$/).click()
    cy.findByText(/^2$/).click()
    cy.findByText(/^=$/).click()
    cy.findByTestId('total').should('have.text', '3')

More discussion about this decision on: https://github.com/testing-library/cypress-testing-library/issues/110

@kentcdodds @tlrobinson please correct me if I'm wrong. Also, it would be helpful to add this change to release changelog: https://github.com/testing-library/cypress-testing-library/releases.

kentcdodds commented 4 years ago

Hi @ayush000,

It was included 😁

Screenshot_20200404-144059

ayush000 commented 4 years ago

ahh! Missed it.

gusaiani commented 4 years ago

Hi all, regarding https://testingjavascript.com/lessons/cypress-installing-cypress-testing-library,

I tried to not add data-testid to markup that would be sent to prod, so ended up writing it this way:

    cy.visit('/')
    cy.findByText(/^1$/).click()
    cy.findByText(/^\+$/).click()
    cy.findByText(/^2$/).click()
    cy.findByText(/^=$/).click()
    cy.findAllByText(/^3$/).should('have.length', 2)

or we could sacrifice smallest addends and have no ambiguity, for example

    cy.findByText(/^5$/).click()
    cy.findByText(/^\+$/).click()
    cy.findByText(/^6$/).click()
    cy.findByText(/^=$/).click()
    cy.findByText(/^11$/).should('be.visible')

I'm currently learning from the course in real time (wonderful course btw), so if there are better ways, please @ me 👋

kentcdodds commented 4 years ago

I agree that would be better for the course. Will probably do something like that in the next update 👍 thanks!