RSpec/ScatteredLet is not safe to autocorrect

[thisismydesign]

The order of let! and before is relevant. before might set mocks that let! relies on.

Please set this to work only -A flag. Optionally, let is safe to move but let! isn't, so there could be separate rules for autocorrection purposes.

There could actually be a rule for defining before blocks before let! because it's quite confusing why mocks won't work in other cases. But that's a whole other topic.

[pirj]

Can you please provide a code example?

[thisismydesign]

Can't share the exact code but you can see how this will go wrong:

let(...)
let(...)

before do
  set_mock
end

let!(something) { create_something_that_relies_on_mock }

This will get autocorrected into

let(...)
let(...)
let!(something) { create_something_that_relies_on_mock }

before do
  set_mock
end

And now it's broken because let! happens before before.

[pirj]

To my best knowledge, their definition happens in the example group context, and their blocks are never executed until later.

What may affect it is the implicit order of hooks as per let! definition:

          let(name, &block)
          before { __send__(name) }

I can get a rough idea that the order of hooks may affect the spec.

Often times we suggest how to adjust specs rather than adjust cops. In this case I'd suggest using prepend_before for the hook that memoized helpers rely on.

Still, given enough evidence that this is a major problem and a respectable spec style, we'll consider an adjustment to the cop to take special care of the bang-let!.
Please provide a runnable spec that would clearly indicate that the order of memoized helpers and hooks affects the results.

[thisismydesign]

I see, thanks! It's possible that I got the cause wrong. Unfortunately, I won't have the time to provide more detailed examples. My use case looks like the dummy one above and was solved by moving the before hook before the let declarations. This also passes cops.

Adjusting the specs is a perfectly fine approach. It's just that introducing rubocop into a big project has been very painful due to issues like this and it'd really help if -a didn't break things. Having this correction with -A would be perfectly fine. I proposed that as a quick&easy fix at least for the time being.

[pirj]

introducing rubocop into a big project has been very painful.

Sure, I understand. Sometimes a single cop can raise thousands of offences. And have no autocorrection at all. No doubt, rubocop-rspec's offences are typically the hardest to fix - one change and everything falls apart.

Adhering to a good style, being consistent, writing canonical specs is a hard and tedious work.

I'm personally a proponent of let!, it has some unsuspected side effects when overloaded due to the implicit before. There's lengthy controversial discussion regarding memoized helpers.

Marking RSpec/ScatteredLet as unsafe is an option, however this problem you've been struck seems to so rare that we'd better take out time and properly address it.

Would you like to hack on the cop so that it doesn't rearrange bang-let! in auto-correction? I'd suggest still keeping the offence, though.

[thisismydesign]

To clarify what is making it hard to work with rubocop isn't rules without autocorrection or adhering to strictly good style. That is all perfectly fine. Autocorrect is great because it gets rid of a huge amount of issues at ~no cost. However, when there's even a single issue with autocorrect, that ends up lowering everyone's trust greatly, to the degree that now we have to look through thousands of changes because we can no longer trust that -a doesn't break the code.

Unfortunately, I won't have time to contribute to the solution but I appreciate that you're considering to fix this.

[pirj]

we have to look through thousands of changes

I won't have time to contribute

Those two paragraphs sound contradictory to me.
No library is bug-free. rubocop-rspec is part of your application, and if you're experiencing pain with it, it's your call to fix it.

A pull request is welcome, however, if there are no volunteers at the moment, I'll more inclined to close this ticket.

[thisismydesign]

I don't understand why close an acknowledged issue. This discourages outside contribution more than anything. Your message is: "no need to report issues unless you intend to fix them". Which is overwhelmingly not the case - even if in an ideal world it would be.

[pirj]

You are encouraged to report issues. Still, we strive to keep issues open if someone eventually decides to fix them.
For this specific one, the perspective is quite vague, and there's no clear actionable solution.

Marking the cop as unsafe for autocorrect without a clear reproduction example is not an option.

I admit there are over 100 open issues, and we encourage contribution. If you check recent ones, the reported usually submits a fix. I'm out of capacity for a fix streak at the moment.

Still, I commit to research problems with let! further, and to file an actionable ticket. Your effort is not wasted.

Thanks for understanding.

[thisismydesign]

Thank you

[pirj]

@thisismydesign I couldn't actually reproduce the side effect of a let! overridden in a nested example group. I've spotted a side effect, and attributed it to bang-let!s widely used in that project. Quickly replacing some with a non-bang version solved the problem, and I didn't investigate much deeper apart from making a note that let! may have a side effect when overridden. However, it might have been my bias against bang-let!, and the root problem might have been different.

Back to your case with mocks. I understand that the order of hooks can affect the spec. Still, I'm puzzled to come up with a reproduction example. I really appreciate if you could simplify/obscure your real spec down to a snippet that would reproduce the issue. This time not for putting the style of the spec in question to judgement and protecting the existing state of affairs of the cop, but rather to understand how it's best to address the issue and avoid frustration over auto-correction, wherever best, rubocop-rspec, rspec itself, or the community rspec-style-guide.

[dleavitt]

@pirj Below is a runnable example - works before autocorrecting w/ ScatteredLet and is broken after. It's terrible code that deserves to be broken, but this rule is for sure unsafe under some circumstances.

require "rails_helper"

DB = {}

RSpec.describe "Unsafe Scattered Let" do
  let!(:first) do
    DB[:first] = 1
  end

  before do
    DB[:second] = DB.fetch(:first) + 1
  end

  let!(:third) do
    DB[:third] = DB.fetch(:second) + 1
  end

  it "is technically unsafe" do
    expect(first).to eq 1
    expect(third).to eq 3
  end
end

[pirj]

@dleavitt Thanks for the example.
I suppose the cop should ignore such cases in its autocorrection.

[timdiggins]

FYI same issue with let and include_context, where the context includes the same let.

so you might write

shared_context "projecty" do
  let(:project) { raise "define project in spec" } # or it could be a default
  # custom stuff to do with project 
end

context "some context" do 
  let(:something) { "something I've added at the top for speed"} 
  include_context "projecty"
  let(:project) { "my project" }
  # tests which depend on project and the projecty context
end

You might start without the let(:something) line
Then in a new commit add the let(:something) line, run your specs - everything's fine

run rubocop -a

and specs are broken.

I mention this because it has just happened to me about 3 times in the last two weeks.

But making all scattered lets unsafe seems dubious - because most of the time (let interspersed with examples) it is safe. Not sure of the right resolution