Improve cops by using TopLevelGroup

CHANGELOG.md

@@ -8,6 +8,7 @@
 * Relax `RSpec/VariableDefinition` cop so interpolated and multiline strings are accepted even when configured to enforce the `symbol` style. ([@bquorning][])
 * Fix `RSpec/EmptyExampleGroup` to flag example groups with examples in invalid scopes. ([@mlarraz][])
 * Fix `RSpec/EmptyExampleGroup` to ignore examples groups with examples defined inside iterators. ([@pirj][])
+* Improve `RSpec/NestedGroups`, `RSpec/FilePath`, `RSpec/DescribeMethod`, `RSpec/MultipleDescribes`, `RSpec/DescribeClass`'s top-level example group detection. ([@pirj][])
 
 ## 1.42.0 (2020-07-09)
 

config/default.yml

@@ -74,7 +74,7 @@ RSpec/ContextWording:
   StyleGuide: https://www.rubydoc.info/gems/rubocop-rspec/RuboCop/Cop/RSpec/ContextWording
 
 RSpec/DescribeClass:
-  Description: Check that the first argument to the top level describe is a constant.
+  Description: Check that the first argument to the top-level describe is a constant.
   Enabled: true
   VersionAdded: '1.0'
   StyleGuide: https://www.rubydoc.info/gems/rubocop-rspec/RuboCop/Cop/RSpec/DescribeClass
@@ -381,7 +381,7 @@ RSpec/MissingExampleGroupArgument:
   StyleGuide: https://www.rubydoc.info/gems/rubocop-rspec/RuboCop/Cop/RSpec/MissingExampleGroupArgument
 
 RSpec/MultipleDescribes:
-  Description: Checks for multiple top level describes.
+  Description: Checks for multiple top-level example groups.
   Enabled: true
   VersionAdded: '1.0'
   StyleGuide: https://www.rubydoc.info/gems/rubocop-rspec/RuboCop/Cop/RSpec/MultipleDescribes

lib/rubocop/cop/rspec/describe_class.rb

@@ -3,7 +3,7 @@
 module RuboCop
   module Cop
     module RSpec
-      # Check that the first argument to the top level describe is a constant.
+      # Check that the first argument to the top-level describe is a constant.
       #
       # @example
       #   # bad
@@ -22,49 +22,40 @@ module RSpec
       #   describe "A feature example", type: :feature do
       #   end
       class DescribeClass < Base
-        include RuboCop::RSpec::TopLevelDescribe
+        include RuboCop::RSpec::TopLevelGroup
 
         MSG = 'The first argument to describe should be '\
               'the class or module being tested.'
 
