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: add prefer-user-facing-locators rule #160

Conversation

Haberkamp
Copy link
Contributor

@Haberkamp Haberkamp commented Aug 11, 2023

This merge request adds a rule that prefers the usage of user-facing locators over page.locator(). Using user-facing selectors makes the tests more robust.

Resolves: #123

@mskelton
Copy link
Member

Not really a fan of the name here, what about

  • prefer-testing-library-methods
  • disallow-raw-locators
  • ...

Honestly not sure I like those options either, hmm.

@Haberkamp
Copy link
Contributor Author

@mskelton What about disallow-page-locator-selector? As this is essentially what the rule is doing. It checks for the usage of page.locator() every other method is allowed.

@grant
Copy link

grant commented Aug 15, 2023

Would definitely use this rule! the prefer- prefix makes sense to me.

@mskelton
Copy link
Member

Problem with disallow-page-locator-selector is that there is also frame.locator, locator.locator, etc.

I think I would like to proceed with disallow-raw-locators, it makes the most sense of all the options given.

@Haberkamp
Copy link
Contributor Author

@mskelton That's a good point you're making. I would then change the name of the rule to your suggested disallow-raw-locators. And to be sure that I'm understanding you correctly. Should I also modify the rule in a way that will disallow the usage of frame.locator and locator.locator?

@mskelton
Copy link
Member

Yes, the rule should detect all usages of .locator, not just page.locator.

@Haberkamp
Copy link
Contributor Author

Talking about the naming again. I think it would make sense to align it with other rules like no-page-pause or no-useless-await. So the rule would be named no-raw-selector.

Another thing: should this rule be included in the recommended config and with which level of severity should this rule report?

@mskelton
Copy link
Member

Yeah, good call, no-raw-selector is better.

This should definitely NOT be in the recommended config. While the other methods are better when possible to use, there are still plenty of valid use cases for .locator() and I do not want that warning users all the time unnecessarily.

@Haberkamp Haberkamp force-pushed the add-prefer-user-facing-locators-rule branch from f9bb9d3 to 7b9ec83 Compare August 25, 2023 09:04
@Haberkamp
Copy link
Contributor Author

Ok, I've made the changes you've requested.

docs/rules/prefer-user-facing-locators.md Outdated Show resolved Hide resolved
docs/rules/prefer-user-facing-locators.md Outdated Show resolved Hide resolved
@Haberkamp Haberkamp force-pushed the add-prefer-user-facing-locators-rule branch from 5f91eab to a6e5ad4 Compare August 27, 2023 11:31
test/spec/no-raw-locators.spec.ts Outdated Show resolved Hide resolved
@mskelton mskelton merged commit 81d7f54 into playwright-community:main Aug 27, 2023
2 checks passed
@github-actions
Copy link

🎉 This PR is included in version 0.16.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@seanlaff
Copy link

Should this rule be added to the list of rules in the readme?

@mskelton
Copy link
Member

@seanlaff Indeed it should, I missed that before. If you are willing to open a PR, that'd be appreciated!

@mskelton
Copy link
Member

@seanlaff It's in the readme now

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.

Add no-class-selectors rule
4 participants