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.

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

RSpec/SubjectStub. Refactor and decrease complexity #925

Merged
merged 6 commits into from Jun 11, 2020
72 changes: 25 additions & 47 deletions lib/rubocop/cop/rspec/subject_stub.rb
@@ -1,5 +1,7 @@
# frozen_string_literal: true

require 'set'

module RuboCop
module Cop
module RSpec
Expand Down Expand Up @@ -75,70 +77,46 @@ class SubjectStub < Cop

def on_block(node)
return unless example_group?(node)
return unless (processed_example_groups & node.ancestors).empty?
Copy link
Member

Choose a reason for hiding this comment

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

How big are your specs so this makes a significant difference?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I ran Rubocop on its own source code so the test suite isn't large enough.

Regarding significance... This check avoids repeated analyzing of nested context/describe and I assume (I didn't test this TBH) it decrease the execution time by a factor of 2/3/4/... it depends on how many levels of nesting there are in the spec file.

Copy link
Member

Choose a reason for hiding this comment

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

I would prefer to avoid optimizations while sacrificing simplicity if we don't have enough proof that it gets us significant performance benefits. Also, long specs are hard to read and change. I'm not saying that we should deliberately make our tools slow to induce frustration when working with such specs, but primarily aiming to work with that kind of specs is not our aim.

Copy link
Member

Choose a reason for hiding this comment

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

Regarding the performance testing in the wild - you can pick some projects out of this list, I find https://github.com/discourse/discourse/ specifically good.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Checked this optimization on Gitlab source code and got the following results:

  • it speeds up the cope from 13% to 5.8% of samples
  • the whole execution time decreased by ~10s

Command to reproduce:

time bundle exec exe/rubocop --cache false --out gitlab.out --force-default-config --require rubocop-rspec --only RSpec/SubjectStub ../rubocop-profiling-examples/gitlabhq/spec

Copy link
Member

Choose a reason for hiding this comment

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

You can use on_top_level describe and then search for example groups, in order to avoid double lookups. E.g. once you are in an example group, you can break from going to nested example groups,

Copy link
Member

Choose a reason for hiding this comment

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

@Darhazer This is a truly amazing proposal.

Copy link
Member

Choose a reason for hiding this comment

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

But then it turns out that RuboCop::RSpec::TopLevelDescribe#on_send which triggers on_top_level_descrive, is one of the slowest methods

Copy link
Member

Choose a reason for hiding this comment

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

Ha, fair enough. Never looked closely at TopLevelDescribe. Frankly, I'm having a hard time understanding why it's implemented that way. Should be: "call on_top_level_describe if method name is :describe, and there are no ancestors of the type :block with a method name :describe". It'd better keep a set of already detected TLDs to quickly return from on_send if (node.ancestors & top_level_describes).any? just like Andrew has implemented in this pull request.

Luckily, it doesn't fit our purposes in this pull request anyway, since it's only for describe:

return false unless node.method_name == :describe

while we need something like TopLevelExampleGroup, probably implemented in a way I suggest above, but focusing on all example group methods, not only :describe.

Would you like to give this idea a shot in the scope of this pull request, or do you feel this improvement that you've done is good on its own already, @andrykonchin?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would love to optimize TopLevelDescribe but think it deserves a separate PR.

Currently I am working on optimizing cops from the main rubocop gem but will definitely look at rubocop-rspec after that.

P.S.

But then it turns out that RuboCop::RSpec::TopLevelDescribe#on_send which triggers on_top_level_descrive, is one of the slowest methods

Switching from manual caching to #on_top_level_describe callback decreased profiler samples for SubjectStub cop from 0.9% to 0% so it isn't as bad as you may think 😉.

pirj marked this conversation as resolved.
Show resolved Hide resolved

processed_example_groups << node
pirj marked this conversation as resolved.
Show resolved Hide resolved
@explicit_subjects = find_all_explicit_subjects(node)

find_subject_stub(node) do |stub|
find_subject_expectations(node) do |stub|
add_offense(stub)
end
end

private

# Find subjects within tree and then find (send) nodes for that subject
#
# @param node [RuboCop::Node] example group
#
# @yield [RuboCop::Node] message expectations for subject
def find_subject_stub(node, &block)
find_subject(node) do |subject_name, context|
find_subject_expectation(context, subject_name, &block)
end
def processed_example_groups
@processed_example_groups ||= Set.new
end

# Find a subject message expectation
#
# @param node [RuboCop::Node]
# @param subject_name [Symbol] name of subject
#
# @yield [RuboCop::Node] message expectation
def find_subject_expectation(node, subject_name, &block)
# Do not search node if it is an example group with its own subject.
return if example_group?(node) && redefines_subject?(node)

# Yield the current node if it is a message expectation.
yield(node) if message_expectation?(node, subject_name)
def find_all_explicit_subjects(node)
node.each_descendant(:block).each_with_object({}) do |child, h|
name = subject(child)
next unless name

# Recurse through node's children looking for a message expectation.
node.each_child_node do |child|
find_subject_expectation(child, subject_name, &block)
end
end
outer_example_group = child.each_ancestor.find do |a|
example_group?(a)
end

# Check if node's children contain a subject definition
#
# @param node [RuboCop::Node]
#
# @return [Boolean]
def redefines_subject?(node)
node.each_child_node.any? do |child|
subject(child) || redefines_subject?(child)
h[outer_example_group] ||= []
h[outer_example_group] << name
end
end

# Find a subject definition
#
# @param node [RuboCop::Node]
# @param parent [RuboCop::Node,nil]
#
# @yieldparam subject_name [Symbol] name of subject being defined
# @yieldparam parent [RuboCop::Node] parent of subject definition
def find_subject(node, parent: nil, &block)
# An implicit subject is defined by RSpec when no subject is declared
subject_name = subject(node) || :subject
def find_subject_expectations(node, subject_names = [], &block)
subject_names = @explicit_subjects[node] if @explicit_subjects[node]

yield(subject_name, parent) if parent
expectation_detected = (subject_names + [:subject]).any? do |name|
message_expectation?(node, name)
end
return yield(node) if expectation_detected
pirj marked this conversation as resolved.
Show resolved Hide resolved

node.each_child_node do |child|
find_subject(child, parent: node, &block)
find_subject_expectations(child, subject_names, &block)
end
end
end
Expand Down
20 changes: 20 additions & 0 deletions spec/rubocop/cop/rspec/subject_stub_spec.rb
Expand Up @@ -20,6 +20,26 @@
RUBY
end

it 'flags when subject is stubbed and there are several named subjects ' \
'in the same example group' do
expect_offense(<<-RUBY)
describe Foo do
subject(:foo) { described_class.new }
subject(:bar) { described_class.new }
subject(:baz) { described_class.new }

before do
allow(bar).to receive(:bar).and_return(baz)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Do not stub methods of the object under test.
end

it 'uses expect twice' do
expect(foo.bar).to eq(baz)
end
end
RUBY
end

it 'flags when subject is mocked' do
expect_offense(<<-RUBY)
describe Foo do
Expand Down