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(rule): add 'require-data-selectors' rule #30

Merged
merged 14 commits into from Sep 26, 2019

Conversation

sandeepbaldawa
Copy link
Contributor

@sandeepbaldawa sandeepbaldawa commented Sep 16, 2019

Goal :- Add best practices rule related to https://docs.cypress.io/guides/references/best-practices.html#Selecting-Elements

Testing:- Added few unit tests

npm run test require-data-selectors

> eslint-plugin-cypress@0.0.0-development test /Users/sb/contri_sandeep/eslint-plugin-cypress
> jest "require-data-selectors"

 PASS  tests/lib/rules/require-data-selectors.js
  no-dynamic-id-classes
    valid
      ✓ cy.get('[data-cy=submit]').click() (40ms)
      ✓ cy.get('[data-QA=submit]') (2ms)
      ✓ cy.clock(5000) (1ms)
      ✓ cy.scrollTo(0, 10) (1ms)
      ✓ cy.tick(500) (1ms)
    invalid
      ✓ cy.get('[daedta-cy=submit]').click() (3ms)
      ✓ cy.get('[d-cy=submit]') (1ms)
      ✓ cy.get(".btn-large").click() (2ms)
      ✓ cy.get(".btn-.large").click() (1ms)
      ✓ cy.get(".a") (1ms)

kuceb
kuceb previously requested changes Sep 17, 2019
Copy link
Contributor

@kuceb kuceb left a comment

Choose a reason for hiding this comment

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

I think this would be better named no-brittle-selectors, since that is its purpose.

I like how it is not part of the recommended rules, since some projects do have valid use cases for them; however it could be enabled in the default if it's set to warn error level.

docs: {
description: 'Use data-* attributes to provide context to your selectors and insulate them from CSS or JS changes https://docs.cypress.io/guides/references/best-practices.html#Selecting-Elements',
category: 'Possible Errors',
recommended: true,
Copy link
Contributor

Choose a reason for hiding this comment

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

I know this doesn't do anything, but should be false here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, changed

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.

@bkucera What about calling it something like require-data-selectors, since that's what it basically doing?

lib/rules/no-dynamic-id-classes.js Outdated Show resolved Hide resolved
@sandeepbaldawa
Copy link
Contributor Author

Ready for a re-review 🙇

@sandeepbaldawa sandeepbaldawa changed the title feat(no_dynamic_id_classes): add 'no_dynamic_id_classes' rule feat(require_data_selectors): add 'require-data-selectors' rule Sep 18, 2019
@bahmutov bahmutov dismissed kuceb’s stale review September 25, 2019 22:49

the author has addressed the comments

@bahmutov
Copy link
Contributor

@bkucera could you review this PR again - the author has addressed your comments I think

@kuceb kuceb changed the title feat(require_data_selectors): add 'require-data-selectors' rule feat(rule): add 'require-data-selectors' rule Sep 26, 2019
@kuceb kuceb merged commit a4a0e8e into cypress-io:master Sep 26, 2019
@chrisbreiding
Copy link
Contributor

🎉 This PR is included in version 2.7.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@Svish
Copy link

Svish commented Oct 3, 2019

This new rule should probably be added to the table in the README as well? 🙂
https://github.com/cypress-io/eslint-plugin-cypress#rules

@kuceb
Copy link
Contributor

kuceb commented Oct 4, 2019

@Svish good catch, PRs welcome

@LeoWW
Copy link

LeoWW commented Apr 17, 2020

Hi -- there's no way I know of getting this to work with a helper function:

e.g.

export const getTestId = (id: string) => `[data-testid="${id}"]`;

getTestId('headernav')

will there be functionality to support this?

thanks!

jaffrepaul pushed a commit that referenced this pull request Sep 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants