-
-
Notifications
You must be signed in to change notification settings - Fork 276
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add MultipleMemoizedHelpers cop #863
Changes from all commits
74dde0e
d761fc3
3732a9c
418c7fc
3fcd0bb
c3ab68f
8d7b3c9
edb87e8
986537f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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 [%<count>d/%<max>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)) | ||
pirj marked this conversation as resolved.
Show resolved
Hide resolved
|
||
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)) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In deeply nested code, or very long spec files, I imagine this line has the potential to be a performance hotspot, right? We might want to do some memoization for each ancestor. I’d need to do some measurement to figure out if it’s really a problem or not. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @bquorning Fair enough.
The results (cleaned up, no idea how to get a more compact output by default):
I'm not certain how to interpret this, is it the first or the second percentage that is important. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. First, we should determine if I ran the same command as yours, but on another spec suite. Next, I search the stackprof result for all
The final lines of my result is
So it seems that There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added a trivial memoization. Before
After
It doesn't look like those results are very consistent, as other cops are shuffled. But at least Is this good enough, @bquorning ? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, sampling results aren’t always consistent. But moving from 27000 samples down to 7600 is a significant change for a relatively simple memoization. Thank you for taking the time to implement this. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Happy to do so, especially when it's that trivial. |
||
] | ||
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I personally don't have many doubts that this cop should be enabled by default. Open for the discussion, though.
Just in case, we only have a handful of disabled cops, and we encourage (not documented) to tune the configuration to meet the project style, not bend all projects to our defaults.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm fine with having it enabled, with
AllowSubject
true, by defaultThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about
Enabled: pending
? This cop broke my builds.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@AlexWayfer We didn't formally agree to adhere to this policy yet, neither we're sure that it works for extensions.
We'll do our best to make it work and to introduce new cops between 2.0 and 3.0 as
pending
.Meanwhile - please accept my apologies.