Cop Idea: NoLet

[mockdeep]

It would be nice to have a rule that disallows the use of let. I've been moving in the direction of avoiding them in my tests. Instead I'll err on the side of a little more duplication in my tests. In cases where there is a lot of setup, I'll instead define methods or factories to encapsulate the logic. More in this thoughtbot article.

[pirj]

This style exists and has its proponents, see discussion in the style guide.
Please keep in mind that a named subject is also just a regular let.

There's also a word of precaution in RSpec docs:

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.

[mockdeep]

Thanks for the links. I'm not necessarily advocating for changing the style guide, though, just adding rules to support this style of writing tests.

One thing I see differently from the person in the style guide thread is that I also prefer to avoid instance variables. To me they have their own caveats that make them troublesome. For example, they are un-lintable. Rubocop cannot tell us when they are unused, and when we misspell one, we might get a confusing error or no error at all. I've had tests where the instance variables were misspelled for one reason or another and the test doesn't fail:

expect(@undefined_var1).to eq(@undefined_var2)

Also, there is a similar tendency to let blocks where they start to fill up with stuff that may or may not be relevant in different tests. In refactoring some tests, I've discovered we're doing a ton of setup for variables that may be used in only a handful of test blocks. Or there is some followup in the test block to update the thing being instantiated elsewhere, which is especially problematic in tests that touch the database, making extra queries:

let(:doc) { create(:doc) }
# or `before { @doc = create(:doc) }`

it 'does a thing when doc is draft' do
  doc.update!(status: 'draft')
  ...
end

A method is more flexible. It allows us to have a default and also pass parameters to customize it.

it 'does a thing when doc is draft' do
  doc = create_doc(status: 'draft')
  ...
end

[pirj]

Indeed. Would you still be interested in advocating helper methods in the style guide? There's a related discussion there.

[pirj]

And, of course, a pull request for NoLet is always welcome.

[Darhazer]

What do you think of having a cop for a maximum number of lets an example group may have?

[mockdeep]

@Darhazer I'm open to that. I've noticed that rules seem to avoid using No in the name so I was wondering if there was a positive phrasing. MaxLet or MaxLets. I've noticed that Max tends to be avoided in naming as well, though... LetCount, LetUsage?

[pirj]

I support the idea. We can even enable the cop by default with Max: 3 setting.
Does RSpec/MemoizedHelpersInExampleGroup sound like a reasonable name for such a cop?

[mockdeep]

@pirj ExampleGroup makes me wonder how nesting is going to play into this. Do we want total per file? Or total per nesting level? This is starting to sound complicated...

[pirj]

Good question. Also keeping in mind included contexts that might contain lets as well, it's an unreachable goal to properly count them all.

[pirj]

Anyway, since it's Ruby, it sounds near impossible to count all lets. WDYT of taking a rough approach and only count those lets that are clearly in the scope, e.g. those in the example group and its ancestors, excluding duplicates? The cop, if Max is exceeded, would raise an offence.

Would you like to tackle this? I'll be happy to help you along the way.

[pirj]

My bad, it slipped my mind that pull request existed 🤕

[mockdeep]

@pirj sorry, I haven't had a chance to follow up on this. I think maybe it makes sense to count let calls in the entire file?

[pirj]

I think it won't be fair, e.g.:

RSpec.describe A do
  subject { described_class.new(x) }
  context 'with a' do
    let(:x) { :a }
    it { ... }
  end
  context 'with b' do
    let(:x) { :b }
    it { ... }
  end
  context 'with c' do
    let(:x) { :c }
    it { ... }
  end
end

would count 3, but there's actually just one in each given scope.

[mockdeep]

Hmm, yeah, I guess that's true. I can try and set it up per-scope. I'll try to work on it this afternoon.

[marcandre]

@mockdeep said:

One thing I see differently from the person in the style guide thread is that I also prefer to avoid instance variables.

Absolutely. That's one thing that let are for though!

In refactoring some tests, I've discovered we're doing a ton of setup for variables that may be used in only a handful of test blocks.

Since let are lazy, there is no difference between that and having a method and not calling it.

Or there is some followup in the test block to update the thing being instantiated elsewhere, which is especially problematic in tests that touch the database, making extra queries

Both helpers or let have the same issue if they create side effect. It's not inherent to let

A method is more flexible. It allows us to have a default and also pass parameters to customize it.

The equivalent of a parameter with a default is to refer to another let defined:

def help_me(options = {})
  # ...
