Merge pull request #1340 from ydah/add_specific_finders

Add new `RSpec/Capybara/SpecificFinders` cop

.rubocop.yml

@@ -114,6 +114,8 @@ RSpec/SubjectDeclaration:
   Enabled: true
 RSpec/VerifiedDoubleReference:
   Enabled: true
+RSpec/Capybara/SpecificFinders:
+  Enabled: true
 RSpec/Capybara/SpecificMatcher:
   Enabled: true
 RSpec/FactoryBot/SyntaxMethods:

CHANGELOG.md

@@ -12,6 +12,7 @@
 * Fix a false negative for `RSpec/Capybara/SpecificMatcher` for `have_field`. ([@ydah][])
 * Fix a false positive for `RSpec/Capybara/SpecificMatcher` when may not have a `href` by `have_link`. ([@ydah][])
 * Add `NegatedMatcher` configuration option to `RSpec/ChangeByZero`. ([@ydah][])
+* Add new `RSpec/Capybara/SpecificFinders` cop. ([@ydah][])
 
 ## 2.12.1 (2022-07-03)
 

config/default.yml

@@ -851,6 +851,12 @@ RSpec/Capybara/FeatureMethods:
   VersionChanged: '2.0'
   Reference: https://www.rubydoc.info/gems/rubocop-rspec/RuboCop/Cop/RSpec/Capybara/FeatureMethods
 
+RSpec/Capybara/SpecificFinders:
+  Description: Checks if there is a more specific finder offered by Capybara.
+  Enabled: pending
+  VersionAdded: '2.13'
+  Reference: https://www.rubydoc.info/gems/rubocop-rspec/RuboCop/Cop/RSpec/Capybara/SpecificFinders
+
 RSpec/Capybara/SpecificMatcher:
   Description: Checks for there is a more specific matcher offered by Capybara.
   Enabled: pending

docs/modules/ROOT/pages/cops.adoc

@@ -94,6 +94,7 @@
 
 * xref:cops_rspec_capybara.adoc#rspeccapybara/currentpathexpectation[RSpec/Capybara/CurrentPathExpectation]
 * xref:cops_rspec_capybara.adoc#rspeccapybara/featuremethods[RSpec/Capybara/FeatureMethods]
+* xref:cops_rspec_capybara.adoc#rspeccapybara/specificfinders[RSpec/Capybara/SpecificFinders]
 * xref:cops_rspec_capybara.adoc#rspeccapybara/specificmatcher[RSpec/Capybara/SpecificMatcher]
 * xref:cops_rspec_capybara.adoc#rspeccapybara/visibilitymatcher[RSpec/Capybara/VisibilityMatcher]
 

docs/modules/ROOT/pages/cops_rspec_capybara.adoc

@@ -112,6 +112,37 @@ end
 
 * https://www.rubydoc.info/gems/rubocop-rspec/RuboCop/Cop/RSpec/Capybara/FeatureMethods
 
+== RSpec/Capybara/SpecificFinders
+
+|===
+| Enabled by default | Safe | Supports autocorrection | Version Added | Version Changed
+
+| Pending
+| Yes
+| Yes
+| 2.13
+| -
+|===
+
+Checks if there is a more specific finder offered by Capybara.
+
+=== Examples
+
+[source,ruby]
+----
+# bad
+find('#some-id')
+find('[visible][id=some-id]')
+
+# good
+find_by_id('some-id')
+find_by_id('some-id', visible: true)
+----
+
+=== References
+
+* https://www.rubydoc.info/gems/rubocop-rspec/RuboCop/Cop/RSpec/Capybara/SpecificFinders
+
 == RSpec/Capybara/SpecificMatcher
 
 |===

lib/rubocop-rspec.rb

@@ -21,6 +21,7 @@
 require_relative 'rubocop/cop/rspec/mixin/empty_line_separation'
 require_relative 'rubocop/cop/rspec/mixin/inside_example_group'
 require_relative 'rubocop/cop/rspec/mixin/namespace'