-        def_node_matcher :valid_describe?, <<-PATTERN
-          {
-            (send #rspec? :describe const ...)
-            (send #rspec? :describe)
-          }
-        PATTERN
-
-        def_node_matcher :describe_with_rails_metadata?, <<-PATTERN
-          (send #rspec? :describe !const ...
-            (hash <#rails_metadata? ...>)
-          )
-        PATTERN
-
         def_node_matcher :rails_metadata?, <<-PATTERN
           (pair
             (sym :type)
-            (sym {
-                   :channel :controller :helper :job :mailer :model :request
-                   :routing :view :feature :system :mailbox
-                 }
-            )
+            (sym { :channel :controller :helper :job :mailer :model :request
+                   :routing :view :feature :system :mailbox })
           )
         PATTERN
 
-        def on_top_level_describe(node, (described_value, _))
-          return if shared_group?(root_node)
-          return if valid_describe?(node)
-          return if describe_with_rails_metadata?(node)
-          return if string_constant_describe?(described_value)
+        def_node_matcher :example_group_with_rails_metadata?, <<~PATTERN
+          (send #rspec? :describe ... (hash <#rails_metadata? ...>))
+        PATTERN
+
+        def_node_matcher :not_a_const_described, <<~PATTERN
+          (send #rspec? :describe $[!const !#string_constant?] ...)
+        PATTERN
+
+        def on_top_level_group(top_level_node)
+          return if example_group_with_rails_metadata?(top_level_node.send_node)
 
-          add_offense(described_value)
+          not_a_const_described(top_level_node.send_node) do |described|
+            add_offense(described)
+          end
         end
 
         private
 
-        def string_constant_describe?(described_value)
-          described_value.str_type? &&
-            described_value.value.match?(/^(?:(?:::)?[A-Z]\w*)+$/)
+        def string_constant?(described)
+          described.str_type? &&
+            described.value.match?(/^(?:(?:::)?[A-Z]\w*)+$/)
         end
       end
     end

lib/rubocop/cop/rspec/describe_method.rb

@@ -17,16 +17,24 @@ module RSpec
       #   describe MyClass, '.my_class_method' do
       #   end
       class DescribeMethod < Base
-        include RuboCop::RSpec::TopLevelDescribe
+        include RuboCop::RSpec::TopLevelGroup
 
         MSG = 'The second argument to describe should be the method '\
               "being tested. '#instance' or '.class'."
 
-        def on_top_level_describe(_node, (_, second_arg))
-          return unless second_arg&.str_type?
-          return if second_arg.str_content.start_with?('#', '.')
+        def_node_matcher :second_argument, <<~PATTERN
+          (block
+            (send #rspec? :describe _first_argument $(str _) ...) ...
+          )
+        PATTERN
 
-          add_offense(second_arg)
+        def on_top_level_group(node)
+          second_argument = second_argument(node)
+
+          return unless second_argument
+          return if second_argument.str_content.start_with?('#', '.')
+
+          add_offense(second_argument)
         end
       end
     end

lib/rubocop/cop/rspec/file_path.rb

@@ -57,34 +57,42 @@ module RSpec
       #   my_class_spec.rb         # describe MyClass, '#method'
       #
       class FilePath < Base
-        include RuboCop::RSpec::TopLevelDescribe
+        include RuboCop::RSpec::TopLevelGroup
 
         MSG = 'Spec path should end with `%<suffix>s`.'
 
-        def_node_search :const_described?,  '(send _ :describe (const ...) ...)'
+        def_node_matcher :const_described, <<~PATTERN
+          (block
+            $(send #rspec? _example_group $(const ...) $...) ...
+          )
+        PATTERN
+
         def_node_search :routing_metadata?, '(pair (sym :type) (sym :routing))'
 
-        def on_top_level_describe(node, args)
-          return unless const_described?(node) && single_top_level_describe?
-          return if routing_spec?(args)
+        def on_top_level_group(node)
+          return unless top_level_groups.one?
 
-          glob = glob_for(args)
+          const_described(node) do |send_node, described_class, arguments|
+            next if routing_spec?(arguments)
 
-          return if filename_ends_with?(glob)
-
-          add_offense(
-            node,
-            message: format(MSG, suffix: glob)
-          )
+            ensure_correct_file_path(send_node, described_class, arguments)
+          end
         end
 
         private
 
+        def ensure_correct_file_path(send_node, described_class, arguments)
+          glob = glob_for(described_class, arguments.first)
+          return if filename_ends_with?(glob)
+
+          add_offense(send_node, message: format(MSG, suffix: glob))
+        end
+
         def routing_spec?(args)
           args.any?(&method(:routing_metadata?))
         end
 
-        def glob_for((described_class, method_name))
+        def glob_for(described_class, method_name)
           return glob_for_spec_suffix_only? if spec_suffix_only?
 
           "#{expected_path(described_class)}#{name_glob(method_name)}*_spec.rb"
@@ -94,10 +102,10 @@ def glob_for_spec_suffix_only?
           '*_spec.rb'
         end
 
-        def name_glob(name)
-          return unless name&.str_type?
+        def name_glob(method_name)
+          return unless method_name&.str_type?
 
-          "*#{name.str_content.gsub(/\W/, '')}" unless ignore_methods?
+          "*#{method_name.str_content.gsub(/\W/, '')}" unless ignore_methods?
         end
 
         def expected_path(constant)

lib/rubocop/cop/rspec/multiple_describes.rb

@@ -3,7 +3,7 @@
 module RuboCop
   module Cop
     module RSpec
-      # Checks for multiple top level describes.
+      # Checks for multiple top-level example groups.
       #
       # Multiple descriptions for the same class or module should either
       # be nested or separated into different test files.
@@ -23,16 +23,19 @@ module RSpec
       #     end
       #   end
       class MultipleDescribes < Base
-        include RuboCop::RSpec::TopLevelDescribe
+        include RuboCop::RSpec::TopLevelGroup
 
-        MSG = 'Do not use multiple top level describes - '\
+        MSG = 'Do not use multiple top-level example groups - '\
               'try to nest them.'
 
-        def on_top_level_describe(node, _args)
-          return if single_top_level_describe?
-          return unless top_level_nodes.first.equal?(node)
+        def on_top_level_group(node)
+          top_level_example_groups =
+            top_level_groups.select(&method(:example_group?))
 
-          add_offense(node)
+          return if top_level_example_groups.one?
+          return unless top_level_example_groups.first.equal?(node)
+
+          add_offense(node.send_node)
         end
       end
     end

lib/rubocop/cop/rspec/nested_groups.rb

@@ -87,7 +87,7 @@ module RSpec
       #
       class NestedGroups < Base
         include ConfigurableMax
-        include RuboCop::RSpec::TopLevelDescribe
+        include RuboCop::RSpec::TopLevelGroup
 
         MSG = 'Maximum example group nesting exceeded [%<total>d/%<max>d].'
 
@@ -97,8 +97,8 @@ class NestedGroups < Base
           "Configuration key `#{DEPRECATED_MAX_KEY}` for #{cop_name} is " \
           'deprecated in favor of `Max`. Please use that instead.'
 
-        def on_top_level_describe(node, _args)
-          find_nested_example_groups(node.parent) do |example_group, nesting|
+        def on_top_level_group(node)
+          find_nested_example_groups(node) do |example_group, nesting|
             self.max = nesting
             add_offense(
               example_group.send_node,

lib/rubocop/rspec/top_level_group.rb

@@ -10,29 +10,40 @@ module TopLevelGroup
       def_node_matcher :example_or_shared_group?,
                        (ExampleGroups::ALL + SharedGroups::ALL).block_pattern
 
-      def on_block(node)
-        return unless respond_to?(:on_top_level_group)
-        return unless top_level_group?(node)
+      def on_new_investigation
+        super
 
-        on_top_level_group(node)
+        return unless root_node
+
+        top_level_groups.each do |node|
+          example_group?(node, &method(:on_top_level_example_group))
+          on_top_level_group(node)
+        end
+      end
+
+      def top_level_groups
+        @top_level_groups ||=
+          top_level_nodes(root_node).select { |n| example_or_shared_group?(n) }
       end
 
       private
 
+      # Dummy methods to be overridden in the consumer
+      def on_top_level_example_group; end
+
+      def on_top_level_group; end
+
       def top_level_group?(node)
         top_level_groups.include?(node)
       end
 
-      def top_level_groups
-        @top_level_groups ||=
-          top_level_nodes.select { |n| example_or_shared_group?(n) }
-      end
-
-      def top_level_nodes
-        if root_node.begin_type?
-          root_node.children
+      def top_level_nodes(node)
+        if node.begin_type?
+          node.children
+        elsif node.module_type? || node.class_type?
+          top_level_nodes(node.body)
         else
-          [root_node]
+          [node]
         end
       end
 

manual/cops_rspec.md

@@ -322,7 +322,7 @@ Enabled by default | Safe | Supports autocorrection | VersionAdded | VersionChan
 --- | --- | --- | --- | ---
 Enabled | Yes | No | 1.0 | -
 
-Check that the first argument to the top level describe is a constant.
+Check that the first argument to the top-level describe is a constant.
 
 ### Examples
 
@@ -2033,7 +2033,7 @@ Enabled by default | Safe | Supports autocorrection | VersionAdded | VersionChan
 --- | --- | --- | --- | ---
 Enabled | Yes | No | 1.0 | -
 
-Checks for multiple top level describes.
+Checks for multiple top-level example groups.
 
 Multiple descriptions for the same class or module should either
 be nested or separated into different test files.

spec/rubocop/cop/rspec/expect_output_spec.rb

@@ -1,7 +1,5 @@
 # frozen_string_literal: true
 
-require 'spec_helper'
-
 RSpec.describe RuboCop::Cop::RSpec::ExpectOutput do
   subject(:cop) { described_class.new }
 

spec/rubocop/cop/rspec/file_path_spec.rb

@@ -8,6 +8,13 @@
     RUBY
   end
 
+  it 'registers an offense for a bad path for all kinds of example groups' do
+    expect_offense(<<-RUBY, 'wrong_path_foo_spec.rb')
+      example_group MyClass, 'foo' do; end
+      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Spec path should end with `my_class*foo*_spec.rb`.
+    RUBY
+  end
+
   it 'registers an offense for a wrong class but a correct method' do
     expect_offense(<<-RUBY, 'wrong_class_foo_spec.rb')
       describe MyClass, '#foo' do; end