Don't Recommend let or let!

[sshaw]

Opening this per @bbatsov's response to my comment on Reddit.

The meat of my argument (as copied from here):

There's a comment from Myron Marston, long time RSpec maintainer explaining this...

I've seen Marston's arguments from 2011 and mostly have never agreed. Here ~7 years later in 2018, it seems he may be starting to realize they're bad:

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.

No. There's never a compelling benefit: interpreter detecting your typo and lazy loading objects under test are not compelling.

If I was concerned about an interpreter detecting a typo I'd be using a different language.

Lazy loading, what is the point? If your tests are consuming resources to the point that you need to lazy evaluate them your tests have a problem.

Remember, we're talking maintenance phase here: people need to go back to tests written months and years ago. Tracing through lazy loaded let hierarchies sprinkled with eager loaded let! with before hooks and shared examples that depend on the caller to define more lets is not maintainable.

Obscure tests is not a compelling benefit.

And really, we're talking maintainability and style, so consistency is important. Let's throwout the very important argument above and consider this. It's safe to say (I think) that after 20+ years the TMTOWTDI camp has lost out to the There should be one—and preferably only one—obvious way to do it camp. I mean that's why we're here talking style guides, right? We don't want TMTOWTDI.

In this case not everything can be done with let, you need let!. And not everything can be done with let, you need before. Why have some tests use X and some use Y and some use X and Y and maybe Z? Be consistent. KISS: use before. It's all you need.

[dgollahon]

To be clear (and for the benefit of future readers skimming this issue) what you're advocating for instead is using instance variables instead, right?

[andyw8]

This is a very difficult one because let and its companions are a core feature of RSpec.

You are in good company though, as Thoughtbot take a similar position: https://thoughtbot.com/blog/lets-not

In my own experience, let is typically heavily overused, and I aim to use it minimally. I find it occasionally useful when writing tests for gnarly legacy code, if refactoring isn't an option.

I would certainly support adding a substantial warning of the drawbacks of using it, illustrated with some alternatives.

But if the aim to completely eliminate let, then I think that's a campaign should be targeted at the RSpec project itself, which will be a hard battle.

It would be disingenuous for the styleguide to ban let at this time.

[pirj]

Agree with your points @sshaw. You may also be interested in those two open issues in the guide:
#56 [Only] use before and let when it's beneficial
#70 Recommend preferring helper methods to hooks
Please check it out, I'd love to hear your opinion on that.

Speaking of the example you provided back then, would you consider the code below a good approach to testing?

describe "#search" do
  before do 
    macro_hood = Fabricate(:macro_neighborhood)
    hood = Fabricate(:neighborhood, :macro_neighborhood => macro_hood, :name => hood_name)
    _restaurant = Fabricate(:restaurant, :neighborhood => hood, name => restaurant_name)
  end

  let(:hood_name) { "Jersey" }
  let(:restaurant_name) { "Cowley's" }

  context "given the name of a neighborhood" do 
    it "returns restaurants in that neighborhood" do
      result = RestaurantSearch.search(hood_name)
      expect(result.size).to eq(1)
      expect(result[0].name).to eq(restaurant_name)
    end
  end

  context "given the name of a restaurant" do 
    it "returns the restaurant" do
      result = RestaurantSearch.search(restaurant_name)
      expect(result.size).to eq(1)
      expect(result[0].name).to eq(restaurant_name)
    end
  end
end

The reason I changed the expectation is that id is not something under our control. I trust Fabricator, but sometimes due to human error, id may be assigned to nil, and this is what you'll be comparing in your spec, nil == nil.

Since we're speaking about lets, and those two lets in the example definitely don't follow to a category of a compelling benefit, how would you feel about using variables?

describe "#search" do
  before do 
    # ...
  end

  hood_name = "Jersey"
  restaurant_name = "Cowley's"

  context "given the name of a neighborhood" do 
    it "returns restaurants in that neighborhood" do
      result = RestaurantSearch.search(hood_name)
      expect(result.size).to eq(1)
      expect(result[0].name).to eq(restaurant_name)
    end
  end

This is possible and works as you would expect. This is some middle ground

