Rubocop-rspec: Cop idea: detect not_to have_selector capybara matchers

[Darhazer]

The have_* collection of capybara matchers implement implicit ways. For checking that there is no content matching there is the same set of have_no matchers. I've seen a lot of people however using expect(page).not_to have_selector which is wrong:

• it does wait until the selector appears or timeout occurs, meaning that all passing tests have unneeded wait
• it does not wait for the selector to disappear, if it's removed with JS, making flaky tests

This goes for all of:

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 have_no_* versions with RSpec matchers because should_not 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