diff --git a/CHANGELOG.md b/CHANGELOG.md index 01a492fd3..3dba3633b 100644 --- a/CHANGELOG.md +++ b/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) diff --git a/config/default.yml b/config/default.yml index bd689553f..bd3b93826 100644 --- a/config/default.yml +++ b/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 diff --git a/lib/rubocop/cop/rspec/describe_class.rb b/lib/rubocop/cop/rspec/describe_class.rb index c8057083d..2f0bc17e3 100644 --- a/lib/rubocop/cop/rspec/describe_class.rb +++ b/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 diff --git a/lib/rubocop/cop/rspec/describe_method.rb b/lib/rubocop/cop/rspec/describe_method.rb index 2e8c7dcaa..cfc1e8148 100644 --- a/lib/rubocop/cop/rspec/describe_method.rb +++ b/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 diff --git a/lib/rubocop/cop/rspec/file_path.rb b/lib/rubocop/cop/rspec/file_path.rb index 1297d0839..c67c73faf 100644 --- a/lib/rubocop/cop/rspec/file_path.rb +++ b/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 `%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) diff --git a/lib/rubocop/cop/rspec/multiple_describes.rb b/lib/rubocop/cop/rspec/multiple_describes.rb index c73a20bcf..9ca9fb9ae 100644 --- a/lib/rubocop/cop/rspec/multiple_describes.rb +++ b/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 diff --git a/lib/rubocop/cop/rspec/nested_groups.rb b/lib/rubocop/cop/rspec/nested_groups.rb index 6f2d83f86..44301d789 100644 --- a/lib/rubocop/cop/rspec/nested_groups.rb +++ b/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 [%d/%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, diff --git a/lib/rubocop/rspec/top_level_group.rb b/lib/rubocop/rspec/top_level_group.rb index 356b4f8b2..7f7bb04a1 100644 --- a/lib/rubocop/rspec/top_level_group.rb +++ b/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 diff --git a/manual/cops_rspec.md b/manual/cops_rspec.md index 28fa844a3..e4b798a89 100644 --- a/manual/cops_rspec.md +++ b/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. diff --git a/spec/rubocop/cop/rspec/expect_output_spec.rb b/spec/rubocop/cop/rspec/expect_output_spec.rb index 20308e028..90ca23167 100644 --- a/spec/rubocop/cop/rspec/expect_output_spec.rb +++ b/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 } diff --git a/spec/rubocop/cop/rspec/file_path_spec.rb b/spec/rubocop/cop/rspec/file_path_spec.rb index 833075f52..6ec5b36e1 100644 --- a/spec/rubocop/cop/rspec/file_path_spec.rb +++ b/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 diff --git a/spec/rubocop/cop/rspec/multiple_describes_spec.rb b/spec/rubocop/cop/rspec/multiple_describes_spec.rb index 68376e697..4fd2df231 100644 --- a/spec/rubocop/cop/rspec/multiple_describes_spec.rb +++ b/spec/rubocop/cop/rspec/multiple_describes_spec.rb @@ -3,26 +3,51 @@ RSpec.describe RuboCop::Cop::RSpec::MultipleDescribes do subject(:cop) { described_class.new } - it 'finds multiple top level describes with class and method' do + it 'flags multiple top-level example groups with class and method' do expect_offense(<<-RUBY) describe MyClass, '.do_something' do; end - ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Do not use multiple top level describes - try to nest them. + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Do not use multiple top-level example groups - try to nest them. describe MyClass, '.do_something_else' do; end RUBY end - it 'finds multiple top level describes only with class' do + it 'flags multiple top-level example groups only with class' do expect_offense(<<-RUBY) describe MyClass do; end - ^^^^^^^^^^^^^^^^ Do not use multiple top level describes - try to nest them. + ^^^^^^^^^^^^^^^^ Do not use multiple top-level example groups - try to nest them. describe MyOtherClass do; end RUBY end - it 'skips single top level describe' do + it 'flags multiple top-level example groups with an arbitrary argument' do + expect_offense(<<-RUBY) + describe 'MyClass' do; end + ^^^^^^^^^^^^^^^^^^ Do not use multiple top-level example groups - try to nest them. + describe 'MyOtherClass' do; end + RUBY + end + + it 'flags multiple top-level example groups aliases' do + expect_offense(<<-RUBY) + example_group MyClass do; end + ^^^^^^^^^^^^^^^^^^^^^ Do not use multiple top-level example groups - try to nest them. + feature MyOtherClass do; end + RUBY + end + + it 'ignores single top-level example group' do expect_no_offenses(<<-RUBY) - require 'spec_helper' + describe MyClass do + end + RUBY + end + it 'ignores multiple shared example groups' do + expect_no_offenses(<<-RUBY) + shared_examples_for 'behaves' do + end + shared_examples_for 'misbehaves' do + end describe MyClass do end RUBY diff --git a/spec/rubocop/cop/rspec/nested_groups_spec.rb b/spec/rubocop/cop/rspec/nested_groups_spec.rb index 64640293f..98cfde802 100644 --- a/spec/rubocop/cop/rspec/nested_groups_spec.rb +++ b/spec/rubocop/cop/rspec/nested_groups_spec.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true RSpec.describe RuboCop::Cop::RSpec::NestedGroups, :config do - it 'flags nested contexts' do + it 'flags nested example groups defined inside `describe`' do expect_offense(<<-RUBY) describe MyClass do context 'when foo' do @@ -20,6 +20,36 @@ RUBY end + it 'flags nested example groups' do + expect_offense(<<-RUBY) + example_group MyClass do + context 'when foo' do + context 'when bar' do + context 'when baz' do + ^^^^^^^^^^^^^^^^^^ Maximum example group nesting exceeded [4/3]. + end + end + end + end + RUBY + end + + it 'flags nested example groups inside shared examples' do + expect_offense(<<-RUBY) + shared_examples_for 'nested like express' do + context 'when foo' do + context 'when bar' do + context 'when baz' do + context 'when qux' do + ^^^^^^^^^^^^^^^^^^ Maximum example group nesting exceeded [4/3]. + end + end + end + end + end + RUBY + end + it 'support --auto-gen-config' do inspect_source(<<-RUBY, 'spec/foo_spec.rb') describe MyClass do @@ -35,11 +65,32 @@ expect(cop.config_to_allow_offenses[:exclude_limit]).to eq('Max' => 4) end - it 'ignores non-spec context methods' do - expect_no_offenses(<<-RUBY) + it 'flags example groups wrapped in classes' do + expect_offense(<<-RUBY) class MyThingy - context 'this is not rspec' do - context 'but it uses contexts' do + describe MyClass do + context 'when foo' do + context 'when bar' do + context 'when baz' do + ^^^^^^^^^^^^^^^^^^ Maximum example group nesting exceeded [4/3]. + end + end + end + end + end + RUBY + end + + it 'flags example groups wrapped in modules' do + expect_offense(<<-RUBY) + module MyNamespace + describe MyClass do + context 'when foo' do + context 'when bar' do + context 'when baz' do + ^^^^^^^^^^^^^^^^^^ Maximum example group nesting exceeded [4/3]. + end + end end end end