end
# vs
let(:options) { {} }
let(:help_me) { # call options... }

Moreover, it's much more cumbersome to pass an argument down two levels of context while it's natural to have a context that redefines lets for it's nested specs. That's the real power of let. I don't believe there's any equivalent without instance variables to something like:

  # ...
  context('when debug is set') do
    let(:debug_option) { true }
    context('when foo is :bar') do
      let(:foo) { :bar }
      it_behaves_like :some_shared_example
    end
  end

[marcandre]

In short, it is definitely possible to shoot yourself in the foot with lets, but I strongly believe that they can be a great way to describe specs.

I have no objection that the cop be implemented, but I vigorously object to a default setting (of any value).

Let's minimize the reasons to curse at RuboCop.

[mockdeep]

@marcandre a lot of your arguments are addressed already here and in the linked discussions.

Absolutely. That's one thing that let are for though!

let carries with it many of the same problems as using instance variables, as mentioned previously.

Since let are lazy, there is no difference between that and having a method and not calling it.

There are several differences. For one, a method can accept arguments to customize the result produced. For another, calling a method is more explicit and doesn't magically instantiate a record the moment it is referenced. Sometimes I need a record to be present for one of the tests, but I don't need to test anything against the record itself, or I don't need the record until the code is exercised, which leads to awkward tests:

it 'verifies users' do
  user # this feels really awkward
  UserVerifier.call
  expect(user.status).to be_verified
end

vs:

it 'verifies users' do
  user = create_user # this communicates more clearly to me
  UserVerifier.call
  expect(user.status).to be_verified
end

Both helpers or let have the same issue if they create side effect. It's not inherent to let

The difference is that we can avoid extra unnecessary database queries by creating the record we need in the first place instead of updating them after the fact. Using a method allows for a more flexible interface:

user.update!(status: 'active')

# vs

user = create_user(status: 'active')

The equivalent of a parameter with a default is to refer to another let defined:

Not quite. Again, having a method allows you to customize the result on a per-test basis, not just being limited to a context.

it's natural to have a context that redefines lets for it's nested specs.

You can define methods in the same way, overriding them in contexts, though I'd argue overriding let or method definitions is a recipe for confusion.

I don't believe there's any equivalent without instance variables to something like:

Methods work in the same way. They can be accessed from shared examples, too. In general, methods provide both greater flexibility and explicitness, which tends to make tests way easier to extend and maintain.

[pirj]

I side with @mockdeep in the let vs a method discussion (not lets in general).
A gut feeling makes me think it would be easier to track down methods than lets when they are scattered across your specs. Might be mistaken here though.

I respect and understand let-free specs, pretty sure specs using this style not only exist but can be quite readable. Since RuboCop* is all about enforcing consistency, a NoLet cop would come handy for those sticking to that style. And no trouble for the rest, since it's Enabled: false by default.

[marcandre]

user.update!(status: 'active')

# vs

user = create_user(status: 'active')

I agree that the first example is not the way to go. The let(:user) should have been defined such that one can do:

context 'when the user is active' do
  let(:user_options) { {status: 'active'} }
  # ...
end

That solution is, imo, superior to user = create_user(status: 'active').

Again, having a method allows you to customize the result on a per-test basis, not just being limited to a context.

The only functional difference is if you want to call that functionality twice. Otherwise you can always wrap your test in a context.

the let vs a method discussion

let are implemented as methods, so the argument is really about explicit passing of arguments or implicitly using other lets.

[marcandre]

I'll repeat: I have no problem with the cop, I have a problem with enabling it as a default for RuboCop.

I can't imagine the look of a PR that removes the lets in shared_context, in particular let(:ruby_version) and doesn't use instance variables.

[marcandre]

Maybe I'm misunderstanding...

Are you suggesting that instead of:

  let(:source) { 'code = {some: :ruby}' }
  let(:ruby_version) { '2.4.0' }
  let(:processed_source) { #... using source and ruby_version }

This be written as:

  def source; 'code = {some: :ruby}'; end
  def ruby_version; '2.4.0' end
  def processed_source(source: source, ruby_version: ruby_version)
    # ...
  end

So that one could write processed_source(source: 'my = :ruby') as well as let(:source) { 'my = :ruby' } and call processed_source?

[pirj]

@marcandre https://github.com/rubocop-hq/rubocop-rspec/blob/master/spec/rubocop/rspec/hook_spec.rb is a good no-let example. I believe there can be more.

I'm up for an experiment to change some spec that makes use of lets and refactor it. Do you have some good target in mind?

[pirj]

I have a problem with enabling it as a default for RuboCop.

No worries, I doubt this is ever going to happen. lets are mainstream.

[mockdeep]

@marcandre

That solution is, imo, superior to user = create_user(status: 'active').

That example was meant to suggest some variability between tests. A more thorough example would be something like:

it 'does a when user is "active"' do
  user = create_user(status: 'active')
end

it 'does b when user is "pending"' do
  user = create_user(status: 'pending')
end

it 'does c when user is "blocked"' do
  user = create_user(status: 'blocked')
end

let doesn't give us as many options for that sort of variability. We'd need to define multiple let blocks for different user types, override them in nested contexts, or update the record after the fact in each test. If all of the users were the same then I would make it the default in create_user.

The only functional difference is if you want to call that functionality twice. Otherwise you can always wrap your test in a context.

It's easy to boil it down to a single difference, but it applies in a variety of situations, like the one above where it makes it easier to simplifiy the test structure. As you mention, it allows you to create multiple records in the same test:

active_user = create_user(status: 'active')
pending_user = create_user(status: 'pending')

It helps make each test more declarative and explicit about its dependencies. And aside from those, it can also make for a nice way to extract other repeat logic between tests, as well as potentially moving repeated chunks further up for re-use elsewhere.

the argument is really about explicit passing of arguments or implicitly using other lets.

The argument is only partly about explicit passing of arguments, as mentioned above. It's also about flexibility and simpler test structure, among other things.

Are you suggesting that instead of:
...
This be written as:
...

That's one alternative. It depends on what sort of flexibility you need. If you only need them as constants in your tests, you can actually assign them as locals in you context blocks:

context 'my feature' do
  # don't declare them as CONSTANTS, though, or they will bleed up to the global scope
  source = 'code = {some: :ruby}'
  ruby_version = '2.4.0'

  it { expect(ruby_version).to eq('2.4.0') }
end

A method won't be able to access those directly, though. So they'd need to be passed in, or hard coded as defaults.

If you're dealing with shared examples, you can pass parameters in:

RSpec.shared_examples 'MyThing' do |yabba, dabba|
  it { expect(yabba).to eq(dabba * 2) }
end

it_behaves_like 'MyThing', 4, 2

Or with keywords:

RSpec.shared_examples 'MyThing' do |yabba:, dabba:|
  it { expect(yabba).to eq(dabba * 2) }
end

it_behaves_like 'MyThing', yabba: 4, dabba: 2

This latter has the benefit of throwing an error early with a nice missing keyword: yabba if you don't pass one of the options. Passing parameters to the test might have issues with anything transactional, though, so best not to use it for anything you might mutate or might get rolled back between tests.

Bottom line, methods are probably a good default.

I have a problem with enabling it as a default for RuboCop.

I don't think anybody has been seriously arguing in favor of this. As much as I think this is a better approach, I just want tooling to help guide the codebases I work on. We talked about shifting the naming to something that allows limiting the number of let blocks to a specified amount, as opposed to eliminating them entirely. This should also help in transitioning existing code.

[pirj]

@marcandre a question to you since you've been vocal in this discussion on the side of not enabling this cop by default.
It's still an open question for me. What would you say if the cop is enabled by default with a Max: 5 setting? 5 is not too purist and at the same time, more than 5 seems to be excessive by any measures and hint of a smell.

I understand that this is a LineLength kind of question, and there will never be a single ground between all-in-let and no-let war camps. So it's a question of the balance. Where to draw a (Max) line so that the majority is happy with our default?

[pirj]

@bquorning @Darhazer Why disable any cop by default? It will go under the radar. We offer more cops to satisfy any style. Some of the cops are configurable, and some have to be turned off in the config to meet the style.

Out of currently disabled by default cops I see:

• RSpec/MessageExpectation (looks pointless)
• RSpec/Pending (I have no answer why is it disabled by default)
• RSpec/AlignLeftLetBrace/RSpec/AlignRightLetBrace - ok, rarely seen this the first the wild, and never the second

It's not a big deal to turn off one cop on version update. Especially when it's just once a year. Especially compared to the main RuboCop where new cops appear several times a month.

There's yet another thing, not widely used for sure, Severity. Set your CI to be green on Severity: refactoring and only fail on warning. And let developers set up the severity for their rubocop runs locally the way they like. If the cop is disabled, this flexibility is not possible.

[marcandre]

@marcandre a question to you since you've been vocal...

Me, vocal? Never 😅

What would you say if the cop is enabled by default with a Max: 5 setting?

I'm not sure...

What is the current maximum number of local variables and the maximum of def allowed? 😆

To convince me, maybe someone could refactor this spec file and show me how it improves it?

[pirj]

To convince me, maybe someone could refactor this spec file and show me how it improves it?

That sounds like a challenge, and I accept it :D

[pirj]

What is the current maximum number of local variables and the maximum of def allowed

I can remember those said limitations https://thoughtbot.com/blog/sandi-metz-rules-for-developers, but nothing related to local variables and methods.