Add new RSpec/PreferredSharedExampleMethods cop to enforce consistency when defining shared examples

[dvandersluis]

Allows RSpec/SharedExamples to be configured with preferred methods for defining shared examples and context, so that consistency can be enforced.

Follows rubocop/rubocop#14033, and reimplementation of #2062.

---

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

If you have created a new cop:

• Added the new cop to config/default.yml.
• The cop is configured as Enabled: pending in config/default.yml.
• The cop is configured as Enabled: true in .rubocop.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.
• Set VersionAdded: "<<next>>" in default/config.yml.

If you have modified an existing cop's configuration options:

• Set VersionChanged: "<<next>>" in config/default.yml.

[dvandersluis]

I chose the most commonly used methods in the codebase:

• shared_examples was used 10 times and shared_examples_for was used 4 times.
• include_examples was used 42 times and it_behaves_like was used twice.

[lovro-bikic]

Just fyi, include_examples and it_behaves_like don't do the same thing (unlike shared_examples and shared_examples_for which are synonyms).

Most projects I worked on typically used both, include_examples for example can't be used if you want to do multiple includes which override each other's setups:

RSpec.describe do
  shared_examples 'foo' do |var|
    subject { var }

    it { is_expected.to eq(var) }
  end

  include_examples 'foo', 1 # fails because the second include sets subject to 2
  include_examples 'foo', 2
end

so you need to use it_behaves_like instead to wrap each include in a context.

[dvandersluis]

Thank you for this, I did not realize they behaved differently. So a few thoughts:

1. The easiest solution is probably just to leave PreferredIncludeExamplesMethod unset (which I should probably do here, because I didn't realize the distinction), and allow users to set it if they exclusively want to use one or the other.
2. It is also possible to do what I did in the spec for this cop and define a top level context within the shared examples, in which case include_examples still works since the examples have their own contexts. The changed it_behaves_like examples still work for the same reason, but it might be worthwhile to undo that change.
3. The Language hash doesn't distinguish between the two which is what this cop is based on. I'm not sure if it should, but that is outside the scope of this PR at the least.
4. Given that include_examples doesn't create a context, we should probably have a cop that looks for shared examples that set subject or let that then are called with include_examples multiple times in the same context. Again outside of the scope of this PR, but would be good to prevent a footgun.
5. The other solution here would be to remove the PreferredIncludeExamplesMethod configuration, or even not consider includes at all. I don't think it's a huge risk to keep it given that it defaults to unset (ie. all methods are acceptable).
6. We could theoretically treat include_examples differently, but again because the different Includes are not distinguished in the Language hash, the problem could still exist if it is aliased.
7. At the very least we should document this distinction.

/cc @bquorning @pirj

[bquorning]

@pirj, what do you think?

[pirj]

As @lovro-bikic mentions, include_examples and it_behaves_like are different. Let's split the cop (again!).
One cop will enforce which alias to use.

The other - whether to include examples or include them into nested context, include_examples vs it_behaves_like.

There are use cases for include_examples, but in most cases it's a footgun. I suggest no configurable value, just the cop to flag include_examples as an offence. Those who understand the difference, and who deliberately want it, can disable the cop.

a cop that looks for shared examples that set subject or let that then are called with

Methods as well. Also, before/after hooks can run multiple times if defined in a shared group and included multiple times. Too much of an effort to detect those side effects, and I'm uncertain if this can be done reliably. I can't recall major benefits of using include_examples to justify its usage in specs, and the effort to create a cop to ensure its safe usage.

There may be another cop that would detect shared contexts that contain examples, and yet another, that will detect no examples in a shared example group. We may have an issue to track this idea already.

I've grepped real-world-rspec, and usage stats are like (rough $ rg --no-ignore --hidden --glob *.rb --no-filename shared_examples_for | wc -l, seems not to count blank lines):

aliases:
shared_context - 636
shared_examples - 6401
shared_examples_for - 1295

include_examples - 4450
include_context (does the same as above!) - 2628

aliases:
it_behaves_like - 24441
it_should_behave_like - 562

it_should_behave_like will be removed in RSpec 4, other aliases will remain.

My proposal:

1. For the cop that enforces a preferred alias to use, not consider include_examples and it_behaves_like aliases
2. File an issue to add a cop to flag include_examples
3. File (unless it's there already) an issue to add a cop for enforcing semantic use of shared_context vs shared_examples

[dvandersluis]

@pirj thank you for the feedback!

I have updated this PR to only apply to defining shared examples.

There are use cases for include_examples, but in most cases it's a footgun. I suggest no configurable value, just the cop to flag include_examples as an offence. Those who understand the difference, and who deliberately want it, can disable the cop.

I'll work on this next.

[dvandersluis]

BTW I just realized that it_behaves_like vs it_should_behave_like is already covered by RSpec/ItBehavesLike 😄

[pirj]

I am sorry to have missed that. We seem to be in the spot where we can’t remember all the cops we already have 😅

[dvandersluis]

@pirj @bquorning anything else needed for this one?

[pirj]

I was always curious, and this question reveals each time when I think about directory-local .rubocop.yml

which could come handy in a case when there’s an alias defined

eg ‘shared_context’ is preferred in specs/, but shared_state in features/

How does it work with multi-threaded RuboCop, that nechanism that parallelizes execution? There should be separate ConfigLoaders (or whatever they are called).

How does rubocop-rspec work in this case? We do have a global Language module that keeps a reference to the config?

#racecondition

Can’t this be a source of flaky subtle bugs?

[pirj]

The question above is totally unrelated to the PR’s code.
It just made me remember this again

[dvandersluis]

@pirj I've addressed your requests now.

[dvandersluis]

which could come handy in a case when there’s an alias defined

eg ‘shared_context’ is preferred in specs/, but shared_state in features/

I tried this out and it should work how you expect. You can create .rubocop.yml files in subdirectories, and they do get loaded (and can inherit from the main one too).

How does it work with multi-threaded RuboCop, that nechanism that parallelizes execution? There should be separate ConfigLoaders (or whatever they are called).

There's a lot of layers to investigate here, but ultimately what happens in short is that each inspected file mobilizes a team of cops, during which a config object is created for that file. ConfigStore#for_file eventually calls ConfigFinder.find_project_dotfile, which calls FileFinder.find_file_upwards which searches from the file directory upwards until a configuration file is found (and uses the first one found).

So each file gets the correct config (there's some caching involved as well) for the directory it's in.

We do have a global Language module that keeps a reference to the config?

Language has a class level attr_accessor config, but it is set on_new_investigation which means that each cop will reset the Language data each time it is run. A cop can be run one or more times, per file (depending on if there are autocorrections that cause another iteration), so this reset can happen a lot of times. As far as I can tell, there is no further caching happening (each time a Language method is called it does new hash lookup).

Therefore, per-directory Language should work properly as well.

Can’t this be a source of flaky subtle bugs?

I don't think so because even with parallelization, each file runs in a single thread.

[pirj]

Discussion starter. Should we provide a setting here? Otherwise, the cop is a no-op, no?

Rarely people jump and configure new cops. Unless they’ve implemented them themselves 😅

There’s evidence that one style prevails, but I’m not sufficiently confident that we should.

Why do people use the shared_examples_for? (We did as the second commit says). Does it read better in some cases?

Why do we implement this cop in the first place? Do we want to get rid of shared_examples_for? Or do we want to enforce use of aliases where due? Or prevent such usage?

[dvandersluis]

I think it's useful to enforce consistency, there are plenty of other examples of this across RuboCop of cops that are just meant to enforce one style or another when there isn't an actual difference between them. For instance, Style/CollectionMethods allows users to choose to enforce map vs. collect, etc.

I didn't specify actual defaults because I don't think there is one answer (but we could if we want...).

Why do we implement this cop in the first place?

In a codebase with many contributors, there is no particular reason (IMO) to use one alias over the other so it becomes inconsistent. I noticed this in the main rubocop repo and wanted to fix it as an InternalAffairs cop, and it was suggested to add a RuboCop::RSpec cop instead 😆 In the meantime, the inconsistency was fixed directly in rubocop/rubocop#14034.

[pirj]

On a different note, I suddenly recalled we have RSpec/Dialect cop 🙈😭

I sincerely apologise if that other cop does what this one does with aliases, again, it slipped my mind as I’m less involved with the project lately.

[dvandersluis]

Oh wow! I didn't know about it either. The tests for it are pretty sparse and the documentation is a bit confusing as to how to configure, but it does seem to do what I was looking to do.

RSpec/Dialect:
  Enabled: true
  PreferredMethods:
    shared_examples_for: shared_examples

With this in mind, we can probably close this PR.

[pirj]

Thank you for the in-depth explanation, @dvandersluis !

How does context switch happen, how do we load multiple cores? Do we have a pool of rubocop processes, each with its own set of class-level attributes? Or can a thread context switch happen before the cop finished the investigation, and some other cop would overwrite the cached language DSL config?
How does this isolation work?
Can we guarantee that if a cop finishes with this config being untouched?

[dvandersluis]

@pirj parallelization is done by the parallel gem, with default options (although it can be tuned by ENV vars).

[dvandersluis]

I'm going to close this PR given RSpec/Dialect already covers this. If there's anything you want me to extract from this (for instance, enabling RSpec/Dialect and replacing shared_examples_for, please let me know!)