Cop suggestion: Detect unnecessary usage of let!

[andyw8]

I often come across code which uses let!(...) for setting up state, but then never makes any direct reference to those identifiers. This setup should be in a before block, or inlined into the example's it block:

Bad

let!(:my_widget) { create(:widget) }

it "..." do
  expect(Widget.count).to eq(1)
end

Good

before { create(:widget) }

it "..." do
  expect(Widget.count).to eq(1)
end

or

it "..." do
  create(:widget)
  expect(Widget.count).to eq(1)
end

[nijikon]

👍

[backus]

@andyw8 this is implemented in master

[backus]

@andyw8, @jcoyne asked:

Why is this bad? Can you provide some justification? Is this just personal preference?

Referring to the example

 let!(:my_widget) { create(:widget) }

Care to weigh in? I'll be able to provide context later potentially but I'll let you respond since you originally proposed this cop

[jcoyne]

If this is a problem, why hasn't it be proposed for deprecation/removal in rspec itself?

[backus]

@jcoyne we aren't flagging all usage of let!. The cop detects when you use let! and don't reference the defined method. The cop is encouraging you to only use let! when you need to reference the value later and otherwise use before for setup

[jcoyne]

I guess I don't see what the harm/benefit of that is. If anything the name is giving some context to the reader of the spec.

[backus]

Before I defend the cop further I'll note that these cops are supposed to be opinionated. This is why many are configurable. You're always free to disable particular cases, disable cops for a directory, or disable them entirely.

Consider the following snippet of code

def write_user
  user = database.persist(user_info)

  do_some_work
  do_some_other_work

  true
end

I think this is similar to the argument you are making about an unreferenced let!. I'd say that this local variable assignment to user should be removed because it isn't being used in the rest of the method. If I wanted to add some context for that write then I'd add a comment above it.

Likewise, consider this spec:

let!(:user) { database.write_user(user_info) }

before do
  do_some_work
  do_some_other_work
end

it "asserts something" do
  expect(something_that_depends_on_db_state).to be(true)
end

I think it is odd to read over this spec, see the assignment to user using the let!, but not see any usage of user. Just like the other example I would instead just make the side effect happen in before and if I wanted to give context I would write a comment

[dpisarewski]

We should at least disable this cop by default. Quite the opposite: let! should be generally preferred over before, because before can't be turned off in a nested context.

[backus]

@dpisarewski can you give me an example of "disabling" a let! in a nested context?

[dpisarewski]

@backus For example:

describe Product do
  subject { product }

  let!(:product) { create(:product) }

  it { is_expected.to be }

  context 'when product does not exist' do
    let!(:product) { nil }

    it { is_expected.to be_nil }
  end
end

[RobinDaugherty]

Another reason I think this is a bad rule: If you create records and perform other setup in a before block, there's no description of the record. With a let! you can be descriptive:

let!(:nonmatching_user_in_same_organization) { FactoryBot.create(:user, organization: user.organization) }

In a before block, that isn't easy. It could be placed in a comment, but there's no standard for it and no cop that would be able to enforce it.

[chulkilee]

@dpisarewski I think let!
Overriding let! with nil value is a hack to avoid creating that. Also note that if you have more nested context or shared examples, included context then needs to check whether the referenced variable is nil or not, which indicates bad context structure.

If you need to "undo" let!, probably it would be better to rewrite with different context.

describe Product do
  subject { product }

  context 'when prooduct exists' do
    let!(:product) { create(:product) }
    it { is_expected.to be }
  end

  context 'when product does not exist' do
    it { is_expected.to be_nil }
  end
end

@RobinDaugherty For descriptive name - you may use just variable name with _ prefix. When it's not referenced, no need to use let! without referencing it.

describe Foo do
  context 'when .... ' do
    before do
      _nonmatching_user_in_same_organization = FactoryBot.create(:user, organization: user.organization)
    end
  end
end

[RobinDaugherty]

What benefit is there to placing these in a before block instead of a let!?

The alternative of using a before blocks creates values that are also not referenced in the tests, and are in addition not lintable since they are either not assigned to a variable, or are assigned to an underscore-prefixed variable and are therefore ignored by the linter. This smells like a bad default rule. It removes contextual hints placed in code for little or no benefit.

To return to the original description of this PR:

This setup should be in a before block, or inlined into the example's it block

