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.

Sign up for GitHub

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

Merged
merged 6 commits into from
Jan 3, 2023
Merged

fix: .contains() should only return one element at all times #25250

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
9b2b260
fix: .contains() should only return one element at all times
BlueWinds Dec 21, 2022
30f8e2b
Merge branch 'develop' into issue-25225-contains-one-element
Dec 21, 2022
2d1f87f
Merge remote-tracking branch 'origin/develop' into issue-25225-contai…
BlueWinds Dec 27, 2022
d9976f5
Improve test name
BlueWinds Dec 27, 2022
37fb0db
Add additional test
BlueWinds Jan 3, 2023
93c03f8
Merge remote-tracking branch 'origin/develop' into issue-25225-contai…
BlueWinds Jan 3, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions packages/driver/cypress/e2e/commands/querying/querying.cy.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1020,6 +1020,15 @@ describe('src/cy/commands/querying', () => {
})
})

// https://github.com/cypress-io/cypress/issues/25225
it('returns only one element when given multiple subjects directly match selector', () => {
// A case with only a text selector
cy.get('button').contains('submit').should('have.length', 1)
Copy link
Member

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.

Suggested change
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)

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'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')

Copy link
Member

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

Copy link
Member

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?

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 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.

Copy link
Contributor Author

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.


// A case with a filter + text selector
cy.get('div').contains('div', 'foo').should('have.length', 1)
})

// https://github.com/cypress-io/cypress/issues/25019
it('can locate elements contained inside <form> containers', () => {
cy.get('#focus').contains('button', 'focusable')
Expand Down
6 changes: 3 additions & 3 deletions packages/driver/src/cy/commands/querying/querying.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -327,14 +327,14 @@ export default (Commands, Cypress, cy, state) => {
$el = $el.add(getFn())
})

if ($el.length) {
$el = $dom.getFirstDeepestElement($el)
} else {
if (!$el.length) {
// .get() looks for elements *inside* the current subject, while contains() wants to also match the current
// subject itself if no child matches.
$el = (subject as JQuery).filter(selector)
}

$el = $dom.getFirstDeepestElement($el)

log && cy.state('current') === this && log.set({
$el,
consoleProps: () => {
Expand Down