search

rubocop / rubocop-capybara

Code style checking for Capybara files.
https://docs.rubocop.org/rubocop-capybara
MIT License
40 stars 8 forks source link

Add new `Capybara/RSpec/HaveSelector` cop #63

Closed ydah closed 11 months ago

ydah commented 1 year ago

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:

If you have created a new cop:

phil-workato commented 11 months ago

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 commented 11 months ago

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 commented 11 months ago

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 commented 11 months ago

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.