Placing setup in an it block is bad practice. It leads to non-described pieces of context, which is what let exists to fix. It makes it more difficult to add a second example with the same context, which leads to multiple assertions in the same example, which is generally also bad practice. (In other words, instead of adding another descriptive spec, it's less friction to add more business assertions to the same spec instead of extracting setup from the spec at the same time.)

The point of this comment thread is that this rule is not well though-out and wasn't discussed prior to implementation. The majority of reactions to this issue are downvotes. There was no discussion as to the relative benefits of the rule before it was merged in #162 and enabled by default. Multiple people have asked for that default to be changed. It seems like a bad default that will lead to worse practices, not better.

[mikegee]

Placing setup in an it block is bad practice.

You make strong arguments in support of this statement, but it's not black and white. The Arrange/Act/Assert style of unit testing advocates putting the setup in each unit test.

Sharing setup and context between tests may be a code smell. Perhaps the system under test needs a redesign to simplify its setup. Or, the setup could be extracted such that its just a method call in each test.

[dgollahon]

If you need to refer to a value across example groups, a let is great. Otherwise, why do you need a memoized helper method? I think the logic is almost the same as checking for an unused variable.

Imagine you had this code:

it 'does something' do
  current_user = User.create!(...)
  expect(Users.count).to be(1)
end

RuboCop (and ruby, actually) would flag current_user as an unused variable. Why is the lvar created if it's not going to be used? Remove the assignment and add a comment if you need to explain why it is there (or you can also underscore the variable).

What this cop is advocating is similar: If all you want to do is provide some sort of side effect for setup, that's what before is designed for.

If you have a test where you need to do let!(:user) { User.create!(...) } and then reference user, this cop won't complain. When I see let!, it makes me think that that named memoized helper method should be used somewhere. before communicates to me "hey, here's some setup that changes the state of the world and you should be aware of when evaluating the following examples".

As an aside, I think the many uses of let! are mistakes because they typically involve persisting database records that don't need to be persisted. Unit tests, ideally, should avoid as much I/O as possible and let! is a common trap for this.

The alternative of using a before blocks creates values that are also not referenced in the tests, and are in addition not lintable since they are either not assigned to a variable, or are assigned to an underscore-prefixed variable and are therefore ignored by the linter.

I think it's slightly easier to have errant let!s pile up in your code because it's harder to tell if someone meant to be doing a before and just performing some setup or if someone was originally referencing the let!'d value but have since removed the reference and the let! should be dead code but is executed anyway. I find it easier to audit setup in a before because I know it's clearly intended just to do some setup. With RuboCop and RSpec, a let is also not lintable since it does not tell you if a let is no longer referenced (shameless plug: I made a tool to monkey patch RSpec to do this: https://github.com/dgollahon/rspectre but it's not a perfect solution).

Placing setup in an it block is bad practice.

I'm going to have to disagree with this blanket rule. If the setup is only applicable to a single example, I usually prefer inlining it. It's actually more lintable and, imo, it's easier to understand in one glance since it's all in one place. let, before, etc. are all great when you need to deduplicate setup. If you only use the setup once or twice, my advice is to skip the features you don't need and lift things out as you need.

Your point about adding extra examples requiring an additional step is valid, but it's a tradeoff I'm frequently happy to take. I find I avoid more convoluted RSpec setup this way (I often have to modify let calls and test structure when I add more examples either way).

I don't have that strong of a feeling about this cop because I rarely use let!, but I think the logic behind it is consistent: if you don't use a name, don't bind a name.

The point of this comment thread is that this rule is not well though-out and wasn't discussed prior to implementation. The majority of reactions to this issue are downvotes. There was no discussion as to the relative benefits of the rule before it was merged in #162 and enabled by default. Multiple people have asked for that default to be changed. It seems like a bad default that will lead to worse practices, not better.

I don't think these arguments are accurate or constructive. I think you are assigning blame where none is due:

1. I think it was well thought out and based on a consistent principle.
2. A feature was proposed, agreed on, and implemented--downvotes came much later. Furthermore, there are only a few votes either way. 4 downvotes after more than 2 years is hardly a consensus.
3. It seems bad to blame the implementors for not having enough discussion when no one objected before it was merged. Adding features is better than waiting to see if dissenters show up.
4. I doubt this would lead to worse practices on average, but if you're right, then let's figure out how to improve the documentation and explain the "don't name this if you don't reference it" rationale better.

And, as with all cops, the user is free to disable it for themselves--RuboCop is supposed to be configurable.

[RobinDaugherty]

Look, even the style guide disagrees with this rule.

Use let! to define variables even if they are not referenced in examples. This can be useful to populate your database to test negative cases.

I recommend you adopt better practices for deciding on changes to default rules, and these should probably include following the project's style guide and deliberating on a new rule before adopting it in the default ruleset.

[dgollahon]

Look, even the style guide disagrees with this rule.

I agree this should be made consistent. I think this advice should be removed from the style guide.

As an aside, I don't care for having a style guide just for the difficulty of keeping things like these in sync. Personally I think we should just generate something directly from the config, but that's another problem for another day.

I recommend you adopt better practices for deciding on changes to default rules, and these should probably include following the project's style guide and deliberating on a new rule before adopting it in the default ruleset.

I think the thought, going forward, for rubocop (and I assume rubocop-* projects) is to make all new cops opt-in until a major version is released when they become default but I can't remember all the details. Historically introducing a disabled cop didn't make much sense because no one would ever find it so people have just added and enabled all new cops and users can disable as they like. Defaults are changed pretty rarely outside of newly added cops.

[dpisarewski]

@chulkilee Why would changing a value be a hack? That's one of the basic things we do in imperative programming languages, ruby in particular. I tried to keep the example as slim as possible, please don't focus on irrelevant details. The point is you often have a shared setup in which you make little changes for a particular use case. If you don't share setup, then you have to deal with a huge code duplication.

Here is a more complex example which I hope can make my point more understandable:

describe Product do
  subject { product }

  let!(:product)          { create(:product) }
  let!(:product_category) { create(:product_category, 'some_category') }

  let!(:product_images) do 
    [
      create(:product_image, 'some_image', product: product)
    ]
  end

  let!(:product_attributes) do
    [      
      create(:product_attribute, key: :price, 1,    product: product)
      create(:product_attribute, key: :description, product: product)
    ]
  end

  let!(:product_variants) do
    [
      create(:product_variant, product: product)
    ]
  end

  let(:found_description) { product.attributes.find_by(key: :description) }
  let(:found_price)       { product.attributes.find_by(key: :price) }

  it 'has a description' do
    expect(found_description).to be
  end
  
  it 'has a price' do
    expect(found_price).to be
  end

  context '#images' do
    subject { product.images }

    it { is_expected.to have(1).item }
  end

  context '#attributes' do
    subject { product.attributes }

    it { is_expected.to have_at_least(2).items }
  end

  context '#variants' do
    subject { product.variants }

    it { is_expected.to have(1).item }
  end

  context 'dummy' do
    let(:placeholder_image) { create(:product_image, 'placeholer', product: product) }
    let!(:product_images)   { [placeholder_image] } 

    it 'has only 1 image' do
      expect(product.images).to have(1).item
    end

    it 'has a placeholder image' do
      expect(product.images.first).to eq(placeholder_image)
    end
  end

  context 'generic' do
    let!(:product_attributes) do
      [
        create(:product_attribute, key: :description, product: product)
      ]
    end

    it 'has no price' do
      expect(found_price).to be_nil
    end
  end

  context 'multivariant' do
     let!(:product_variants) do
      [
        create(:product_variant, product: product)
        create(:product_variant, product: product)
      ]
    end

    it 'has many variants' do
      expect(product.variants).to have_at_least(2).items
    end
  end
end

You can extract the setup of this data into factories, which would make it more complex and it's beyond the scope of this discussion.

[dpisarewski]

@mikegee

You make strong arguments in support of this statement, but it's not black and white. The Arrange/Act/Assert style of unit testing advocates putting the setup in each unit test.

Sharing setup and context between tests may be a code smell. Perhaps the system under test needs a redesign to simplify its setup. Or, the setup could be extracted such that its just a method call in each test.

In the opposite, this pattern is about separation of Setup, Action and Assertion phases, not about putting everything together. The code on the page is just a trivial example without using any testing frameworks(the example uses blank lines to separate the phases). Every testing framework defines a DSL/API to separate those, eg. SetUp()/@Before, testMethod(), assert in JUnit, before, it, expect in RSpec, Given, When, Then in Cucumber.
Can you please explain why sharing a setup is a code smell? This is the main purpose of nesting contexts IMO.

[dpisarewski]

@dgollahon In Soviet Russia specifications refer to implementations. 😀
Well, I don't think any style guide should/could be autogenerated from a linter config. People use to agree on the document(style guide), not on linter code/config. There can be many linters with different preferences or configs, and ideally linters precisely follow the style guide. Some rules may be unlintable after all.

I agree this should be made consistent. I think this advice should be removed from the style guide.

Why not remove the cop? I think, we should invert the cop, so let! should be preferred over before for reasons I mentioned above(no way to manage/disable before blocks programmatically) and also because the majority disagrees on this rule.

From my experience before blocks cause much more issues. before blocks also tend to grow with code changes, since they have no name which defines their purpose. Developers just put everything they need for the setup in that damn before block. Then statements in it are executed for each example even when many examples need only some of the statements put into the before.

In general, I don't get why it hurts you to name a piece of code when the name is not used programmatically. In my opinion it greatly improves code readability and is preferred over comments in ruby community.

[bquorning]

Just for your information, this cop was added two years ago, while the let! recommendation was added (copied directly from betterspecs) three weeks ago.

There was a bit of discussion in the style guide PR (rubocop/rspec-style-guide#55 (comment)) and I believe the intention is to change the style guide. See e.g. rubocop/rspec-style-guide#56 which references the RSpec maintainer’s comment on a Reddit AMA (https://www.reddit.com/r/ruby/comments/73nrhk/ama_the_authors_of_effective_testing_with_rspec_3/dnsyanp/):

• I find before and let to be useful for particular situations, but find that they often get mis-used/abused in the wild, unfortunately. My rule of thumb is to use them when there's a compelling benefit, but don't reach for them as the default way to write specs.

[chulkilee]

@dpisarewski

@chulkilee Why would changing a value be a hack?

Don't get me wrong - I'm not against of overriding values with let or let! in nested context.

What I'm calling "a hack" is returning nil in let! only as a workaround to avoid instantiaing the value given by the original let!. It can be avoided by using separate context when let! value is needed - instead of placing the let! unnecessarily at the parent context.

Here is an example of doing that:

describe Product do
  let!(:product_image) { create(:product_image) } # <- is it really needed for all examples?

  describe '#images' do
    it 'returns an image' do
      # ...
    end

    context 'having no image' do
      let!(:product_image) { nil } # <-- I'd call it as a "hack"

      it 'does not return an image' do
        # ...
      end
    end
  end
end

This can be refactored like this - without overriding let! to return nil.

describe Product do
  describe '#images' do
    context 'having an image' do
      # now let! makes sesnses with the context
      let!(:product_image) { create(:product_image) }
      
      it 'returns an image' do
        # ...
      end
    end

    context 'having no image' do
      it 'does not return an image' do
        # ...
      end
    end
  end
end

There are many ways to group examples - for example if you have very large test cases for a setup, then it might be bettero to have separate rspec files, instead of placing all tests under one file - e.g. product_spec.rb and product_with_image_spec.rb. Playing with many level of context and figuring out what's overrided or where a variable is defined is very bad experience.

Also please note that youmay just use plain ruby helper module or factory bot trait to set up, instead of applying lots of let! in spec files.

[goodniceweb]

For descriptive name - you may use just variable name with _ prefix. When it's not referenced, no need to use let! without referencing it.

@chulkilee It sounds like a big hack 😱

A feature was proposed, agreed on, and implemented--downvotes came much later. Furthermore, there are only a few votes either way. 4 downvotes after more than 2 years is hardly a consensus.

@dgollahon 15 downvotes to be exact 😉

[imajes]

so we're still discussing this, a few years after it first came up. I am on the fence of let|let! was implemented specifically to declare variables used (and reused) in specs. it wasn't originally (back in the day) inherent or descriptive about whether they must be used or not, it just said, let enables you to create a macro which will execute the code (like a proc) on call. let! will execute on definition.

yes these have been abused over time, but mostly by newer-to-the-profession engineers who are trying to figure it all out, and for rails apps for which every-guest is both a mystery and a first-class participant.

In actuality, over the past dozen or so years doing this, I can safely say that at least 70% of the apps i've worked on have always-enabled objects that form a basic part of the domain. Who can imagine a webapp that doesn't require some kind of user object?

So, we're forced to use mechanics like this to establish our orchestra before we begin the recital. Yes, we could use before blocks, but we're also encouraged to move towards smaller blocks of code that don't repeat-- something that using before doesn't really support (and has been mentioned above). Some have mentioned calling setup-methods that get your environment ready, but then of course you lean on centralizing these, and wait - that's what shared_context was sorta supposed to enable.

This cop doesn't have any true basis in best practice. I have not found any reasonable reference in the style guide. I know that original reasoning for creating the behavior was designed to set this up. So all I have left is to accept this is @andyw8's (or shopify's) personal preference, and not necessarily something that matches the entire community.

Strong advocate to making this disabled-by-default moving forward.

[andyw8]

So all I have left is to accept this is @andyw8's (or shopify's) personal preference

My initial post was here from four years before I joined Shopify. Anything I post here is in a personal capacity and does not reflect any Shopify preference. (also, RSpec is rarely used at Shopify anyway).

[imajes]

So all I have left is to accept this is @andyw8's (or shopify's) personal preference

My initial post was here from four years before I joined Shopify. Anything I post here is in a personal capacity and does not reflect any Shopify preference. (also, RSpec is rarely used at Shopify anyway).

Ah, thanks for clarifying! I didn't know :D :D

[schmijos]

Let me try to add an aspect to the discussion which wasn't mentioned yet: readability of tests.

Setups like the one mentioned by @chulkilee (let! used as a hack) can lead to huge problems in large test files. The reader of a single test cannot easily grasp the context anymore. before is easier to scan for and to reason about. In tests I prefer DAMP over DRY.

let and let! just look too similar. So I normally use let! in close vicinity to the test (it-block) and before further away.

[shreyasbharath]

before is easier to scan for and to reason about. In tests I prefer DAMP over DRY.

This hits the nail on the head. I wonder if RSpec has encouraged a little too much DRYing which have hence led to less readable tests?