It comes with a downside though, if it happens that your examples or setup code are mutating those objects, the state would leak between examples. Primitive objects can be frozen, but it's not that simple with objects that reference other objects.
This is where compelling benefit of using lets starts kicking in.

[sshaw]

To be clear (and for the benefit of future readers skimming this issue) what you're advocating for instead is using instance variables instead, right?

@dgollahon yes.

But if the aim to completely eliminate let, then I think that's a campaign should be targeted at the RSpec project itself, which will be a hard battle.

@andyw8, no, the aim is to act as a guide to RSpec not to advocate for addition or removal of features from the library.

It would be disingenuous for the styleguide to ban let at this time.

@andyw8, I disagree. It's more disingenuous to keep it when acknowledge it's "typically heavily overused" and you aim to "use it minimally" and want to offer a "substantial warning". Why? It's possibly the most useless and confusing thing in the history of Ruby development. Stop the madness. ✂️ 🗑

You may also be interested in those two open issues in the guide: #56 [Only] use before and let when it's beneficial

@pirj yes, I saw that. Maybe I'll add my 2 cents 😈 but again, let is never beneficial. Please, someone show me a valid argument, I've seen none in 8+ years.

... #70 Recommend preferring helper methods to hooks

@pirj not sure about this. before makes it clear what is being setup. Helper method may present a mystery guest test. Nevertheless, this conversation can happen in #70 😸

... would you consider the code below a good approach to testing?

@pirj, well the let here is pointless and just adds noise. If seems as though you're defining 3 local variables in before just so you can use let elsewhere. Compare:

before do 
  macro_hood = Fabricate(:macro_neighborhood)
  hood = Fabricate(:neighborhood, :macro_neighborhood => macro_hood, :name => hood_name)
  _restaurant = Fabricate(:restaurant, :neighborhood => hood, name => restaurant_name)
end

let(:hood_name) { "Jersey" }
let(:restaurant_name) { "Cowley's" }

context "given the name of a neighborhood" do 
  it "returns restaurants in that neighborhood" do
    result = RestaurantSearch.search(hood_name)
    expect(result.size).to eq(1)
    expect(result[0].name).to eq(restaurant_name)
  end
end

To:

before do 
  macro_hood = Fabricate(:macro_neighborhood)
  @hood = Fabricate(:neighborhood, :macro_neighborhood => macro_hood, :name => "Jersey")
  @restaurant = Fabricate(:restaurant, :neighborhood => hood, :name => "Cowley's")
end

context "given the name of a neighborhood" do 
  it "returns restaurants in that neighborhood" do
    result = RestaurantSearch.search(@hood.name)
    expect(result.size).to eq(1)
    expect(result[0].name).to eq(@restaurant.name)
  end
end

Less noisy and to the point.

The reason I changed the expectation is that id is not something under our control.

ID, where's that used? Maybe I'm missing something.

... how would you feel about using variables?

describe "#search" do
  before do 
    # ...
  end

  hood_name = "Jersey"
  restaurant_name = "Cowley's"

  context "given the name of a neighborhood" do 
    it "returns restaurants in that neighborhood" do
      result = RestaurantSearch.search(hood_name)
      expect(result.size).to eq(1)
      expect(result[0].name).to eq(restaurant_name)
    end
  end

Better, but not much. We must be clear about what we're testing: hood_name belongs in before as we need to assign it before we assert our tests. It's important test setup that shouldn't be floating out in outer space.

[pirj]

@sshaw It took me a while, now I understand that you primarily propose to remove the "Use let definitions instead of instance variables" recommendation.

Your arguments that let does not contribute to clarity and is often abused look pretty valid to me.

I'm not convinced enough though that using instance variables in before would address the problems that you outline for lets, specifically:

1. Obscure tests.
   Growing number of cases might require comes with necessity to define more variables in befores. The variables that we've made introduced locally (e.g. macro_neighbourhood) will have to become an instance variable, since it will be used multiple times in before blocks in nested contexts.
   In addition to defining the state, there would be a perfect case when something needs to be stubbed, or some method needs to be called, and that will land in before.

