Skip to content
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 for RSpec/Capybara/SpecificMatcher when pseudo-classes #1361

Merged
merged 1 commit into from Sep 7, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Expand Up @@ -19,6 +19,7 @@
* Deprecate `IgnoredPatterns` option in favor of the `AllowedPatterns` options. ([@ydah][])
* Add `AllowedPatterns` configuration option to `RSpec/ContextWording`. ([@ydah][])
* Add `RSpec/ClassCheck` cop. ([@r7kamura][])
* Fix a false positive for `RSpec/Capybara/SpecificMatcher` when pseudo-classes. ([@ydah][])

## 2.12.1 (2022-07-03)

Expand Down
35 changes: 30 additions & 5 deletions lib/rubocop/cop/rspec/capybara/specific_matcher.rb
Expand Up @@ -26,7 +26,7 @@ module Capybara
# expect(page).to have_select
# expect(page).to have_field('foo')
#
class SpecificMatcher < Base
class SpecificMatcher < Base # rubocop:disable Metrics/ClassLength
MSG = 'Prefer `%<good_matcher>s` over `%<bad_matcher>s`.'
RESTRICT_ON_SEND = %i[have_selector have_no_selector have_css
have_no_css].freeze
Expand Down Expand Up @@ -62,6 +62,9 @@ class SpecificMatcher < Base
]
).freeze
}.freeze
SPECIFIC_MATCHER_PSEUDO_CLASSES = %w[
not() disabled enabled checked unchecked
].freeze

# @!method first_argument(node)
def_node_matcher :first_argument, <<-PATTERN
Expand All @@ -78,6 +81,7 @@ def on_send(node)
next unless (matcher = specific_matcher(arg))
next if CssSelector.multiple_selectors?(arg)
next unless specific_matcher_option?(node, arg, matcher)
next unless specific_matcher_pseudo_classes?(arg)

add_offense(node, message: message(node, matcher))
end
Expand All @@ -90,10 +94,6 @@ def specific_matcher(arg)
SPECIFIC_MATCHER[splitted_arg]
end

def acceptable_pattern?(arg)
arg.match?(/[ >,+]/)
end

def specific_matcher_option?(node, arg, matcher)
attrs = CssSelector.attributes(arg).keys
return true if attrs.empty?
Expand All @@ -104,6 +104,31 @@ def specific_matcher_option?(node, arg, matcher)
end
end

def specific_matcher_pseudo_classes?(arg)
CssSelector.pseudo_classes(arg).all? do |pseudo_class|
replaceable_pseudo_class?(pseudo_class, arg)
end
end

def replaceable_pseudo_class?(pseudo_class, arg)
unless SPECIFIC_MATCHER_PSEUDO_CLASSES.include?(pseudo_class)
return false
end

case pseudo_class
when 'not()' then replaceable_pseudo_class_not?(arg)
else true
end
end

def replaceable_pseudo_class_not?(arg)
arg.scan(/not\(.*?\)/).all? do |not_arg|
CssSelector.attributes(not_arg).values.all? do |v|
v.is_a?(TrueClass) || v.is_a?(FalseClass)
end
end
end

def replaceable_matcher?(node, matcher, attrs)
case matcher
when 'link' then replaceable_to_have_link?(node, attrs)
Expand Down
14 changes: 14 additions & 0 deletions lib/rubocop/cop/rspec/mixin/css_selector.rb
Expand Up @@ -55,6 +55,20 @@ def common_attributes?(selector)
attributes(selector).keys.difference(COMMON_OPTIONS).none?
end

# @param selector [String]
# @return [Array<String>]
# @example
# pseudo_classes('button:not([disabled])') # => ['not()']
# pseudo_classes('a:enabled:not([valid])') # => ['enabled', 'not()']
def pseudo_classes(selector)
# Attributes must be excluded or else the colon in the `href`s URL
# will also be picked up as pseudo classes.
# "a:not([href='http://example.com']):enabled" => "a:not():enabled"
ignored_attribute = selector.gsub(/\[.*?\]/, '')
# "a:not():enabled" => ["not()", "enabled"]
ignored_attribute.scan(/:([^:]*)/).flatten
end

# @param selector [String]
# @return [Boolean]
# @example
Expand Down
43 changes: 41 additions & 2 deletions spec/rubocop/cop/rspec/capybara/specific_matcher_spec.rb
Expand Up @@ -194,17 +194,56 @@ class style visible obscured exact exact_text normalize_ws match wait
expect_offense(<<-RUBY)
expect(page).to have_css('button[disabled][name="foo"]', exact_text: 'foo')
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Prefer `have_button` over `have_css`.
expect(page).to have_css('button:not([name="foo"][disabled])', exact_text: 'bar')
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Prefer `have_button` over `have_css`.
RUBY
end

it 'registers an offense when using abstract matcher with state' do
expect_offense(<<-RUBY)
expect(page).to have_css('button[disabled]', exact_text: 'foo')
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Prefer `have_button` over `have_css`.
RUBY
end

it 'registers an offense when using abstract matcher with ' \
'first argument is element with replaceable pseudo-classes' do
expect_offense(<<-RUBY)
expect(page).to have_css('button:not([disabled])', exact_text: 'bar')
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Prefer `have_button` over `have_css`.
expect(page).to have_css('button:not([disabled][visible])')
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Prefer `have_button` over `have_css`.
RUBY
end

it 'registers an offense when using abstract matcher with ' \
'first argument is element with multiple replaceable pseudo-classes' do
expect_offense(<<-RUBY)
expect(page).to have_css('button:not([disabled]):enabled')
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Prefer `have_button` over `have_css`.
expect(page).to have_css('button:not([disabled=false]):disabled')
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Prefer `have_button` over `have_css`.
expect(page).to have_css('button:not([disabled]):not([disabled])')
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Prefer `have_button` over `have_css`.
expect(page).to have_css('input:not([unchecked]):checked')
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Prefer `have_field` over `have_css`.
expect(page).to have_css('input:not([unchecked=false]):unchecked')
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Prefer `have_field` over `have_css`.
expect(page).to have_css('input:not([unchecked]):not([unchecked])')
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Prefer `have_field` over `have_css`.
RUBY
end

it 'does not register an offense when using abstract matcher with ' \
'first argument is element with replaceable pseudo-classes' \
'and not boolean attributes' do
expect_no_offenses(<<-RUBY)
expect(page).to have_css('button:not([name="foo"][disabled])')
RUBY
end

it 'does not register an offense when using abstract matcher with ' \
'first argument is element with multiple nonreplaceable pseudo-classes' do
expect_no_offenses(<<-RUBY)
expect(page).to have_css('button:first-of-type:not([disabled])')
RUBY
end

Expand Down