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

feat: add more helpful debugging information to queries #108

Merged
35 changes: 30 additions & 5 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -109,24 +109,49 @@ To show some simple examples (from
[cypress/integration/query.spec.js](cypress/integration/query.spec.js) or [cypress/integration/find.spec.js](cypress/integration/find.spec.js)):

```javascript
cy.queryByText('Button Text').should('exist')
cy.queryByText('Non-existing Button Text').should('not.exist')
cy.queryByLabelText('Label text', {timeout: 7000}).should('exist')
cy.queryAllByText('Button Text').should('exist')
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure I have a good reason for changing these. It is part of the questions of the pull request. I think *By and *AllBy make sense for @test-library/dom, @testing-library/react, and company, but I'm not sure it does for Cypress.

Personally, I recommend always using the *ByAll since that's what native Cypress commands do now, but I'm welcoming other opinions

cy.queryAllByText('Non-existing Button Text').should('not.exist')
cy.queryAllByLabelText('Label text', {timeout: 7000}).should('exist')
cy.findAllByText('Jackie Chan').click({multiple: true})

// findAllByText _inside_ a form element
cy.get('form').within(() => {
cy.findByText('Button Text').should('exist')
cy.findAllByText('Button Text').should('exist')
})
cy.get('form').then(subject => {
cy.findByText('Button Text', {container: subject}).should('exist')
cy.findAllByText('Button Text', {container: subject}).should('exist')
})
cy.get('form').findAllByText('Button Text').should('exist')
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I felt this example was missing. It was one of the reasons I overlooked this library and the I found out #100 was merged just recently. This is more idiomatic for Cypress.

```

### Differences DOM Testing Library

`Cypress Testing Library` supports both jQuery elements and DOM nodes. This is
necessary because Cypress uses jQuery elements, while `DOM Testing Library`
expects DOM nodes. When you pass a jQuery element as `container`, it will get
the first DOM node from the collection and use that as the `container` parameter
for the `DOM Testing Library` functions.

`get*` queries are disabled. `find*` queries do not use the Promise API of
NicholasBoll marked this conversation as resolved.
Show resolved Hide resolved
`DOM Testing Library`, but instead forward to the `get*` queries and use Cypress'
built-in retryability using error messages from `get*` APIs to forward as error
messages if a query fails. `query*` also uses `get*` APIs, but disables retryability.

`findBy*` is less useful in Cypress compared to `findAllBy*`. If you intend to limit
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Question:

Does the difference between *By* and *AllBy* make sense in the context of Cypress? Cypress DOM APIs return a jQuery NodeList with one or more elements. Most of the time this is completely fine and expected, but certain commands expect only one element. For example, .click will throw an error if handed multiple elements. .first() can easily be used to return only the first one or the previous query can be updated to return only one. If a user expects a certain number (including only one), the command can be followed by .should('have.length', 1) if the test is meant to ensure only a single element exists in the DOM and fail otherwise - so single vs multiple is opt-in for other elements.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure why I would use findAllBy* when I expect there to only be one in the first place. Is the appeal just that you can use findAllBy* 100% of the time and not worry about how many elements are returned?

