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

fix: .within() now throws an error if given more than one subject. #24975

Merged
merged 7 commits into from Dec 5, 2022

Conversation

BlueWinds
Copy link
Contributor

@BlueWinds BlueWinds commented Dec 5, 2022

User facing changelog

BREAKING: .within() now throws an error if given more than one subject.

This brings the behavior in line with the documentation, and adds consistency around how .within() behaves across commands. Some commands inside a within block would silently select the first element, while others would use all of them, and still others would throw an error.

This ambiguity is now resolved - all commands use the first subject element, because only one element is allowed.

Additional details

Steps to test

See the new test added in this PR for an example of the change in action.

How has the user experience changed?

A visual example of the error thrown.

image

PR Tasks

@cypress-bot
Copy link
Contributor

cypress-bot bot commented Dec 5, 2022

Thanks for taking the time to open a PR!

@@ -106,6 +106,7 @@ describe('runner/cypress sessions.ui.spec', {
validateSessionsInstrumentPanel(['blank_session'])

cy.get('.command-name-session')
.first()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are a couple of examples where our tests used to work, but required a change because of this PR. For example, this test had multiple commands - we now are explicitly (rather than implicitly) asserting on the first of them.

cy.containsPath('cypress/support/commands.js')
cy.containsPath('cypress/fixtures/example.json')
})
cy.get('[data-cy=valid]').as('valid').contains('cypress.config.js')
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here's an example of where we were genuinely relying on multiple subjects from .within(), and it required something more than just .first() to fix the test.

@cypress
Copy link

cypress bot commented Dec 5, 2022



Test summary

199 0 26 0Flakiness 0


Run details

Project cypress
Status Passed
Commit 11b2684
Started Dec 5, 2022 9:29 PM
Ended Dec 5, 2022 9:39 PM
Duration 09:55 💡
OS Linux Debian -
Browser Chrome 106

View run in Cypress Dashboard ➡️


This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Dashboard

Co-authored-by: Emily Rohrbough <emilyrohrbough@users.noreply.github.com>
Copy link
Contributor

@marktnoonan marktnoonan left a comment

Choose a reason for hiding this comment

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

Works well, I get the error when there's more than 1 subject.

Copy link
Contributor

@chrisbreiding chrisbreiding left a comment

Choose a reason for hiding this comment

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

Looks good! I like the error message.

Copy link
Contributor

@mschile mschile left a comment

Choose a reason for hiding this comment

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

Should we have a test that explicitly tests our recommendation of use .each?

@BlueWinds
Copy link
Contributor Author

Should we have a test that explicitly tests our recommendation of use .each?

I don't see the value. Each command is tested individually - we know they all work. 🤷‍♀️

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

Successfully merging this pull request may close these issues.

None yet

6 participants