Use let and let!

[andreareginato]

Write your thoughts about the "use let and let!" best practice.

[myronmarston]

I'm a big fan of the let syntax, but I think the description in this section doesn't explain much about why it's so useful (and preferable over other options, like before { @blah = Blah.new }. I posted an answer on stack overflow that lots of people have upvoted that discusses why I think let is so useful--feel free to borrow parts of that (or link to it).

[myronmarston]

Also, one other thing: I've found that people are often initially confused by the semantics of let and some folks refer to the let definitions as "variables", which adds to the confusion. I think it'd be helpful if this section showed an example of what let actually is:

# this:
let(:foo) { Foo.new }

# ...is very nearly equivalent to this:
def foo
  @foo ||= Foo.new
end

[yfeldblum]

In this example:

context 'when updates a not existing property value' do
  let(:properties) { { id: Settings.resource_id, value: 'on'} }
  let(:update)     { resource.properties = properties }

  it 'raises a not found error' do
    expect { update }.to raise_error Mongoid::Errors::DocumentNotFound
  end
end

this bit:

let(:update)     { resource.properties = properties }

is not a good practice. The reason is that let is to build/fetch/compute a value, while caching the resulting value for the duration of the example. But in this instance, the intention is specifically to perform an action. There is no intention of building, fetching, or computing a value.

Sure, it kind of works in this case. But when you're talking about best practices, you really want to use the best tool for the job, and not just a tool that kind of works in this case.

In this case, a method would be best:

def update
  resource.properties = properties
end

The full example using a method would be:

context 'when updates a not existing property value' do
  let(:properties) { { id: Settings.resource_id, value: 'on'} }

  def update
    resource.properties = properties
  end

  it 'raises a not found error' do
    expect { update }.to raise_error Mongoid::Errors::DocumentNotFound
  end
end

[nclark]

i think in this case, exactly nothing would be best:

context 'when updates a not existing property value' do
  let(:properties) { { id: Settings.resource_id, value: 'on'} }

  it 'raises a not found error' do
    expect { resource.properties = properties }.to raise_error Mongoid::Errors::DocumentNotFound
  end
end

[yfeldblum]

@nclark That's true in this particular case only because this particular case is so simple.

But this particular case really stands in as an example for many like cases, some of which are simple and some of which are much more complex. In some such complex cases, the action may be a very long line of code or may be two, three, or ten lines of code. Or maybe there are twenty it blocks with assertions based on very similar cases, and you want to be sure that exactly the same conditions are being tested in all twenty it blocks.

In these complex cases, depending on the details of the cases, you may want to wrap up that complexity in a normal Ruby method. But, as from my previous comment, you would not want to wrap it up in a let declaration because that's the wrong tool for the job.

[hosh]

The declarative nature of let() and let!() encourages you to write side-effect-free expressions.

let(:update) { resources.properties = properties }

... looks awkward because you've dropped into imperative semantics.

let(:updated_resources) { resources.tap { |r| r.properties = properties } }
it 'raises a not found error' do
  expects { updated_resources }.to raise_error Mongoid::Errors::DocumentNotFound
end

... while it looks more verbose, fits better because you are declaring what updated_resources is.

I think @nclark is right in embedding it. You are trying to test the assignment of properties and testing for a raised error. You would want to declare it in let() when you start grouping them into mixins and you want to make it overrideable.For example: https://github.com/intermodal/intermodal/blob/master/lib/intermodal/rspec/requests/rack.rb#L3

[yfeldblum]

