Merge pull request #784 from pirj/fix-pending-spec

Improve `RSpec/DescribedClass` cop

CHANGELOG.md

@@ -9,6 +9,8 @@
 * Change message of `RSpec/LetSetup` cop to be more descriptive. ([@foton][])
 * Improve `RSpec/ExampleWording` to handle interpolated example messages. ([@nc-holodakg][])
 * Improve detection by allowing the use of `RSpec` as a top-level constant. ([@pirj][])
+* Fix `RSpec/DescribedClass`'s incorrect detection. ([@pirj][])
+* Improve `RSpec/DescribedClass`'s ability to detect inside modules and classes. ([@pirj][])
 
 ## 1.33.0 (2019-05-13)
 

lib/rubocop/cop/rspec/described_class.rb

@@ -33,7 +33,6 @@ module RSpec
       #   end
       #
       class DescribedClass < Cop
-        include RuboCop::RSpec::TopLevelDescribe
         include ConfigurableEnforcedStyle
 
         DESCRIBED_CLASS = 'described_class'
@@ -48,19 +47,19 @@ class DescribedClass < Cop
 
         def_node_matcher :scope_changing_syntax?, '{def class module}'
 
+        def_node_matcher :described_constant, <<-PATTERN
+          (block (send _ :describe $(const ...) ...) (args) $_)
+        PATTERN
+
         def on_block(node)
-          # In case the explicit style is used, we needs to remember what's
-          # being described. Thus, we use an ivar for @described_class.
-          describe, @described_class, body = described_constant(node)
+          # In case the explicit style is used, we need to remember what's
+          # being described.
+          @described_class, body = described_constant(node)
 
-          return if body.nil?
-          return unless top_level_describe?(describe)
+          return unless body
 
           find_usage(body) do |match|
-            add_offense(
-              match,
-              message: message(match.const_name)
-            )
+            add_offense(match, message: message(match.const_name))
           end
         end
 
@@ -107,16 +106,84 @@ def skippable_block?(node)
         end
 
         def skip_blocks?
-          cop_config['SkipBlocks'].equal?(true)
+          cop_config['SkipBlocks']
         end
 
         def offensive?(node)
           if style == :described_class
-            node.eql?(@described_class)
+            offensive_described_class?(node)
           else
             node.send_type? && node.method_name == :described_class
           end
         end
+
+        def offensive_described_class?(node)
+          return unless node.const_type?
+
+          nearest_described_class, = node.each_ancestor(:block)
+            .map { |ancestor| described_constant(ancestor) }.find(&:itself)
+
+          return if nearest_described_class.equal?(node)
+
+          full_const_name(nearest_described_class) == full_const_name(node)
+        end
+
+        def full_const_name(node)
+          collapse_namespace(namespace(node), const_name(node))
+        end
+
+        # @param namespace [Array<Symbol>]
+        # @param const [Array<Symbol>]
+        # @return [Array<Symbol>]
+        # @example
+        #   # nil represents base constant
+        #   collapse_namespace([], :C)                 # => [:C]
+        #   collapse_namespace([:A, :B], [:C)          # => [:A, :B, :C]
+        #   collapse_namespace([:A, :B], [:B, :C)      # => [:A, :B, :C]
+        #   collapse_namespace([:A, :B], [nil, :C)     # => [nil, :C]
+        #   collapse_namespace([:A, :B], [nil, :B, :C) # => [nil, :B, :C]
+        def collapse_namespace(namespace, const)
+          return const if namespace.empty?
+          return const if const.first.nil?
+
+          start = [0, (namespace.length - const.length)].max
+          max = namespace.length
+          intersection = (start..max).find do |shift|
+            namespace[shift, max - shift] == const[0, max - shift]
+          end
+          [*namespace[0, intersection], *const]
+        end
+
+        # @param node [RuboCop::AST::Node]
+        # @return [Array<Symbol>]
+        # @example
+        #   const_name(s(:const, nil, :C))                # => [:C]
+        #   const_name(s(:const, s(:const, nil, :M), :C)) # => [:M, :C]
+        #   const_name(s(:const, s(:cbase), :C))          # => [nil, :C]
+        def const_name(node)
+          # rubocop:disable InternalAffairs/NodeDestructuring
+          namespace, name = *node
+          # rubocop:enable InternalAffairs/NodeDestructuring
+          if !namespace
+            [name]
+          elsif namespace.cbase_type?
+            [nil, name]
+          else
+            [*const_name(namespace), name]
+          end
+        end
+
+        # @param node [RuboCop::AST::Node]
+        # @return [Array<Symbol>]
+        # @example
+        #   namespace(node) # => [:A, :B, :C]
+        def namespace(node)
+          node
+            .each_ancestor(:class, :module)
+            .reverse_each
+            .flat_map { |ancestor| ancestor.defined_module_name.split('::') }
+            .map(&:to_sym)
+        end
       end
     end
   end

lib/rubocop/cop/rspec/let_setup.rb

@@ -26,8 +26,6 @@ module RSpec
       #     expect(Widget.count).to eq(1)
       #   end
       class LetSetup < Cop
-        include RuboCop::RSpec::TopLevelDescribe
-
         MSG = 'Do not use `let!` to setup objects not referenced in tests.'
 
         def_node_search :let_bang, <<-PATTERN

lib/rubocop/cop/rspec/subject_stub.rb

@@ -20,8 +20,6 @@ module RSpec
       #   end
       #
       class SubjectStub < Cop
-        include RuboCop::RSpec::TopLevelDescribe
-
         MSG = 'Do not stub methods of the object under test.'
 
         # @!method subject(node)

lib/rubocop/rspec/top_level_describe.rb

@@ -6,10 +6,6 @@ module RSpec
     module TopLevelDescribe
       extend NodePattern::Macros
 
-      def_node_matcher :described_constant, <<-PATTERN
-        (block $(send _ :describe $(const ...)) (args) $_)
-      PATTERN
-
       def on_send(node)
         return unless respond_to?(:on_top_level_describe)
         return unless top_level_describe?(node)