Improve cops by using TopLevelGroup #977

pirj · 2020-07-22T22:32:19Z

The previously used TopLevelDescribe suffered a few flaws:

it was slow: RSpec/SubjectStub. Refactor and decrease complexity #925 (comment)
it ignored non-describe top-level example groups RSpec/SubjectStub. Refactor and decrease complexity #925 (comment)

This pull request fixes those problems by using a newly introduced TopLevelGroup.
Also fixes a flaw in TopLevelGroup when example groups wrapped inside module or class were missed.

NOT DONE: I'm on the fence if we should deprecate TopLevelDescribe before removing it. We most probably don't have much releases left before 2.0, and the majority of people will jump straight to 2.0 skipping that pre-2.0 release. We may add this pull request (and others that replace usages of TopLevelDescribe for TopLevelGroup) as reference examples for migration.

Fixes #948

Before submitting the PR make sure the following are checked:

Feature branch is up-to-date with master (if not - rebase it).
Squashed related commits together.
Added tests.
Updated documentation.
Added an entry to the CHANGELOG.md if the new code introduces user-observable changes.
The build (bundle exec rake) passes (be sure to run this locally, since it may produce updated documentation that you will need to commit).

pirj · 2020-07-22T22:40:51Z

lib/rubocop/cop/rspec/file_path.rb

-            node,
-            message: format(MSG, suffix: glob)
-          )
+          ensure_correct_file_path(send_node, described_class, arguments)


Code climate insisted on breaking this method apart.

spec/rubocop/cop/rspec/multiple_describes_spec.rb

pirj · 2020-07-22T22:42:27Z

spec/rubocop/cop/rspec/nested_groups_spec.rb

@@ -35,11 +54,32 @@
    expect(cop.config_to_allow_offenses[:exclude_limit]).to eq('Max' => 4)
  end

-  it 'ignores non-spec context methods' do


It wasn't ignoring. Just the example didn't have enough nesting to trigger the offence.

Why? - it was slow #925 (comment) - it ignored non-describe top-level example groups #925 (comment) `TopLevelGroup` is a modern replacement for `TopLevelDescribe`. Examples how to migrate cops from TopLevelDescribe to TopLevelGroup: - #932 - #977

lib/rubocop/cop/rspec/describe_class.rb

lib/rubocop/cop/rspec/describe_method.rb

spec/rubocop/cop/rspec/multiple_describes_spec.rb

pirj

@Darhazer Addressed your notes and went a few steps further in hope to foresee your next round of code review notes :)

@bquorning Please take a look.

I didn't do the performance impact. Our spec/rubocop speeds seem to be in the range of a measurement error.

pirj · 2020-07-23T20:02:11Z

lib/rubocop/cop/rspec/describe_class.rb

+              !(send #{RSPEC} :describe ... (hash <#rails_metadata? ...>)) ]
+            ...
+          )
+        PATTERN


Collapsed several significantly overlapping node patterns into one.
Moved guard statements into node pattern as well.

There still seems to be some redundancy in this node pattern, similarly to SubjectStub:

{ (block (send nil? :subject (sym $_)) args ...) (block (send nil? $:subject) args ...) }

I'll open a ticket for rubocop-ast.

Is [ ...] a new syntax for { ... }? Does it work on the minimum supported rubocop version or should we bump the requirements?

[ ] is logical "and", it's around for a while, just not too frequently used.

I find it too hard to reason about boolean login, this node pattern sounds like:

a block with a send of `describe` to `RSpec`/nil? where the first argument is neither a const not a string resembling a const *and* the rest of the arguments don't contain a hash with Rails-like metadata

I'm confused by the (send #rspec? :describe part in the second argument of [ ]

@Darhazer Why?

It's basically the same as previously (which I forgot to remove during merge resolution):

