Unhelpful error messages for query* commands

[NicholasBoll]

• cypress-testing-library version: 5.1.0

Relevant code or config

Test

cy.queryByLabelText('Test 1').click()
cy.queryByLabelText('Test 2').click()
cy.queryByLabelText('Test 3').click()
cy.queryByLabelText('Test 4').click()

HTML

Passing - no issues
<label for="1">Test 1</label>
<input id="1" />

Failing - couldn't find the label element
<label for="2">Test Oops</label>
<input id="2">

Failing - there is no `for` attribute on the label element
<label>Test 3</label>
<input id="3" />

Failing - couldn't find the input element
<label for="4">Test 4</label>
<input />

What happened:

The first passes fine. The consoleProps do not have the same description level as built-in Cypress commands

The second fails, but not as expected. The issue is the label element could not be found by the text "Test 2"

The third fails, but not as expected. The issue is the label element doesn't have a for attribute and has no way to associate with an input

The fourth fails, but not as expected. The issue is the label element has no corresponding input element.

All failures result in this error message:

CypressError: cy.click() failed because it requires a DOM element.

The subject received was:

  > {}

The previous command that ran was:

  > cy.then()

Note that the failure is on the cy.click command and not the queryByLabelText command. This is because https://github.com/testing-library/cypress-testing-library/blob/master/src/index.js#L87 simply wraps any returned value in an jQuery wrapper. It seems to be returning an empty object rather than an empty NodeList which Cypress automatically detects.

Also notice the error mentions the previous command was cy.then, even though that command wasn't actually used anywhere in user code. This is confusing. This is because of https://github.com/testing-library/cypress-testing-library/blob/master/src/index.js#L84 (use of cy.window().then()

There is also a comment about Promises not working with Cypress retryability which is not accurate. The problem probably comes from the use of cy.then. The original intent of Cypress Custom commands was not to use any cy commands at all. Internal Cypress Commands do not use them. Here's the source code for all the traversal-style commands: https://github.com/cypress-io/cypress/blob/develop/packages/driver/src/cy/commands/traversals.coffee. Notice promises are used in there just fine.

Problem description:
Failures to find elements returned from @testing-library/dom are unhelpful and confusing

Suggested solution:
Refactor the use of Cypress Custom Commands a bit to better handle not finding an element and remove the use of cy.window() and cy.then. Cypress has a cy.state function and it saves the window in cy.state('window') which is synchronously resolved.

Also try some of the query commands with known failures and see if the errors are useful. This goes a long way to guiding developers to finding out why something went wrong.

[NicholasBoll]

I also noticed $el isn't set correctly in the Cypress.log which means debugging won't give you the element returned by the query in the UI even if it shows up in the Yielded of the consoleProps

[NicholasBoll]

I also noticed the command does not retry queries even though there is a comment that says it doesn't retry for get* queries...

[kentcdodds]

These are fantastic recommendations! Would you be willing to make contributions to improve things as you suggest?

[NicholasBoll]

Yes! Working on those now

[NicholasBoll]

@kentcdodds Question: I read up on all the query types and looked through implementations in this repository: findBy, findByAll, queryBy, queryByAll, getBy, and getByAll.

getBy and getByAll have already been removed from this repo stating it doesn't make as much sense in the context of Cypress.

findBy and findByAll are actually delegating to the queryBy and queryByAll since Cypress handles retries.

queryBy and queryByAll synchronously resolve normally where findBy and findByAll resolve asynchronously. This is somewhat enforced in the code here by only allowing the find types to retry and not letting the query types retry. Does this make sense? All normal Cypress commands always retry. One of the stated usefulness of query resolving synchronously in the docs is to verify non-existence, but Cypress allows testing existence through .should('exist') and non-existence through .should('not.exist').

Does it make still make sense to not retry query* matchers? cy.find* already doesn't match the documentation because it can be modified by .should('not.exist').

I'm debating removing the restriction to not retry cy.query*, but then the functionality is exactly the same as cy.find*

[alexkrolick]

find queries are the most idiomatic in Cypress, we only have the query ones for legacy reasons at this point I think

[NicholasBoll]

@alexkrolick thanks! I agree. In order to have more useful errors, I changed the logic a bit to follow the same codepath as find queries, but with a 1ms duration. This keeps the messages consistent.

As I was thinking about my original issue with the more complex find[All]ByLabelText, I realized the get* query types already have more descriptive error messages. I'll try switching both query* and find* queries to use get* under the hood. Adding a retry-on-error loop will ensure the same retry as before, but with more descriptive error messages straight from @tesing-library/dom

We can see if that seems like the way to go