Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Performance. RSpec/NestedGroups #950

Merged
merged 1 commit into from Jul 2, 2020
Merged

Performance. RSpec/NestedGroups #950

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
19 changes: 9 additions & 10 deletions lib/rubocop/cop/rspec/nested_groups.rb
View file
Open in desktop
Expand Up @@ -97,27 +97,26 @@ class NestedGroups < Cop
"Configuration key `#{DEPRECATED_MAX_KEY}` for #{cop_name} is " \
'deprecated in favor of `Max`. Please use that instead.'

def_node_search :find_contexts, ExampleGroups::ALL.block_pattern

def on_top_level_describe(node, _args)
find_nested_contexts(node.parent) do |context, nesting|
find_nested_example_groups(node.parent) do |example_group, nesting|
self.max = nesting
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if this works correctly, e.g. if there are two example groups on the first level, and the first one has a larger number of nested groups, would the max be set to that number, or to the nesting level of the last offending group?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should work correctly as far as we don't track nesting level and calculate it every time:

        def nesting_count(node)
          count = node.each_ancestor(:block).count { |n| example_group?(n) }
          count + 1
        end

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe I don't understand you well. What in your opinion may work incorrectly?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, not sure how this self.max value is used but looks like setting a value smaller that previous one is handled correctly:

      def max=(value)
        cfg = config_to_allow_offenses
        cfg[:exclude_limit] ||= {}
        current_max = cfg[:exclude_limit][max_parameter_name]
        value = [current_max, value].max if current_max          # <====
        cfg[:exclude_limit][max_parameter_name] = value
      end

https://github.com/rubocop-hq/rubocop/blob/master/lib/rubocop/cop/mixin/configurable_max.rb#L10-L16

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it's used when you run --auto-gen-config
Thank you for checking it though. I was going to verify myself, as it's not in the scope of the PR but I just noticed while reviewing the code

add_offense(
context.send_node,
example_group.send_node,
message: message(nesting)
)
end
end

private

def find_nested_contexts(node, nesting: 1, &block)
find_contexts(node) do |nested_context|
yield(nested_context, nesting) if nesting > max_nesting
def find_nested_example_groups(node, nesting: 1, &block)
example_group = example_group?(node)
yield node, nesting if example_group && nesting > max_nesting
bquorning marked this conversation as resolved.
Show resolved Hide resolved

next_nesting = example_group ? nesting + 1 : nesting

nested_context.each_child_node do |child|
find_nested_contexts(child, nesting: nesting + 1, &block)
end
node.each_child_node(:block, :begin) do |child|
find_nested_example_groups(child, nesting: next_nesting, &block)
end
end

Expand Down
16 changes: 16 additions & 0 deletions spec/rubocop/cop/rspec/nested_groups_spec.rb
View file
Open in desktop
Expand Up @@ -76,4 +76,20 @@ class MyThingy
).to_stderr
end
end

it 'counts nesting correctly when non-spec nesting' do
expect_offense(<<-RUBY)
describe MyClass do
context 'when foo' do
context 'when bar' do
[].each do |i|
context 'when baz' do
^^^^^^^^^^^^^^^^^^ Maximum example group nesting exceeded [4/3].
end
end
end
end
end
RUBY
end
end