def_node_matcher :describe_with_rails_metadata?, <<-PATTERN (send #rspec? :describe !const ... (hash <#rails_metadata? ...>) ) PATTERN

!const check is not necessary anymore, it's checked in the first part of [ ].

I too find this node pattern a bit cryptic. Perhaps a bit more duplication can be justified if it makes this code easier to read?

Good point guys. Split this node pattern into two parts. Looks more readable now.

FYI the shiny newly released rubocop-ast allows comments in patterns

pirj · 2020-07-23T20:03:27Z

lib/rubocop/cop/rspec/describe_method.rb

+          second_argument = second_argument(node)
+
+          return unless second_argument
+          return if second_argument.str_content.start_with?('#', '.')


I'm ok to turn this into a method and pushing this into the node pattern, but decided not to fix everything in one go.

lib/rubocop/cop/rspec/file_path.rb

pirj · 2020-07-23T20:17:34Z

lib/rubocop/cop/rspec/file_path.rb

-            node,
-            message: format(MSG, suffix: glob)
-          )
+            ensure_correct_file_path(send_node, described_class, arguments)


Code climate insisted on this extraction.

pirj · 2020-07-23T20:19:23Z

lib/rubocop/rspec/top_level_group.rb

-      def on_block(node)
-        return unless respond_to?(:on_top_level_group)
-        return unless top_level_group?(node)
+      def on_new_investigation


This is a hair more efficient than on_block.

lib/rubocop/rspec/top_level_group.rb

pirj · 2020-07-23T20:21:09Z

lib/rubocop/rspec/top_level_group.rb

+      def top_level_nodes(node)
+        if node.begin_type?
+          node.children
+        elsif node.module_type? || node.class_type?


Previously,

module A class B RSpec.describe 'C' do

was missed.

pirj · 2020-07-23T20:33:16Z

spec/rubocop/cop/rspec/nested_groups_spec.rb

+          context 'when bar' do
+            context 'when baz' do
+              context 'when qux' do
+              ^^^^^^^^^^^^^^^^^^ Maximum example group nesting exceeded [4/3].


shared_examples_for is not counted.
If we count it in, even one of our own specs triggers an offence:

RSpec.describe RuboCop::Cop::RSpec::PredicateMatcher, :config do context 'when enforced style is `explicit`' do shared_examples 'explicit common' do context 'when custom matchers are allowed' do ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Maximum example group nesting exceeded [4/3].

pirj · 2020-07-28T14:24:12Z

Ping @bquorning @Darhazer

Darhazer · 2020-07-29T13:28:18Z

lib/rubocop/cop/rspec/describe_class.rb

+              !(send #{RSPEC} :describe ... (hash <#rails_metadata? ...>)) ]
+            ...
+          )
+        PATTERN


Is [ ...] a new syntax for { ... }? Does it work on the minimum supported rubocop version or should we bump the requirements?

lib/rubocop/cop/rspec/describe_method.rb

Darhazer · 2020-07-29T13:31:43Z

lib/rubocop/rspec/top_level_group.rb

+      def on_new_investigation
+        top_level_groups.each do |node|
+          if respond_to?(:on_top_level_example_group)
+            example_group?(node, &method(:on_top_level_example_group))


and we can have the same for other methods like describe

Yep, if we'll eventually need them.

Initially, I was going to add code that would call on_top_level_shared_group, but that would only count one usage, so decided not to add it just now.

lib/rubocop/rspec/top_level_group.rb

pirj · 2020-07-29T20:00:02Z

Simplified TopLevelGroup by introducing dummy methods.
I've removed TopLevelGroup's checks if those methods exist in the including class, since of TopLevelGroup is included, that means that one of those methods is overridden. Hence, we can use dummy methods and call them instead of checking if they exist in the consumer.

pirj · 2020-08-01T14:32:13Z

@bquorning @Darhazer ping

lib/rubocop/rspec/top_level_group.rb

lib/rubocop/cop/rspec/file_path.rb

bquorning · 2020-08-02T20:27:12Z

lib/rubocop/rspec/top_level_group.rb

-      def single_top_level_group?
-        top_level_groups.one?
+      def top_level_groups
+        @top_level_groups ||=


