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

Don't spend CPU finding the same node twice #960

Merged
merged 2 commits into from Jul 15, 2020
Merged

Don't spend CPU finding the same node twice #960

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
887a06d
Don't spend CPU finding the same node twice
bquorning Jul 10, 2020
fb10f1e
Improve readability of #check_previous_nodes
bquorning Jul 14, 2020
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
29 changes: 14 additions & 15 deletions lib/rubocop/cop/rspec/leading_subject.rb
View file
Open in desktop
Expand Up @@ -43,35 +43,34 @@ def on_block(node)
end

def check_previous_nodes(node)
node.parent.each_child_node do |sibling|
if offending?(sibling)
msg = format(MSG, offending: sibling.method_name)
add_offense(node, message: msg) do |corrector|
autocorrect(corrector, node)
end
offending_node(node) do |offender|
msg = format(MSG, offending: offender.method_name)
add_offense(node, message: msg) do |corrector|
autocorrect(corrector, node, offender)
end

break if offending?(sibling) || sibling.equal?(node)
end
end

private

def autocorrect(corrector, node)
first_node = find_first_offending_node(node)
def offending_node(node)
node.parent.each_child_node.find do |sibling|
break if sibling.equal?(node)

yield sibling if offending?(sibling)
end
end

def autocorrect(corrector, node, sibling)
RuboCop::RSpec::Corrector::MoveNode.new(
node, corrector, processed_source
).move_before(first_node)
).move_before(sibling)
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 does it change autocorrect with multiple offending nodes, or the first_offending_node was recalculated after each autocorrelation?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don’t think I understand your question. Could you add an example?

Copy link
Member

Choose a reason for hiding this comment

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

RSpec.describe User do
        let(:params) { foo } # first_offsending_node
        let(:bar) { baz } # sibling

        subject { described_class.new }
        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Declare `subject` above any other `let` declarations.
      end

Previously it would always move before the first offending node. Now in each iteration, it would move before the current offending node. Unless those are re-evaluated. I added the test and it's not broken.
And the reason for this is that we break after the first offending node, so sibling and first_offending_node are always the same. I missed that detail in the review

Copy link
Member

Choose a reason for hiding this comment

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

Perhaps the readability of the method could be improved as well

         def check_previous_nodes(node)
-          node.parent.each_child_node do |sibling|
-            if offending?(sibling)
-              msg = format(MSG, offending: sibling.method_name)
-              add_offense(node, message: msg) do |corrector|
-                autocorrect(corrector, node, sibling)
-              end
+          offending_node(node) do |offender|
+            msg = format(MSG, offending: offender.method_name)
+            add_offense(node, message: msg) do |corrector|
+              autocorrect(corrector, node, offender)
             end
-
-            break if offending?(sibling) || sibling.equal?(node)
           end
         end
 
+        def offending_node(node)
+          node.parent.each_child_node.find do |sibling|
+            break if sibling.equal?(node)
+
+            yield sibling if offending?(sibling)
+          end
+        end
+

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I like where this is going.

I tried replacing the last block body with

break if sibling.equal?(node)
break unless offending?(sibling)

yield sibling

and all tests still pass. if feels wrong. Do we need another test case, or is it ok to rewrite yield sibling if offending?(sibling)?

Copy link
Member

@Darhazer Darhazer Jul 14, 2020 •
edited

Choose a reason for hiding this comment

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

It could be some def or any other ruby code actually that goes before the subject

Edit: I see @pirj actually already answered. Should read all my mails before replying

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fyi @pirj calling each_child_node(:block) fails for the spec containing let(:user, &args[:build_user]).

Copy link
Member

Choose a reason for hiding this comment

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

As far as I remember, you can pass multiple node types, like each_child_node(:block, :blockpass)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I tried that but didn’t make it work :-)

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, the block_pass is an argument, and the node itself is a send.

s(:send, nil, :let,
  s(:sym, :user),
  s(:block_pass,
    s(:send,
      s(:send, nil, :args), :[],
      s(:sym, :build_user))))

You can do each_child_node(:block, :send). Not sure if is worthy

end

def offending?(node)
let?(node) || hook?(node) || example?(node)
Copy link
Member

Choose a reason for hiding this comment

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

Observation:
Shouldn't || example_group?(node) be added here?
I can think of:

describe do
  describe do
    ...
  end

  subject(:i_am_offended_by_a_preceeding_example_group_where_i_am_a_full_fledged_subject) { ... }
end

end

def find_first_offending_node(node)
node.parent.children.find { |sibling| offending?(sibling) }
end

def in_spec_block?(node)
node.each_ancestor(:block).any? do |ancestor|
Copy link
Member

Choose a reason for hiding this comment

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

Observation:
any? it feels weird. first should work just fine. Can't imagine subject(...) { } being defined in an iterator, neither I'm aware of any special syntax to wrap subject, e.g. isolated { subject(...) { } }. first should work fine for most cases.

example?(ancestor)
Expand Down
25 changes: 25 additions & 0 deletions spec/rubocop/cop/rspec/leading_subject_spec.rb
View file
Open in desktop
Expand Up @@ -135,4 +135,29 @@
end
RUBY
end

it 'checks also when subject is below a non-offending node' do
expect_offense(<<~RUBY)
RSpec.describe do
def helper_method
end

it { is_expected.to be_present }

subject { described_class.new }
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Declare `subject` above any other `it` declarations.
end
RUBY

expect_correction(<<~RUBY)
RSpec.describe do
def helper_method
end

subject { described_class.new }
it { is_expected.to be_present }

end
RUBY
end
end