RSpec/LetBeforeExamples is not a safe autocorrection

[gburgett]

Hello,

I have found an instance where the autocorrection for RSpec/LetBeforeExamples changes the behavior. I believe this cop should be marked as an unsafe autocorrection.

The issue is when a RSpec.shared_examples block defines a let with the same name as one defined closer to the main examples:

# frozen_string_literal: true

Box = Struct.new(:value)

RSpec.shared_examples 'when overriding value' do
  let(:value) { 1 }
end

RSpec.describe 'RSpec/LetBeforeExamples' do
  include_examples 'when overriding value'

  let(:value) { 2 }

  it { expect(value).to eq(2) }
end

In this case, the cop will move the let(:value) above the include_examples call, which changes the behavior.

See example repo here: https://github.com/gburgett/rubocop-let-before-examples

[Darhazer]

This over-simplified example shows few bad patterns. First, when overriding value should be a shared context, and not examples, as it only defines the context. Next, the way I'd expect the value to be overridden, is:

RSpec.describe 'RSpec/LetBeforeExamples' do
  include_examples 'when overriding value' do
    let(:value) { 2 }

    it { expect(value).to eq(2) }
  end
end

[gburgett]

Hi Darhazer, thanks for the comments. Yes this over-simplified example does have bad patterns, that's primarily because I simplified it significantly from my real-world code in our closed source app. In that app the shared_examples do define some examples but they were not relevant for reproducing this issue so I eliminated them.

Secondly in our code, the solution was to wrap the shared examples in their own describe block so that they did not override the value.

All of that however is irrelevant to the issue described in this PR, which is simply this: LetBeforeExamples autocorrection can in certain circumstances change the behavior of the code.

[pirj]

Have you considered using it_behaves_like instead of include_examples, @gburgett ? The difference is that the former wraps all shared examples in an example group, and lets defined in it take priority over the ones defined in the including example group.

I'm always opposed to the use of include_examples for no really good reason, as it introduces such hard to discover and understand bugs. And it's not only RuboCop that may introduce such bugs, people, too.

Even though there's a clear unsafety here, I'm more inclined to close this, as the reference code does have have code smells. @bquorning @Darhazer ?

[gburgett]

Thank you @pirj, yes all our newer code uses it_behaves_like and we do attempt to refactor include_examples when possible.

As a point of argument towards marking this cop as unsafe, it did take me a couple of hours to track down which particular cop was causing my specs to fail. I had upgraded Rubocop and introduced several new cops, ran safe autocorrection on all of them, and then had to work backwards to isolate which particular cop caused the failure.

[bquorning]

I agree with @gburgett on this one. In my opinion a cop is “safe” until you can show one example that it is not.

Could we change the autocorrect to do nothing if the source file includes include_examples?

[pirj]

@gburgett May I kindly ask you to check if #1413 keeps your specs green after autocorrection?

[gburgett]

That would work! Thank you very much @pirj