I like that findBy* communicates (through code) the intent of the test writer (that there should only be one) without having to make an explicit assertion for that.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the question is around idiomatic Cypress code vs idiomatic @testing-library/* code.

All Cypress commands that return elements (actually jQuery-wrapped NodeLists) return 1 or more elements or fail. The only exception to this is if the command is followed by as .should('not.exist'). This is because every command that uses cy.verifyUpcomingAssertions implicitly adds .should('exist') if no assertions are found. jQuery doesn't fail if it can't find an element by a selector, it just returns an empty node list. Cypress ensures there is at least one element. This combined with the implicit retry mechanism ensures you just tell Cypress what you want and it will retry in the background regardless of application timing. This is what makes Cypress so nice to work with.

Most of the time this is exactly what you expect to happen. It avoids a larger API footprint of ensuring only 1 element. Most of the time there is no issue.

For example:

cy.contains('button', 'Submit').click()

cy.contains can return more than 1 element. By default, cy.click needs one element and will throw an error saying it expected only one element. If you get an error, you can choose to narrow down the selector. I think Cypress did this so that developers wouldn't be forced to choose between wanting one selector and wanting many - also developers don't have to choose to always do single queries vs many queries and switch when there is a specific case. The Cypress API and errors guide the user into a single API with optional narrowing only when needed.

I'm perfectly happy to remove any documentation suggesting single vs many queries. I just wanted to pose a question - should the API reflect something Cypress users would most likely expect or something non-Cypress users would most expect?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The thing I personally find most useful about this library is helpers for common things I need to do. Like selecting an input element by its label.

Perhaps other people value the API to be more strictly adhering to the @testing-library/dom API. We've already determined it doesn't make sense in the Cypress context to do that exactly (the get* queries are already disabled for example)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cypress does have a mechanism for asserting there should be only one element if necessary:

cy.get('some-selector').should('have.length', 1)

Sometimes the intent is to make sure there is only one

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I'll make the language more positive.

But I do think we should keep findBy* around for the foreseeable future

Do you mean the current functionality that findBy* throws an error if more than one 1 element is found?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mean the current functionality that findBy* throws an error if more than one 1 element is found?

Yes

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the question is around idiomatic Cypress code vs idiomatic @testing-library/* code.

IMHO idiomatic @testing-library/* code not only provides useful common helpers (generally for selecting elements) but also encourages best practices. So, it may deviate from idiomatic code in the library / framework it augments because that library doesn't support/encourage best practice.

Consider the example:

  • cy.findAllBy*(...).first().click() (More common to Cypress)
  • cy.findBy*(...).click() (testing library recommended).

I would always prefer use the second version, because when my code changes so that selector matches a second element, I expect a test failure on selecting an element, not clicking the incorrect element.

It's possible to avoid this case using:
cy.findAllBy*('some-selector').should('have.length', 1)
But doing that after every findAllBy* is tedious, and likely to eventually be forgotten / missed - so in this case I think findBy* encourages better testing practices than findAllBy* and I don't think @testing-library/* should shy away from recommending different, better practices than Cypress or any other library it integrates with.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The update to the readme definitely documents this change 👍

I'm not sure I agree it should be the recommendation of @testing-library/* 👐

to only 1 element, the following will work:

```javascript
cy.findAllByText('Some Text').should('have.length', 1)
```

Cypress handles actions when there is only one element found. For example, the following
will work without having to limit to only 1 returned element. The `cy.click` will
automatically fail if more than 1 element is returned by the `findAllByText`:

```javascript
cy.findAllByText('Some Text').click()
```

## Other Solutions

I'm not aware of any, if you are please [make a pull request][prs] and add it
Expand Down
21 changes: 21 additions & 0 deletions cypress/fixtures/test-app/index.html
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,9 @@ <h2>*ByLabel and *ByPlaceholder</h2>

<label for="by-text-input-2">Label 2</label>
<input type="text" placeholder="Input 2" id="by-text-input-2" />

<p>Intentionally inaccessible label for error checking</p>
<label>Label 3</label>
</section>
<section>
<h2>*ByText</h2>
Expand Down Expand Up @@ -89,6 +92,24 @@ <h2>*AllByText</h2>
<h2>*ByText on another page</h2>
<a onclick='setTimeout(function() { window.location = "/cypress/fixtures/test-app/next-page.html"; }, 100);'>Next Page</a>
</section>
<section>
<h2>Eventual existence</h2>
<button id="eventually-will-exist"></button>
<script>
setTimeout(() => {
document.querySelector('#eventually-will-exist').innerHTML = 'Eventually Exists'
}, 500)
</script>
NicholasBoll marked this conversation as resolved.
Show resolved Hide resolved
</section>
<section>
<h2>Eventual non-existence</h2>
<button id="eventually-will-not-exist">Eventually not exists</button>
<script>
setTimeout(() => {
document.querySelector('#eventually-will-not-exist').remove()
}, 500)
</script>
</section>
<!-- Prettier unindents the script tag below -->
<script>
document
Expand Down
110 changes: 102 additions & 8 deletions cypress/integration/find.spec.js
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
/* eslint-disable max-lines-per-function */
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure this rule even makes sense in the context of a test file. Is the intent to limit how many tests there are in a file?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I don't use describe blocks much in my regular tests, but in Cypress it's kinda necessary. We can add an .eslintrc to the cypress/ directory and disable it in there. Should probably disable the jest promise one too:

