Even better specs

We’re all getting quite familiar with the sunk cost fallacy as it applies to building software products. Projects that run to completion when their failure is a foregone conclusion. Dead-on-arrival features that live on solely because they took six months to build.

There’s another way that we throw good effort after bad in our particular field: holding on to a technology, language, or library that’s incredibly familiar to us long past the moment where it stopped being the right answer. It’s hard to let it go when it took so damn long to master it.

Enter RSpec 

I came to RSpec about seven years ago. At first, I didn’t quite grasp how its best-practice approach was a little bit different. It took me a while to rewire my brain to nest contexts and override lets with the best of them. But, after a few months, I was quoting https://www.betterspecs.org/ multiple times a week and passing on the wisdom of my journey to others who were new to it. I’d solemnly suggest that they treat RSpec as its own piece of the puzzle to learn. “If you’re spending time to learn about Ruby and Rails, then you’ll need to really work to learn about RSpec too. You won’t pick it up as you go like you would an xUnit testing framework. It’s just too different."

describe Searcher do
  subject { Searcher.new(array).find(value) }

  let(:array) { %w[a b c] }
  let(:value) { "b" }

  it "returns the correct index when the value exists in the array" do
    expect(subject).to eq(1)
  end

  context "with a value that doesn't exist in the array" do
    let(:value) { "d" }

    it "returns nil" do
      expect(subject).to eq(nil)
    end
  end

  context "with an array that contains nil and a nil value" do
    let(:array) { ["a", nil, "c"] }
    let(:value) { nil }

    it "returns the correct index" do
      expect(subject).to eq(1)
    end

    # This demonstrates a common problem that will happen in practice as bugs are found and new
    # edge case specs are incrementally added - context nesting can really get out of control.
    context "with an empty array" do
      let(:array) { [] }

      it "returns nil" do
        expect(subject).to eq(nil)
      end
    end
  end
end

A somewhat artificial example that illustrates the style of using lets and contexts extensively to vary the inputs to the unit under test.

Seeing the light 

When Stephen joined us as an engineer last year, he posted a pull request that fully eschewed the Better Specs gospel as part of a larger change. The PR description included the following:

(Sidenote, I refactored the tests a little to make it easier to extend them. Now, they have a slightly more linear/imperative style, with the advantage being that an individual test is easier to follow. This is obviously subjective, though, so please let me know if this doesn't feel sufficiently aligned with how we test!)

By this point, I had actually expended some effort going in exactly the opposite direction in our codebase, really bringing the Better Specs practices to bear.

Keen to avoid us bouncing back and forth with every PR and thinking Stephen had just yet to see the light, I went looking for a few references to really justify my approach. I needed to sell him on why lots of nested contexts and overridden 'lets,' the approach that had taken me years to hone, was the right way to go.

A short while later, finally understanding that Stephen had, in fact, just seen through the light show, I was very surprised to find myself replying with this:

I've always just sort of blindly followed the guidance at http://www.betterspecs.org/

When I went looking, I found a few interesting bits and pieces:

• Here are the RSpec docs contradicting Better Specs about using subject: https://relishapp.com/rspec/rspec-core/v/3-8/docs/subject/explicit-subject vs. http://www.betterspecs.org/#subject

• Here's an author of a book on RSpec warning about overuse of let and subject: https://www.reddit.com/r/ruby/comments/73nrhk/ama_the_authors_of_effective_testing_with_rspec_3/dnsyanp/

• Here are a couple of ThoughtBot articles on it: https://thoughtbot.com/blog/my-issues-with-let and https://thoughtbot.com/blog/lets-not

There are many people arguing in favour of using subject and let with nested contexts to build up the test state. However, most of their arguments seem to centre around just vaguely calling them "best practice." Any more concrete arguments I found seem focused on relatively unimportant things, like making the "documentation" output format look good or blindly trying to achieve one assertion per example so it's easy to hunt down failures (failures give line numbers, so I don't get this one?!)

Any arguments against subject/let seem focused on readability and maintainability, which really matter (and which I strongly agree with).

I'm starting to think we should adopt your more imperative style as our preferred approach and generally push towards using it where we can.

Simpler Specs 

Stephen subsequently captured our approach quite succinctly to share it with every new hire:

Features we avoid using by default:

• Before and after hooks

Why not? Using these places a burden on the reader, requiring them to read two or more discontiguous parts of the file to understand a single test case

What to use instead? When it's just a couple of lines, consider duplicating the setup code and placing it in each test example. When this becomes unwieldy, consider extracting a plain old Ruby method that encapsulates the setup, and call it within each test example.

• Subject, Let and let!

Why not? Similar to the situation with before and after hooks, this feature harms readability by dispersing the logic of the test case. The lazy-evaluation and override behaviour adds further complexity, which the reader must overcome.

What to use instead? Use plain old Ruby variables within each test case. As before, extract a method if setup is too involved or highly similar across many tests. Frequently, simple duplication will do fine.

I actually haven't written a single let statement since.

describe Searcher do
  # As test setups get more complicated, keyword arguments really help:
  def find(array:, value:)
    Searcher.new(array).find(value)
  end

  it "returns the correct index when the value exists in the array" do
    expect(find(array: %w[a b c], value: "b")).to eq(1)
  end

  it "returns nil when the value doesn't exist in the array" do
    # The small amount of repetition of "%w[a b c]" makes this far easier to read:
    expect(find(array: %w[a b c], value: "d")).to eq(nil)
  end

  it "returns the correct index when the value is nil and exists in the array" do
    expect(find(array: ["a", nil, "c"], value: nil)).to eq(1)
  end

  # New specs for newly discovered edge cases slot in more easily:
  it "returns nil when the value is nil and the array is empty" do
    expect(find(array: [], value: nil)).to eq(nil)
  end
end

The same tests from above, rewritten in our simpler, more imperative style.

Sunk costs 

It seems so incredibly obvious now how far off the mark I was in saying, "this is complicated, you'll need to spend time learning it" instead of "this is complicated, we should make it simpler."

Having spent so many years becoming deeply familiar with one way of using RSpec, it was tough to see and accept the sunk cost of that accumulated knowledge and experience. But, as with the failing project or the unused feature, the right time to stop throwing good money after bad is always right now.