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

Support running commands against the previous yielded subject #100

Merged

Conversation

tlrobinson
Copy link
Member

What:

Changes the custom Cypress commands to respect the previously yielded subject, for example, with the following document:

    <button>Button Text 1</button>
    <div id="nested">
      <button>Button Text 2</button>
    </div>

this

cy.get('#nested').findByText('Button Text 1').should('not.exist')

would previously fail because findByText was not passed the result of cy.get('#nested') as its container (defaulting to document)

Why:

It's a convenient way to scope commands to the result of a previous command, and for users of builtin Cypress commands it's surprising that @testing-library/cypress doesn't currently behave like this.

How:

The prevSubject option is now passed to Cypress.Commands.add() (with a value of ['optional', 'document', 'element', 'window'] which matches builtin commands like contains. This causes the subject (if any) to be passed as the first argument to the command. The subject is given lower precedence than the testing-library's container option, but higher precedence than the document itself.

Checklist:

  • Documentation N/A
  • Tests
  • Ready to be merged

Copy link
Member

@kentcdodds kentcdodds left a comment

Choose a reason for hiding this comment

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

This is fantastic. Thank you so much!

Would this mean this also works: cy.findByRole('dialog').findByText('submit')?

That would be stellar. Thanks again!

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

Choose a reason for hiding this comment

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

We should probably disable the jest plugin for these tests. But we can do that in another PR :)


Copy link
Member

Choose a reason for hiding this comment

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

Sorry about all these. Don't know how those slipped through in the first place

Comment on lines +96 to +102
it('findByText with a previous subject', () => {
cy.get('#nested')
.findByText('Button Text 1')
.should('not.exist')
cy.get('#nested')
.findByText('Button Text 2')
.should('exist')
Copy link
Member

Choose a reason for hiding this comment

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

Love this

@kentcdodds kentcdodds merged commit e32c8e4 into testing-library:master Jan 28, 2020
@kentcdodds
Copy link
Member

@all-contributors please add @tlrobinson for code and tests

@allcontributors
Copy link
Contributor

@kentcdodds

I've put up a pull request to add @tlrobinson! 🎉

@kentcdodds
Copy link
Member

Thanks so much for your help! I've added you as a collaborator on the project. Please make sure that you review the other/MAINTAINING.md and CONTRIBUTING.md files (specifically the bit about the commit messages and the git hooks) and familiarize yourself with the code of conduct (we're using the contributor covenant). You might also want to watch the repo to be notified when someone files an issue/PR. Please continue to make PRs as you feel the need (you can make your branches directly on the repo rather than your fork if you want). Thanks! And welcome to the team :)

@kentcdodds
Copy link
Member

🎉 This PR is included in version 5.1.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@twgraham
Copy link

FYI this was a breaking change - all my tests that assumed findByText used the document broke after upgrading. It might be worth mentioning this in the releases, or bumping the major version if your doing semver.

@tlrobinson
Copy link
Member Author

tlrobinson commented Jan 30, 2020

@twgraham out of curiosity, what did your tests that broke look like? This change shouldn’t break queries like cy.findByText(...). It would break some chained queries like cy.get(...).findByText(...), but IMHO the old behavior was broken.

@twgraham
Copy link

@tlrobinson kind of like the latter... cy.get(...).click(...).findBy*(...).click. E.g. Get this form field and type, then this field and type, then submit, all chained together.

I suppose we were treating a bit like 'get' because we realised early on that the previous subject wasn't yielded, so we had to do something like cy.get(...).then(el => cy.findByText(..., { container: el }).should(...)) if we wanted to scope it.

I think this PR is definitely a great idea - just didn't want people to get bitten unnecessarily.

@kentcdodds
Copy link
Member

More discussion on this in #109. Please comment when you're able :)

@adamtay
Copy link

adamtay commented Jan 30, 2020

Great update and definitely prefer this style but as mentioned above, this has introduced a breaking change.

Example usage: cy.findByText(...).click().findByText('BREAK')

@kentcdodds
Copy link
Member

Thanks @adamtay,

We are aware of this. Please see issue #109 where we're discussing what to do about it.

kentcdodds added a commit that referenced this pull request Jan 30, 2020
NicholasBoll added a commit to NicholasBoll/cypress-testing-library that referenced this pull request Jan 31, 2020
kentcdodds pushed a commit that referenced this pull request 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 <[email protected]>

Closes #103, Closes #109, Closes #110
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 this pull request may close these issues.

4 participants