diff --git a/CHANGELOG.md b/CHANGELOG.md index 36833bd22..bb80c9b79 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,7 @@ * Improve `RSpec/NestedGroups`, `RSpec/FilePath`, `RSpec/DescribeMethod`, `RSpec/MultipleDescribes`, `RSpec/DescribeClass`'s top-level example group detection. ([@pirj][]) * Add detection of `let!` with a block-pass or a string literal to `RSpec/LetSetup`. ([@pirj][]) * Add `IgnoredPatterns` configuration option to `RSpec/VariableName`. ([@jtannas][]) +* Add `RSpec/MultipleMemoizedHelpers` cop. ([@mockdeep][]) ## 1.42.0 (2020-07-09) @@ -542,3 +543,4 @@ Compatibility release so users can upgrade RuboCop to 0.51.0. No new features. [@elliterate]: https://github.com/elliterate [@mlarraz]: https://github.com/mlarraz [@jtannas]: https://github.com/jtannas +[@mockdeep]: https://github.com/mockdeep diff --git a/config/default.yml b/config/default.yml index c59d31955..946f854f8 100644 --- a/config/default.yml +++ b/config/default.yml @@ -394,6 +394,13 @@ RSpec/MultipleExpectations: VersionChanged: '1.21' StyleGuide: https://www.rubydoc.info/gems/rubocop-rspec/RuboCop/Cop/RSpec/MultipleExpectations +RSpec/MultipleMemoizedHelpers: + Description: Checks if example groups contain too many `let` and `subject` calls. + Enabled: true + AllowSubject: true + Max: 5 + StyleGuide: https://www.rubydoc.info/gems/rubocop-rspec/RuboCop/Cop/RSpec/MultipleMemoizedHelpers + RSpec/MultipleSubjects: Description: Checks if an example group defines `subject` multiple times. Enabled: true diff --git a/lib/rubocop/cop/rspec/multiple_memoized_helpers.rb b/lib/rubocop/cop/rspec/multiple_memoized_helpers.rb new file mode 100644 index 000000000..4ad7a0cf2 --- /dev/null +++ b/lib/rubocop/cop/rspec/multiple_memoized_helpers.rb @@ -0,0 +1,148 @@ +# frozen_string_literal: true + +module RuboCop + module Cop + module RSpec + # Checks if example groups contain too many `let` and `subject` calls. + # + # This cop is configurable using the `Max` option and the `AllowSubject` + # which will configure the cop to only register offenses on calls to + # `let` and not calls to `subject`. + # + # @example + # # bad + # describe MyClass do + # let(:foo) { [] } + # let(:bar) { [] } + # let!(:baz) { [] } + # let(:qux) { [] } + # let(:quux) { [] } + # let(:quuz) { {} } + # end + # + # describe MyClass do + # let(:foo) { [] } + # let(:bar) { [] } + # let!(:baz) { [] } + # + # context 'when stuff' do + # let(:qux) { [] } + # let(:quux) { [] } + # let(:quuz) { {} } + # end + # end + # + # # good + # describe MyClass do + # let(:bar) { [] } + # let!(:baz) { [] } + # let(:qux) { [] } + # let(:quux) { [] } + # let(:quuz) { {} } + # end + # + # describe MyClass do + # context 'when stuff' do + # let(:foo) { [] } + # let(:bar) { [] } + # let!(:booger) { [] } + # end + # + # context 'when other stuff' do + # let(:qux) { [] } + # let(:quux) { [] } + # let(:quuz) { {} } + # end + # end + # + # @example when disabling AllowSubject configuration + # + # # rubocop.yml + # # RSpec/MultipleMemoizedHelpers: + # # AllowSubject: false + # + # # bad - `subject` counts towards memoized helpers + # describe MyClass do + # subject { {} } + # let(:foo) { [] } + # let(:bar) { [] } + # let!(:baz) { [] } + # let(:qux) { [] } + # let(:quux) { [] } + # end + # + # @example with Max configuration + # + # # rubocop.yml + # # RSpec/MultipleMemoizedHelpers: + # # Max: 1 + # + # # bad + # describe MyClass do + # let(:foo) { [] } + # let(:bar) { [] } + # end + # + class MultipleMemoizedHelpers < Base + include ConfigurableMax + include RuboCop::RSpec::Variable + + MSG = 'Example group has too many memoized helpers [%d/%d]' + + def on_block(node) + return unless spec_group?(node) + + count = all_helpers(node).uniq.count + + return if count <= max + + self.max = count + add_offense(node, message: format(MSG, count: count, max: max)) + end + + def on_new_investigation + @example_group_memoized_helpers = {} + end + + private + + attr_reader :example_group_memoized_helpers + + def all_helpers(node) + [ + *helpers(node), + *node.each_ancestor(:block).flat_map(&method(:helpers)) + ] + end + + def helpers(node) + @example_group_memoized_helpers[node] ||= + variable_nodes(node).map do |variable_node| + if variable_node.block_type? + variable_definition?(variable_node.send_node) + else # block-pass (`let(:foo, &bar)`) + variable_definition?(variable_node) + end + end + end + + def variable_nodes(node) + example_group = RuboCop::RSpec::ExampleGroup.new(node) + if allow_subject? + example_group.lets + else + example_group.lets + example_group.subjects + end + end + + def max + cop_config['Max'] + end + + def allow_subject? + cop_config['AllowSubject'] + end + end + end + end +end diff --git a/lib/rubocop/cop/rspec_cops.rb b/lib/rubocop/cop/rspec_cops.rb index 4fe56e997..b835bf3d6 100644 --- a/lib/rubocop/cop/rspec_cops.rb +++ b/lib/rubocop/cop/rspec_cops.rb @@ -65,6 +65,7 @@ require_relative 'rspec/missing_example_group_argument' require_relative 'rspec/multiple_describes' require_relative 'rspec/multiple_expectations' +require_relative 'rspec/multiple_memoized_helpers' require_relative 'rspec/multiple_subjects' require_relative 'rspec/named_subject' require_relative 'rspec/nested_groups' diff --git a/lib/rubocop/rspec/variable.rb b/lib/rubocop/rspec/variable.rb index 2dfe92c0c..887f20ca7 100644 --- a/lib/rubocop/rspec/variable.rb +++ b/lib/rubocop/rspec/variable.rb @@ -8,7 +8,7 @@ module Variable extend RuboCop::NodePattern::Macros def_node_matcher :variable_definition?, <<~PATTERN - (send #rspec? #{(Helpers::ALL + Subject::ALL).node_pattern_union} + (send nil? #{(Helpers::ALL + Subject::ALL).node_pattern_union} $({sym str dsym dstr} ...) ...) PATTERN end diff --git a/manual/cops.md b/manual/cops.md index 1e191ee0e..75a49daa3 100644 --- a/manual/cops.md +++ b/manual/cops.md @@ -64,6 +64,7 @@ * [RSpec/MissingExampleGroupArgument](cops_rspec.md#rspecmissingexamplegroupargument) * [RSpec/MultipleDescribes](cops_rspec.md#rspecmultipledescribes) * [RSpec/MultipleExpectations](cops_rspec.md#rspecmultipleexpectations) +* [RSpec/MultipleMemoizedHelpers](cops_rspec.md#rspecmultiplememoizedhelpers) * [RSpec/MultipleSubjects](cops_rspec.md#rspecmultiplesubjects) * [RSpec/NamedSubject](cops_rspec.md#rspecnamedsubject) * [RSpec/NestedGroups](cops_rspec.md#rspecnestedgroups) diff --git a/manual/cops_rspec.md b/manual/cops_rspec.md index 5ca0c1e12..4d2a14dc2 100644 --- a/manual/cops_rspec.md +++ b/manual/cops_rspec.md @@ -2119,6 +2119,108 @@ Max | `1` | Integer * [https://www.rubydoc.info/gems/rubocop-rspec/RuboCop/Cop/RSpec/MultipleExpectations](https://www.rubydoc.info/gems/rubocop-rspec/RuboCop/Cop/RSpec/MultipleExpectations) +## RSpec/MultipleMemoizedHelpers + +Enabled by default | Safe | Supports autocorrection | VersionAdded | VersionChanged +--- | --- | --- | --- | --- +Enabled | Yes | No | - | - + +Checks if example groups contain too many `let` and `subject` calls. + +This cop is configurable using the `Max` option and the `AllowSubject` +which will configure the cop to only register offenses on calls to +`let` and not calls to `subject`. + +### Examples + +```ruby +# bad +describe MyClass do + let(:foo) { [] } + let(:bar) { [] } + let!(:baz) { [] } + let(:qux) { [] } + let(:quux) { [] } + let(:quuz) { {} } +end + +describe MyClass do + let(:foo) { [] } + let(:bar) { [] } + let!(:baz) { [] } + + context 'when stuff' do + let(:qux) { [] } + let(:quux) { [] } + let(:quuz) { {} } + end +end + +# good +describe MyClass do + let(:bar) { [] } + let!(:baz) { [] } + let(:qux) { [] } + let(:quux) { [] } + let(:quuz) { {} } +end + +describe MyClass do + context 'when stuff' do + let(:foo) { [] } + let(:bar) { [] } + let!(:booger) { [] } + end + + context 'when other stuff' do + let(:qux) { [] } + let(:quux) { [] } + let(:quuz) { {} } + end +end +``` +#### when disabling AllowSubject configuration + +```ruby +# rubocop.yml +# RSpec/MultipleMemoizedHelpers: +# AllowSubject: false + +# bad - `subject` counts towards memoized helpers +describe MyClass do + subject { {} } + let(:foo) { [] } + let(:bar) { [] } + let!(:baz) { [] } + let(:qux) { [] } + let(:quux) { [] } +end +``` +#### with Max configuration + +```ruby +# rubocop.yml +# RSpec/MultipleMemoizedHelpers: +# Max: 1 + +# bad +describe MyClass do + let(:foo) { [] } + let(:bar) { [] } +end +``` + +### Configurable attributes + +Name | Default value | Configurable values +--- | --- | --- +AllowSubject | `true` | Boolean +Max | `5` | Integer + +### References + +* [https://www.rubydoc.info/gems/rubocop-rspec/RuboCop/Cop/RSpec/MultipleMemoizedHelpers](https://www.rubydoc.info/gems/rubocop-rspec/RuboCop/Cop/RSpec/MultipleMemoizedHelpers) + ## RSpec/MultipleSubjects Enabled by default | Safe | Supports autocorrection | VersionAdded | VersionChanged diff --git a/spec/rubocop/cop/rspec/multiple_memoized_helpers_spec.rb b/spec/rubocop/cop/rspec/multiple_memoized_helpers_spec.rb new file mode 100644 index 000000000..771e03493 --- /dev/null +++ b/spec/rubocop/cop/rspec/multiple_memoized_helpers_spec.rb @@ -0,0 +1,162 @@ +# frozen_string_literal: true + +RSpec.describe RuboCop::Cop::RSpec::MultipleMemoizedHelpers, :config do + let(:cop_config) { { 'Max' => 1 } } + + it 'flags excessive `#let`' do + expect_offense(<<~RUBY) + describe Foo do + ^^^^^^^^^^^^^^^ Example group has too many memoized helpers [2/1] + let(:foo) { Foo.new } + let(:bar) { Bar.new } + end + RUBY + end + + it 'flags excessive `#let!`' do + expect_offense(<<~RUBY) + describe Foo do + ^^^^^^^^^^^^^^^ Example group has too many memoized helpers [2/1] + let(:foo) { Foo.new } + let!(:bar) { Bar.new } + end + RUBY + end + + it 'ignores a reasonable number of memoized helpers' do + expect_no_offenses(<<~RUBY) + describe Foo do + let(:foo) { Foo.new } + end + RUBY + end + + it 'ignores overridden `#let`' do + expect_no_offenses(<<~RUBY) + describe Foo do + let(:foo) { Foo.new } + context do + let(:foo) { Foo.new(1) } + context do + let(:foo) { Foo.new(1, 2) } + end + end + end + RUBY + end + + it 'ignores `#subject`' do + expect_no_offenses(<<~RUBY) + describe Foo do + subject(:bar) { Bar.new } + let(:foo) { Foo.new } + end + RUBY + end + + it 'ignores `#subject!`' do + expect_no_offenses(<<~RUBY) + describe Foo do + subject!(:bar) { Bar.new } + let(:foo) { Foo.new } + end + RUBY + end + + it 'ignores `#subject` without a name' do + expect_no_offenses(<<~RUBY) + describe Foo do + subject { Foo.new } + let(:bar) { Bar.new } + end + RUBY + end + + it 'flags nested `#let`' do + expect_offense(<<~RUBY) + describe Foo do + let(:foo) { Foo.new } + + context 'when blah' do + ^^^^^^^^^^^^^^^^^^^^^^ Example group has too many memoized helpers [2/1] + let(:bar) { Bar.new } + end + end + RUBY + end + + it 'ignores distributed `#let`' do + expect_no_offenses(<<~RUBY) + describe Foo do + context 'when bloo' do + let(:foo) { Foo.new } + end + + context 'when blah' do + let(:bar) { Bar.new } + end + end + RUBY + end + + context 'when using AllowSubject configuration', :config do + let(:cop_config) { { 'Max' => 1, 'AllowSubject' => false } } + + it 'flags `#subject` without name' do + expect_offense(<<~RUBY) + describe Foo do + ^^^^^^^^^^^^^^^ Example group has too many memoized helpers [2/1] + subject { Foo.new } + let(:foo) { Foo.new } + end + RUBY + end + + it 'flags `#subject`' do + expect_offense(<<~RUBY) + describe Foo do + ^^^^^^^^^^^^^^^ Example group has too many memoized helpers [2/1] + subject(:foo) { Foo.new } + let(:bar) { Foo.new } + end + RUBY + end + + it 'flags `#subject!`' do + expect_offense(<<~RUBY) + describe Foo do + ^^^^^^^^^^^^^^^ Example group has too many memoized helpers [2/1] + subject!(:foo) { Foo.new } + let(:bar) { Foo.new } + end + RUBY + end + + it 'ignores overridden subjects' do + expect_no_offenses(<<~RUBY) + describe Foo do + subject { Foo.new } + context do + subject { Foo.new(1) } + context do + subject { Foo.new(1, 2) } + end + end + end + RUBY + end + end + + it 'support --auto-gen-config' do + inspect_source(<<-RUBY, 'spec/foo_spec.rb') + describe Foo do + let(:foo) { Foo.new } + let(:bar) { Bar.new } + let(:baz) { Baz.new } + let(:qux) { Qux.new } + end + RUBY + + expect(cop.config_to_allow_offenses[:exclude_limit]).to eq('Max' => 4) + end +end diff --git a/spec/smoke_tests/weird_rspec_spec.rb b/spec/smoke_tests/weird_rspec_spec.rb index df876e6c6..4d3d29dc6 100644 --- a/spec/smoke_tests/weird_rspec_spec.rb +++ b/spec/smoke_tests/weird_rspec_spec.rb @@ -11,6 +11,9 @@ let (:foo) { 1 } let! (:bar){} + bar = -> {} + let(:foo, &bar) + let :a do end let(:bar) { <<-HEREDOC }