+require_relative 'rubocop/cop/rspec/mixin/css_selector'
 
 require_relative 'rubocop/rspec/concept'
 require_relative 'rubocop/rspec/example_group'

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

@@ -0,0 +1,86 @@
+# frozen_string_literal: true
+
+module RuboCop
+  module Cop
+    module RSpec
+      module Capybara
+        # Checks if there is a more specific finder offered by Capybara.
+        #
+        # @example
+        #   # bad
+        #   find('#some-id')
+        #   find('[visible][id=some-id]')
+        #
+        #   # good
+        #   find_by_id('some-id')
+        #   find_by_id('some-id', visible: true)
+        #
+        class SpecificFinders < Base
+          extend AutoCorrector
+
+          include RangeHelp
+
+          MSG = 'Prefer `find_by` over `find`.'
+          RESTRICT_ON_SEND = %i[find].freeze
+
+          # @!method find_argument(node)
+          def_node_matcher :find_argument, <<~PATTERN
+            (send _ :find (str $_) ...)
+          PATTERN
+
+          def on_send(node)
+            find_argument(node) do |arg|
+              next if CssSelector.multiple_selectors?(arg)
+
+              on_attr(node, arg) if attribute?(arg)
+              on_id(node, arg) if CssSelector.id?(arg)
+            end
+          end
+
+          private
+
+          def on_attr(node, arg)
+            return unless (id = CssSelector.attributes(arg)['id'])
+
+            register_offense(node, replaced_argments(arg, id))
+          end
+
+          def on_id(node, arg)
+            register_offense(node, "'#{arg.to_s.delete('#')}'")
+          end
+
+          def attribute?(arg)
+            CssSelector.attribute?(arg) &&
+              CssSelector.common_attributes?(arg)
+          end
+
+          def register_offense(node, arg_replacemenet)
+            add_offense(offense_range(node)) do |corrector|
+              corrector.replace(node.loc.selector, 'find_by_id')
+              corrector.replace(node.first_argument.loc.expression,
+                                arg_replacemenet)
+            end
+          end
+
+          def replaced_argments(arg, id)
+            options = to_options(CssSelector.attributes(arg))
+            options.empty? ? id : "#{id}, #{options}"
+          end
+
+          def to_options(attrs)
+            attrs.each.map do |key, value|
+              next if key == 'id'
+
+              "#{key}: #{value}"
+            end.compact.join(', ')
+          end
+
+          def offense_range(node)
+            range_between(node.loc.selector.begin_pos,
+                          node.loc.end.end_pos)
+          end
+        end
+      end
+    end
+  end
+end

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

@@ -37,29 +37,26 @@ class SpecificMatcher < Base
             'select' => 'select',
             'input' => 'field'
           }.freeze