The memoization needs to be reset on each new investigation, right?

In all cases it is recommended to to invalidate your cache data during the on_new_investigation callback

https://github.com/rubocop-hq/rubocop/blob/6b36c4490c2d7c41349e4f71721bf0cf91920578/docs/modules/ROOT/pages/v1_upgrade_notes.adoc#if-your-class-uses-instance-variables

I was following this note:

Right now a new cop instance is created for every file

There might have been some changes after the doc has been introduced.

As a quick experiment, I've set a breakpoint in random cop's initialize and ran rubocop (0.88.0):

From: /Users/pirj/source/rubocop-rspec/lib/rubocop/cop/rspec/described_class.rb:63 RuboCop::Cop::RSpec::DescribedClass#initialize: 61: def initialize(*args) 62: require 'pry'; binding.pry => 63: super 64: end [1] pry(#<RuboCop::Cop::RSpec::DescribedClass>)> . From: /Users/pirj/source/rubocop-rspec/lib/rubocop/cop/rspec/described_class.rb:63 RuboCop::Cop::RSpec::DescribedClass#initialize: 61: def initialize(*args) 62: require 'pry'; binding.pry => 63: super 64: end [1] pry(#<RuboCop::Cop::RSpec::DescribedClass>)> . From: /Users/pirj/source/rubocop-rspec/lib/rubocop/cop/rspec/described_class.rb:63 RuboCop::Cop::RSpec::DescribedClass#initialize: 61: def initialize(*args) 62: require 'pry'; binding.pry => 63: super 64: end [1] pry(#<RuboCop::Cop::RSpec::DescribedClass>)> . From: /Users/pirj/source/rubocop-rspec/lib/rubocop/cop/rspec/described_class.rb:63 RuboCop::Cop::RSpec::DescribedClass#initialize: 61: def initialize(*args) 62: require 'pry'; binding.pry => 63: super 64: end [1] pry(#<RuboCop::Cop::RSpec::DescribedClass>)> .

@marcandre Can you please shed light on this? Isn't this doc misleading?

I used to think it would be quite hard to reuse cop instances since depending on the directory they may receive different config (due to directory-local .rubocop.yml).

The cop mentioned in the referenced discussion, SurroundingSpace does reset its state:

def on_new_investigation @token_table = nil

What's even more strange:

def on_new_investigation print "N" super end def initialize(*args) print "#" super end

outputs:

##.#.#.#.#.#.#.#.#.#.#.#.#.#.#.#.#.#.#.#.#.#C#.#.#.#.#.#.#.#.#.#.#.#.#.#.#.#.#.#.#.#.#.#.#.#.#.#.#.#.#.#.#.#.#.#.#.#.#.#.#.#.#.#.#.#.#.#.#.#.#.#.#.#.#.#.#.#.#.#.#.#.#.#.#.#.#.#.#.#.#.#.#.#.#.#.#.#.#.#.#.#.#.#.#.#.#.#.#.#.#.#.#.#N.#N.#N.#N.#N.#N.#N.#N.#N.#N.#N.#N.#N.#N.#N.#N.#N.#N.#N.#N.#N.#N.#N.#N.#N.#N.#N.#N.#N.#N.#N.#N.#N.#N.#N.#N.#N.#N.#N.#N.#N.#N.#N.#N.#N.#N.#N.#N.#N.#N.#N.#N.#N.#N.#N.#N.#N.#N.#N.#N.#N.#N.#N.#N.#N.#N.#N.#N.#N.#N.#N.#N.#N.#N.#N.#N.#N.#N.#N.#N.#N.#N.#N.#N.#N.#N.#N.#N.#N.#N.#N.#N.#N.#N.#N.#N.#N.#N.#N.#N.#.

reduced for better visibility:

#.#.#.#.#.#.#.#.#N.#N.#N.#N.#N.#N.#N.

🤔

