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

feat(driver): add select by index #18201

Merged
merged 14 commits into from
Sep 24, 2021
Merged

Conversation

davidmunechika
Copy link
Contributor

@davidmunechika davidmunechika commented Sep 22, 2021

Closes #757

User facing changelog

Users now have the ability to select an option by index within the .select() command.

Additional details

These are now the updated accepted arguments for the .select() command:

.select(value)
.select(values)
.select(value, options)
.select(values, options)

value can be the value, index, or text content of the option to be selected
values is an array of values, indexes, or text contents of the options to be selected
Updated documentation: cypress-io/cypress-documentation#4105

Tests

Additional tests added include:

  • Ability to select by index
  • Can select an array with multiple indexes
  • Can select an array with same value and index
  • Throws error if given negative index
  • Throws error if given non-integer index

PR Tasks

@cypress-bot
Copy link
Contributor

cypress-bot bot commented Sep 22, 2021

Thanks for taking the time to open a PR!

@CLAassistant
Copy link

CLAassistant commented Sep 22, 2021

CLA assistant check
All committers have signed the CLA.

@cypress
Copy link

cypress bot commented Sep 22, 2021



Test summary

18502 0 214 7Flakiness 1


Run details

Project cypress
Status Passed
Commit 745c46c
Started Sep 24, 2021 7:57 PM
Ended Sep 24, 2021 8:07 PM
Duration 10:31 💡
OS Linux Debian - 10.9
Browser Multiple

View run in Cypress Dashboard ➡️


Flakiness

cypress/integration/cypress/proxy-logging-spec.ts Flakiness
1 ... > works with forceNetworkError

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

@davidmunechika davidmunechika marked this pull request as ready for review September 23, 2021 12:41
@davidmunechika davidmunechika requested a review from a team as a code owner September 23, 2021 12:41
Copy link
Member

@jennifer-shehane jennifer-shehane left a comment

Choose a reason for hiding this comment

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

The code looks great and well tested. Nice first PR. 🎉 There's one edge case not considered and just another note I ran across with select in general. I left a couple notes inline in the code as well.

  1. Users passing in an option of an index that doesn't exist in the select needs to be considered.
<select>
  <option>zero</option>
<select>
cy.select(3)

The error message here is confusing as it says it's looking for text/value of 3 and can't find it.

Screen Shot 2021-09-23 at 1 03 03 PM

  1. This is actually not within the scope of work outlined (this is an existing issue before this PR), so perhaps you could make a new issue for this, but if you call cy.select() with no value then there's a weird error. It should say something along the lines of - cy.select wasn't called with any value, text, index, etc etc.

Screen Shot 2021-09-23 at 1 05 52 PM

packages/driver/src/cy/commands/actions/select.ts Outdated Show resolved Hide resolved
packages/driver/src/cy/commands/actions/select.ts Outdated Show resolved Hide resolved
@jennifer-shehane
Copy link
Member

I'm not sure why Percy is failing here. Should be able to be ignored though since this definitely doesn't touch the UI that Percy is checking.

davidmunechika and others added 2 commits September 24, 2021 15:37
Co-authored-by: Jennifer Shehane <jennifer@cypress.io>
Copy link
Member

@jennifer-shehane jennifer-shehane left a comment

Choose a reason for hiding this comment

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

Great! 💯

@davidmunechika davidmunechika merged commit a5356b8 into develop Sep 24, 2021
@davidmunechika davidmunechika deleted the issue-757-select-by-index branch September 24, 2021 20:13
flotwig added a commit that referenced this pull request Sep 30, 2021
Co-authored-by: Jennifer Shehane <jennifer@cypress.io>
Co-authored-by: Zach Bloomquist <github@chary.us>
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.

Ability to select by index within .select() command
4 participants