-
Notifications
You must be signed in to change notification settings - Fork 153
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 #110
Comments
For me personally, I like to think of the commands I give to Cypress to be instructions I would give to a user, so chaining like this makes a lot of sense to me: describe('anonymous calculator', () => {
it('can make calculations', () => {
cy.visit('/') // visit this site
.findByText(/^1$/) // then find this element
.click() // then click it
.findByText(/^\+$/) // then find this element
.click() // then click it
.findByText(/^2$/) // then find this element
.click() // then click it
.findByText(/^=$/) // then find this element
.click() // then click it
.findByTestId('total') // then find this element
.should('have.text', '3') // then assert it has this text
})
}) I personally don't think that these queries make sense to scope each other (we already have that with the Thoughts? |
I'm strongly in favor of this, despite it technically being a breaking change. I would prefer to match semantics of existing Cypress commands over supporting non-idiomatic style of writing tests. IMHO the above is not idiomatic in Cypress, and isn't nearly as readable as: cy.visit('/')
cy.findByText(/^1$/).click()
cy.findByText(/^\+$/).click()
cy.findByText(/^2$/).click()
cy.findByText(/^=$/).click()
cy.findByTestId('total').should('have.text', '3') Cypress already does the work of queuing up commands that aren't chained, so chaining unrelated commands isn't necessary. Cypress's builtin commands (e.x. Cypress has
|
Hi @tlrobinson, Somehow I've managed to write non-idiomatic cypress for years and nobody told me 🙃 I just checked their kitchen sink example and boom: it('.check() - check a checkbox or radio element', () => {
// https://on.cypress.io/check
// By default, .check() will check all
// matching checkbox or radio elements in succession, one after another
cy.get('.action-checkboxes [type="checkbox"]').not('[disabled]')
.check().should('be.checked')
cy.get('.action-radios [type="radio"]').not('[disabled]')
.check().should('be.checked')
// .check() accepts a value argument
cy.get('.action-radios [type="radio"]')
.check('radio1').should('be.checked')
// .check() accepts an array of values
cy.get('.action-multiple-checkboxes [type="checkbox"]')
.check(['checkbox1', 'checkbox2']).should('be.checked')
// Ignore error checking prior to checking
cy.get('.action-checkboxes [disabled]')
.check({ force: true }).should('be.checked')
cy.get('.action-radios [type="radio"]')
.check('radio3', { force: true }).should('be.checked')
}) Yeah, looks like when you want to change the subject of the chain, in this example at least, you should create a new This is unfortunate for me because I've been doing it differently so long and I've taught thousands of people to do it the way I do. But I think you're right and what you have described is probably better (it just means I have to re-record a bunch of cypress videos on testingjavascript.com 🤦♂️) Ok, so for this, we're doing some breaking changes in the Testing Library realm in the next few weeks for other projects, so I think we could coordinate this and some other changes at the same time. I'd love to hear other people's thoughts on this. |
@NicholasBoll, just commented in #109
That would be fantastic. Whatever we do, having an easier migration path to the new recommendation would be wonderful. |
@tlrobinson is right, starting new chains is more idiomatic. I've taught people to think of a chain as a sentence with a complete subject. If I have to start with a new idea, I start a new sentence. I think it is easier to understand when I think of a chain as a complete thought.
There are I try to avoid I think it is worthwhile to introduce a breaking change, but we'd have to communicate that. Chaining off previous commands and inheriting the previous element is how all other traversal commands work in Cypress. This will delay #108, but that's okay. |
Oh. I know why this is happening.
|
Thanks for your work here Nicholas! I wonder if we could make a codemod to update people's code automatically for them 🤔 |
My thought was to try to run the query scoped to the previous subject. If that fails, try running again unscoped with a warning message. |
@kentcdodds The idea seems to work! It will first try scoping to the previous subject given by Cypress. If an error is encountered, it will record the error and then try with functionality previous to #100. |
That's fantastic! That way we can upgrade without the breaking change and update all the documentation to recommend a more idiomatic approach, and people can migrate over time. Then maybe eventually we can remove the old behavior. |
I've updated #108 with the code. The code in #108 was primed for this type of change, so I tacked it on. The PR is kind of large. I could spend time breaking it down if we prioritize the features listed there. The code changes are interdependent so if I break it up, they would have to be done in a sequence. |
Amazing. Thank you @NicholasBoll! In a perfect world we'd probably just do the breaking change and move forward, but a lot of people rely on the current behavior so supporting them is important 👍 I'm fine putting it all in one PR. Normally like to have things split up a bit, but I'm not too concerned 🤷♂️ |
🎉 This issue has been resolved in version 5.2.0 🎉 The release is available on:
Your semantic-release bot 📦🚀 |
I'm converting #100 by @tlrobinson to an issue. cc @twgraham, @intelligentspark, and @NicholasBoll.
Here's the original PR's comment (to save you a click)
Merging #100 caused a breaking change (#109). I've reverted it (0b73b69) and now we need to discuss whether we want to make this change and whether it's possible to solve the originally stated problem in the PR and have the testing style mentioned in #109.
The text was updated successfully, but these errors were encountered: