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

Add new Capybara/RSpec/HaveSelector cop #63

Merged
merged 1 commit into from
Sep 19, 2023
Merged

Conversation

ydah
Copy link
Member

@ydah ydah commented Aug 27, 2023

This PR add new Capybara/RSpec/HaveSelector cop.

The first argument of have_selector is kind, which can be either :css or :xpath (if not specified, it works with the default value)
https://www.rubydoc.info/gems/capybara/Capybara#configure-class_method

It is passed to Matchers::HaveSelector and new.

https://github.com/teamcapybara/capybara/blob/0b43cb90ae633ce9b2008e07b997c76833fa570e/lib/capybara/rspec/matchers.rb#L19

have_css and have_xpath are passed to Matchers::HaveSelector as :css or :xpath as the first argument, respectively, and new is done.

https://github.com/teamcapybara/capybara/blob/0b43cb90ae633ce9b2008e07b997c76833fa570e/lib/capybara/rspec/matchers.rb#L50-L53

So they work the same but can be written in two ways.
In the case of have_selector, I think it is better to use have_css and have_xpath because the behavior changes depending on the defaultkind and it is easier to understand if you specify more details.

# @example
# bad
expect(foo).to have_selector(:css, 'bar')
# good
expect(foo).to have_css('bar')
# bad
expect(foo).to have_selector(:xpath, 'bar')
# good
expect(foo).to have_xpath('bar')

# @example DefaultSelector: css (default)
# bad
expect(foo).to have_selector('bar')
# good
expect(foo).to have_css('bar')

# @example DefaultSelector: xpath
# bad
expect(foo).to have_selector('bar')
# good
expect(foo).to have_xpath('bar')

Before submitting the PR make sure the following are checked:

  • Feature branch is up-to-date with main (if not - rebase it).
  • Squashed related commits together.
  • Added tests.
  • Updated documentation.
  • Added an entry to the CHANGELOG.md if the new code introduces user-observable changes.
  • The build (bundle exec rake) passes (be sure to run this locally, since it may produce updated documentation that you will need to commit).

If you have created a new cop:

  • Added the new cop to config/default.yml.
  • The cop is configured as Enabled: pending in config/default.yml.
  • The cop documents examples of good and bad code.
  • The tests assert both that bad code is reported and that good code is not reported.
  • Set VersionAdded: "<<next>>" in default/config.yml.

This PR add new `Capybara/RSpec/HaveSelector` cop.

```ruby
expect(foo).to have_selector(:css, 'bar')
expect(foo).to have_css('bar')
expect(foo).to have_selector(:xpath, 'bar')
expect(foo).to have_xpath('bar')

expect(foo).to have_selector('bar')
expect(foo).to have_css('bar')

expect(foo).to have_selector('bar')
expect(foo).to have_xpath('bar')
```
@ydah ydah merged commit 02e0187 into main Sep 19, 2023
22 checks passed
@ydah ydah deleted the new_cop_have_selector branch September 19, 2023 13:44
@phil-workato
Copy link

It feels that this cop's docs lack rationale as to why it does it this way.

Do you think we need more active maintainers in this repo? I see this PR and #61 were merged without code reviews. #55 have been approved before re-targeting, so it's fine.

@ydah
Copy link
Member Author

ydah commented Sep 20, 2023

It feels that this cop's docs lack rationale as to why it does it this way.

The first argument of have_selector is kind, which can be either :css or :xpath (if not specified, it works with the default value)
https://www.rubydoc.info/gems/capybara/Capybara#configure-class_method

It is passed to Matchers::HaveSelector and new.

https://github.com/teamcapybara/capybara/blob/0b43cb90ae633ce9b2008e07b997c76833fa570e/lib/capybara/rspec/matchers.rb#L19

have_css and have_xpath are passed to Matchers::HaveSelector as :css or :xpath as the first argument, respectively, and new is done.
https://github.com/teamcapybara/capybara/blob/0b43cb90ae633ce9b2008e07b997c76833fa570e/lib/capybara/rspec/matchers.rb#L50-L53

So they work the same but can be written in two ways.
In the case of have_selector, I think it is better to use have_css and have_xpath because the behavior changes depending on the defaultkind and it is easier to understand if you specify more details.

Do you think we need more active maintainers in this repo?

Yes, I think so.

@phil-workato
Copy link

Please consider this an open discussion, not a request for a change in any way.

In my current project, a question for justification for this cop was raised. As it is the default in Capybara, the terms "selector" and "CSS selectors" de facto became synonyms. So why do we have to replace existing have_selector usages?

From myself - if we now provide a cop to flag the generic have_selector matcher, why we still allow e.g. the find which also allows for an optional kind parameter?

page.find(:xpath, './/div[contains(., "bar")]')
page.find('li', text: 'Quox').click_link('Delete')

I believe there are many, e.g. has_no_selector that accept kind. Playing the devil's advocate, should we flag all of them?

In addition to that, there are methods like all that do not have a specific css counterpart (like e.g. find_css), but still accept a kind parameter.

So what is the actual purpose of the cop? "When looking at a method call, avoid confusing the selector kind".
Well, remembering the XPath, I don't think that in the vast majority of the cases, a CSS selector can be taken as XPath or vice versa.

@bquorning @Darhazer I'd love to hear your thoughts on this, too. I don't want to burden the development of this extension, but we are less experienced with Capybara than @ydah. Should we take shifts with code reviews etc?

@ydah
Copy link
Member Author

ydah commented Sep 20, 2023

In my current project, a question for justification for this cop was raised. As it is the default in Capybara, the terms "selector" and "CSS selectors" de facto became synonyms. So why do we have to replace existing have_selector usages?

I see, selector in have_selector, but I think it refers to the capybara selector.
https://www.rubydoc.info/gems/capybara/Capybara/Selector

The use of have_selector and have_css appears to be intentional, and the behavior of each appears to be different, but the behavior is the same. (Unless the default KIND of have_selector is also :xpath)
In such a case, what do you think, it would be better to be more specific like other RuboCop Capybara's Capybara/SpecificMatcher and Capybara/SpecificFinders?

In addition to that, there are methods like all that do not have a specific css counterpart (like e.g. find_css), but still accept a kind parameter.

It certainly seems like a good idea to list violations for redundant kind designations since the default_selector should be fixed.

I believe there are many, e.g. has_no_selector that accept kind. Playing the devil's advocate, should we flag all of them?

I don't have a strong opinion, but it may be added if necessary. However, it would be added as a separate cop.

So what is the actual purpose of the cop?

the purpose of this cop is to explicitly specify selectors like have_css or have_xpath rather than using have_selector. This aligns with the philosophy held by Capybara/SpecificMatcher and Capybara/SpecificFinders, which stems from being more specific.

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.

2 participants