@hosh Remember that "the assignment of properties" is a very simple example whose purpose is to illustrate a whole class of examples, many of which are very complex. And remember that the text right before the example (at http://betterspecs.org/#let) is: "Use let to initialize actions that are lazy loaded to test your specs." So my point is not really about this particular simple example alone. My point is really about the whole class of both simple and complex examples which this particular simple example represents: the class of all examples relating to deferring possibly-complex actions so that we can test the actions directly for failure, rather than just inspecting the results or side-effects of these actions.

[urbanautomaton]

@yfeldblum why would you be writing a complex series of actions for your test stimulus, though? Setup might be complex but the stimulus should surely never be more than a single method call. In the example, the actual thing being tested is the method #properties=, and the use of let (or a test method) serves only to obscure this fact.

Even assuming for the sake of argument that we might want to make assertions about complex series of actions that for some reason aren't modelled in our application's code, I still struggle to see an advantage in putting a layer of indirection between the thing we're making assertions about and the assertion being made. Can you give an example of a more complex test that would benefit from this use of let?

[yfeldblum]

subject(:thing) { described_class.new }

def run_thing
  thing.prop1 = val1
  thing.prop2 = val2
  thing.prop3 = val3
  thing.run_and_clear_props
end

specify { expect{run_thing}.to_not raise_error }
specify { run_thing ; expect{run_thing}.to raise_error }
specify { run_thing ; thing.reset ; expect{run_thing}.to_not raise_error }

[urbanautomaton]

Maybe this is personal preference, but it's highly opaque to me what is being tested in those examples. Is it the method Thing#run_and_clear_props? Is it Thing#reset? Is it Thing#some_other_method? It seems like you're trying to say four things:

1. Thing#run_and_clear_props raises no error if properties are present
2. Thing#run_and_clear_props raises an error if the properties are blank
3. Thing#run_and_clear_props clears the props
4. Thing#reset restores the properties (to what is unclear - the state before #run_and_clear_props was called? The pristine state before we set properties?)

Only two of these statements are actually about errors (and IMO the first statement is not a valuable assertion). That leaves two statements, 3 and 4, that are being indirectly tested using repetition of statements 1 and 2. Surely it's far better to directly test them:

let(:thing) { described_class.new }

describe "#run_and_clear_props" do
  it "raises an error if no properties are set" do
    expect { thing.run_and_clear_props }.to raise_error
  end

  it "clears the properties" do
    thing.prop1 = val1
    thing.prop2 = val2
    thing.prop3 = val3

    thing.run_and_clear_props
    # make assertions about properties here
  end
end

describe "#reset" do
  it "resets the object to some condition" do
    # test whatever it is that #reset actually does
  end
end

It also seems that Thing#run_and_clear_props is doing too much (the name is a giveaway) - I would decompose it into #run and #clear methods that could be independently tested. This is my point, really - the necessity of eliding complex test actions using let or a test method is, to me, a code smell; it's an indication that we're testing something that needs to be broken down, or that there's some process or entity that we should be modelling in our application code, not our tests.

Test complexity is design feedback - we should eliminate it by improving our design, not by disguising it in our test suite.

[yfeldblum]

In the example, think of the prop-setting like passing multiple named arguments to run, but with accessors instead of arguments. Or replace it in your mind with a builder pattern which modifies the receiver until the final construction method is called.

It's important not to get bogged down in the simplistic details of a stylized example. In any particular example, of course, there will be many ways to improve the code (from at least some people's viewpoints on code goodness).

It's also important not to force the principles of good unit tests onto tests that may not be unit tests. In non-unit tests, we may find it desirable to test a sequence of steps as one, rather than just each step individually.

In the particular example, run_and_clear_props is called after setting certain props (like passing arguments), and then may not be called again until reset is called first. So calling it once is good, calling it twice is bad, but calling it once, then calling reset, then calling it again is once again good. The point isn't that this is a good API for some class. The point is that this is the API for some class and we wish to bring the class under test.

[urbanautomaton]

In the particular example, run_and_clear_props is called after setting certain props (like passing arguments), and then may not be called again until reset is called first. So calling it once is good, calling it twice is bad, but calling it once, then calling reset, then calling it again is once again good.

