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: wrap playground selectors in double quotes if not included #18442

Merged
merged 17 commits into from
Nov 18, 2021
Merged

fix: wrap playground selectors in double quotes if not included #18442

merged 17 commits into from
Nov 18, 2021

Conversation

Manuel-Suarez-Abascal
Copy link
Contributor

@Manuel-Suarez-Abascal Manuel-Suarez-Abascal commented Oct 11, 2021

User facing changelog

When using the Selector Playground, we'll wrap the "HTML attribute's value" in double-quotes if they were not present.

Additional details

Why was this change necessary?

When using selector playground to select elements with preferred data attributes (data-cy, data-test, data-testid), the provided selector is missing quotes around the value

What is affected by this change?

Only the selectors with data attributes using the Selector Playground were affected by this change.

Any implementation details to explain?

  • We check the quotes are included with the selector when using the Playground Selector. If they're included no manipulation is done. If they're not then we wrap the selector within double-quotes.

How has the user experience changed?

playground-selector-wrapped-with-double-quotes.mp4

All unit tests passing:

Screen Shot 2021-10-11 at 3 40 30 PM

PR Tasks

  • Have tests been added/updated?

Should I add some tests for this fix? Can you provide some guidance on where to add them?

@cypress-bot
Copy link
Contributor

cypress-bot bot commented Oct 11, 2021

Thanks for taking the time to open a PR!

@Manuel-Suarez-Abascal Manuel-Suarez-Abascal marked this pull request as ready for review October 11, 2021 19:42
@Manuel-Suarez-Abascal Manuel-Suarez-Abascal requested a review from a team as a code owner October 11, 2021 19:42
@Manuel-Suarez-Abascal Manuel-Suarez-Abascal requested review from flotwig and BlueWinds and removed request for a team October 11, 2021 19:43
@jennifer-shehane jennifer-shehane requested review from chrisbreiding and removed request for flotwig October 11, 2021 19:48
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.

This logic should be implemented in @cypress/unique-selector instead of changing the selector after it's been determined.

Can you create a PR updating the logic in that repo? Then we'll release a new version of @cypress/unique-selector and this PR will just bump its version.

@Manuel-Suarez-Abascal
Copy link
Contributor Author

This logic should be implemented in @cypress/unique-selector instead of changing the selector after it's been determined.

Can you create a PR updating the logic in that repo? Then we'll release a new version of @cypress/unique-selector and this PR will just bump its version.

Roger! A quick question though, can you provide me the steps on how to test my fix in @cypress/unique-selector in this branch without merging it? I'm not sure how to test it if I need to bump the version once it's merged.

@chrisbreiding
Copy link
Contributor

@cypress/unique-selector has unit tests. It looks like a couple of them will need to be updated with this change in any case since the behavior is changing. You can run them with npm run test.

If you'd like to test it out in the context of Cypress, you can use yarn link. Run yarn link in @cypress/unique-selector and then run yarn link @cypress/unique-selector while in packages/driver in this repo.

@Manuel-Suarez-Abascal
Copy link
Contributor Author

Manuel-Suarez-Abascal commented Oct 15, 2021

@cypress/unique-selector has unit tests. It looks like a couple of them will need to be updated with this change in any case since the behavior is changing. You can run them with npm run test.

@chrisbreiding I tinkered with it for an hour or so... I couldn't manage to run the unit tests on the repository. It seems this package hasn't been actively maintained for a long time. The dependency packages are massively outdated, refer to the screenshot below:

outdate-packages

I tried updating them & tried to run the unit tests, but no luck. I might try later on if I have time. In the meantime, if you have any advice let me know 😄

@flotwig flotwig self-requested a review October 15, 2021 15:37
@Manuel-Suarez-Abascal Manuel-Suarez-Abascal changed the title issue-1884: wrap playground selectors in double quotes if they're not… fix: wrap playground selectors in double quotes if not included Oct 16, 2021
@chrisbreiding
Copy link
Contributor

@Manuel-Suarez-Abascal Sorry about the difficulty with that repo. I updated all the dependencies and got the tests working, so it should be good to go once you update with the latest in the master branch.

@Manuel-Suarez-Abascal
Copy link
Contributor Author

@Manuel-Suarez-Abascal Sorry about the difficulty with that repo. I updated all the dependencies and got the tests working, so it should be good to go once you update with the latest in the master branch.

@chrisbreiding thanks for the help. I have added a PR here. Unfortunately, I haven't been able to test on the Cypress Playground Selector. I'm not quite sure if it's due to my changes are not being correct (although it seems to work on the other repo) or I'm not linking properly.