@pirj Currently, cops being persisted is opt-in. That being said, I'm wondering if a cop that implements on_new_investigation shouldn't be persisted by default.
BTW, different config => new cop, but that's a very rare setup.
It looks like the cop is initialized a bunch of times before doing the actual run, that seems like a bug somewhere, I'll check it out.

Cop specs won't be bringing memoization to the light, will they? With all numerous well-unknown RuboCop extensions this can have quite an impact.

Yes, that was my thinking too.

I don't see a reason to disrespect this option.

Absolutely :-)

However, does it make sense to keep this notice for the rest of the cops

You mean the recommendation to clear the data with on_new_investigation? I guess it's not necessary. I should also be clearer reword the "will be called on different if" to "will not be called unless"...

I would remove the whole "If your class uses instance variables" section, as it's more confusing than helpful considering the current "one source" - "one cop instance" behaviour.

Even for those cops that need to be persisted, I can think of ~~global variables~~ class variables to persist the data between the runs.

By the way, there's one more thing that can impact those persisted cops - --parallel. Do we parallelize by cops or by source?

=> rubocop/rubocop#8440

Even for those cops that need to be persisted, I can think of class variables to persist the data between the runs.

Not quite... See rubocop/rubocop#7968 if interested

By the way, there's one more thing that can impact those persisted cops - --parallel. Do we parallelize by cops or by source?

I imagine it's by source, but we could segregate those cops that need it; I suspect they simply run them without --parallel though.

@bquorning As a bottom-line: I wouldn't worry about @top_level_groups ||= used here unless any of the cops would decide to opt-in for reusage and use TopLevelGroup which I highly doubt.

lib/rubocop/rspec/top_level_group.rb

lib/rubocop/cop/rspec/file_path.rb

lib/rubocop/cop/rspec/describe_method.rb

bquorning · 2020-08-02T21:23:09Z

lib/rubocop/cop/rspec/describe_class.rb

+              !(send #{RSPEC} :describe ... (hash <#rails_metadata? ...>)) ]
+            ...
+          )
+        PATTERN


I too find this node pattern a bit cryptic. Perhaps a bit more duplication can be justified if it makes this code easier to read?

bquorning

I think the changes look very good now. I’m curious to hear @Darhazer comment as well.

Example groups were not nested enough to demonstrate the offence. Describing inside a module and a class is actually quite legitimate, but the limit for nested example groups still applies.

TopLevelDescribe was only able to detect top-level `describe`. TopLevelGroup had a deficiency of missing top-level example groups wrapped in module and class namespaces.

Also, shared groups now

pirj · 2020-08-03T21:19:55Z

Loud thinking

describe Weather do
  let(:temp) { 20.celsius }
  context "when it's warm" do
    let(:temp) { 30.celsius } # <== should it count as an additional `let`, or should it collapse with an existing one?

marcandre · 2020-08-04T06:51:41Z

Loud thinking

describe Weather do
  let(:temp) { 20.celsius }
  context "when it's warm" do
    let(:temp) { 30.celsius } # <== should it count as an additional `let`, or should it collapse with an existing one?

I assumed it was one. It makes even less sense to me to count the repeated let.

Why? - it was slow #925 (comment) - it ignored non-describe top-level example groups #925 (comment) `TopLevelGroup` is a modern replacement for `TopLevelDescribe`. Examples how to migrate cops from TopLevelDescribe to TopLevelGroup: - #932 - #977

Why? - it was slow rubocop/rubocop-rspec#925 (comment) - it ignored non-describe top-level example groups rubocop/rubocop-rspec#925 (comment) `TopLevelGroup` is a modern replacement for `TopLevelDescribe`. Examples how to migrate cops from TopLevelDescribe to TopLevelGroup: - rubocop/rubocop-rspec#932 - rubocop/rubocop-rspec#977

pirj self-assigned this Jul 22, 2020

pirj requested review from Darhazer and bquorning July 22, 2020 22:32

pirj force-pushed the replace-top_level_describe branch 2 times, most recently from 9b3146f to 32188de Compare July 22, 2020 22:43

pirj commented Jul 22, 2020

View reviewed changes

