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
Cop idea: detect not_to have_selector capybara matchers #378
Comments
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. expect(page).not_to have_xpath('a')
expect(page).to have_no_xpath('a') I could be misunderstanding! |
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 |
I'm not going to have time to jump on this for now I'm afraid! |
Anyway your input was very valuable, thank you for looking at this |
This is a very good suggestion, while functionally they are equivalent, |
This is a very nice suggestion and must be useful for so many developers! I found a similar warning on the "Capybara cheatsheet":
|
A similar warning on the Codeship blog:
|
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 So as far as it goes my opinion is that, apart from stylistic preferences, this cop is not necessary. On the other hand, a |
This cop would be great! For now I used this regular expression:
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. |
@claudiosv that would be awesome! 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 |
There is also a pull request to move hardcoded definitions into configuration #956, you may as well move |
I've checked that |
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. |
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. |
Removes the bad negative example and adds a new one that still has bad performance. It use to be that `not_to` would wait in a non-performant way: https://www.cloudbees.com/blog/faster-rails-tests but now it it no longer waits: rubocop/rubocop-rspec#378 (comment)
Removes the bad negative example and adds a new one that still has bad performance. It use to be that `not_to` would wait in a non-performant way: https://www.cloudbees.com/blog/faster-rails-tests but now it it no longer waits: rubocop/rubocop-rspec#378 (comment)
The
have_*
collection of capybara matchers has the same set ofhave_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:
Additionally the cop could detect usage of the node_matchers with eq false:
expect(has_selector?('#something')).to eq false
The text was updated successfully, but these errors were encountered: