Use `before` and `let` when it's beneficial

[pirj]

Only use before and let, when there's a compelling benefit, don't use them as the default way to write specs. https://reddit.com/r/ruby/comments/73nrhk/ama_the_authors_of_effective_testing_with_rspec_3/dnsyanp/

An example I can think of:

# bad
describe '#filename' do
  let(:filename) { 'input.csv' }

  before { settings.load!(filename) }

  specify do
    expect(settings.filename).to eq('input.csv')
  end
end

# good
describe '#filename' do
  it 'returns filename' do
    filename = 'input.csv'
    settings.load!(filename)

    expect(settings.filename).to eq('input.csv')
  end
end

[RobinDaugherty]

I disagree with this, because quite frequently a single example (as you showed) turns into multiple examples. When let and before are used, adding another example to the same context has low friction. Alternately, when they are written as you described as "good", adding another example is much more difficult and it will likely turn into a full refactor of the original example.

# bad
describe '#filename' do
  it 'returns filename' do
    filename = 'input.csv'
    settings.load!(filename)

    expect(settings.filename).to eq('input.csv')
  end
end

# good
describe Settings do
  subject(:settings) { described_class.new(filename) }
  let(:filename) { 'input.csv' }

  describe '#filename' do
    let(:filename) { 'a_different_filename.csv' }

    it 'returns the filename provided to the constructor' do
      expect(settings.filename).to eq('a_different_filename.csv')
    end
  end
end

While this example is somewhat contrived, when more examples are added to the module, it's easier to understand how the Settings object is instantiated and how it acts as a whole.

describe Settings do
  subject(:settings) { described_class.new(filename, mode) }
  let(:filename) { 'input.csv' }
  let(:mode) { 'default_mode' }

  describe '#filename' do
    let(:filename) { 'a_different_filename.csv' }

    it 'returns the filename provided to the constructor' do
      expect(settings.filename).to eq('a_different_filename.csv')
    end
  end

  describe '#special_mode?' do
    subject { settings.special_mode? }

    context 'when "default_mode" was specified' do
      let(:mode) { 'default_mode' }

      it { is_expected.to be_falsey }
    end

    context 'when "special_mode" was specified' do
      let(:mode) { 'special_mode' }

      it { is_expected.to be_truthy }
    end
  end
end

[pirj]

I believe your example @RobinDaugherty falls into a category of specs that have a compelling benefit, while the one from description doesn't yet.

There's yet another recommendation that is mentioned in the Effective testing with RSpec book, that might be used to flatten the about-to-happen combinatorial explosion.

[mockdeep]

@RobinDaugherty I actually have a harder time understanding your example as written. There are several places in the test file where global state is being aggregated and overwritten, such that it's a little hard for me to put together a complete picture of what is being tested and what code executes before what. I generally don't shy away from code duplication in my tests, and rather avoid indirection. I'd probably write the spec above something like:

describe Settings do
  describe '#filename' do
    it 'returns the filename passed to the constructor' do
      filename = 'a_different_filename.csv'
      settings = described_class.new(filename, 'default_mode')

      expect(settings.filename).to eq(filename)
    end
  end

  describe '#special_mode?' do
    it 'returns false when "default_mode" is passed to the constructor' do
      settings = described_class.new('file.csv', 'default_mode')

      expect(settings.special_mode?).to be(false)
    end

    it 'returns true when "special_mode" is passed to the constructor' do
      settings = described_class.new('file.csv', 'special_mode')

      expect(settings.special_mode?).to be(true)
    end
  end
end

In this way, there is nothing in the spec that wasn't explicitly called for in the spec. If I wanted to factor out a little of the detail that doesn't matter, I might add a method to instantiate the settings with defaults:

describe Settings do
  def make_settings(filename: 'file.csv', mode: 'default_mode')
    settings = described_class.new(filename, mode)
  end

  describe '#filename' do
    it 'returns the filename passed to the constructor' do
      filename = 'a_different_filename.csv'
      settings = make_settings(filename: filename)

      expect(settings.filename).to eq(filename)
    end
  end

  describe '#special_mode?' do
    it 'returns false when "default_mode" is passed to the constructor' do
      settings = make_settings(mode: 'default_mode')

      expect(settings.special_mode?).to be(false)
    end

    it 'returns true when "special_mode" is passed to the constructor' do
      settings = make_settings(mode: 'special_mode')

      expect(settings.special_mode?).to be(true)
    end
  end
end

I think I'd advocate for avoiding before and let entirely in favor of a method based approach. I can see a case for after hooks, and maybe around in cases where we need to ensure that some cleanup happens, as they run even when the test fails.

[RobinDaugherty]

We're talking about a basic feature of RSpec that allows for declared variables whose values are memoized when and if needed. If you want to avoid RSpec's features and write tests that look like Minitest, then do so. However, people that use RSpec effectively understand how let-declared variables work and their precedence based on scope.

The RSpec Style Guide should recommend the effective use of RSpec features, not avoid using them because they are unfamiliar to some developers.

[pirj]

@mockdeep I like the use of a helper method in your example. @RobinDaugherty your example looks like a typical use of RSpec.

Not arguing with anyone in particular here, you both have pretty valid points. However, you seem to be sticking to one specific style, one you got used to. Both are quite good and readable.

I have some concerns regarding "effective", this term is a subject for interpretation, just like "compelling benefit" that appeared at the beginning of this discussion.

One approach is more concise and uses more of DSL capabilities RSpec provides. Another approach is slightly more code, is more verbose and is arguably easier to read due to less (human) lookups. The latter still has a thing to lookup, the helper method, but it's just one as opposed to several lets of the former.

