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

@mockdeep mockdeep commented Jan 20, 2020

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

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 mockdeep force-pushed the mockdeep:rf-no_let branch from 2b8310d to 5d70d0b Jan 20, 2020
@mockdeep
Copy link
Contributor Author

@mockdeep mockdeep commented Jan 20, 2020

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

@pirj
pirj approved these changes Jan 20, 2020
Copy link
Member

@pirj pirj left a comment

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

@mockdeep mockdeep force-pushed the mockdeep:rf-no_let branch from 5d70d0b to 8d08f59 Jan 21, 2020
@mockdeep
Copy link
Contributor Author

@mockdeep mockdeep commented Jan 21, 2020

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

@mockdeep mockdeep commented Jan 21, 2020

Discussing renaming the rule back in the issue.

@pirj
Copy link
Member

@pirj 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

@mockdeep mockdeep commented Jan 24, 2020

@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 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 mockdeep force-pushed the mockdeep:rf-no_let branch from 72e5bca to 7407c51 Mar 20, 2020
@mockdeep
Copy link
Contributor Author

@mockdeep mockdeep commented Mar 20, 2020

@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 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 pirj commented Jun 9, 2020

@mockdeep A friendly reminder 🔔

@mockdeep mockdeep force-pushed the mockdeep:rf-no_let branch from 7407c51 to 5d51ade Jun 13, 2020
@mockdeep
Copy link
Contributor Author

@mockdeep mockdeep commented Jun 13, 2020

@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-hq#863
@mockdeep mockdeep mentioned this pull request Jun 14, 2020
3 of 3 tasks complete
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-hq#863
@mockdeep mockdeep force-pushed the mockdeep:rf-no_let branch from 5d51ade to e76b245 Jun 14, 2020
Copy link
Member

@pirj pirj left a comment

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

config/default.yml Outdated Show resolved Hide resolved
@pirj pirj mentioned this pull request Jul 15, 2020
3 of 6 tasks complete
@pirj
Copy link
Member

@pirj 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 mockdeep force-pushed the mockdeep:rf-no_let branch from e76b245 to c2c21a8 Aug 2, 2020
@mockdeep
Copy link
Contributor Author

@mockdeep 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 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 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 Aug 3, 2020
@pirj
pirj approved these changes Aug 3, 2020
@@ -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

This comment has been minimized.

@pirj

pirj Aug 3, 2020
Member

I would personally be happy to enable it by default with AllowSubject: true and Max: 5.

This comment has been minimized.

@Darhazer

Darhazer Aug 4, 2020
Member

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

This comment has been hidden.

@Darhazer

Darhazer Aug 4, 2020
Member

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

This comment has been minimized.

@pirj

pirj Aug 4, 2020
Member

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

This comment has been minimized.

@pirj

pirj Aug 6, 2020
Member

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.

This comment has been minimized.

@pirj

pirj Aug 6, 2020
Member

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.

This comment has been minimized.

@pirj

pirj Aug 7, 2020
Member

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.

variable_nodes(node).map do |variable_node|
if variable_node.block_type?
variable_definition?(variable_node.send_node)
else # block-pass (`let(:foo, &bar)`)

This comment has been minimized.

@pirj

pirj Aug 7, 2020
Member

Yet another candidate for the global block-pass support induced refactoring.

@pirj pirj requested a review from Darhazer Aug 7, 2020
@@ -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: true

This comment has been minimized.

@pirj

pirj Aug 7, 2020
Member

⚠️
I personally don't have many doubts that this cop should be enabled by default. Open for the discussion, though.

Just in case, we only have a handful of disabled cops, and we encourage (not documented) to tune the configuration to meet the project style, not bend all projects to our defaults.

This comment has been minimized.

@Darhazer

Darhazer Aug 7, 2020
Member

I'm fine with having it enabled, with AllowSubject true, by default

This comment has been minimized.

@AlexWayfer

AlexWayfer Aug 17, 2020
Contributor

What about Enabled: pending? This cop broke my builds.

This comment has been minimized.

@pirj

pirj Aug 17, 2020
Member

@AlexWayfer We didn't formally agree to adhere to this policy yet, neither we're sure that it works for extensions.
We'll do our best to make it work and to introduce new cops between 2.0 and 3.0 as pending.
Meanwhile - please accept my apologies.

@pirj pirj changed the title Add NoLet cop Add MultipleMemoizedHelpers cop Aug 7, 2020
@pirj
Copy link
Member

@pirj pirj commented Aug 7, 2020

@bquorning Do you mind to take a look?

The reasoning for turning it on by default:

@pirj pirj force-pushed the mockdeep:rf-no_let branch from e3b1ec9 to caf0f48 Aug 7, 2020
@pirj pirj force-pushed the mockdeep:rf-no_let branch from caf0f48 to 1a3d3fe Aug 7, 2020
Copy link
Member

@bquorning bquorning left a comment

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))

This comment has been minimized.

@bquorning

bquorning Aug 8, 2020
Member

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.

This comment has been minimized.

@pirj

pirj Aug 8, 2020
Member

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

This comment has been minimized.

@bquorning

bquorning Aug 8, 2020
Member

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)

This comment has been minimized.

@pirj

pirj Aug 9, 2020
Member

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 ?

This comment has been minimized.

@bquorning

bquorning Aug 9, 2020
Member

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.

This comment has been minimized.

@pirj

pirj Aug 9, 2020
Member

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

@pirj pirj assigned pirj and mockdeep Aug 8, 2020
pirj added 7 commits Aug 3, 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-hq/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.
@pirj pirj force-pushed the mockdeep:rf-no_let branch from f833dda to edb87e8 Aug 8, 2020
@@ -11,6 +11,9 @@
let (:foo) { 1 }
let! (:bar){}

bar = -> {}
let(:foo, &bar)

This comment has been minimized.

@mockdeep

mockdeep Aug 8, 2020
Author Contributor

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 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 pirj commented Aug 9, 2020

@pirj pirj merged commit a2ef166 into rubocop-hq:master Aug 9, 2020
13 checks passed
13 checks passed
ci/circleci: code-climate Your tests passed on CircleCI!
Details
ci/circleci: confirm_config_and_documentation Your tests passed on CircleCI!
Details
ci/circleci: edge-rubocop Your tests passed on CircleCI!
Details
ci/circleci: jruby Your tests passed on CircleCI!
Details
ci/circleci: rspec-2.4 Your tests passed on CircleCI!
Details
ci/circleci: rspec-2.5 Your tests passed on CircleCI!
Details
ci/circleci: rspec-2.6 Your tests passed on CircleCI!
Details
ci/circleci: rspec-2.7 Your tests passed on CircleCI!
Details
ci/circleci: rubocop-2.4 Your tests passed on CircleCI!
Details
ci/circleci: rubocop-2.5 Your tests passed on CircleCI!
Details
ci/circleci: rubocop-2.6 Your tests passed on CircleCI!
Details
ci/circleci: rubocop-2.7 Your tests passed on CircleCI!
Details
codeclimate All good!
Details
@pirj
Copy link
Member

@pirj pirj commented Aug 9, 2020

@mockdeep Thank you!

@bquorning
Copy link
Member

@bquorning bquorning commented Aug 9, 2020

👍 Thank you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

5 participants
You can’t perform that action at this time.