2. Tracing hierarchies.
   Instance variables overridden in blocks nested deeper.
   Instance variables defined on higher level will use non-overridden instance variables defined on the same level or higher up. While the other, defined in a deeply nested context, will use the overridden instance variable value.
   Or, if you happen to need to define something on the top level, e.g. hood, and be testing something with different macro_hoods, defining them in nested contexts, you won't be able to do so, because befores are executed from outermost to innermost.

With no single doubt that you can write good code with befores only and avoid the pitfalls that I've outlined, but this doesn't unfortunately mean that for someone else won't be able to turn their code into complete mess.

@cupakromer's comment brings up some interesting points as well to that discussion.

[pirj]

@sshaw What do you think?

[sshaw]

@cupakromer's comment...
let defines barewords around a variable; this falls largely in personal preference but I prefer barewords (message sending) to ivar accesses, it feels more flexible though I have no data to prove this

Not going to argue about ones preference really.

It may be flexible in production if you need to go back and add a cache or increment/decrement a counter. You can do so without breaking contract of callers, e.g., @foo vs foo. But we're talking testing and even in prod this is questionable default practice.

let reads more as a proof statement:

This statement is silly.let is a solution without a problem and therefore people have to say things like this to justify its existence or their usage of it.

What are we proving? That we disagree with the KISS principle? The proof is in the assertion.

In this context, let variable foo represent the object defined by this block definition

context "something amaaaaaaaazing" do
  before do 
    @foo = Object.new
  end
end

In this context, let variable @foo represent the object defined by this block definition.

🤷‍♂️

let and before have different semantic meanings. let is semantically telling me about a domain object definition. before is telling me what actions are going to happen before each of the following of specs in the context.

Before you run your tests you need tests fixtures. It's that simple:

context "something amaaaaaaaazing" do
  before do 
    @foo = MyDomainObject.new
    def @foo.bar
      123
    end

    @baz = MyClass.new(@foo)
  end
end

How can that be improved with let? I don't even think you could write that with let using his definition. Why force yourself to code yourself into a corner because you want something with a "different semantic meaning"? Makes 0.0 sense. How does this help with development and maintenance really?

I would not use a let in the example you posted, I would start by inlining the variables.

So X% of tests use let + before and Y% using before with instance variables? Style guides are about consistency. before + instance variables is more than capable of addressing all cases.

Growing number of cases might require comes with necessity to define more variables in befores...
In addition to defining the state, there would be a perfect case when something needs to be stubbed, or some method needs to be called, and that will land in before.

Not sure I understand argument here. lets can be replaced by local or instance variables defined in before. There's zero downside. I've been waiting for someone to show me for years.

Tracing hierarchies. Instance variables overridden in blocks nested deeper....

Maybe maybe not. But one thing is 100% certain: let requires you to trace your lazy loading with 0.0 nesting. before does not.

[sshaw]

Also it appears from this commit message that let was flawed from the beginning? Solution: let!

Commit for let is here: rspec/rspec-core@4d67748

[sshaw]

But nevertheless thank you @pirj for entertaining/understanding my argument.

[pirj]

@sshaw Nice historical research. Indeed there's not much rationale or context given in those commit messages introducing let and let!.
At the time the tool was shaping up, and there were 300 times fewer downloads for those versions (2.0.0a1 - 2.1.0) than for the recent versions (3.8.0). It looks like this was an experiment that went mainstream.
It seems that at the time usage of instance vars in before was not uncommon.

Looking forward, this is the first usage example of let in rspec-core itself after its introduction:

describe "#metadata_for_example" do
  # ...
  let(:metadata)           { Metadata.new.process(:caller => caller_to_use) }
  let(:mfe)                { metadata.for_example("this description", {:caller => caller_to_use, :arbitrary => :options}) }

  it "stores the description" do
    mfe[:description].should == "this description"
  end

  it "creates an empty execution result" do
    mfe[:execution_result].should == {}
  end

  it "stores the caller" do
    mfe[:caller].should == caller_to_use
  end

  # ...
end

and I find it really hard to explain why RSpec went that way, as opposed to:

