Fix a false positive and incorrect autocorrect for `RSpec/Capybara/Sp…

…ecificActions`, `RSpec/Capybara/SpecificFinders` and `RSpec/Capybara/SpecificMatcher`

Fix: https://github.com/rubocop/rubocop-rspec/issues/1468

CHANGELOG.md

@@ -2,6 +2,7 @@
 
 ## Master (Unreleased)
 
+- Fix a false positive and incorrect autocorrect for `RSpec/Capybara/SpecificActions`, `RSpec/Capybara/SpecificFinders` and `RSpec/Capybara/SpecificMatcher`. ([@ydah])
 - Add new `RSpec/FactoryBot/FactoryNameStyle` cop. ([@ydah])
 - Improved processing speed for `RSpec/Be`, `RSpec/ExpectActual`, `RSpec/ImplicitExpect`, `RSpec/MessageSpies`, `RSpec/PredicateMatcher` and `RSpec/Rails/HaveHttpStatus`. ([@ydah])
 - Fix wrong autocorrection in `n_times` style on `RSpec/FactoryBot/CreateList`. ([@r7kamura])

lib/rubocop/cop/rspec/capybara/specific_actions.rb

@@ -42,9 +42,7 @@ def on_send(node)
               # which the last selector points.
               next unless (selector = last_selector(arg))
               next unless (action = specific_action(selector))
-              next unless CapybaraHelp.specific_option?(node.receiver, arg,
-                                                        action)
-              next unless CapybaraHelp.specific_pseudo_classes?(arg)
+              next unless replaceable?(node, arg, action)
 
               range = offense_range(node, node.receiver)
               add_offense(range, message: message(action, selector))
@@ -57,6 +55,18 @@ def specific_action(selector)
             SPECIFIC_ACTION[last_selector(selector)]
           end
 
+          def replaceable?(node, arg, action)
+            replaceable_attributes?(arg) &&
+              CapybaraHelp.replaceable_option?(node.receiver, arg, action) &&
+              CapybaraHelp.replaceable_pseudo_classes?(arg)
+          end
+
+          def replaceable_attributes?(selector)
+            CapybaraHelp.replaceable_attributes?(
+              CssSelector.attributes(selector)
+            )
+          end
+
           def supported_selector?(selector)
             !selector.match?(/[>,+~]/)
           end

lib/rubocop/cop/rspec/capybara/specific_finders.rb

@@ -28,8 +28,14 @@ class SpecificFinders < Base
             (send _ :find (str $_) ...)
           PATTERN
 
+          # @!method class_options(node)
+          def_node_search :class_options, <<~PATTERN
+            (pair (sym :class) $_ ...)
+          PATTERN
+
           def on_send(node)
             find_argument(node) do |arg|
+              next if CssSelector.pseudo_classes(arg).any?
               next if CssSelector.multiple_selectors?(arg)
 
               on_attr(node, arg) if attribute?(arg)
@@ -40,28 +46,57 @@ def on_send(node)
           private
 
           def on_attr(node, arg)
-            return unless (id = CssSelector.attributes(arg)['id'])
+            attrs = CssSelector.attributes(arg)
+            return unless (id = attrs['id'])
+            return if attrs['class']
 
             register_offense(node, replaced_arguments(arg, id))
           end
 
           def on_id(node, arg)
-            register_offense(node, "'#{arg.to_s.delete('#')}'")
+            return if CssSelector.attributes(arg).any?
+
+            id = CssSelector.id(arg)
+            register_offense(node, "'#{id.delete('#')}'",
+                             CssSelector.classes(arg.sub(id, '')))
           end
 
           def attribute?(arg)
             CssSelector.attribute?(arg) &&
-              CssSelector.common_attributes?(arg)
+              CapybaraHelp.common_attributes?(arg)
           end
 
-          def register_offense(node, arg_replacement)
+          def register_offense(node, id, classes = [])
             add_offense(offense_range(node)) do |corrector|
               corrector.replace(node.loc.selector, 'find_by_id')
               corrector.replace(node.first_argument.loc.expression,
-                                arg_replacement)
+                                id.delete('\\'))
+              unless classes.compact.empty?
+                autocorrect_classes(corrector, node, classes)
+              end
             end
           end
 
