-
-
Notifications
You must be signed in to change notification settings - Fork 278
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
Fix a false positive and incorrect autocorrect for RSpec/Capybara/SpecificActions
, RSpec/Capybara/SpecificFinders
and RSpec/Capybara/SpecificMatcher
#1481
Conversation
@@ -66,14 +147,32 @@ | |||
^^^^^^^^^^^^^^^^^^^^ Prefer `find_by` over `find`. | |||
find('[visible][id=some-id]') | |||
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Prefer `find_by` over `find`. | |||
find('[id=some-id][class=some-cls][focused]') | |||
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Prefer `find_by` over `find`. | |||
find('[id=some-id][visible][focused]') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CSS selector [visible]
means the element has an attribute named visible
(e.g. <span visible="yes">
), NOT "the element is actually visible". CSS does not have :visible
pseudo-class --- it's a jQuery's extension.
And also, [focused]
means the element has an attribute named focused
, NOT "the element is now focused". :focus
pseudo-class should be used instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is indeed true. I guess I was mistaken. Thanks for pointing that out to me.
RSpec/Capybara/SpecificFinders
RSpec/Capybara/SpecificActions
, RSpec/Capybara/SpecificFinders
and RSpec/Capybara/SpecificMatcher
I updated this PR. |
I can't tell much about those cops, just that there were no errors:
Thank you! PS In the output there were only offences from |
for the other |
|
4bc501e
to
62d7243
Compare
I rebased this PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would really like to see some direct spec coverage of the mixins, especially lib/rubocop/cop/rspec/mixin/css_selector.rb. For a start, it could just test the mentioned @example
lines of each method.
@@ -87,21 +57,11 @@ def attribute?(selector) | |||
def attributes(selector) | |||
selector.scan(/\[(.*?)\]/).flatten.to_h do |attr| | |||
key, value = attr.split('=') | |||
value << ']' if value&.include?('[') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This does not look right. Probably a shortcoming of us using regular expressions to parse CSS :-)
Please don't merge this just yet, I'm extracting |
May I kindly ask to re-target this to |
…ecificActions`, `RSpec/Capybara/SpecificFinders` and `RSpec/Capybara/SpecificMatcher` Fix: https://github.com/rubocop/rubocop-rspec/issues/1468
@pirj OK, I'll do it 👍 It may take a little time as we add mixin testing as well. |
We are now open and PR here is closed. |
Fix: rubocop/rubocop-capybara#4
Before submitting the PR make sure the following are checked:
master
(if not - rebase it).CHANGELOG.md
if the new code introduces user-observable changes.bundle exec rake
) passes (be sure to run this locally, since it may produce updated documentation that you will need to commit).