Right, but why should the reader of these tests have to infer Thing's real behaviour from this complex series of X = good, Y = bad statements? And how can we be sure that we've covered every possible sequence of API calls if we test in this fashion? Even if we assume for the sake of argument that we really are bringing an unchangeable API under test (and that really isn't a stated condition of what are supposed to be a set of universally-applicable "best practices") then this is still a poor way of communicating behaviour.

I agree that obsessing over the details of simplified examples can be counterproductive, but if the only example we can find where we might want to put stimuli in a test method is illustrative of both bad API design and bad testing, then it hardly recommends the approach as a "best practice" of any sort. On the contrary, it illustrates precisely my objection: that if your required stimulus is complex, then you're trying to test too much at once.

[hosh]

@yfeldblum Sure, I understand that. I also point out that often times, there is needlessly complex side effects. Some things really do need a lot of operations jammed together, but not everything.

My main use for methods is not complex/simple distinction. It is whether I want to write a macro or not. I tend to use Ruby methods to set up context, let, and it blocks rather than using shared_example().

As far as betterspecs goes and your argument that this example is a simple case: this means that we should write a better example.

[mike-burns]

The data introduced by let should be well-named so you don't need to look it up. One way to encourage this is to move all let definitions to the bottom of the scope, so they are out of the way. If they were actually important they would be right above the assertion, afterall.

[andreareginato]

I've updated the main example, added a simple example of what let looks like and the link to the @myronmarston stackoverflow definition. Any correction and comment to the updated guideline is appreciated.

[killthekitten]

We should give an example of let! usage.

One can miss this tiny comment that is in guideline and get some pain.

[johnrees]

Is there a way to force-load/instantiate a let variable inside a spec, (so it behaves the way that let! does)?

If I have a lot of specs that use let in the way it was designed, i.e. lazy loading, but then have one spec that needs the variable to be loaded immediately I will begin with the line let.reload. Is this a bad practice?

[cupakromer]

@johnrees can you give an example? let! is just syntactic sugar for:

let(:special) { 'hi' }
before { special }

So you can always just call the bareword name at the start of a spec.

[hadees]

Should let always be just one line?

For example if I had

let(:book) { Book.new }

before { book.read }

that just looks off to me. Should book.read go into the let block? Or should it be pushed off into a factory.

[alexandru-calinoiu]

@hadees

What are you testing afterwards because if you are testing the read functions result you can:

subject { book.read }

it { should be_true }

if you are testing the book object after you called read on it, you are better off returning self in book.read so your test will look like

subject { book.read }

its(:read?) { should be_true }

[sshaw]

As a user of let, I often question its benefits over before (particularly let!). At times it seems like cargo culting at its finest. Is it not just a gateway to writing obscure tests?

If we're concerned about expressing intent, how is the word "let" clearer than the word "before"?

Lazy evaluation? Maybe in production. Not sure how this is useful when testing. Are @myronmarston's stackoverflow answers really that compelling?

1) okay, can't argue with personal preference, but otherwise seems very minor

2) seems to be the most "legit" answer, but doesn't this mean that let lets you keep your problematic tests around without a refactor? Or that it prevents one from even knowing that you should refactor? Ignorance is bliss, maybe?

3) somewhat the same as 1, but does this really make a difference at all?

4) Easier to read, but not necessarily easier to comprehend. Lazy evaluation masks intent, failures can lead one to waste time tracing the execution order of the lazy evaluation.

How would one rewrite this failing test using let? Would you have to use let!?

describe "#search" do
  let(:macro_hood) { Fabricate(:macro_neighborhood) }
  let(:hood) { Fabricate(:neighborhood, :macro_neighborhood => macro_hood) }
  let(:restaurant) { Fabricate(:restaurant, :neighborhood => hood) }

  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].id).to eq restaurant.id
    end
  end

  context "given the name of a restaurant" do 
    # ...
  end
end

And if so it that really clearer/cleaner than:

describe "#search" do
  before do 
    @macro_hood = Fabricate(:macro_neighborhood)
    @hood = Fabricate(:neighborhood, :macro_neighborhood => @macro_hood)
    @restaurant = Fabricate(:restaurant, :neighborhood => @hood)
  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].id).to eq @restaurant.id
    end
  end
end

[mike-burns]

