-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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: .contains() should only return one element at all times #25250
Conversation
Thanks for taking the time to open a PR!
|
Test summaryRun details
View run in Cypress Dashboard ➡️ Flakiness
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 |
@@ -1020,6 +1020,11 @@ describe('src/cy/commands/querying', () => { | |||
}) | |||
}) | |||
|
|||
// https://github.com/cypress-io/cypress/issues/25225 | |||
it('returns one element when given multiple subjects with no children', () => { | |||
cy.get('button').contains('submit').should('have.length', 1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like to see test coverage from returning one element when multiple subjects regardless if the subjects have children or not. I glanced through these tests and not seeing an equivalent. When debugging this I didn't expect the issue to related to the subject's dom tree. Would always expect the first matching element
This might not be the right get selector - but I expect both scenarios to pass.
cy.get('button').contains('submit').should('have.length', 1) | |
cy.get('button').contains('submit').should('have.length', 1) | |
cy.get('div').contains('submit').should('have.length', 1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll edit the test name to be a bit more descriptive - "has no children" isn't quite accurate. It's "when the subjects themselves match the selector, and none of their children do."
I think we already have lots of coverage around the case where .contains() simply matches multiple elements. Other tests in this file we have covering similar cases:
it('searches multiple subject elements', () => {
cy.get('ul').contains('li', 'asdf 3')
})
it('resets the subject between chain invocations', () => {
...snip...
cy.get('#complex-contains').contains('nested contains').then(($label) => {
it('reduces right by priority element', () => {
...snip...
// it should find label because label is the first priority element
// out of the collection of contains elements
cy.get('#complex-contains').contains('nested contains')
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's "when the subjects themselves match the selector, and none of their children do."
And do we have cases for these scenarios?
- the children match the selector and the subjects don't
- both the children and the subjects match the selector
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Its seems we've had a few holes when 12 went out with our test coverage, so Im surprised each of fixes has only had one test verifying its behavior. If we have existing tests for the above scenarios, mind linking them?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the children match the selector and the subjects don't
This is super common. For example,
<div>
<span>Foo</span>
</div>
cy.get('div').contains('span', 'Foo')
The child matches the selector, but the parent does not. Some examples from the file:
it('searches multiple subject elements'
it('finds the furthest descendent when filter matches more than 1 element'
it('retries until it finds a filtered contains has the matching text node'
both the children and the subjects match the selector
Every time a child matches, the parent also does.
<div>
<div>Foo</div>
</div>
cy.get('div').contains('Foo')
The div contains the text 'Foo', and so does the span. Basically every test in the file covers this case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are missing one in the form .get('div').contains('div', 'text')
. I'll add that in a moment here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Definitely a fan of more thorough tests - I can't believe we didn't have a test around this, I write code like this all the time.
I couldn't find any other broken use cases with contains
. I tested the fix and it works.
* develop: (45 commits) fix: re-enable CYPRESS_INTERNAL_VITE_DEV development (#25364) fix: add skip domain injection description (#25463) fix: revert CSP header and script-src addition (#25445) chore: Update v8 snapshot cache (#25401) feat: Do not strip CSP headers from HTTPResponse (#24760) fix: keep spaces in formatted output in test runner (#24687) fix: Restrict dependency versions to known supported ranges (#25380) chore: Update v8 snapshot cache (#25370) feat: experimental skip domain injection (#25307) chore: support vite v4 for component testing (#25365) feat: Use JSX/TSX in generated spec filenames (#25318) docs(angular): Properties that are spied upon have to be defined within `componentProperties` instead of on root level. (#25359) chore: remove lint-changed from scripts/docs (#25308) chore: bump to 12.3.0 [skip ci] (#25355) fix: make NODE_ENV "production" for prod builds of launchpad (#25320) fix: .contains() should only return one element at all times (#25250) feat: add currentRetry to Cypress API (#25297) chore: release @cypress/webpack-dev-server-v3.2.2 chore: release create-cypress-tests-v2.0.1 fix: change wording for spec creation (#25271) ...
User facing changelog
Fix for regression introduced in 12.1.0, where
.contains()
could return multiple elements when it was matching directly on the subject, rather than on the subject's children.Additional details
Basically, we were either filtering down one element or checking to see if the subject matched the selector - so if there were multiple subjects that matched the contains filter (but none of their child elements matched), we'd return them all.
Fixed by moving the filtering to be the last step.
Steps to test
In addition to the included driver test, you can reproduce the issue like so:
How has the user experience changed?
PR Tasks
cypress-documentation
?type definitions
?