-          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_MATCHER_OPTIONS = {
             'button' => (
-              COMMON_OPTIONS + %w[disabled name value title type]
+              CssSelector::COMMON_OPTIONS + %w[disabled name value title type]
             ).freeze,
             'link' => (
-              COMMON_OPTIONS + %w[href alt title download]
+              CssSelector::COMMON_OPTIONS + %w[href alt title download]
             ).freeze,
             'table' => (
-              COMMON_OPTIONS + %w[caption with_cols cols with_rows rows]
+              CssSelector::COMMON_OPTIONS + %w[
+                caption with_cols cols with_rows rows
+              ]
             ).freeze,
             'select' => (
-              COMMON_OPTIONS + %w[
+              CssSelector::COMMON_OPTIONS + %w[
                 disabled name placeholder options enabled_options
                 disabled_options selected with_selected multiple with_options
               ]
             ).freeze,
             'field' => (
-              COMMON_OPTIONS + %w[
+              CssSelector::COMMON_OPTIONS + %w[
                 checked unchecked disabled valid name placeholder
                 validation_message readonly with type multiple
               ]
@@ -77,12 +74,13 @@ class SpecificMatcher < Base
           PATTERN
 
           def on_send(node)
-            return unless (arg = first_argument(node))
-            return unless (matcher = specific_matcher(arg))
-            return if acceptable_pattern?(arg)
-            return unless specific_matcher_option?(node, arg, matcher)
+            first_argument(node) do |arg|
+              next unless (matcher = specific_matcher(arg))
+              next if CssSelector.multiple_selectors?(arg)
+              next unless specific_matcher_option?(node, arg, matcher)
 
-            add_offense(node, message: message(node, matcher))
+              add_offense(node, message: message(node, matcher))
+            end
           end
 
           private
@@ -97,26 +95,24 @@ def acceptable_pattern?(arg)
           end
 
           def specific_matcher_option?(node, arg, matcher)
-            # If `button[foo-bar_baz=foo][bar][baz]`:
-            # extract ["foo-bar_baz", "bar", "baz"]
-            attributes = arg.scan(/\[(.*?)(?:=.*?)?\]/).flatten
-            return true if attributes.empty?
-            return false unless replaceable_matcher?(node, matcher, attributes)
+            attrs = CssSelector.attributes(arg).keys
+            return true if attrs.empty?
+            return false unless replaceable_matcher?(node, matcher, attrs)
 
-            attributes.all? do |attr|
+            attrs.all? do |attr|
               SPECIFIC_MATCHER_OPTIONS.fetch(matcher, []).include?(attr)
             end
           end
 
-          def replaceable_matcher?(node, matcher, attributes)
+          def replaceable_matcher?(node, matcher, attrs)
             case matcher
-            when 'link' then replaceable_to_have_link?(node, attributes)
+            when 'link' then replaceable_to_have_link?(node, attrs)
             else true
             end
           end
 
-          def replaceable_to_have_link?(node, attributes)
-            option?(node, :href) || attributes.include?('href')
+          def replaceable_to_have_link?(node, attrs)
+            option?(node, :href) || attrs.include?('href')
           end
 
           def message(node, matcher)

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

@@ -0,0 +1,85 @@
+# frozen_string_literal: true
+
+module RuboCop
+  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
+
+        module_function
+
+        # @param selector [String]
+        # @return [Boolean]
+        # @example
+        #   id?('#some-id') # => true
+        #   id?('.some-class') # => false
+        def id?(selector)
+          selector.start_with?('#')
+        end
+
+        # @param selector [String]
+        # @return [Boolean]
+        # @example
+        #   attribute?('[attribute]') # => true
+        #   attribute?('attribute') # => false
+        def attribute?(selector)
+          selector.start_with?('[')
+        end
+
+        # @param selector [String]
+        # @return [Array<String>]
+        # @example
+        #   attributes('a[foo-bar_baz]') # => {"foo-bar_baz=>true}
+        #   attributes('button[foo][bar]') # => {"foo"=>true, "bar"=>true}
+        #   attributes('table[foo=bar]') # => {"foo"=>"'bar'"}
+        def attributes(selector)
+          selector.scan(/\[(.*?)\]/).flatten.to_h do |attr|
+            key, value = attr.split('=')
+            [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 [Boolean]
+        # @example
+        #   multiple_selectors?('a.cls b#id') # => true
+        #   multiple_selectors?('a.cls') # => false
+        def multiple_selectors?(selector)
+          selector.match?(/[ >,+]/)
+        end
+
+        # @param selector [String]
+        # @return [Boolean, String]
+        # @example
+        #   normalize_value('true') # => true
+        #   normalize_value('false') # => false
+        #   normalize_value(nil) # => false
+        #   normalize_value("foo") # => "'foo'"
+        def normalize_value(value)
+          case value
+          when 'true' then true
+          when 'false' then false
+          when nil then true
+          else "'#{value}'"
+          end
+        end
+      end
+    end
+  end
+end

lib/rubocop/cop/rspec_cops.rb

@@ -2,6 +2,7 @@
 
 require_relative 'rspec/capybara/current_path_expectation'
 require_relative 'rspec/capybara/feature_methods'
+require_relative 'rspec/capybara/specific_finders'
 require_relative 'rspec/capybara/specific_matcher'
 require_relative 'rspec/capybara/visibility_matcher'