@sshaw I don't think I'd use a let at all in your example:

describe '#search' do
  it 'returns results in the given neighborhood' do
    neighborhood_name = 'Vasastan'
    restaurant = create(:restaurant, neighborhood_name: 'Vasastan')
    create(:restaurant, neighborhood_name: 'Solna')

    RestaurantSearch.search(neighborhood_name)
    expect(result.size).to eq 1
    expect(result[0].id).to eq restaurant.id
  end
end

Maybe I'm not the target audience for this spec suggestion, though; I tend to avoid let in general ...

[cupakromer]

@sshaw so just my additional 2¢ on the benefits of let:

• 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

• let reads more as a proof statement:

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

• Related, 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.

My personal feelings about let include:

• let is abused and often overused (I am guilty of this myself)
• let should be used only after you have written several specs, and not necessarily be the first line of code you are writing
• I am not a fan of let!, but I know some of the RSpec core team members feel differently (I wrote about some of the surprises let! may bring: The Bang is for Surprise)

As I said, I feel too many people abuse let. I agree with @mike-burns in that I would not use a let in the example you posted, I would start by inlining the variables. Additionally, the macro_hood seems to only be used (from the sample portion) by the hood definition; I would only use it inline and not define it with a let.

For me, my general guideline is:

Use let only to extract an object in a context where you intentionally mean to state: "This object should represent the exact same state / concept across ALL of the following specs"

If you change a let variable you violate that contract. At this point you should remove the let and inline the variables or you should reconsider the organization of your specs.

This concept comes from DRY. However, not in the way most people think. I'm not referring to pure code duplication here. Often variables in specs have the same name but this is simply coincidental duplication and not actual. What I mean by that is, while the code looks the same, the variables aren't meant to be the same, I could change the names and the duplication would disappear while the specs remain the same.

When I refer to DRY here I mean: A single authoritative source for the definition of a domain object / concept. If I change the meaning of that domain object, then it should propagate to all places that use it.

[reconstructions]

I am convinced by this argument concerning the dynamic scoping that comes with using let. If you want to add the complexity of dynamic scoping to your tests, go ahead, but in my opinion it is a KIS violation to use it unless you actually need it. Like recursion, some developers are going to love using dynamic scoping because it is intellectually challenging and makes them feel smart, but avoiding this kind of lets make it complicated for the sake of it attitude is why KIS had to be invented to get things done faster.

I have done everything under the sun with instance variables, which are Ruby convention and totally predictable, and not once have I found myself saying, "this test would work better/be easier to write/be more legible" if only I had the dynamic scoping that let would provide. The idea that it is really important to lazy load variables to speed up your tests seems silly as well, because if you want to run some of them to save time, you can just use rspec spec/ --tag tag_name to run a few of the tests, and if you are going to run all of them you are going to load all the variables anyway, so who cares? In any case, all my tests run just fine, and I certainly have saved a lot of time relative to the old days when I was trying to figure out how let side effects were affecting my tests.

The only down side to instance variables is that it is hard to learn all the cool stuff you can do with them because all the tutorials and Stack Overflow examples are using let. let is one of the places I think DHH has traction in his criticism of RSpec. Not KIS and YAGNI, cargo cult for sure.

Everyday Testing in RSpec was a refreshing breath of 'let' free air...

[wconrad]

The examples use tabular code alignment (inserting horizontal whitespace to make things line up):

  let(:resource) { FactoryGirl.create :device }
  let(:type)     { Type.find resource.type_id }

It's a bit of an anti-pattern and perhaps shouldn't be shown to programmers who might imitate it without understand its costs. That said, it is a nit and not a big deal here.

[onebree]

@wconrad I really like your blog post. I think, however, for tests that tabular alignment is okay. Especially since RSpec tests can be pretty long. In my experience, the individual example does not change, but rather more examples are added.

[wconrad]

@onebree It seems to me that the longer the block of tabularly aligned code, the more the cost when the alignment changes. That said, you're right that it's less costly if the block seldom changes than if it changes often.