+          def autocorrect_classes(corrector, node, classes)
+            if (options = class_options(node).first)
+              append_options(classes, options)
+              corrector.replace(options, classes.to_s)
+            else
+              corrector.insert_after(node.first_argument,
+                                     keyword_argument_class(classes))
+            end
+          end
+
+          def append_options(classes, options)
+            classes << options.value if options.str_type?
+            options.each_value { |v| classes << v.value } if options.array_type?
+          end
+
+          def keyword_argument_class(classes)
+            value = classes.size > 1 ? classes.to_s : "'#{classes.first}'"
+            ", class: #{value}"
+          end
+
           def replaced_arguments(arg, id)
             options = to_options(CssSelector.attributes(arg))
             options.empty? ? id : "#{id}, #{options}"

lib/rubocop/cop/rspec/capybara/specific_matcher.rb

@@ -47,8 +47,7 @@ def on_send(node)
             first_argument(node) do |arg|
               next unless (matcher = specific_matcher(arg))
               next if CssSelector.multiple_selectors?(arg)
-              next unless CapybaraHelp.specific_option?(node, arg, matcher)
-              next unless CapybaraHelp.specific_pseudo_classes?(arg)
+              next unless replaceable?(node, arg, matcher)
 
               add_offense(node, message: message(node, matcher))
             end
@@ -61,6 +60,18 @@ def specific_matcher(arg)
             SPECIFIC_MATCHER[splitted_arg]
           end
 
