Cop idea: detect not_to have_selector capybara matchers

[Darhazer]

The have_* collection of capybara matchers has the same set of have_no matches, for checking that there is no content matching. This creates the option to write the expectation in two forms:

expect(page).to have_no_selector expect(page).not_to have_selector

For consistency, it would be nice to have a cop that enforces only one of the styles.

This goes for all of:

have_button
have_checked_field
have_css
have_field
have_link
have_select
have_selector
have_table
have_text
have_unchecked_field
have_xpath

Additionally the cop could detect usage of the node_matchers with eq false: expect(has_selector?('#something')).to eq false

[timrogers]

Are we sure this is a problem? The Capybara readme says the following:

Capybara's RSpec matchers, however, are smart enough to handle either form. The two following statements are functionally equivalent:

expect(page).not_to have_xpath('a')
expect(page).to have_no_xpath('a')

I could be misunderstanding!

[Darhazer]

I can't prove (although I'm pretty sure they have different behavior), but even if that is the case, it still good to have cop enforcing consistent usage. Maybe it should be configurable if one prefers the not_to style

[timrogers]

I'm not going to have time to jump on this for now I'm afraid!

[Darhazer]

Anyway your input was very valuable, thank you for looking at this

[kirhgoff]

This is a very good suggestion, while functionally they are equivalent, expect(page).to have_no_css('aaa') will actually wait for a timeout until the object disappears, while expect(page).not_to have_css('aaa') will momentarily fail if object is still there.

[ybiquitous]

This is a very nice suggestion and must be useful for so many developers!

I found a similar warning on the "Capybara cheatsheet":

Use should haveno versions with RSpec matchers because shouldnot have doesn’t wait for a timeout from the driver.

[ybiquitous]

A similar warning on the Codeship blog:

... That brings us to a common mistake: flipping the assertion instead of the finder, like this: refute page.has_content?("Dashboard"). ... Which means that this is 15 seconds (the default) slower than assert page.has_no_content?("Dashboard").

https://blog.codeship.com/faster-rails-tests/

[badosu]

Just for reference I tested a big codebase refactor from one form to another and did not see any improvement.

It seems that for rspec the matcher is smart enough to understand not_to have_selector, as the README says (although not emphatic enough for me).

So as far as it goes my opinion is that, apart from stylistic preferences, this cop is not necessary.

On the other hand, a !expect(page).to have_selector seems like a pretty important cop to have as it still suffers from the timeout issue.

[claudiosv]

This cop would be great! For now I used this regular expression:

.not_to\s+(have_button|have_checked_field|have_css|have_field|have_link|have_select|have_selector|have_table|have_text|have_unchecked_field|have_xpath)

It seems negated matchers are defined here. They are not all functionally equivalent, as mentioned above.

Side note: I'd be happy to write a cop for this if there is a tutorial on how to write one.

[pirj]

@claudiosv that would be awesome! You can copy-paste lib/rubocop/cop/rspec/capybara/visibility_matcher.rb, extract the common CAPYBARA_MATCHER_METHODS and capybara_matcher?. For the node pattern to match this should work:

          def_node_matcher :negative_expectation_matcher?, <<~PATTERN
            (send nil? (send nil? :expect _) {:not_to :to_not} (send nil? #capybara_matcher? ...))
          PATTERN

For the hook:

def on_send(node)
  negative_expectation_matcher?(node) { |node| add_offense(node) }
end

[pirj]

There is also a pull request to move hardcoded definitions into configuration https://github.com/rubocop-hq/rubocop-rspec/pull/956, you may as well move CAPYBARA_MATCHER_METHODS there @claudiosv

[gonzaloriestra]

I've checked that expect(page).not_to have_selector does wait for the selector to disappear when it's removed, so I think the Capybara documentation is right: both ways are equivalent.

[dacook]

Thanks for confirming that Capybara does handle these two forms the same way (at least, it does now).

But as Darhazer pointed out, it's still worth having the cop even just to enforce consistent usage.

[Darhazer]

Updated the description to make it clear that it's about the style preference/consistency, and not for an actual issue with the implicit waits.