From 74dde0e64da36188aebaf961df8f6c91ee9436f4 Mon Sep 17 00:00:00 2001 From: Robert Fletcher Date: Sun, 19 Jan 2020 16:41:38 -0800 Subject: [PATCH 1/9] Add NoLet cop Fixes #862 --- CHANGELOG.md | 2 + config/default.yml | 7 + .../memoized_helpers_in_example_group.rb | 136 ++++++++++++++++++ lib/rubocop/cop/rspec_cops.rb | 1 + manual/cops.md | 1 + manual/cops_rspec.md | 116 +++++++++++++++ .../memoized_helpers_in_example_group_spec.rb | 127 ++++++++++++++++ 7 files changed, 390 insertions(+) create mode 100644 lib/rubocop/cop/rspec/memoized_helpers_in_example_group.rb create mode 100644 spec/rubocop/cop/rspec/memoized_helpers_in_example_group_spec.rb diff --git a/CHANGELOG.md b/CHANGELOG.md index 36833bd22..763896658 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -55,6 +55,7 @@ * Add `RSpec/RepeatedExampleGroupDescription` cop. ([@lazycoder9][]) * Add block name and other lines to `RSpec/ScatteredSetup` message. ([@elebow][]) * Fix `RSpec/RepeatedDescription` to take into account example metadata. ([@lazycoder9][]) +* Add `RSpec/MemoizedHelpersInExampleGroup` cop. ([@mockdeep][]) ## 1.37.1 (2019-12-16) @@ -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..803d8f8d9 100644 --- a/config/default.yml +++ b/config/default.yml @@ -347,6 +347,13 @@ RSpec/LetSetup: VersionAdded: '1.7' StyleGuide: https://www.rubydoc.info/gems/rubocop-rspec/RuboCop/Cop/RSpec/LetSetup +RSpec/MemoizedHelpersInExampleGroup: + Description: Checks if example groups contain too many `let` and `subject` calls. + Enabled: false + AllowSubject: false + Max: 5 + StyleGuide: https://www.rubydoc.info/gems/rubocop-rspec/RuboCop/Cop/RSpec/MemoizedHelpersInExampleGroup + RSpec/MessageChain: Description: Check that chains of messages are not being stubbed. Enabled: true diff --git a/lib/rubocop/cop/rspec/memoized_helpers_in_example_group.rb b/lib/rubocop/cop/rspec/memoized_helpers_in_example_group.rb new file mode 100644 index 000000000..e18b36cf4 --- /dev/null +++ b/lib/rubocop/cop/rspec/memoized_helpers_in_example_group.rb @@ -0,0 +1,136 @@ +# 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!(:booger) { [] } + # subject { {} } + # subject(:wat) { {} } + # subject!(:boo) { {} } + # end + # + # describe MyClass do + # let(:foo) { [] } + # let(:bar) { [] } + # + # context 'when stuff' do + # let!(:booger) { [] } + # subject { {} } + # subject(:wat) { {} } + # subject!(:boo) { {} } + # end + # end + # + # # good + # describe MyClass do + # let(:foo) { [] } + # let!(:booger) { [] } + # subject { {} } + # subject(:wat) { {} } + # subject!(:boo) { {} } + # end + # + # describe MyClass do + # context 'when stuff' do + # let(:foo) { [] } + # let(:bar) { [] } + # let!(:booger) { [] } + # end + # + # context 'when other stuff' do + # subject { {} } + # subject(:wat) { {} } + # subject!(:boo) { {} } + # end + # end + # + # @example with AllowSubject configuration + # + # # rubocop.yml + # # RSpec/MemoizedHelpersInExampleGroup: + # # AllowSubject: true + # + # # bad + # describe MyClass do + # let(:foo) { [] } + # let(:bar) { [] } + # let!(:booger) { [] } + # let(:subject) { {} } + # let(:wat) { {} } + # let!(:boo) { {} } + # end + # + # # good + # describe MyClass do + # let(:foo) { [] } + # let(:bar) { [] } + # let!(:booger) { [] } + # subject { {} } + # subject(:wat) { {} } + # subject!(:boo) { {} } + # end + # + # @example with Max configuration + # + # # rubocop.yml + # # RSpec/MemoizedHelpersInExampleGroup: + # # Max: 0 + # + # # bad + # describe MyClass do + # let(:foo) { [] } + # end + # + # # good + # describe MyClass do + # def foo; []; end + # end + # + class MemoizedHelpersInExampleGroup < Cop + MSG = 'Example has too many memoized helpers [%d/%d]' + + def on_block(node) + return unless spec_group?(node) + + count = count_helpers(node) + + node.each_ancestor(:block) do |ancestor| + count += count_helpers(ancestor) + end + + return if count <= max + + add_offense(node, message: format(MSG, count: count, max: max)) + end + + private + + def count_helpers(node) + example_group = RuboCop::RSpec::ExampleGroup.new(node) + count = example_group.lets.count + count += example_group.subjects.count unless allow_subject? + count + 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..5ecabb42b 100644 --- a/lib/rubocop/cop/rspec_cops.rb +++ b/lib/rubocop/cop/rspec_cops.rb @@ -59,6 +59,7 @@ require_relative 'rspec/leaky_constant_declaration' require_relative 'rspec/let_before_examples' require_relative 'rspec/let_setup' +require_relative 'rspec/memoized_helpers_in_example_group' require_relative 'rspec/message_chain' require_relative 'rspec/message_expectation' require_relative 'rspec/message_spies' diff --git a/manual/cops.md b/manual/cops.md index 1e191ee0e..5f57c2636 100644 --- a/manual/cops.md +++ b/manual/cops.md @@ -58,6 +58,7 @@ * [RSpec/LeakyConstantDeclaration](cops_rspec.md#rspecleakyconstantdeclaration) * [RSpec/LetBeforeExamples](cops_rspec.md#rspecletbeforeexamples) * [RSpec/LetSetup](cops_rspec.md#rspecletsetup) +* [RSpec/MemoizedHelpersInExampleGroup](cops_rspec.md#rspecmemoizedhelpersinexamplegroup) * [RSpec/MessageChain](cops_rspec.md#rspecmessagechain) * [RSpec/MessageExpectation](cops_rspec.md#rspecmessageexpectation) * [RSpec/MessageSpies](cops_rspec.md#rspecmessagespies) diff --git a/manual/cops_rspec.md b/manual/cops_rspec.md index 5ca0c1e12..22163d99f 100644 --- a/manual/cops_rspec.md +++ b/manual/cops_rspec.md @@ -1890,6 +1890,122 @@ end * [https://www.rubydoc.info/gems/rubocop-rspec/RuboCop/Cop/RSpec/LetSetup](https://www.rubydoc.info/gems/rubocop-rspec/RuboCop/Cop/RSpec/LetSetup) +## RSpec/MemoizedHelpersInExampleGroup + +Enabled by default | Safe | Supports autocorrection | VersionAdded | VersionChanged +--- | --- | --- | --- | --- +Disabled | 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!(:booger) { [] } + subject { {} } + subject(:wat) { {} } + subject!(:boo) { {} } +end + +describe MyClass do + let(:foo) { [] } + let(:bar) { [] } + + context 'when stuff' do + let!(:booger) { [] } + subject { {} } + subject(:wat) { {} } + subject!(:boo) { {} } + end +end + +# good +describe MyClass do + let(:foo) { [] } + let!(:booger) { [] } + subject { {} } + subject(:wat) { {} } + subject!(:boo) { {} } +end + +describe MyClass do + context 'when stuff' do + let(:foo) { [] } + let(:bar) { [] } + let!(:booger) { [] } + end + + context 'when other stuff' do + subject { {} } + subject(:wat) { {} } + subject!(:boo) { {} } + end +end +``` +#### with AllowSubject configuration + +```ruby +# rubocop.yml +# RSpec/MemoizedHelpersInExampleGroup: +# AllowSubject: true + +# bad +describe MyClass do + let(:foo) { [] } + let(:bar) { [] } + let!(:booger) { [] } + let(:subject) { {} } + let(:wat) { {} } + let!(:boo) { {} } +end + +# good +describe MyClass do + let(:foo) { [] } + let(:bar) { [] } + let!(:booger) { [] } + subject { {} } + subject(:wat) { {} } + subject!(:boo) { {} } +end +``` +#### with Max configuration + +```ruby +# rubocop.yml +# RSpec/MemoizedHelpersInExampleGroup: +# Max: 0 + +# bad +describe MyClass do + let(:foo) { [] } +end + +# good +describe MyClass do + def foo; []; end +end +``` + +### Configurable attributes + +Name | Default value | Configurable values +--- | --- | --- +AllowSubject | `false` | Boolean +Max | `5` | Integer + +### References + +* [https://www.rubydoc.info/gems/rubocop-rspec/RuboCop/Cop/RSpec/MemoizedHelpersInExampleGroup](https://www.rubydoc.info/gems/rubocop-rspec/RuboCop/Cop/RSpec/MemoizedHelpersInExampleGroup) + ## RSpec/MessageChain Enabled by default | Safe | Supports autocorrection | VersionAdded | VersionChanged diff --git a/spec/rubocop/cop/rspec/memoized_helpers_in_example_group_spec.rb b/spec/rubocop/cop/rspec/memoized_helpers_in_example_group_spec.rb new file mode 100644 index 000000000..d71559b1e --- /dev/null +++ b/spec/rubocop/cop/rspec/memoized_helpers_in_example_group_spec.rb @@ -0,0 +1,127 @@ +# frozen_string_literal: true + +RSpec.describe RuboCop::Cop::RSpec::MemoizedHelpersInExampleGroup, :config do + let(:cop_config) { { 'Max' => 1 } } + + it 'flags an offense when using more than max `#let` calls' do + expect_offense(<<~RUBY) + describe Foo do + ^^^^^^^^^^^^^^^ Example has too many memoized helpers [2/1] + let(:foo) { Foo.new } + let(:bar) { Bar.new } + end + RUBY + end + + it 'flags an offense when using `#subject` without name' do + expect_offense(<<~RUBY) + describe Foo do + ^^^^^^^^^^^^^^^ Example has too many memoized helpers [2/1] + subject { Foo.new } + let(:foo) { Foo.new } + end + RUBY + end + + it 'flags an offense when using `#subject` with name' do + expect_offense(<<~RUBY) + describe Foo do + ^^^^^^^^^^^^^^^ Example has too many memoized helpers [2/1] + subject(:foo) { Foo.new } + let(:foo) { Foo.new } + end + RUBY + end + + it 'flags an offense when using `#let!`' do + expect_offense(<<~RUBY) + describe Foo do + ^^^^^^^^^^^^^^^ Example has too many memoized helpers [2/1] + let(:foo) { Foo.new } + let!(:foo) { Foo.new } + end + RUBY + end + + it 'flags an offense when using `#subject!`' do + expect_offense(<<~RUBY) + describe Foo do + ^^^^^^^^^^^^^^^ Example has too many memoized helpers [2/1] + subject!(:foo) { Foo.new } + let(:foo) { Foo.new } + end + RUBY + end + + it 'does not flag an offense when using <= max `#let` calls' do + expect_no_offenses(<<~RUBY) + describe Foo do + let(:foo) { Foo.new } + end + RUBY + end + + it 'flags an offense when too many `#let` calls are nested' do + expect_offense(<<~RUBY) + describe Foo do + let(:foo) { Foo.new } + + context 'when blah' do + ^^^^^^^^^^^^^^^^^^^^^^ Example has too many memoized helpers [2/1] + let(:bar) { Bar.new } + end + end + RUBY + end + + it 'does not flag an offense when `#let` calls are distributed' 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 + + it 'does not flag an offense when using `#before`' do + expect_no_offenses(<<~RUBY) + describe Foo do + before { foo } + end + RUBY + end + + context 'when using AllowSubject configuration', :config do + let(:cop_config) { { 'Max' => 0, 'AllowSubject' => true } } + + it 'flags an offense when using `#let`' do + expect_offense(<<~RUBY) + describe Foo do + ^^^^^^^^^^^^^^^ Example has too many memoized helpers [1/0] + let(:foo) { Foo.new } + end + RUBY + end + + it 'does not flag an offense when using `#subject` without a name' do + expect_no_offenses(<<~RUBY) + describe Foo do + subject { Foo.new } + end + RUBY + end + + it 'does not flag an offense when using `#subject` with a name' do + expect_no_offenses(<<~RUBY) + describe Foo do + subject(:foo) { Foo.new } + end + RUBY + end + end +end From d761fc30b2c49537d4ac3338a01f2b2f51e80f0f Mon Sep 17 00:00:00 2001 From: Phil Pirozhkov Date: Mon, 3 Aug 2020 03:52:40 +0300 Subject: [PATCH 2/9] Rename the cop and fix the build --- CHANGELOG.md | 2 +- config/default.yml | 14 ++--- ..._group.rb => multiple_memoized_helpers.rb} | 60 ++++++++----------- lib/rubocop/cop/rspec_cops.rb | 2 +- ...c.rb => multiple_memoized_helpers_spec.rb} | 16 ++--- 5 files changed, 42 insertions(+), 52 deletions(-) rename lib/rubocop/cop/rspec/{memoized_helpers_in_example_group.rb => multiple_memoized_helpers.rb} (67%) rename spec/rubocop/cop/rspec/{memoized_helpers_in_example_group_spec.rb => multiple_memoized_helpers_spec.rb} (81%) diff --git a/CHANGELOG.md b/CHANGELOG.md index 763896658..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) @@ -55,7 +56,6 @@ * Add `RSpec/RepeatedExampleGroupDescription` cop. ([@lazycoder9][]) * Add block name and other lines to `RSpec/ScatteredSetup` message. ([@elebow][]) * Fix `RSpec/RepeatedDescription` to take into account example metadata. ([@lazycoder9][]) -* Add `RSpec/MemoizedHelpersInExampleGroup` cop. ([@mockdeep][]) ## 1.37.1 (2019-12-16) diff --git a/config/default.yml b/config/default.yml index 803d8f8d9..61f67c9b7 100644 --- a/config/default.yml +++ b/config/default.yml @@ -347,13 +347,6 @@ RSpec/LetSetup: VersionAdded: '1.7' StyleGuide: https://www.rubydoc.info/gems/rubocop-rspec/RuboCop/Cop/RSpec/LetSetup -RSpec/MemoizedHelpersInExampleGroup: - Description: Checks if example groups contain too many `let` and `subject` calls. - Enabled: false - AllowSubject: false - Max: 5 - StyleGuide: https://www.rubydoc.info/gems/rubocop-rspec/RuboCop/Cop/RSpec/MemoizedHelpersInExampleGroup - RSpec/MessageChain: Description: Check that chains of messages are not being stubbed. Enabled: true @@ -401,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: false + AllowSubject: false + 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/memoized_helpers_in_example_group.rb b/lib/rubocop/cop/rspec/multiple_memoized_helpers.rb similarity index 67% rename from lib/rubocop/cop/rspec/memoized_helpers_in_example_group.rb rename to lib/rubocop/cop/rspec/multiple_memoized_helpers.rb index e18b36cf4..471710a5e 100644 --- a/lib/rubocop/cop/rspec/memoized_helpers_in_example_group.rb +++ b/lib/rubocop/cop/rspec/multiple_memoized_helpers.rb @@ -14,31 +14,31 @@ module RSpec # describe MyClass do # let(:foo) { [] } # let(:bar) { [] } - # let!(:booger) { [] } - # subject { {} } - # subject(:wat) { {} } - # subject!(:boo) { {} } + # let!(:baz) { [] } + # let(:qux) { [] } + # let(:quux) { [] } + # subject(:quuz) { {} } # end # # describe MyClass do # let(:foo) { [] } # let(:bar) { [] } + # let!(:baz) { [] } # # context 'when stuff' do - # let!(:booger) { [] } - # subject { {} } - # subject(:wat) { {} } - # subject!(:boo) { {} } + # let(:qux) { [] } + # let(:quux) { [] } + # subject(:quuz) { {} } # end # end # # # good # describe MyClass do - # let(:foo) { [] } - # let!(:booger) { [] } - # subject { {} } - # subject(:wat) { {} } - # subject!(:boo) { {} } + # let(:bar) { [] } + # let!(:baz) { [] } + # let(:qux) { [] } + # let(:quux) { [] } + # subject(:quuz) { {} } # end # # describe MyClass do @@ -49,42 +49,32 @@ module RSpec # end # # context 'when other stuff' do - # subject { {} } - # subject(:wat) { {} } - # subject!(:boo) { {} } + # let(:qux) { [] } + # let(:quux) { [] } + # subject(:quuz) { {} } # end # end # - # @example with AllowSubject configuration + # @example when disabling AllowSubject configuration # # # rubocop.yml - # # RSpec/MemoizedHelpersInExampleGroup: + # # RSpec/MultipleMemoizedHelpers: # # AllowSubject: true # - # # bad - # describe MyClass do - # let(:foo) { [] } - # let(:bar) { [] } - # let!(:booger) { [] } - # let(:subject) { {} } - # let(:wat) { {} } - # let!(:boo) { {} } - # end - # # # good # describe MyClass do + # subject { {} } # let(:foo) { [] } # let(:bar) { [] } - # let!(:booger) { [] } - # subject { {} } - # subject(:wat) { {} } - # subject!(:boo) { {} } + # let!(:baz) { [] } + # let(:qux) { [] } + # let(:quux) { [] } # end # # @example with Max configuration # # # rubocop.yml - # # RSpec/MemoizedHelpersInExampleGroup: + # # RSpec/MultipleMemoizedHelpers: # # Max: 0 # # # bad @@ -97,8 +87,8 @@ module RSpec # def foo; []; end # end # - class MemoizedHelpersInExampleGroup < Cop - MSG = 'Example has too many memoized helpers [%d/%d]' + class MultipleMemoizedHelpers < Base + MSG = 'Example group has too many memoized helpers [%d/%d]' def on_block(node) return unless spec_group?(node) diff --git a/lib/rubocop/cop/rspec_cops.rb b/lib/rubocop/cop/rspec_cops.rb index 5ecabb42b..b835bf3d6 100644 --- a/lib/rubocop/cop/rspec_cops.rb +++ b/lib/rubocop/cop/rspec_cops.rb @@ -59,13 +59,13 @@ require_relative 'rspec/leaky_constant_declaration' require_relative 'rspec/let_before_examples' require_relative 'rspec/let_setup' -require_relative 'rspec/memoized_helpers_in_example_group' require_relative 'rspec/message_chain' require_relative 'rspec/message_expectation' require_relative 'rspec/message_spies' 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/spec/rubocop/cop/rspec/memoized_helpers_in_example_group_spec.rb b/spec/rubocop/cop/rspec/multiple_memoized_helpers_spec.rb similarity index 81% rename from spec/rubocop/cop/rspec/memoized_helpers_in_example_group_spec.rb rename to spec/rubocop/cop/rspec/multiple_memoized_helpers_spec.rb index d71559b1e..6e3951a89 100644 --- a/spec/rubocop/cop/rspec/memoized_helpers_in_example_group_spec.rb +++ b/spec/rubocop/cop/rspec/multiple_memoized_helpers_spec.rb @@ -1,12 +1,12 @@ # frozen_string_literal: true -RSpec.describe RuboCop::Cop::RSpec::MemoizedHelpersInExampleGroup, :config do +RSpec.describe RuboCop::Cop::RSpec::MultipleMemoizedHelpers, :config do let(:cop_config) { { 'Max' => 1 } } it 'flags an offense when using more than max `#let` calls' do expect_offense(<<~RUBY) describe Foo do - ^^^^^^^^^^^^^^^ Example has too many memoized helpers [2/1] + ^^^^^^^^^^^^^^^ Example group has too many memoized helpers [2/1] let(:foo) { Foo.new } let(:bar) { Bar.new } end @@ -16,7 +16,7 @@ it 'flags an offense when using `#subject` without name' do expect_offense(<<~RUBY) describe Foo do - ^^^^^^^^^^^^^^^ Example has too many memoized helpers [2/1] + ^^^^^^^^^^^^^^^ Example group has too many memoized helpers [2/1] subject { Foo.new } let(:foo) { Foo.new } end @@ -26,7 +26,7 @@ it 'flags an offense when using `#subject` with name' do expect_offense(<<~RUBY) describe Foo do - ^^^^^^^^^^^^^^^ Example has too many memoized helpers [2/1] + ^^^^^^^^^^^^^^^ Example group has too many memoized helpers [2/1] subject(:foo) { Foo.new } let(:foo) { Foo.new } end @@ -36,7 +36,7 @@ it 'flags an offense when using `#let!`' do expect_offense(<<~RUBY) describe Foo do - ^^^^^^^^^^^^^^^ Example has too many memoized helpers [2/1] + ^^^^^^^^^^^^^^^ Example group has too many memoized helpers [2/1] let(:foo) { Foo.new } let!(:foo) { Foo.new } end @@ -46,7 +46,7 @@ it 'flags an offense when using `#subject!`' do expect_offense(<<~RUBY) describe Foo do - ^^^^^^^^^^^^^^^ Example has too many memoized helpers [2/1] + ^^^^^^^^^^^^^^^ Example group has too many memoized helpers [2/1] subject!(:foo) { Foo.new } let(:foo) { Foo.new } end @@ -67,7 +67,7 @@ let(:foo) { Foo.new } context 'when blah' do - ^^^^^^^^^^^^^^^^^^^^^^ Example has too many memoized helpers [2/1] + ^^^^^^^^^^^^^^^^^^^^^^ Example group has too many memoized helpers [2/1] let(:bar) { Bar.new } end end @@ -102,7 +102,7 @@ it 'flags an offense when using `#let`' do expect_offense(<<~RUBY) describe Foo do - ^^^^^^^^^^^^^^^ Example has too many memoized helpers [1/0] + ^^^^^^^^^^^^^^^ Example group has too many memoized helpers [1/0] let(:foo) { Foo.new } end RUBY From 3732a9c695adba877bf83e6080417aa3a01a05d6 Mon Sep 17 00:00:00 2001 From: Phil Pirozhkov Date: Mon, 3 Aug 2020 03:59:59 +0300 Subject: [PATCH 3/9] Minor restyling --- .../cop/rspec/multiple_memoized_helpers.rb | 21 +++++++++++-------- 1 file changed, 12 insertions(+), 9 deletions(-) diff --git a/lib/rubocop/cop/rspec/multiple_memoized_helpers.rb b/lib/rubocop/cop/rspec/multiple_memoized_helpers.rb index 471710a5e..9e29430a2 100644 --- a/lib/rubocop/cop/rspec/multiple_memoized_helpers.rb +++ b/lib/rubocop/cop/rspec/multiple_memoized_helpers.rb @@ -93,11 +93,7 @@ class MultipleMemoizedHelpers < Base def on_block(node) return unless spec_group?(node) - count = count_helpers(node) - - node.each_ancestor(:block) do |ancestor| - count += count_helpers(ancestor) - end + count = total_helpers(node) return if count <= max @@ -106,11 +102,18 @@ def on_block(node) private - def count_helpers(node) + def total_helpers(node) + helpers(node) + + node.each_ancestor(:block).map(&method(:helpers)).sum + end + + def helpers(node) example_group = RuboCop::RSpec::ExampleGroup.new(node) - count = example_group.lets.count - count += example_group.subjects.count unless allow_subject? - count + if allow_subject? + example_group.lets.count + else + example_group.lets.count + example_group.subjects.count + end end def max From 418c7fcb810e74302476cc7b7885dff82fc8ed57 Mon Sep 17 00:00:00 2001 From: Phil Pirozhkov Date: Mon, 3 Aug 2020 04:26:03 +0300 Subject: [PATCH 4/9] Update the manual --- manual/cops.md | 2 +- manual/cops_rspec.md | 222 +++++++++++++++++++++---------------------- 2 files changed, 107 insertions(+), 117 deletions(-) diff --git a/manual/cops.md b/manual/cops.md index 5f57c2636..75a49daa3 100644 --- a/manual/cops.md +++ b/manual/cops.md @@ -58,13 +58,13 @@ * [RSpec/LeakyConstantDeclaration](cops_rspec.md#rspecleakyconstantdeclaration) * [RSpec/LetBeforeExamples](cops_rspec.md#rspecletbeforeexamples) * [RSpec/LetSetup](cops_rspec.md#rspecletsetup) -* [RSpec/MemoizedHelpersInExampleGroup](cops_rspec.md#rspecmemoizedhelpersinexamplegroup) * [RSpec/MessageChain](cops_rspec.md#rspecmessagechain) * [RSpec/MessageExpectation](cops_rspec.md#rspecmessageexpectation) * [RSpec/MessageSpies](cops_rspec.md#rspecmessagespies) * [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 22163d99f..358b315d1 100644 --- a/manual/cops_rspec.md +++ b/manual/cops_rspec.md @@ -1890,122 +1890,6 @@ end * [https://www.rubydoc.info/gems/rubocop-rspec/RuboCop/Cop/RSpec/LetSetup](https://www.rubydoc.info/gems/rubocop-rspec/RuboCop/Cop/RSpec/LetSetup) -## RSpec/MemoizedHelpersInExampleGroup - -Enabled by default | Safe | Supports autocorrection | VersionAdded | VersionChanged ---- | --- | --- | --- | --- -Disabled | 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!(:booger) { [] } - subject { {} } - subject(:wat) { {} } - subject!(:boo) { {} } -end - -describe MyClass do - let(:foo) { [] } - let(:bar) { [] } - - context 'when stuff' do - let!(:booger) { [] } - subject { {} } - subject(:wat) { {} } - subject!(:boo) { {} } - end -end - -# good -describe MyClass do - let(:foo) { [] } - let!(:booger) { [] } - subject { {} } - subject(:wat) { {} } - subject!(:boo) { {} } -end - -describe MyClass do - context 'when stuff' do - let(:foo) { [] } - let(:bar) { [] } - let!(:booger) { [] } - end - - context 'when other stuff' do - subject { {} } - subject(:wat) { {} } - subject!(:boo) { {} } - end -end -``` -#### with AllowSubject configuration - -```ruby -# rubocop.yml -# RSpec/MemoizedHelpersInExampleGroup: -# AllowSubject: true - -# bad -describe MyClass do - let(:foo) { [] } - let(:bar) { [] } - let!(:booger) { [] } - let(:subject) { {} } - let(:wat) { {} } - let!(:boo) { {} } -end - -# good -describe MyClass do - let(:foo) { [] } - let(:bar) { [] } - let!(:booger) { [] } - subject { {} } - subject(:wat) { {} } - subject!(:boo) { {} } -end -``` -#### with Max configuration - -```ruby -# rubocop.yml -# RSpec/MemoizedHelpersInExampleGroup: -# Max: 0 - -# bad -describe MyClass do - let(:foo) { [] } -end - -# good -describe MyClass do - def foo; []; end -end -``` - -### Configurable attributes - -Name | Default value | Configurable values ---- | --- | --- -AllowSubject | `false` | Boolean -Max | `5` | Integer - -### References - -* [https://www.rubydoc.info/gems/rubocop-rspec/RuboCop/Cop/RSpec/MemoizedHelpersInExampleGroup](https://www.rubydoc.info/gems/rubocop-rspec/RuboCop/Cop/RSpec/MemoizedHelpersInExampleGroup) - ## RSpec/MessageChain Enabled by default | Safe | Supports autocorrection | VersionAdded | VersionChanged @@ -2235,6 +2119,112 @@ 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 +--- | --- | --- | --- | --- +Disabled | 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) { [] } + subject(:quuz) { {} } +end + +describe MyClass do + let(:foo) { [] } + let(:bar) { [] } + let!(:baz) { [] } + + context 'when stuff' do + let(:qux) { [] } + let(:quux) { [] } + subject(:quuz) { {} } + end +end + +# good +describe MyClass do + let(:bar) { [] } + let!(:baz) { [] } + let(:qux) { [] } + let(:quux) { [] } + subject(: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) { [] } + subject(:quuz) { {} } + end +end +``` +#### when disabling AllowSubject configuration + +```ruby +# rubocop.yml +# RSpec/MultipleMemoizedHelpers: +# AllowSubject: true + +# good +describe MyClass do + subject { {} } + let(:foo) { [] } + let(:bar) { [] } + let!(:baz) { [] } + let(:qux) { [] } + let(:quux) { [] } +end +``` +#### with Max configuration + +```ruby +# rubocop.yml +# RSpec/MultipleMemoizedHelpers: +# Max: 0 + +# bad +describe MyClass do + let(:foo) { [] } +end + +# good +describe MyClass do + def foo; []; end +end +``` + +### Configurable attributes + +Name | Default value | Configurable values +--- | --- | --- +AllowSubject | `false` | 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 From 3fcd0bb76c25d606914b27d8032afdb2ef0dd885 Mon Sep 17 00:00:00 2001 From: Phil Pirozhkov Date: Tue, 4 Aug 2020 22:55:56 +0300 Subject: [PATCH 5/9] Only count unique variable name definitions --- .../cop/rspec/multiple_memoized_helpers.rb | 24 ++++++++----- .../rspec/multiple_memoized_helpers_spec.rb | 34 +++++++++++++++++-- 2 files changed, 46 insertions(+), 12 deletions(-) diff --git a/lib/rubocop/cop/rspec/multiple_memoized_helpers.rb b/lib/rubocop/cop/rspec/multiple_memoized_helpers.rb index 9e29430a2..47d2b799c 100644 --- a/lib/rubocop/cop/rspec/multiple_memoized_helpers.rb +++ b/lib/rubocop/cop/rspec/multiple_memoized_helpers.rb @@ -88,12 +88,14 @@ module RSpec # end # class MultipleMemoizedHelpers < Base + include RuboCop::RSpec::Variable + MSG = 'Example group has too many memoized helpers [%d/%d]' def on_block(node) return unless spec_group?(node) - count = total_helpers(node) + count = all_helpers(node).uniq.count return if count <= max @@ -102,18 +104,22 @@ def on_block(node) private - def total_helpers(node) - helpers(node) + - node.each_ancestor(:block).map(&method(:helpers)).sum + def all_helpers(node) + [ + *helpers(node), + *node.each_ancestor(:block).flat_map(&method(:helpers)) + ] end def helpers(node) example_group = RuboCop::RSpec::ExampleGroup.new(node) - if allow_subject? - example_group.lets.count - else - example_group.lets.count + example_group.subjects.count - end + variables = + if allow_subject? + example_group.lets + else + example_group.lets + example_group.subjects + end + variables.map { |variable| variable_definition?(variable.send_node) } end def max diff --git a/spec/rubocop/cop/rspec/multiple_memoized_helpers_spec.rb b/spec/rubocop/cop/rspec/multiple_memoized_helpers_spec.rb index 6e3951a89..21b814867 100644 --- a/spec/rubocop/cop/rspec/multiple_memoized_helpers_spec.rb +++ b/spec/rubocop/cop/rspec/multiple_memoized_helpers_spec.rb @@ -28,7 +28,7 @@ describe Foo do ^^^^^^^^^^^^^^^ Example group has too many memoized helpers [2/1] subject(:foo) { Foo.new } - let(:foo) { Foo.new } + let(:bar) { Foo.new } end RUBY end @@ -38,7 +38,7 @@ describe Foo do ^^^^^^^^^^^^^^^ Example group has too many memoized helpers [2/1] let(:foo) { Foo.new } - let!(:foo) { Foo.new } + let!(:bar) { Foo.new } end RUBY end @@ -48,7 +48,7 @@ describe Foo do ^^^^^^^^^^^^^^^ Example group has too many memoized helpers [2/1] subject!(:foo) { Foo.new } - let(:foo) { Foo.new } + let(:bar) { Foo.new } end RUBY end @@ -61,6 +61,34 @@ RUBY end + it 'ignores overridden variables' 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 overridden nameless 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 + it 'flags an offense when too many `#let` calls are nested' do expect_offense(<<~RUBY) describe Foo do From c3ab68f0a0a398bf008c18d7bcd5a4035d1b752e Mon Sep 17 00:00:00 2001 From: Phil Pirozhkov Date: Wed, 5 Aug 2020 01:37:31 +0300 Subject: [PATCH 6/9] Allow for --auto-gen-config --- lib/rubocop/cop/rspec/multiple_memoized_helpers.rb | 2 ++ .../cop/rspec/multiple_memoized_helpers_spec.rb | 13 +++++++++++++ 2 files changed, 15 insertions(+) diff --git a/lib/rubocop/cop/rspec/multiple_memoized_helpers.rb b/lib/rubocop/cop/rspec/multiple_memoized_helpers.rb index 47d2b799c..d91b00e03 100644 --- a/lib/rubocop/cop/rspec/multiple_memoized_helpers.rb +++ b/lib/rubocop/cop/rspec/multiple_memoized_helpers.rb @@ -88,6 +88,7 @@ module RSpec # end # class MultipleMemoizedHelpers < Base + include ConfigurableMax include RuboCop::RSpec::Variable MSG = 'Example group has too many memoized helpers [%d/%d]' @@ -99,6 +100,7 @@ def on_block(node) return if count <= max + self.max = count add_offense(node, message: format(MSG, count: count, max: max)) end diff --git a/spec/rubocop/cop/rspec/multiple_memoized_helpers_spec.rb b/spec/rubocop/cop/rspec/multiple_memoized_helpers_spec.rb index 21b814867..24b5424e6 100644 --- a/spec/rubocop/cop/rspec/multiple_memoized_helpers_spec.rb +++ b/spec/rubocop/cop/rspec/multiple_memoized_helpers_spec.rb @@ -152,4 +152,17 @@ 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 From 8d7b3c9713f44687e41104e0650770522f8d02d6 Mon Sep 17 00:00:00 2001 From: Phil Pirozhkov Date: Fri, 7 Aug 2020 02:16:20 +0300 Subject: [PATCH 7/9] Add block-pass support to MultipleMemoizedHelpers --- .../cop/rspec/multiple_memoized_helpers.rb | 22 +++++++++++++------ lib/rubocop/rspec/variable.rb | 2 +- spec/smoke_tests/weird_rspec_spec.rb | 3 +++ 3 files changed, 19 insertions(+), 8 deletions(-) diff --git a/lib/rubocop/cop/rspec/multiple_memoized_helpers.rb b/lib/rubocop/cop/rspec/multiple_memoized_helpers.rb index d91b00e03..93b614c02 100644 --- a/lib/rubocop/cop/rspec/multiple_memoized_helpers.rb +++ b/lib/rubocop/cop/rspec/multiple_memoized_helpers.rb @@ -114,14 +114,22 @@ def all_helpers(node) end def helpers(node) - example_group = RuboCop::RSpec::ExampleGroup.new(node) - variables = - if allow_subject? - example_group.lets - else - example_group.lets + example_group.subjects + 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 - variables.map { |variable| variable_definition?(variable.send_node) } + 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 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/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 } From edb87e8b0b667d8be9c759ffe6a21d62e08189da Mon Sep 17 00:00:00 2001 From: Phil Pirozhkov Date: Fri, 7 Aug 2020 02:50:19 +0300 Subject: [PATCH 8/9] Enable MultipleMemoizedHelpers by default Switching the cop on by default to match RSpec recommendations from its docs and the community RSpec Style Guide recommendations. `let` _can_ enhance readability when used sparingly (1,2, or maybe 3 declarations) in any given example group, but that can quickly degrade with overuse. YMMV. https://github.com/rspec/rspec-core/blob/5d87c28bb828ba14b54a0d8cfddf044894009314/lib/rspec/core/memoized_helpers.rb#L257 https://reddit.com/r/ruby/comments/73nrhk/ama_the_authors_of_effective_testing_with_rspec_3/dnsyanp/ https://rspec.rubystyle.guide/#let-blocks Example change to dramatically reduce the number of used memoized helpers without resorting to methods https://github.com/rubocop-hq/rubocop/pull/8447 There is no limit to the number of memoized helpers being used in practice: 10 lets https://github.com/forem/forem/blob/master/spec/support/api_analytics_shared_examples.rb#L2 20 lets https://github.com/chef/chef/blob/master/spec/unit/provider/systemd_unit_spec.rb#L23 30 lets https://github.com/chef/chef/blob/master/spec/unit/policy_builder/policyfile_spec.rb#L24 40 lets https://github.com/diaspora/diaspora/blob/develop/spec/integration/migration_service_spec.rb#L7 50 lets https://github.com/chef/chef/blob/master/spec/functional/resource/dsc_script_spec.rb#L71 Allowing `subject` by default, i.e. subjects don't count towards the number of memoized helpers, since there is a dedicated `RSpec/MultipleSubjects` cop. Combination with it provides infinite possibilities to tweak the maximum allowed number of memoized helpers to meet the project style. --- config/default.yml | 4 +- .../cop/rspec/multiple_memoized_helpers.rb | 20 ++-- manual/cops_rspec.md | 24 ++-- .../rspec/multiple_memoized_helpers_spec.rb | 104 +++++++++--------- 4 files changed, 69 insertions(+), 83 deletions(-) diff --git a/config/default.yml b/config/default.yml index 61f67c9b7..946f854f8 100644 --- a/config/default.yml +++ b/config/default.yml @@ -396,8 +396,8 @@ RSpec/MultipleExpectations: RSpec/MultipleMemoizedHelpers: Description: Checks if example groups contain too many `let` and `subject` calls. - Enabled: false - AllowSubject: false + Enabled: true + AllowSubject: true Max: 5 StyleGuide: https://www.rubydoc.info/gems/rubocop-rspec/RuboCop/Cop/RSpec/MultipleMemoizedHelpers diff --git a/lib/rubocop/cop/rspec/multiple_memoized_helpers.rb b/lib/rubocop/cop/rspec/multiple_memoized_helpers.rb index 93b614c02..ea19b78ce 100644 --- a/lib/rubocop/cop/rspec/multiple_memoized_helpers.rb +++ b/lib/rubocop/cop/rspec/multiple_memoized_helpers.rb @@ -17,7 +17,7 @@ module RSpec # let!(:baz) { [] } # let(:qux) { [] } # let(:quux) { [] } - # subject(:quuz) { {} } + # let(:quuz) { {} } # end # # describe MyClass do @@ -28,7 +28,7 @@ module RSpec # context 'when stuff' do # let(:qux) { [] } # let(:quux) { [] } - # subject(:quuz) { {} } + # let(:quuz) { {} } # end # end # @@ -38,7 +38,7 @@ module RSpec # let!(:baz) { [] } # let(:qux) { [] } # let(:quux) { [] } - # subject(:quuz) { {} } + # let(:quuz) { {} } # end # # describe MyClass do @@ -51,7 +51,7 @@ module RSpec # context 'when other stuff' do # let(:qux) { [] } # let(:quux) { [] } - # subject(:quuz) { {} } + # let(:quuz) { {} } # end # end # @@ -59,9 +59,9 @@ module RSpec # # # rubocop.yml # # RSpec/MultipleMemoizedHelpers: - # # AllowSubject: true + # # AllowSubject: false # - # # good + # # bad - `subject` counts towards memoized helpers # describe MyClass do # subject { {} } # let(:foo) { [] } @@ -75,16 +75,12 @@ module RSpec # # # rubocop.yml # # RSpec/MultipleMemoizedHelpers: - # # Max: 0 + # # Max: 1 # # # bad # describe MyClass do # let(:foo) { [] } - # end - # - # # good - # describe MyClass do - # def foo; []; end + # let(:bar) { [] } # end # class MultipleMemoizedHelpers < Base diff --git a/manual/cops_rspec.md b/manual/cops_rspec.md index 358b315d1..4d2a14dc2 100644 --- a/manual/cops_rspec.md +++ b/manual/cops_rspec.md @@ -2123,7 +2123,7 @@ Max | `1` | Integer Enabled by default | Safe | Supports autocorrection | VersionAdded | VersionChanged --- | --- | --- | --- | --- -Disabled | Yes | No | - | - +Enabled | Yes | No | - | - Checks if example groups contain too many `let` and `subject` calls. @@ -2141,7 +2141,7 @@ describe MyClass do let!(:baz) { [] } let(:qux) { [] } let(:quux) { [] } - subject(:quuz) { {} } + let(:quuz) { {} } end describe MyClass do @@ -2152,7 +2152,7 @@ describe MyClass do context 'when stuff' do let(:qux) { [] } let(:quux) { [] } - subject(:quuz) { {} } + let(:quuz) { {} } end end @@ -2162,7 +2162,7 @@ describe MyClass do let!(:baz) { [] } let(:qux) { [] } let(:quux) { [] } - subject(:quuz) { {} } + let(:quuz) { {} } end describe MyClass do @@ -2175,7 +2175,7 @@ describe MyClass do context 'when other stuff' do let(:qux) { [] } let(:quux) { [] } - subject(:quuz) { {} } + let(:quuz) { {} } end end ``` @@ -2184,9 +2184,9 @@ end ```ruby # rubocop.yml # RSpec/MultipleMemoizedHelpers: -# AllowSubject: true +# AllowSubject: false -# good +# bad - `subject` counts towards memoized helpers describe MyClass do subject { {} } let(:foo) { [] } @@ -2201,16 +2201,12 @@ end ```ruby # rubocop.yml # RSpec/MultipleMemoizedHelpers: -# Max: 0 +# Max: 1 # bad describe MyClass do let(:foo) { [] } -end - -# good -describe MyClass do - def foo; []; end + let(:bar) { [] } end ``` @@ -2218,7 +2214,7 @@ end Name | Default value | Configurable values --- | --- | --- -AllowSubject | `false` | Boolean +AllowSubject | `true` | Boolean Max | `5` | Integer ### References diff --git a/spec/rubocop/cop/rspec/multiple_memoized_helpers_spec.rb b/spec/rubocop/cop/rspec/multiple_memoized_helpers_spec.rb index 24b5424e6..771e03493 100644 --- a/spec/rubocop/cop/rspec/multiple_memoized_helpers_spec.rb +++ b/spec/rubocop/cop/rspec/multiple_memoized_helpers_spec.rb @@ -3,7 +3,7 @@ RSpec.describe RuboCop::Cop::RSpec::MultipleMemoizedHelpers, :config do let(:cop_config) { { 'Max' => 1 } } - it 'flags an offense when using more than max `#let` calls' do + it 'flags excessive `#let`' do expect_offense(<<~RUBY) describe Foo do ^^^^^^^^^^^^^^^ Example group has too many memoized helpers [2/1] @@ -13,83 +13,66 @@ RUBY end - it 'flags an offense when using `#subject` without name' do + it 'flags excessive `#let!`' do expect_offense(<<~RUBY) describe Foo do ^^^^^^^^^^^^^^^ Example group has too many memoized helpers [2/1] - subject { Foo.new } let(:foo) { Foo.new } + let!(:bar) { Bar.new } end RUBY end - it 'flags an offense when using `#subject` with name' 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 an offense when using `#let!`' do - expect_offense(<<~RUBY) + it 'ignores a reasonable number of memoized helpers' do + expect_no_offenses(<<~RUBY) describe Foo do - ^^^^^^^^^^^^^^^ Example group has too many memoized helpers [2/1] let(:foo) { Foo.new } - let!(:bar) { Foo.new } end RUBY end - it 'flags an offense when using `#subject!`' do - expect_offense(<<~RUBY) + it 'ignores overridden `#let`' do + expect_no_offenses(<<~RUBY) describe Foo do - ^^^^^^^^^^^^^^^ Example group has too many memoized helpers [2/1] - subject!(:foo) { Foo.new } - let(:bar) { Foo.new } + 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 'does not flag an offense when using <= max `#let` calls' do + it 'ignores `#subject`' do expect_no_offenses(<<~RUBY) describe Foo do + subject(:bar) { Bar.new } let(:foo) { Foo.new } end RUBY end - it 'ignores overridden variables' do + it 'ignores `#subject!`' do expect_no_offenses(<<~RUBY) describe Foo do + subject!(:bar) { Bar.new } 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 overridden nameless subjects' do + it 'ignores `#subject` without a name' 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 + let(:bar) { Bar.new } end RUBY end - it 'flags an offense when too many `#let` calls are nested' do + it 'flags nested `#let`' do expect_offense(<<~RUBY) describe Foo do let(:foo) { Foo.new } @@ -102,7 +85,7 @@ RUBY end - it 'does not flag an offense when `#let` calls are distributed' do + it 'ignores distributed `#let`' do expect_no_offenses(<<~RUBY) describe Foo do context 'when bloo' do @@ -116,38 +99,49 @@ RUBY end - it 'does not flag an offense when using `#before`' do - expect_no_offenses(<<~RUBY) - describe Foo do - before { foo } - end - RUBY - end - context 'when using AllowSubject configuration', :config do - let(:cop_config) { { 'Max' => 0, 'AllowSubject' => true } } + let(:cop_config) { { 'Max' => 1, 'AllowSubject' => false } } - it 'flags an offense when using `#let`' do + it 'flags `#subject` without name' do expect_offense(<<~RUBY) describe Foo do - ^^^^^^^^^^^^^^^ Example group has too many memoized helpers [1/0] + ^^^^^^^^^^^^^^^ Example group has too many memoized helpers [2/1] + subject { Foo.new } let(:foo) { Foo.new } end RUBY end - it 'does not flag an offense when using `#subject` without a name' do - expect_no_offenses(<<~RUBY) + it 'flags `#subject`' do + expect_offense(<<~RUBY) describe Foo do - subject { Foo.new } + ^^^^^^^^^^^^^^^ Example group has too many memoized helpers [2/1] + subject(:foo) { Foo.new } + let(:bar) { Foo.new } end RUBY end - it 'does not flag an offense when using `#subject` with a name' do + 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) { Foo.new } + subject { Foo.new } + context do + subject { Foo.new(1) } + context do + subject { Foo.new(1, 2) } + end + end end RUBY end From 986537f5872ea2c67abf189db8b5e97b223e6b08 Mon Sep 17 00:00:00 2001 From: Phil Pirozhkov Date: Sun, 9 Aug 2020 14:53:57 +0300 Subject: [PATCH 9/9] Improve the performance of MultipleMemoizedHelpers Fetching memoized helpers for each of the ancestors is not optimal, is not optimal and has non-linear complexity. --- .../cop/rspec/multiple_memoized_helpers.rb | 19 +++++++++++++------ 1 file changed, 13 insertions(+), 6 deletions(-) diff --git a/lib/rubocop/cop/rspec/multiple_memoized_helpers.rb b/lib/rubocop/cop/rspec/multiple_memoized_helpers.rb index ea19b78ce..4ad7a0cf2 100644 --- a/lib/rubocop/cop/rspec/multiple_memoized_helpers.rb +++ b/lib/rubocop/cop/rspec/multiple_memoized_helpers.rb @@ -100,8 +100,14 @@ def on_block(node) 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), @@ -110,13 +116,14 @@ def all_helpers(node) end def 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) + @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 end def variable_nodes(node)