[mariozaizar]

Wondering what's the big reason to use let or let! instead of CONSTANTS?
I'm in favor of let, but what would be the big reason to do use them?

Like in this example:

let!(:admin_emails){ [ 'admin@site.com', 'boss@site.com'  ...] } 
admin_emails.each do |e|
  it "uses #{e} email as admin" do
    pending
  end
end 

vs

ADMIN_EMAILS = [ 'admin@site.com', 'boss@site.com' ... ]
ADMIN_EMAILS.each do |e|
  it "uses #{e} email as admin" do
    pending
  end
end 

UPDATE: in my scenario, seems like you can't use let's to build dynamic specs (it). producing:

test.rb:12:in `block in <top (required)>': undefined local variable or method `admin_emails' for Class.

[wconrad]

@mariozaizar :

A constant is neither better nor worse than let or let!; it is merely different.

A constant is accessible to both the class, at load time, and to the instance, at run time.

letis accessible only to the instance, at run time.

A loop that defines multiple test cases using it is load-time, and so cannot access let declarations.

A let declaration is roughly equivalent to defining a method with caching. So this:

let(:foo) { "bar" }

is like this:

def foo
  @foo ||= "bar"
end

Let is necessary for definitions that cannot happen at load time. For example, in a test using ActiveRecord models, you may need for an instance of an ActiveRecord model to be created:

let(:user) { User.create(username: "fred") }

You could not define this as a constant:

USER = User.create(username: "fred")

Because the constant is defined at load time, none of the database setup has been done yet. There probably isn't even a database connection.

[graywolf-at-work]

I was linked http://www.betterspecs.org/ by collegue to rewrite my spec but I struggle with let...

Assuming I currently have a code like this

describe Foo do
  before(:each) { @user = User.new }

  context 'something' do
    ...
  end

  context 'cancelled' do
    before(:each) { @user.cancelled = true }
    ...
  end
end

how can I rewrite this using let while keeping it dry? Sure, I can do

describe Foo do
  let(:user) { User.new }

  context 'something' do
    ...
  end

  context 'cancelled' do
    let(:user) { User.new.tap { |u| u.cancelled = true } }
    ...
  end
end

but that means repeating the User.new every time. Is there something like
let(:user) { outer_let(:user).tap { |u| u.cancelled = true } } ?

[wconrad]

@graywolf-at-work -- I don't worry too much about DRY in my tests. I find clarity and simplicity to be of greater value in a test than DRY. I can't think of any way to have just a single User.new that isn't more trouble than it's worth. For example, you could do this:

describe Foo do
  let(:user_attrs) { {} }
  let(:user) { User.new(user_attrs) }

  context 'something' do
    ...
  end

  context 'cancelled' do
    let(:user_attrs) { { cancelled: true } }
    ...
  end
end

But that's really no better. Instead of having to repeat User.new, you have to repeat user_attrs.

I have done something like this, that's slightly better, on occasion:

describe Foo do
  let(:cancelled) { false }
  let(:user) { User.new(cancelled: cancelled) }

  context 'something' do
    ...
  end

  context 'cancelled' do
    let(:cancelled} { true }
    ...
  end
end

But I don't see anything wrong with repeating User.new. Clarity and simplicity trump DRY in tests.

[myronmarston]

Is there something like let(:user) { outer_let(:user).tap { |u| u.cancelled = true } } ?

There is--it's super(). let defines a method and nested groups are subclasses of their parent group, so you can use super():

describe Foo do
  let(:user) { User.new }

  context 'something' do
    ...
  end

  context 'cancelled' do
    let(:user) { super().tap { |u| u.cancelled = true } }
    ...
  end
end

That said, I wouldn't recommend that here. I think using a before to set the cancelled flag makes sense -- you intend to mutate the user as a side effect, and that's what before hooks are for!

describe Foo do
 let(:user) { User.new }

  context 'something' do
    ...
  end

  context 'cancelled' do
    before(:each) { user.cancelled = true }
    ...
  end
end