Could you take a look at it? In case, it's working... I'll wait for my PR to be merged in the other repo before reverting the changes here & updating the library @cypress/unique-selector to the newest version instead.

chrisbreiding
chrisbreiding previously approved these changes Nov 17, 2021
emilyrohrbough
emilyrohrbough previously approved these changes Nov 17, 2021
@BlueWinds BlueWinds removed their request for review November 17, 2021 16:55
@flotwig flotwig removed their request for review November 17, 2021 19:22
@chrisbreiding chrisbreiding dismissed stale reviews from emilyrohrbough and themself via bf90e2f November 18, 2021 20:32
@chrisbreiding chrisbreiding merged commit e9accb4 into cypress-io:develop Nov 18, 2021
@Manuel-Suarez-Abascal Manuel-Suarez-Abascal deleted the issue-1884 branch November 18, 2021 21:30
tgriesser added a commit that referenced this pull request Nov 20, 2021
* develop: (52 commits)
  feat: use hoisted yarn install in binary build (#17285)
  fix: compile npm packages for node 12 (#18989)
  fix: show call count even if `cy.stub().log(false)`. (#18907)
  chore: Update TypeScript to 4.4.4 (#18930)
  fix: wrap playground selectors in double quotes if not included (#18442)
  fix: flaky settings_spec test (#18979)
  chore: Update Chrome (stable) to 96.0.4664.45 (#18931)
  fix: Loading of specs with % in the filename (#18877)
  chore: refactor `create` into class `$Cy` (#18715)
  chore: Update Chrome (beta) to 96.0.4664.45 (#18891)
  fix: flaky `system-tests-firefox` job (#18848)
  chore: release @cypress/webpack-preprocessor-v5.10.0
  chore: release @cypress/vue-v3.0.5
  chore: release @cypress/schematic-v1.6.0
  chore: release create-cypress-tests-v1.2.0
  release 9.0.0
  feat: ensure major release
  have conduit app wait on localhost:3000
  fix install-required-node
  use --legacy-peer-deps
  ...
tgriesser added a commit that referenced this pull request Nov 21, 2021
* 10.0-release: (56 commits)
  chore: post-merge cleanup
  feat: use hoisted yarn install in binary build (#17285)
  fix: fix spec list header, "Create specs" prompt, add workspace recommended apollo extension (#18993)
  feat(unify): reporter settings (#18946)
  feat: add devServer to config file (#18962)
  fix: compile npm packages for node 12 (#18989)
  fix: show call count even if `cy.stub().log(false)`. (#18907)
  chore: Update TypeScript to 4.4.4 (#18930)
  fix: wrap playground selectors in double quotes if not included (#18442)
  fix: flaky settings_spec test (#18979)
  chore: Update Chrome (stable) to 96.0.4664.45 (#18931)
  fix: Loading of specs with % in the filename (#18877)
  chore: refactor `create` into class `$Cy` (#18715)
  chore: Update Chrome (beta) to 96.0.4664.45 (#18891)
  fix: flaky `system-tests-firefox` job (#18848)
  chore: release @cypress/webpack-preprocessor-v5.10.0
  chore: release @cypress/vue-v3.0.5
  chore: release @cypress/schematic-v1.6.0
  chore: release create-cypress-tests-v1.2.0
  release 9.0.0
  ...
tgriesser added a commit that referenced this pull request Nov 21, 2021
…e-data-clean-refactor

* tgriesser/chore/e2e-data-clean: (76 commits)
  chore: post-merge cleanup
  feat: use hoisted yarn install in binary build (#17285)
  fix: fix spec list header, "Create specs" prompt, add workspace recommended apollo extension (#18993)
  feat(unify): reporter settings (#18946)
  feat: add devServer to config file (#18962)
  fix: compile npm packages for node 12 (#18989)
  fix: show call count even if `cy.stub().log(false)`. (#18907)
  chore: Update TypeScript to 4.4.4 (#18930)
  feat: use fuzzy search (#18966)
  fix: onUnmounted warning in topnav (#18988)
  fix: wrap playground selectors in double quotes if not included (#18442)
  fix: flaky settings_spec test (#18979)
  fix: CYPRESS_INTERNAL_VITE_DEV for development
  feat: Create default config file (#18943)
  feat(app): support editor preference (#18932)
  chore: Update Chrome (stable) to 96.0.4664.45 (#18931)
  fix: Loading of specs with % in the filename (#18877)
  feat: improve vite DX (#18937)
  chore: refactor `create` into class `$Cy` (#18715)
  feat: Use plugins on config files (#18798)
  ...
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.

Selector playground omits quotes for preferred data- attribute selectors
3 participants