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: .contains() should only return one element at all times #25250

Merged
merged 6 commits into from Jan 3, 2023

Conversation

BlueWinds
Copy link
Contributor

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:

    it('issue ', () => {
      cy.visit('https://example.cypress.io')
      cy.get('a').contains('next').click()
    })

How has the user experience changed?

PR Tasks

  • Have tests been added/updated?
  • Has the original issue (or this PR, if no issue exists) been tagged with a release in ZenHub? (user-facing changes only)
  • [n/a] Has a PR for user-facing changes been opened in cypress-documentation?
  • [n/a] Have API changes been updated in the type definitions?

@cypress-bot
Copy link
Contributor

cypress-bot bot commented Dec 21, 2022

Thanks for taking the time to open a PR!

@cypress
Copy link

cypress bot commented Dec 21, 2022



Test summary

26398 0 1179 0Flakiness 29


Run details

Project cypress
Status Passed
Commit 93c03f8
Started Jan 3, 2023 5:02 PM
Ended Jan 3, 2023 5:22 PM
Duration 19:58 💡
OS Linux Debian -
Browser Multiple

View run in Cypress Dashboard ➡️


Flakiness

create-from-component.cy.ts Flakiness
1 ... > runs generated spec
commands/net_stubbing.cy.ts Flakiness
1 network stubbing > intercepting request > can delay and throttle a StaticResponse
2 network stubbing > intercepting request > can delay and throttle a StaticResponse
3 network stubbing > intercepting request > can delay and throttle a StaticResponse
4 ... > with `times` > only uses each handler N times
This comment includes only the first 5 flaky tests. See all 29 flaky tests in the 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

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

@lmiller1990 lmiller1990 self-requested a review December 22, 2022 04:20
Copy link
Contributor

@lmiller1990 lmiller1990 left a 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.

@BlueWinds BlueWinds merged commit 05b9f10 into develop Jan 3, 2023
@BlueWinds BlueWinds deleted the issue-25225-contains-one-element branch January 3, 2023 18:42
tgriesser added a commit that referenced this pull request Jan 18, 2023
* 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)
  ...
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.

Problem with cy.contains(). It find two elements. It should find one
3 participants