+          def replaceable?(node, arg, matcher)
+            replaceable_attributes?(arg) &&
+              CapybaraHelp.replaceable_option?(node, arg, matcher) &&
+              CapybaraHelp.replaceable_pseudo_classes?(arg)
+          end
+
+          def replaceable_attributes?(selector)
+            CapybaraHelp.replaceable_attributes?(
+              CssSelector.attributes(selector)
+            )
+          end
+
           def message(node, matcher)
             format(MSG,
                    good_matcher: good_matcher(node, matcher),

lib/rubocop/cop/rspec/mixin/capybara_help.rb

@@ -5,24 +5,69 @@ module Cop
     module RSpec
       # Help methods for capybara.
       module CapybaraHelp
+        COMMON_OPTIONS = %w[
+          id class style
+        ].freeze
+        SPECIFIC_OPTIONS = {
+          'button' => (
+            COMMON_OPTIONS + %w[disabled name value title type]
+          ).freeze,
+          'link' => (
+            COMMON_OPTIONS + %w[href alt title download]
+          ).freeze,
+          'table' => (
+            COMMON_OPTIONS + %w[cols rows]
+          ).freeze,
+          'select' => (
+            COMMON_OPTIONS + %w[
+              disabled name placeholder
+              selected multiple
+            ]
+          ).freeze,
+          'field' => (
+            COMMON_OPTIONS + %w[
+              checked disabled name placeholder
+              readonly type multiple
+            ]
+          ).freeze
+        }.freeze
+        SPECIFIC_PSEUDO_CLASSES = %w[
+          not() disabled enabled checked unchecked
+        ].freeze
+
         module_function
 
         # @param node [RuboCop::AST::SendNode]
         # @param locator [String]
         # @param element [String]
         # @return [Boolean]
-        def specific_option?(node, locator, element)
+        def replaceable_option?(node, locator, element)
           attrs = CssSelector.attributes(locator).keys
           return false unless replaceable_element?(node, element, attrs)
 
           attrs.all? do |attr|
-            CssSelector.specific_options?(element, attr)
+            SPECIFIC_OPTIONS.fetch(element, []).include?(attr)
           end
         end
 
+        # @param selector [String]
+        # @return [Boolean]
+        # @example
+        #   common_attributes?('a[focused]') # => true
+        #   common_attributes?('button[focused][visible]') # => true
+        #   common_attributes?('table[id=some-id]') # => true
+        #   common_attributes?('h1[invalid]') # => false
+        def common_attributes?(selector)
+          CssSelector.attributes(selector).keys.difference(COMMON_OPTIONS).none?
+        end
+
+        def replaceable_attributes?(attrs)
+          attrs.values.none?(&:nil?)
+        end
+
         # @param locator [String]
         # @return [Boolean]
-        def specific_pseudo_classes?(locator)
+        def replaceable_pseudo_classes?(locator)
           CssSelector.pseudo_classes(locator).all? do |pseudo_class|
             replaceable_pseudo_class?(pseudo_class, locator)
           end
@@ -32,7 +77,7 @@ def specific_pseudo_classes?(locator)
         # @param locator [String]
         # @return [Boolean]
         def replaceable_pseudo_class?(pseudo_class, locator)
-          return false unless CssSelector.specific_pesudo_classes?(pseudo_class)
+          return false unless SPECIFIC_PSEUDO_CLASSES.include?(pseudo_class)
 
           case pseudo_class
           when 'not()' then replaceable_pseudo_class_not?(locator)

lib/rubocop/cop/rspec/mixin/css_selector.rb

@@ -5,59 +5,18 @@ module Cop
     module RSpec
       # Helps parsing css selector.
       module CssSelector
-        COMMON_OPTIONS = %w[
-          above below left_of right_of near count minimum maximum between text
-          id class style visible obscured exact exact_text normalize_ws match
-          wait filter_set focused
-        ].freeze
-        SPECIFIC_OPTIONS = {
-          'button' => (
-            COMMON_OPTIONS + %w[disabled name value title type]
-          ).freeze,
-          'link' => (
-            COMMON_OPTIONS + %w[href alt title download]
-          ).freeze,
-          'table' => (
-            COMMON_OPTIONS + %w[
-              caption with_cols cols with_rows rows
-            ]
-          ).freeze,
-          'select' => (
-            COMMON_OPTIONS + %w[
-              disabled name placeholder options enabled_options
-              disabled_options selected with_selected multiple with_options
-            ]
-          ).freeze,
-          'field' => (
-            COMMON_OPTIONS + %w[
-              checked unchecked disabled valid name placeholder
-              validation_message readonly with type multiple
-            ]
-          ).freeze
-        }.freeze
-        SPECIFIC_PSEUDO_CLASSES = %w[
-          not() disabled enabled checked unchecked
-        ].freeze
-
         module_function
 
-        # @param element [String]
-        # @param attribute [String]
-        # @return [Boolean]
+        # @param selector [String]
+        # @return [String]
         # @example
-        #   specific_pesudo_classes?('button', 'name') # => true
-        #   specific_pesudo_classes?('link', 'invalid') # => false
-        def specific_options?(element, attribute)
-          SPECIFIC_OPTIONS.fetch(element, []).include?(attribute)
-        end
+        #   id('#some-id') # => some-id
+        #   id('.some-class') # => nil
+        #   id('#some-id.cls') # => some-id
+        def id(selector)
+          return unless id?(selector)
 
-        # @param pseudo_class [String]
-        # @return [Boolean]
-        # @example
-        #   specific_pesudo_classes?('disabled') # => true
-        #   specific_pesudo_classes?('first-of-type') # => false
-        def specific_pesudo_classes?(pseudo_class)
-          SPECIFIC_PSEUDO_CLASSES.include?(pseudo_class)
+          selector.gsub(selector.scan(/[^\\]([>,+~.].*)/).join, '')
         end
 
         # @param selector [String]
@@ -69,6 +28,17 @@ def id?(selector)
           selector.start_with?('#')
         end
 
+        # @param selector [String]
+        # @return [Array<String>]
+        # @example
+        #   classes('#some-id') # => []
+        #   classes('.some-class') # => ['some-class']
+        #   classes('#some-id.some-class') # => ['some-class']
+        #   classes('#some-id.cls1.cls2') # => ['cls1', 'cls2']
+        def classes(selector)
+          selector.scan(/.([\w-]*)/).flatten
+        end
+
         # @param selector [String]
         # @return [Boolean]
         # @example
@@ -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?('[')
             [key, normalize_value(value)]
           end
         end
 
-        # @param selector [String]
-        # @return [Boolean]
-        # @example
-        #   common_attributes?('a[focused]') # => true
-        #   common_attributes?('button[focused][visible]') # => true
-        #   common_attributes?('table[id=some-id]') # => true
-        #   common_attributes?('h1[invalid]') # => false
-        def common_attributes?(selector)
-          attributes(selector).keys.difference(COMMON_OPTIONS).none?
-        end
-
         # @param selector [String]
         # @return [Array<String>]
         # @example
@@ -122,22 +82,23 @@ def pseudo_classes(selector)
         #   multiple_selectors?('a.cls b#id') # => true
         #   multiple_selectors?('a.cls') # => false
         def multiple_selectors?(selector)
-          selector.match?(/[ >,+~]/)
+          normalize = selector.gsub(/(\\[>,+~]|\(.*\))/, '')
+          normalize.match?(/[ >,+~]/)
         end
 
         # @param value [String]
         # @return [Boolean, String]
         # @example
         #   normalize_value('true') # => true
         #   normalize_value('false') # => false
-        #   normalize_value(nil) # => false
+        #   normalize_value(nil) # => nil
         #   normalize_value("foo") # => "'foo'"
         def normalize_value(value)
           case value
           when 'true' then true
           when 'false' then false
-          when nil then true
-          else "'#{value}'"
+          when nil then nil
+          else "'#{value.gsub(/["']/, '')}'"
           end
         end
       end