{
  "rules": {
    "max-lines-per-function": "off",
    "jest/valid-expect-in-promise": "off"
  }
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That worked, thank you


describe('find* dom-testing-library commands', () => {
beforeEach(() => {
cy.visit('cypress/fixtures/test-app/')
Expand Down Expand Up @@ -86,22 +88,50 @@ describe('find* dom-testing-library commands', () => {

/* Test the behaviour around these queries */

it('findByText should handle non-existence', () => {
cy.findByText('Does Not Exist')
.should('not.exist')
})

it('findByText should handle eventual existence', () => {
cy.findByText('Eventually Exists')
.should('exist')
})

it('findByText should handle eventual non-existence', () => {
cy.findByText('Eventually Not exists')
.should('not.exist')
})

it("findByText with should('not.exist')", () => {
cy.findAllByText(/^Button Text \d$/).should('exist')
cy.findByText('Non-existing Button Text', {timeout: 100}).should(
'not.exist',
)
})

it('findByText with a previous subject', () => {
cy.get('#nested')
.findByText('Button Text 1', { fallbackToPreviousFunctionality: false })
.should('not.exist')
cy.get('#nested')
.findByText('Button Text 2')
.should('exist')
})

it('findByText within', () => {
cy.get('#nested').within(() => {
cy.findByText('Button Text 2').click()
cy.findByText('Button Text 1').should('not.exist')
cy.findByText('Button Text 2').should('exist')
})
})

it('findByText in container', () => {
return cy.get('#nested').then(subject => {
cy.findByText(/^Button Text/, {container: subject}).click()
// NOTE: Cypress' `then` doesn't actually return a promise
// eslint-disable-next-line jest/valid-expect-in-promise
cy.get('#nested').then(subject => {
cy.findByText('Button Text 1', {container: subject}).should('not.exist')
cy.findByText('Button Text 2', {container: subject}).should('exist')
})
})

Expand All @@ -110,23 +140,87 @@ describe('find* dom-testing-library commands', () => {
cy.findByText('New Page Loaded').should('exist')
})

it('findByText should set the Cypress element to the found element', () => {
// This test is a little strange since snapshots show what element
// is selected, but snapshots themselves don't give access to those
// elements. I had to make the implementation specific so that the `$el`
// is the `subject` when the log is added and the `$el` is the `value`
// when the log is changed. It would be better to extract the `$el` from
// each snapshot
Comment on lines +141 to +146
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍


cy.on('log:changed', (attrs, log) => {
if (log.get('name') === 'findByText') {
expect(log.get('$el')).to.have.text('Button Text 1')
}
})

cy.findByText('Button Text 1')
})

it('findByText should error if no elements are found', () => {
const regex = /Supercalifragilistic/
const errorMessage = `Timed out retrying: Expected to find element: 'findByText(${regex})', but never found it.`
const errorMessage = `Unable to find an element with the text: /Supercalifragilistic/`
cy.on('fail', err => {
expect(err.message).to.contain(errorMessage)
})

cy.findByText(regex, {timeout: 100}) // Every find query is implicitly a `.should('exist')
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should explicitly use .should('exist')

cy.verifyUpcomingAssertions has a default of existence if no other assertion is found. That's why it isn't explicitly needed. I like to err on the side of being more explicit, but I left it as is from previous code. I'm happy to change it if others agree

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with you.

In the jest world, I recommend people wrap get* queries with assertions of existence even though they throw:

expect(getByText(/success/i)).toBeInTheDocument()

})

it('findByText should default to Cypress non-existence error message', () => {
const errorMessage = `Expected <button> not to exist in the DOM, but it was continuously found.`
cy.on('fail', err => {
expect(err.message).to.contain(errorMessage)
})

cy.findByText('Button Text 1', {timeout: 100})
.should('not.exist')
})

it('findByLabelText should forward useful error messages from @testing-library/dom', () => {
const errorMessage = `Found a label with the text of: Label 3, however no form control was found associated to that label.`
cy.on('fail', err => {
expect(err.message).to.eq(errorMessage)
expect(err.message).to.contain(errorMessage)
NicholasBoll marked this conversation as resolved.
Show resolved Hide resolved
})

cy.findByText(regex, {timeout: 100}) // Doesn't explicitly need .should('exist') if it's the last element?
cy.findByLabelText('Label 3', {timeout: 100}).should('exist')
})

it('findByText finding multiple items should error', () => {
const errorMessage = `Found multiple elements with the text: /^Button Text/i\n\n(If this is intentional, then use the \`*AllBy*\` variant of the query (like \`queryAllByText\`, \`getAllByText\`, or \`findAllByText\`)).`
cy.on('fail', err => {
expect(err.message).to.eq(errorMessage)
expect(err.message).to.contain(errorMessage)
})

cy.findByText(/^Button Text/i)
cy.findByText(/^Button Text/i, {timeout: 100})
})

it('findByText should not break existing code', () => {
cy.window()
.findByText('Button Text 1')
.should('exist')
})
Comment on lines +195 to +199
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test verifies non-breaking changes for #110.


it('findByText should show as a parent command if it starts a chain', () => {
const assertLog = (attrs, log) => {
if(log.get('name') === 'findByText') {
expect(log.get('type')).to.equal('parent')
cy.off('log:added', assertLog)
}
}
cy.on('log:added', assertLog)
cy.findByText('Button Text 1')
})

it('findByText should show as a child command if it continues a chain', () => {
const assertLog = (attrs, log) => {
if(log.get('name') === 'findByText') {
expect(log.get('type')).to.equal('child')
cy.off('log:added', assertLog)
}
}
cy.on('log:added', assertLog)
cy.get('body').findByText('Button Text 1')
Comment on lines +201 to +220
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tests for
Screen Shot 2020-01-30 at 10 33 01 PM

})
})

Expand Down
57 changes: 48 additions & 9 deletions cypress/integration/query.spec.js
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
/* eslint-disable max-lines-per-function */

describe('query* dom-testing-library commands', () => {
beforeEach(() => {
cy.visit('cypress/fixtures/test-app/')
Expand Down Expand Up @@ -95,12 +97,30 @@ describe('query* dom-testing-library commands', () => {
})
})

it('queryByText should set the Cypress element to the found element', (done) => {
// This test is a little strange since snapshots show what element
// is selected, but snapshots themselves don't give access to those
// elements. I had to make the implementation specific so that the `$el`
// is the `subject` when the log is added and the `$el` is the `value`
// when the log is changed. It would be better to extract the `$el` from
// each snapshot

cy.on('log:changed', (attrs, log) => {
if (log.get('name') === 'queryByText') {
expect(log.get('$el')).to.have.text('Button Text 1')
done()
}
})

cy.queryByText('Button Text 1')
})

it('query* will return immediately, and never retry', () => {
cy.queryByText('Next Page').click()

const errorMessage = `expected 'queryByText(\`New Page Loaded\`)' to exist in the DOM`
const errorMessage = `Unable to find an element with the text: New Page Loaded.`
cy.on('fail', err => {
expect(err.message).to.eq(errorMessage)
expect(err.message).to.contain(errorMessage)
})

cy.queryByText('New Page Loaded', { timeout: 300 }).should('exist')
Expand Down Expand Up @@ -129,23 +149,42 @@ describe('query* dom-testing-library commands', () => {
.and('not.exist')
})

it('queryAllByText with a should(\'exist\') must provide selector error message', () => {
it('queryAllByText should forward existence error message from @testing-library/dom', () => {
const text = 'Supercalifragilistic'
const errorMessage = `expected 'queryAllByText(\`${text}\`)' to exist in the DOM`
const errorMessage = `Unable to find an element with the text: Supercalifragilistic.`
cy.on('fail', err => {
expect(err.message).to.contain(errorMessage)
})

cy.queryAllByText(text, {timeout: 100}).should('exist')
})

it('queryByLabelText should forward useful error messages from @testing-library/dom', () => {
const errorMessage = `Found a label with the text of: Label 3, however no form control was found associated to that label.`
cy.on('fail', err => {
expect(err.message).to.contain(errorMessage)
})

cy.queryByLabelText('Label 3', {timeout: 100}).should('exist')
})

it('queryAllByText should default to Cypress non-existence error message', () => {
const errorMessage = `Expected <button> not to exist in the DOM, but it was continuously found.`
cy.on('fail', err => {
expect(err.message).to.eq(errorMessage)
expect(err.message).to.contain(errorMessage)
})

cy.queryAllByText(text, {timeout: 100}).should('exist') // NOT POSSIBLE WITH QUERYALL?
cy.queryAllByText('Button Text 1', {timeout: 100})
.should('not.exist')
})

it('queryByText finding multiple items should error', () => {
const errorMessage = `Found multiple elements with the text: /^queryByText/i\n\n(If this is intentional, then use the \`*AllBy*\` variant of the query (like \`queryAllByText\`, \`getAllByText\`, or \`findAllByText\`)).`
const errorMessage = `Found multiple elements with the text: /^Button Text/i\n\n(If this is intentional, then use the \`*AllBy*\` variant of the query (like \`queryAllByText\`, \`getAllByText\`, or \`findAllByText\`)).`
cy.on('fail', err => {
expect(err.message).to.eq(errorMessage)
expect(err.message).to.contain(errorMessage)
})

cy.queryByText(/^queryByText/i)
cy.queryByText(/^Button Text/i)
})
})

Expand Down
2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@
"test:cypress:run": "cypress run",
"test:cypress:open": "cypress open",
"test:cypress": "npm run test:cypress:run",
"test:cypress:dev": "test:cypress:open",
"test:cypress:dev": "npm run test:cypress:open",
NicholasBoll marked this conversation as resolved.
Show resolved Hide resolved
"validate": "kcd-scripts validate build,lint,test",
"setup": "npm install && npm run validate -s"
},
Expand Down
1 change: 1 addition & 0 deletions src/__tests__/add-commands.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ test('adds commands to Cypress', () => {
commands.forEach(({name}, index) => {
expect(addMock.mock.calls[index]).toMatchObject([
name,
{},
// We get a new function that is `command.bind(null, cy)` i.e. global `cy` passed into the first argument.
// The commands themselves will be tested separately in the Cypress end-to-end tests.
expect.any(Function),
Expand Down
4 changes: 2 additions & 2 deletions src/add-commands.js
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import {commands} from './'

commands.forEach(({name, command}) => {
Cypress.Commands.add(name, command)
commands.forEach(({name, command, options = {}}) => {
Cypress.Commands.add(name, options, command)
})

/* global Cypress */
Loading