pirj mentioned this pull request Jul 23, 2020

Retire TopLevelDescribe #978

Merged

7 tasks

Darhazer reviewed Jul 23, 2020

View reviewed changes

lib/rubocop/cop/rspec/describe_class.rb Outdated Show resolved Hide resolved

lib/rubocop/cop/rspec/describe_method.rb Show resolved Hide resolved

spec/rubocop/cop/rspec/multiple_describes_spec.rb Outdated Show resolved Hide resolved

pirj marked this pull request as draft July 23, 2020 15:48

pirj force-pushed the replace-top_level_describe branch from 82099fd to 74e094f Compare July 23, 2020 15:50

pirj commented Jul 23, 2020

View reviewed changes

pirj requested a review from Darhazer July 23, 2020 20:42

pirj marked this pull request as ready for review July 23, 2020 20:42

Darhazer reviewed Jul 29, 2020

View reviewed changes

pirj force-pushed the replace-top_level_describe branch 2 times, most recently from d829ec5 to b02f725 Compare July 29, 2020 19:59

pirj force-pushed the replace-top_level_describe branch from b02f725 to 9634e85 Compare July 29, 2020 20:06

pirj force-pushed the replace-top_level_describe branch 2 times, most recently from 33399eb to 89c3550 Compare August 2, 2020 16:48

bquorning reviewed Aug 2, 2020

View reviewed changes

pirj force-pushed the replace-top_level_describe branch 6 times, most recently from 7bcab69 to edddcfe Compare August 2, 2020 23:29

pirj force-pushed the replace-top_level_describe branch from edddcfe to 7714312 Compare August 2, 2020 23:31

marcandre mentioned this pull request Aug 2, 2020

Cops are initialized too much rubocop/rubocop#8436

Closed

bquorning approved these changes Aug 3, 2020

View reviewed changes

pirj added 7 commits August 3, 2020 18:58

Fix misleading spec

4070d70

Example groups were not nested enough to demonstrate the offence. Describing inside a module and a class is actually quite legitimate, but the limit for nested example groups still applies.

Detect all kinds of example groups in NestedGroups

330929f

TopLevelDescribe was only able to detect top-level `describe`. TopLevelGroup had a deficiency of missing top-level example groups wrapped in module and class namespaces.

Detect all kinds of example groups in FilePath cop

5f46a62

Refactor DescribeMethod to use TopLevelGroup

df0ea6e

Detect all kinds of example groups in MultipleDescribes

6276370

Also, shared groups now

Refactor DescribeClass to use TopLevelGroup

5f715e3

Remove redundant require

130e720

pirj force-pushed the replace-top_level_describe branch from 7714312 to 130e720 Compare August 3, 2020 15:58

pirj requested a review from Darhazer August 3, 2020 19:27

Darhazer approved these changes Aug 4, 2020

View reviewed changes

pirj merged commit 90a6067 into master Aug 4, 2020

pirj deleted the replace-top_level_describe branch August 4, 2020 09:29

pirj mentioned this pull request Aug 17, 2020

Fix RSpec/FilePath when checking an empty class #1000

Merged

5 tasks

pirj mentioned this pull request Nov 5, 2020

Use case instead of if #1075

Merged

3 tasks

Improve cops by using TopLevelGroup #977

Improve cops by using TopLevelGroup #977

Conversation

pirj commented Jul 22, 2020 • edited

pirj Jul 22, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pirj left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pirj Jul 29, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pirj commented Jul 28, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pirj commented Jul 29, 2020

pirj commented Aug 1, 2020

Choose a reason for hiding this comment

pirj Aug 2, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pirj Aug 2, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bquorning left a comment

Choose a reason for hiding this comment

pirj commented Aug 3, 2020

marcandre commented Aug 4, 2020

pirj commented Jul 22, 2020 •

edited

pirj Jul 22, 2020 •

edited

pirj Jul 29, 2020 •

edited

pirj Aug 2, 2020 •

edited

pirj Aug 2, 2020 •

edited