diff --git a/CHANGELOG.md b/CHANGELOG.md index 45af4591a..49280c6d8 100644 --- a/CHANGELOG.md +++ b/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) diff --git a/lib/rubocop/cop/rspec/described_class.rb b/lib/rubocop/cop/rspec/described_class.rb index 2d0fdb579..712d0d314 100644 --- a/lib/rubocop/cop/rspec/described_class.rb +++ b/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] + # @param const [Array] + # @return [Array] + # @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] + # @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] + # @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 diff --git a/lib/rubocop/cop/rspec/let_setup.rb b/lib/rubocop/cop/rspec/let_setup.rb index 97da006ca..b98a67551 100644 --- a/lib/rubocop/cop/rspec/let_setup.rb +++ b/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 diff --git a/lib/rubocop/cop/rspec/subject_stub.rb b/lib/rubocop/cop/rspec/subject_stub.rb index f25d27032..cb9c5c55d 100644 --- a/lib/rubocop/cop/rspec/subject_stub.rb +++ b/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) diff --git a/lib/rubocop/rspec/top_level_describe.rb b/lib/rubocop/rspec/top_level_describe.rb index 041693d87..4502ae27d 100644 --- a/lib/rubocop/rspec/top_level_describe.rb +++ b/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) diff --git a/spec/rubocop/cop/rspec/described_class_spec.rb b/spec/rubocop/cop/rspec/described_class_spec.rb index 506358def..92a46e47d 100644 --- a/spec/rubocop/cop/rspec/described_class_spec.rb +++ b/spec/rubocop/cop/rspec/described_class_spec.rb @@ -3,12 +3,12 @@ RSpec.describe RuboCop::Cop::RSpec::DescribedClass, :config do subject(:cop) { described_class.new(config) } - let(:cop_config) do - { 'EnforcedStyle' => enforced_style } - end + let(:cop_config) { {} } + + context 'when SkipBlocks is `true`' do + let(:cop_config) { { 'SkipBlocks' => true } } - shared_examples 'SkipBlocks enabled' do - it 'does not flag violations within non-rspec blocks' do + it 'ignores violations within non-rspec blocks' do expect_offense(<<-RUBY) describe MyClass do controller(ApplicationController) do @@ -28,7 +28,7 @@ end end - shared_examples 'SkipBlocks disabled' do + context 'when SkipBlocks is `false`' do it 'flags violations within all blocks' do expect_offense(<<-RUBY) describe MyClass do @@ -37,7 +37,7 @@ ^^^^^^^ Use `described_class` instead of `MyClass`. end - before(:each) do + before do MyClass ^^^^^^^ Use `described_class` instead of `MyClass`. @@ -51,34 +51,10 @@ end end - context 'when SkipBlocks is `true`' do - let(:cop_config) { { 'SkipBlocks' => true } } - - include_examples 'SkipBlocks enabled' - end - - context 'when SkipBlocks anything besides `true`' do - let(:cop_config) { { 'SkipBlocks' => 'yes' } } - - include_examples 'SkipBlocks disabled' - end - - context 'when SkipBlocks is not set' do - let(:cop_config) { {} } - - include_examples 'SkipBlocks disabled' - end - - context 'when SkipBlocks is `false`' do - let(:cop_config) { { 'SkipBlocks' => false } } - - include_examples 'SkipBlocks disabled' - end - context 'when EnforcedStyle is :described_class' do - let(:enforced_style) { :described_class } + let(:cop_config) { { 'EnforcedStyle' => :described_class } } - it 'checks for the use of the described class' do + it 'flags for the use of the described class' do expect_offense(<<-RUBY) describe MyClass do include MyClass @@ -91,6 +67,25 @@ ^^^^^^^ Use `described_class` instead of `MyClass`. end RUBY + + expect_correction(<<-RUBY) + describe MyClass do + include described_class + + subject { described_class.do_something } + + before { described_class.do_something } + end + RUBY + end + + it 'flags with metadata' do + expect_offense(<<-RUBY) + describe MyClass, some: :metadata do + subject { MyClass } + ^^^^^^^ Use `described_class` instead of `MyClass`. + end + RUBY end it 'ignores described class as string' do @@ -131,14 +126,14 @@ module MyModule RUBY end - it 'only takes class from top level describes' do + it 'takes class from innermost describe' do expect_offense(<<-RUBY) describe MyClass do describe MyClass::Foo do subject { MyClass::Foo } + ^^^^^^^^^^^^ Use `described_class` instead of `MyClass::Foo`. let(:foo) { MyClass } - ^^^^^^^ Use `described_class` instead of `MyClass`. end end RUBY @@ -152,7 +147,7 @@ module MyModule RUBY end - it 'ignores if namespace is not matching' do + it 'ignores non-matching namespace defined on `describe` level' do expect_no_offenses(<<-RUBY) describe MyNamespace::MyClass do subject { ::MyClass } @@ -161,7 +156,17 @@ module MyModule RUBY end - it 'checks for the use of described class with namespace' do + it 'ignores non-matching namespace' do + expect_no_offenses(<<-RUBY) + module MyNamespace + describe MyClass do + subject { ::MyClass } + end + end + RUBY + end + + it 'flags the use of described class with namespace' do expect_offense(<<-RUBY) describe MyNamespace::MyClass do subject { MyNamespace::MyClass } @@ -170,7 +175,17 @@ module MyModule RUBY end - it 'does not flag violations within a class scope change' do + it 'ignores non-matching namespace in usages' do + expect_no_offenses(<<-RUBY) + module UnrelatedNamespace + describe MyClass do + subject { MyNamespace::MyClass } + end + end + RUBY + end + + it 'ignores violations within a class scope change' do expect_no_offenses(<<-RUBY) describe MyNamespace::MyClass do before do @@ -182,7 +197,7 @@ class Foo RUBY end - it 'does not flag violations within a hook scope change' do + it 'ignores violations within a hook scope change' do expect_no_offenses(<<-RUBY) describe do before do @@ -192,17 +207,46 @@ class Foo RUBY end - it 'checks for the use of described class with module' do - pending - + it 'flags the use of described class with module' do expect_offense(<<-RUBY) module MyNamespace describe MyClass do subject { MyNamespace::MyClass } - ^^^^^^^^^^^^^^^^^^^^ Use `described_class` instead of `MyNamespace::MyClass` + ^^^^^^^^^^^^^^^^^^^^ Use `described_class` instead of `MyNamespace::MyClass`. end end RUBY + + expect_correction(<<-RUBY) + module MyNamespace + describe MyClass do + subject { described_class } + end + end + RUBY + end + + it 'flags the use of described class with nested namespace' do + expect_offense(<<-RUBY) + module A + class B::C + module D + describe E do + subject { A::B::C::D::E } + ^^^^^^^^^^^^^ Use `described_class` instead of `A::B::C::D::E`. + let(:one) { B::C::D::E } + ^^^^^^^^^^ Use `described_class` instead of `B::C::D::E`. + let(:two) { C::D::E } + ^^^^^^^ Use `described_class` instead of `C::D::E`. + let(:six) { D::E } + ^^^^ Use `described_class` instead of `D::E`. + let(:ten) { E } + ^ Use `described_class` instead of `E`. + end + end + end + end + RUBY end it 'accepts an empty block' do @@ -211,24 +255,12 @@ module MyNamespace end RUBY end - - include_examples 'autocorrect', - 'describe(Foo) { include Foo }', - 'describe(Foo) { include described_class }' - - include_examples 'autocorrect', - 'describe(Foo) { subject { Foo.do_action } }', - 'describe(Foo) { subject { described_class.do_action } }' - - include_examples 'autocorrect', - 'describe(Foo) { before { Foo.do_action } }', - 'describe(Foo) { before { described_class.do_action } }' end context 'when EnforcedStyle is :explicit' do - let(:enforced_style) { :explicit } + let(:cop_config) { { 'EnforcedStyle' => :explicit } } - it 'checks for the use of the described_class' do + it 'flags the use of the described_class' do expect_offense(<<-RUBY) describe MyClass do include described_class @@ -241,6 +273,16 @@ module MyNamespace ^^^^^^^^^^^^^^^ Use `MyClass` instead of `described_class`. end RUBY + + expect_correction(<<-RUBY) + describe MyClass do + include MyClass + + subject { MyClass.do_something } + + before { MyClass.do_something } + end + RUBY end it 'ignores described_class as string' do @@ -259,7 +301,7 @@ module MyNamespace RUBY end - it 'does not flag violations within a class scope change' do + it 'ignores violations within a class scope change' do expect_no_offenses(<<-RUBY) describe MyNamespace::MyClass do before do @@ -271,7 +313,7 @@ class Foo RUBY end - it 'does not flag violations within a hook scope change' do + it 'ignores violations within a hook scope change' do expect_no_offenses(<<-RUBY) describe do before do @@ -281,27 +323,18 @@ class Foo RUBY end - include_examples 'autocorrect', - 'describe(Foo) { include described_class }', - 'describe(Foo) { include Foo }' - - include_examples 'autocorrect', - 'describe(Foo) { subject { described_class.do_action } }', - 'describe(Foo) { subject { Foo.do_action } }' - - include_examples 'autocorrect', - 'describe(Foo) { before { described_class.do_action } }', - 'describe(Foo) { before { Foo.do_action } }' - - original = <<-RUBY - describe(Foo) { include described_class } - describe(Bar) { include described_class } - RUBY - corrected = <<-RUBY - describe(Foo) { include Foo } - describe(Bar) { include Bar } - RUBY + it 'autocorrects corresponding' do + expect_offense(<<-RUBY) + describe(Foo) { include described_class } + ^^^^^^^^^^^^^^^ Use `Foo` instead of `described_class`. + describe(Bar) { include described_class } + ^^^^^^^^^^^^^^^ Use `Bar` instead of `described_class`. + RUBY - include_examples 'autocorrect', original, corrected + expect_correction(<<-RUBY) + describe(Foo) { include Foo } + describe(Bar) { include Bar } + RUBY + end end end