Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

TypeError: container.querySelectorAll is not a function #109

Closed
fredbliss opened this issue Jan 30, 2020 · 20 comments · Fixed by #108
Closed

TypeError: container.querySelectorAll is not a function #109

fredbliss opened this issue Jan 30, 2020 · 20 comments · Fixed by #108
Labels

Comments

@fredbliss
Copy link

fredbliss commented Jan 30, 2020

  • cypress-testing-library version: 5.1.1
  • node version: 12.4.0
  • npm (or yarn) version: 1.21.1 (yarn)

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
Copy link
Member

I think this is the same issue that was brought up here: #100 (comment)

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
Copy link
Member

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

@fredbliss
Copy link
Author

Thanks for the quick response on this @kentcdodds!

@kentcdodds
Copy link
Member

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
Copy link
Member

tlrobinson commented Jan 30, 2020

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
Copy link

@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
Copy link
Member

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

@twgraham
Copy link

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
Copy link
Author

fredbliss commented Jan 30, 2020

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

@twgraham
Copy link

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
Copy link
Member

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 added a commit that referenced this issue Jan 30, 2020
@kentcdodds
Copy link
Member

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
Copy link
Member

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

Please comment if you have opinions: #110

@NicholasBoll
Copy link
Contributor

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

NicholasBoll added a commit to NicholasBoll/cypress-testing-library that referenced this issue Jan 31, 2020
kentcdodds pushed a commit that referenced this issue Feb 2, 2020
* feat: add more helpful debugging information to queries
* Add element selector information for debugging (outlines element when you click on command) (fixes #103)
* Add @testing-library/dom errors (from `get*` queries) to failure messages - these are more helpful than the generic `find*('input') does not exist` messages (fixes #103)
* Add retryability to `findBy*` when multiple elements are found (fixes #83)
* Add option to disable logging of all commands
* `query*` and `find*` have a consistent code path and error messaging (fixes #103)
* Remove usage of Cypress commands in queries (fixes #103)

* feat: add ability for queries to inherit previous subject
* Fixes #109 without breaking change caused by #100

* feat: add parent/child log type detection

* chore: implement feedback

* docs: update readme to complete my thought process

* slightly simplify config fn

* Update README.md

Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>

Closes #103, Closes #109, Closes #110
@kentcdodds
Copy link
Member

🎉 This issue has been resolved in version 5.2.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@ayush000
Copy link

ayush000 commented Apr 4, 2020

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: #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
Copy link
Member

Hi @ayush000,

It was included 😁

Screenshot_20200404-144059

@ayush000
Copy link

ayush000 commented Apr 5, 2020

ahh! Missed it.

@gusaiani
Copy link

gusaiani commented Apr 6, 2020

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
Copy link
Member

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants