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

Add MultipleMemoizedHelpers cop #863

Merged
merged 9 commits into from
Aug 9, 2020
Merged

Add MultipleMemoizedHelpers cop #863

merged 9 commits into from
Aug 9, 2020

Conversation

mockdeep
Copy link
Contributor

Fixes #862

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.
  • Added an entry to the changelog 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).

If you have created a new cop:

  • Added the new cop to config/default.yml.
  • The cop documents examples of good and bad code.
  • The tests assert both that bad code is reported and that good code is not reported.

Copy link
Member

@pirj pirj left a comment

Choose a reason for hiding this comment

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

Looks good!

CHANGELOG.md Outdated Show resolved Hide resolved
lib/rubocop/cop/rspec/no_let.rb Outdated Show resolved Hide resolved
spec/rubocop/cop/rspec/no_let_spec.rb Outdated Show resolved Hide resolved
spec/rubocop/cop/rspec/no_let_spec.rb Outdated Show resolved Hide resolved
@mockdeep
Copy link
Contributor Author

@pirj updated per your comments. Thanks for the review!

Copy link
Member

@pirj pirj left a comment

Choose a reason for hiding this comment

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

Nicely done!
Please remove TODO. Otherwise no objections to merge this.

@mockdeep
Copy link
Contributor Author

Woops, TODO removed.

lib/rubocop/cop/rspec/no_let.rb Outdated Show resolved Hide resolved
lib/rubocop/cop/rspec/no_let.rb Outdated Show resolved Hide resolved
@mockdeep
Copy link
Contributor Author

Discussing renaming the rule back in the issue.

@pirj
Copy link
Member

pirj commented Jan 24, 2020

Let's keep this as is, as this suits your needs. I'm pretty sure this will come handy for other people. cc @sshaw

Do you happen to use let!/subject! in your codebase, @mockdeep ? It's a trivial change as @Darhazer suggests to detect and flag them as well.

@mockdeep
Copy link
Contributor Author

@pirj I'm happy to implement the other approach. It'll help narrow down in codebases that already make heavy use of let and keep it to a reasonable level where people aren't as dogmatic about it as I am.

Sorry about the delay in response. I've been home with a sick kid.

@pirj
Copy link
Member

pirj commented Mar 19, 2020

Didn't intend to push a commit, thought it will add a suggested edit Github #$%^$

So, the plan to match what was discussed in the ticket:

  • Rename to MemoizedHelpersInExampleGroup
  • add a Max setting
  • for each example group
def on_send(node)
  return unless example_group?(node)

find lets/subjects, put them to a Set by var's name (nil for nameless subject)

  • walk up the ancestor chain with each_ancestor, add their lets/subjects to that set
  • if the count in the set exceeds Max, raise an offence
  • Profit!

There should be plenty of examples in the code, but if you get stuck please don't hesitate to ping.

@mockdeep
Copy link
Contributor Author

@pirj I renamed the file. It might take me a few days to get to the rest of it. With the COVID-19 drama, I've got my kid home, so that leaves me like 1 hour a day of concentrated work time while he's napping.

@pirj
Copy link
Member

pirj commented Mar 20, 2020

No worries @mockdeep, take your time and take care.
I know how it is to have kids at home :D

@pirj
Copy link
Member

pirj commented Jun 9, 2020

@mockdeep A friendly reminder 🔔

@mockdeep
Copy link
Contributor Author

@pirj here's a working implementation. I'm planning on doing some cleanup on the tests and docs, as well as maybe de-duplicating some of the logic in ExampleGroup, but I'd appreciate any feedback you have in the meantime.

mockdeep added a commit to mockdeep/rubocop-rspec that referenced this pull request Jun 14, 2020
This refactors to add `shared_group?` and `spec_group?` node matchers in
`NodePattern` for use across cops. I wanted to check shared contexts in
the [rule that I'm working on][1] and noticed that there were similar
implementations in a handful of other places. I thought it was probably
worth abstracting upwards.

[1]: rubocop#863
@mockdeep mockdeep mentioned this pull request Jun 14, 2020
3 tasks
mockdeep added a commit to mockdeep/rubocop-rspec that referenced this pull request Jun 14, 2020
This refactors to add `shared_group?` and `spec_group?` node matchers in
`NodePattern` for use across cops. I wanted to check shared contexts in
the [rule that I'm working on][1] and noticed that there were similar
implementations in a handful of other places. I thought it was probably
worth abstracting upwards.

[1]: rubocop#863
Copy link
Member

@pirj pirj left a comment

Choose a reason for hiding this comment

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

Looks good!
I withheld the final approval until the extracted parts are merged.

config/default.yml Outdated Show resolved Hide resolved
lib/rubocop/cop/rspec/memoized_helpers_in_example_group.rb Outdated Show resolved Hide resolved
lib/rubocop/cop/rspec/memoized_helpers_in_example_group.rb Outdated Show resolved Hide resolved
@pirj pirj mentioned this pull request Jul 15, 2020
6 tasks
@pirj
Copy link
Member

pirj commented Aug 2, 2020

@mockdeep I lost track of what is the state of this.
Do you need some help, at least with rebasing on the latest master?

@mockdeep
Copy link
Contributor Author

mockdeep commented Aug 2, 2020

@pirj I just rebased it. Sorry, I've got this on my list, but it's always 3rd or 4th. I think it should be mostly good to go, so feel free to look it over and see if there's anything important I'm missing on it.

@pirj
Copy link
Member

pirj commented Aug 2, 2020

got this on my list, but it's always 3rd or 4th

Story of my life 👬

@pirj
Copy link
Member

pirj commented Aug 3, 2020

Note to self:

$ hub pr checkout 863
From github.com:rubocop-hq/rubocop-rspec
 ! [rejected]        refs/pull/863/head -> rf-no_let  (non-fast-forward)

can be fixed by:

git branch -D rf-no_let

@pirj pirj requested a review from bquorning August 3, 2020 01:26
@@ -394,6 +394,13 @@ RSpec/MultipleExpectations:
VersionChanged: '1.21'
StyleGuide: https://www.rubydoc.info/gems/rubocop-rspec/RuboCop/Cop/RSpec/MultipleExpectations

RSpec/MultipleMemoizedHelpers:
Description: Checks if example groups contain too many `let` and `subject` calls.
Enabled: false
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 personally be happy to enable it by default with AllowSubject: true and Max: 5.

Copy link
Member

Choose a reason for hiding this comment

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

Did you run stats on some real world projects to see how many offenses are generated at different Max levels?

This comment was marked as resolved.

Copy link
Member

Choose a reason for hiding this comment

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

I'm thinking of gathering a Real World RSpec, since Real World Ruby/Real World Rails don't use RSpec much.

Copy link
Member

Choose a reason for hiding this comment

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

Here it goes https://github.com/pirj/real-world-rspec/

Had to remove all .rubocop*.yml files around those projects before running rubocop on them. RuboCop hiccups when there's an incompatible TargetRubyVersion or a renamed cop in the configuraion.

$ rubocop --only RSpec/MultipleMemoizedHelpers ~/source/real-world-rspec
Inspecting 47132 files
Max helpers Offence count
Max: 5 24640
Max: 7 14418
Max: 10 6093

Notable examples (@mockdeep, fasten your seatbelts):
10 lets https://github.com/forem/forem/blob/master/spec/support/api_analytics_shared_examples.rb#L2
20 lets https://github.com/chef/chef/blob/master/spec/unit/provider/systemd_unit_spec.rb#L23
30 lets https://github.com/chef/chef/blob/master/spec/unit/policy_builder/policyfile_spec.rb#L24
40 lets https://github.com/diaspora/diaspora/blob/develop/spec/integration/migration_service_spec.rb#L7
50 lets https://github.com/chef/chef/blob/master/spec/functional/resource/dsc_script_spec.rb#L71

I can prepare the report per-repo, but I guess those two, diaspora and chef, are the worst offenders.

Copy link
Member

Choose a reason for hiding this comment

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

Also we have a cop for multiple subjects, so I doubt the need to check subject calls at all

I think subject counts towards memoized helpers, as its differences from let are subtle.

Even though with the default configuration we only allow one subject, some may want zero of them. I'm quite ok to make AllowSubject: true the default though for those who don't.

Copy link
Member

Choose a reason for hiding this comment

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

The numbers should be slightly lower now when subject doesn't count towards the total number by default.

A bug in RuboCop prevents from building a script that would run rubocop on each of 35 repositories and tell which ones offend the new cops most of all.

Here's an example pull request showing that it's possible to reduce the use memoized helpers to a reasonable number.

Copy link
Collaborator

@bquorning bquorning left a comment

Choose a reason for hiding this comment

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

Just a couple of nitpicks and a performance concern.

lib/rubocop/cop/rspec/multiple_memoized_helpers.rb Outdated Show resolved Hide resolved
def all_helpers(node)
[
*helpers(node),
*node.each_ancestor(:block).flat_map(&method(:helpers))
Copy link
Collaborator

Choose a reason for hiding this comment

The 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.

Copy link
Member

Choose a reason for hiding this comment

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

@bquorning Fair enough.
Ran it the following against gitlabhq and discourse:

$ bundle exec bin/rubocop-profile --only RSpec --force-default-config  --require rubocop-rspec ../real-world-rspec/gitlabhq ../real-world-rspec/discourse
$ stackprof tmp/stackprof.dump --method 'RuboCop::Cop::RSpec'

The results (cleaned up, no idea how to get a more compact output by default):

RuboCop::Cop::RSpec::Base#rspec_pattern (/Users/pirj/source/rubocop-rspec/lib/rubocop/cop/rspec/base.rb:47)
  samples:  25142 self (4.1%)  /   28918 total (4.8%)
RuboCop::Cop::RSpec::AnyInstance#disallowed_stub (/Users/pirj/source/rubocop-rspec/lib/rubocop/cop/rspec/any_instance.rb:28)
  samples:  1721 self (0.3%)  /   2268 total (0.4%)
RuboCop::Cop::RSpec::Base#relevant_rubocop_rspec_file? (/Users/pirj/source/rubocop-rspec/lib/rubocop/cop/rspec/base.rb:43)
  samples:  1555 self (0.3%)  /   30473 total (5.0%)
RuboCop::Cop::RSpec::ImplicitExpect#implicit_expect (/Users/pirj/source/rubocop-rspec/lib/rubocop/cop/rspec/implicit_expect.rb:33)
  samples:  1478 self (0.2%)  /   2480 total (0.4%)
RuboCop::Cop::RSpec::MessageSpies#message_expectation (/Users/pirj/source/rubocop-rspec/lib/rubocop/cop/rspec/message_spies.rb:38)
  samples:  1392 self (0.2%)  /   1758 total (0.3%)

[45 items skipped]

RuboCop::Cop::RSpec::MultipleMemoizedHelpers#on_block (/Users/pirj/source/rubocop-rspec/lib/rubocop/cop/rspec/multiple_memoized_helpers.rb:92)
  samples:   214 self (0.0%)  /   20335 total (3.4%)

[4 items skipped]

RuboCop::Cop::RSpec::MultipleMemoizedHelpers#all_helpers (/Users/pirj/source/rubocop-rspec/lib/rubocop/cop/rspec/multiple_memoized_helpers.rb:105)
  samples:   179 self (0.0%)  /   18647 total (3.1%)

[38 items skipped]

RuboCop::Cop::RSpec::MultipleMemoizedHelpers#allow_subject? (/Users/pirj/source/rubocop-rspec/lib/rubocop/cop/rspec/multiple_memoized_helpers.rb:135)
  samples:    51 self (0.0%)  /    161 total (0.0%)

[...]

  callees (18 total):
      18  (  100.0%)  RuboCop::Cop::Base.inherited

I'm not certain how to interpret this, is it the first or the second percentage that is important.

Copy link
Collaborator

Choose a reason for hiding this comment

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

First, we should determine if MultipleMemoizedHelpers is slower than the other cops. If its speed is average, there’s no need to dig further.

I ran the same command as yours, but on another spec suite. Next, I search the stackprof result for all on_* method calls on RSpec cops and order them by number of samples (20335 for on_block in your example above)

stackprof tmp/stackprof.dump --method 'RuboCop::Cop::RSpec.*#on_.*' |
  grep -B1 'samples.* total' |
  paste -d " "  - - - |
  sort -k8 -n |
  awk '{ print $8 " samples: " $1 }'

The final lines of my result is

485 samples: RuboCop::Cop::RSpec::RepeatedExample#on_block
517 samples: RuboCop::Cop::RSpec::RepeatedDescription#on_block
577 samples: RuboCop::Cop::RSpec::MultipleExpectations#on_block
844 samples: RuboCop::Cop::RSpec::SubjectStub#on_top_level_group
960 samples: RuboCop::Cop::RSpec::DescribedClass#on_block
1132 samples: RuboCop::Cop::RSpec::MultipleMemoizedHelpers#on_block

So it seems that SubjectStub, DescribedClass and MultipleMemoizedHelpers are significantly slower than our other cops. (Of course, configuration and the nature of the specs may play a role here)

Copy link
Member

Choose a reason for hiding this comment

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

Added a trivial memoization.
Before
713.80s user 42.25s system 80% cpu 15:42.32 total
After
663.91s user 41.03s system 89% cpu 13:10.78 total

Before

7407 samples: RuboCop::Cop::RSpec::MultipleSubjects#on_block
7969 samples: RuboCop::Cop::RSpec::LetSetup#on_block
7998 samples: RuboCop::Cop::RSpec::RepeatedExample#on_block
8527 samples: RuboCop::Cop::RSpec::RepeatedDescription#on_block
9783 samples: RuboCop::Cop::RSpec::MultipleExpectations#on_block
12220 samples: RuboCop::Cop::RSpec::SubjectStub#on_top_level_group
13026 samples: RuboCop::Cop::RSpec::DescribedClass#on_block
27289 samples: RuboCop::Cop::RSpec::MultipleMemoizedHelpers#on_block <===

After

6297 samples: RuboCop::Cop::RSpec::NamedSubject#on_block
6305 samples: RuboCop::Cop::RSpec::MultipleSubjects#on_block
6686 samples: RuboCop::Cop::RSpec::RepeatedExample#on_block
6846 samples: RuboCop::Cop::RSpec::RepeatedDescription#on_block
6939 samples: RuboCop::Cop::RSpec::LetSetup#on_block
7592 samples: RuboCop::Cop::RSpec::MultipleMemoizedHelpers#on_block <===
8599 samples: RuboCop::Cop::RSpec::MultipleExpectations#on_block
10258 samples: RuboCop::Cop::RSpec::SubjectStub#on_top_level_group

It doesn't look like those results are very consistent, as other cops are shuffled. But at least MultipleMemoizedHelpers isn't the by far the worst offender anymore.

Is this good enough, @bquorning ?

Copy link
Collaborator

Choose a reason for hiding this comment

The 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.

Copy link
Member

Choose a reason for hiding this comment

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

Happy to do so, especially when it's that trivial.

@pirj pirj assigned pirj and mockdeep Aug 8, 2020
and fix the build
Switching the cop on by default to match RSpec recommendations from its
docs and the community RSpec Style Guide recommendations.

    `let` _can_ enhance readability when used sparingly (1,2, or
    maybe 3 declarations) in any given example group, but that can
    quickly degrade with overuse. YMMV.

https://github.com/rspec/rspec-core/blob/5d87c28bb828ba14b54a0d8cfddf044894009314/lib/rspec/core/memoized_helpers.rb#L257

https://reddit.com/r/ruby/comments/73nrhk/ama_the_authors_of_effective_testing_with_rspec_3/dnsyanp/

https://rspec.rubystyle.guide/#let-blocks

Example change to dramatically reduce the number of used memoized
helpers without resorting to methods rubocop/rubocop#8447

There is no limit to the number of memoized helpers being used in
practice:

10 lets https://github.com/forem/forem/blob/master/spec/support/api_analytics_shared_examples.rb#L2
20 lets https://github.com/chef/chef/blob/master/spec/unit/provider/systemd_unit_spec.rb#L23
30 lets https://github.com/chef/chef/blob/master/spec/unit/policy_builder/policyfile_spec.rb#L24
40 lets https://github.com/diaspora/diaspora/blob/develop/spec/integration/migration_service_spec.rb#L7
50 lets https://github.com/chef/chef/blob/master/spec/functional/resource/dsc_script_spec.rb#L71

Allowing `subject` by default, i.e. subjects don't count towards the
number of memoized helpers, since there is a dedicated
`RSpec/MultipleSubjects` cop. Combination with it provides infinite
possibilities to tweak the maximum allowed number of memoized helpers to
meet the project style.
@@ -11,6 +11,9 @@
let (:foo) { 1 }
let! (:bar){}

bar = -> {}
let(:foo, &bar)
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 love that this file exists. A really clever way to ensure that future rule changes all account for weird ruby syntax.

@mockdeep
Copy link
Contributor Author

mockdeep commented Aug 8, 2020

@pirj thanks so much for taking the baton on this one. Looks great!

Fetching memoized helpers for each of the ancestors is not optimal, is
not optimal and has non-linear complexity.
@pirj
Copy link
Member

pirj commented Aug 9, 2020

Merging, since @Darhazer informally approved earlier.

@pirj pirj merged commit a2ef166 into rubocop:master Aug 9, 2020
@pirj
Copy link
Member

pirj commented Aug 9, 2020

@mockdeep Thank you!

@bquorning
Copy link
Collaborator

👍 Thank you.

marcandre added a commit to marcandre/rubocop-ast that referenced this pull request Nov 6, 2020
This single cop generates 372 offenses, e.g. "Example group has too many memoized helpers [19/5]"

rubocop/rubocop-rspec#863
marcandre added a commit to rubocop/rubocop-ast that referenced this pull request Nov 6, 2020
This single cop generates 372 offenses, e.g. "Example group has too many memoized helpers [19/5]"

rubocop/rubocop-rspec#863
@marcandre
Copy link
Contributor

For the record, this single cop that is enabled by default generates 311 offenses on the main rubocop gem and 372 offenses on the rubocop-ast gem.

@pirj
Copy link
Member

pirj commented Nov 7, 2020

@marcandre That reminds me of RSpec/ContextWording cop that generated ~11000 offences in a project I was working on. 😱 Those offences, plus hastily added "workarounds" were all fixed by a volunteered colleague of mine, @separatio.

For sure RuboCop's (and extensions') defaults don't fit all, and:

Ruby is all about diversity and doing things the way that makes you happy. Whatever style preferences you have RuboCop is there for you. That’s our promises and our guarantee. Within the subjective limits of sanity that is.

s/Ruby/RSpec/

@separatio
Copy link

separatio commented Nov 9, 2020

Thanks for the summon @pirj. I haven't really fixed ALL of the offenses, but few were indeed left.

@marcandre - by default, Rubocop generates offenses in a new Rails 6 project.
Under 100 offenses, but there are still a few tens of offenses IIRC.

The convention over configuration mindset is hard to overwrite and actually CONFIGURE Rubocop, but keep in mind that Rubocop gives the convention choice with some guidelines, and not setting it.

@pirj
Copy link
Member

pirj commented Nov 9, 2020

Off-topic: There's this https://github.com/toshimaru/rubocop-rails_config. Wondering if for newly-generated Rails app code with this config turned on there will be any offences.

@mockdeep mockdeep deleted the rf-no_let branch February 25, 2022 20:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cop Idea: NoLet
7 participants