A few points (neutral, not intended to prove anyone wrong) from RSpec's codebase:

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.

Also this note from Myron Marston:

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.

And another one:

I definitely prefer and recommend the simpler style [...]. Features like subject, is_expected, etc, have valid use cases, but unfortunately some users think it's "the RSpec style" and try to write all their specs that way. [...] more complex style requires more understanding of RSpec to grok, and the control flow jumps around. The simpler style is less code, and much more straightforward. As spec files get larger, the complex style can quickly become harder to work with, as there's more distance between an example and some of the declarations it depends upon (e.g. subject and let(:factory)).

When picking one of the styles to pick for a spec, from my understanding several factors should be taken into consideration:

• is it a spec for a feature-rich, complicated SUT (I'd go with helper methods for more complicated ones)
• is it going to grow bigger fast over time (same)
• which style will be easier to read so it still fits into "usage examples" concept
• what is the experience level of typical contributors (I've soo often people forget it's just ruby and methods can even be defined inside DSL)

In any case, if a given spec becomes unreadable, it's a good indication that SUT has grown too big, and it's not the test code's fault that hundreds of different branches have to be covered.

[mockdeep]

@RobinDaugherty

However, people that use RSpec effectively understand how let-declared variables work and their precedence based on scope.

This is a little gatekeepy. I understand how let works--I've been using it for more than a decade at this point. I still think it creates confusing indirection in the tests and needless complexity.

The RSpec Style Guide should recommend the effective use of RSpec features, not avoid using them because they are unfamiliar to some developers.

Again, it's not about familiarity, it's about the complexity it creates. It's kind of condescending to imply that people who disagree with you just don't understand the feature.

[RobinDaugherty]

@mockdeep I definitely didn't mean to be condescending here. But this is absolutely about familiarity. If you're writing specs in the style you gave, there's no reason to use RSpec. You can achieve the same with Minitest with RSpec-style matchers.

But when it comes to an RSpec (and really this came from a discussion about default settings in the linter), recommending against using one of the basic advantages of RSpec seems like a bad idea.

When there's a single example in a file calls to let and before can seem needless, but there should almost never be a single example. Almost every file should include at least one deviation from the golden path. And when a file is full of copy+pasted examples that have slight differences, any change requires manually editing all of them. The maintenance burden is far worse than the learning curve to feel comfortable seeing let be used correctly.

I feel this recommendation might be more harmful than good, especially because "when it's beneficial" is open to interpretation. In my experience, it's beneficial in the long term, almost always, because it leads to clearer setup and separation of setup and assertions.

[mockdeep]

@RobinDaugherty

But this is absolutely about familiarity.

As mentioned, I've been writing in the let style for over a decade at this point. I don't know at what point you consider someone to be familiar, but I'm pretty sure I qualify. My position comes from seeing how the let style tends to lead to tests that are more confusing and hard to maintain. Especially as a codebase grows and evolves and requires changes to existing test files.

If you're writing specs in the style you gave, there's no reason to use RSpec

This is a little reductive. RSpec has loads of other differences (and advantages, IMO) from minitest/spec. Not to mention the fact that minitest/spec does in fact have let, though the maintainers and I seem to have similar opinions on that topic.

recommending against using one of the basic advantages of RSpec seems like a bad idea.

You're taking it as a given that it's an advantage, but that's the point under debate. As linked by @pirj above, even the core maintainers have recommended using them sparingly if at all.

any change requires manually editing all of them.

It's rare if ever that I end up needing to modify multiple test examples in this way. If anything, I end up wanting to add a new minor variation and end up needing to figure out how to finagle the existing let blocks to get what I want. It's way easier if I can just copy a complete example and make an adjustment to it. If there is some underlying thing that needs to be adjusted for all the tests, that would likely be a change to make in the factories or helpers.

Keep in mind that the no-let style doesn't mean using no abstractions, but it means using more flexible abstractions, like methods, and probably fewer abstractions. The let style is, in its way, fundamentally at odds with the needs of tests. An important aspect of testing is that you want to test your code with variations of inputs. But let is static and makes variation between tests more indirect and complex. And it's viral in that if you want a variation to a let, your primary option is to define more lets. It's trying to apply DRY where it really doesn't apply.

[pirj]

is open to interpretation

The official RSpec style guide (not community) ticket, https://github.com/rspec/rspec.github.io/issues/28, mentions that the guide should not "be used to silence discussion". So interpretation based on the project specificity is the key.

one of the basic advantages of RSpec

From Myron Marston:

So my general advice isn't so much to restrict yourself per-se, but to start simple, and use additional features when it provides compelling benefits.

Some quite extreme examples from popular open-source projects: 10 lets https://github.com/forem/forem/blob/master/spec/support/api_analytics_shared_examples.rb#L2 20 lets https://github.com/chef/chef/blob/master/spec/unit/provider/systemd_unit_spec.rb#L23 30 lets https://github.com/chef/chef/blob/master/spec/unit/policy_builder/policyfile_spec.rb#L24 40 lets https://github.com/diaspora/diaspora/blob/develop/spec/integration/migration_service_spec.rb#L7 50 lets https://github.com/chef/chef/blob/master/spec/functional/resource/dsc_script_spec.rb#L71

I see this as over-use, as to read and understand this setup takes a while. Probably a completely different approach to testing should be taken for cases when the SUT's behaviour depends on 10+ variations of input data, and there is more than a few input parameters. Property testing?