describe "#metadata_for_example" do
  # ...
  def metadata
    { Metadata.new.process(:caller => caller_to_use) }
  end

  def mfe(key)
    metadata.for_example("this description", {:caller => caller_to_use, :arbitrary => :options})[key]
  end

  it "stores the description" do
    mfe(:description).should == "this description"
  end

  it "creates an empty execution result" do
    mfe(:execution_result).should == {}
  end

  it "stores the caller" do
    mfe(:caller).should == caller_to_use
  end

  # ...
end

that should have already been available at that moment, and is just as lazy as let considering one expectation per example practice, and mfe being used in all the examples with no exception.

This commit is the first one when let replaced an instance variable in before. No visible benefits: laziness is not used - defined variable is used in every single example. Dropping of @ sigil barely counts, since it could have been achieved with:

describe '...' do
  attr_accessor :metadata
  before do
    self.metadata = { Metadata.new.process(:caller => caller_to_use) }
  end
  # ...
end

I completely agree that the introduction of let is more than a mystery and the justifications were made up later on.

[pirj]

I hope you realize that I try to be as unbiased as possible, to listen and understand rather than stand my point, since I clearly realize I might be wrong, and the guide should not even rely on my opinion or anyone else's opinion directly and rather be a common ground, include established practices, some of which may be good, and some not so good, but that can be easily understood by the readers of the code.
Ruby is a rich and expressive language, and it's hard to limit it to the point when it can be understood by most in almost no time.
The guide is here to help to avoid heated style discussions in the companies and projects by providing some defaults. But it's not set in stone in general, and is not a god's gift, and definitely can be adapted to match established style in your company/project.

Some of the practices, though, are so deep-rooted (let is almost 10 years old), like this one with using lets specifically, that it would be barely possible to recommend something cardinally opposite for everyone. It's also risky to relax the common guideline, just imagine a spec file with both lets and instance variables in before, and "edit wars" attempts to fix them in one way or another.

I'm totally not opposed to using instance variables in before, it seems to be a reasonable way to write specs, however, it would be a tough choice to relax the style guide.
Actually, quite recently a 'one expectation per example' was relaxed to sound "For examples two styles are considered acceptable", "Use your best judgement in each case, and apply your strategy consistently" in this pull request. There are two cops ([1], [2]) in rubocop-rspec at the moment (well, the second one is in the acceptance stage), and you can use the one that fits your project style.

To avoid holy wars, edit wars, etc, the point is to pick one style in a given company/project and stick to it.

[pirj]

1.

Growing number of cases might require comes with the necessity to define more variables in befores...

Let me elaborate a bit here with some examples that I believe demonstrate that usage of before with instance variables doesn't prevent from messing up the spec file.

describe '#price' do
  subject(:price) { @rental.price }

  before do
    @state = 'OR'
    @city = 'Portland'
    @rental = Rental.new(@state, @city)
  end

  it 'is quite low' do
    expect(price).to eq(1000)
  end

  context 'when in Bay Area' do
    before do
      # WARNING! those two are not used to calculate `@rental`, and are overwritten by those above
      @state = 'CA'
      @city = 'SF'
    end

    it 'is twice as high' do
      expect(price).to eq(2000)
    end
  end
end

This issue can be solved by an undocumented prepend_before RSpec hook.
hope you'll find it useful.
Specifically by splitting into:

# Those will be overwritten by `prepend_before` in nested context.
prepend_before do
  @state = 'OR'
  @city = 'Portland'
end

# This will run after hooks defined with `prepend_before`, and use ones defined in nested context.
before do
  @rental = Rental.new(@state, @city)
end

But it's quite complicated. And the complexity will keep growing, if, for example you're going to add nested contexts to cover pricing in other cities of the same state.

2.

In addition to defining the state, there would be a perfect case when something needs to be stubbed, or some method needs to be called, and that will land in before.

This is basically the same example, but instead of additional instance variables used to instantiate objects in other instance variables, that would be an example with calling methods on some objects in setup phase.
And boils down to call ordering complexity that comes with nested contexts.

Sorry for late response, it took me a while to collect my thoughts.

[pirj]

I don't have any objections on relaxing the "Use let" rule, especially in cases with no nested contexts, and when the instance variable declarations are not inter-dependent. However, I'm still not convinced enough that it's way better in other cases.

Closing this for now until we have comprehensive and clear good, bad, and fair examples.