RSpec では context 間の違いを表現するときにのみ let を使う

Test which reminded me why I don't really like RSpec | Arkency Blog (日本語訳：Rails: RSpecが好きでないことを思い出したテスト（翻訳）｜TechRacho by BPS株式会社) を見ての感想。

元のコードのイマイチなところは 4 つあって、

params を before で書き換えている *1
it "will succeed" の文言
it { is_expected.to be_success } と expect(result.success?).to eq(true) が混ざっている
let が不思議な順序で連発されていて事前条件を読み解けない

すべて、これによって何をテストしているのかが分かりづらくなっているという問題を引き起こす。

`params` を `before` で書き換えている

let(:params) { { last_name: "something" } }
subject(:result) { described_class.call(current_user: user, params: params) }
...
describe "checking Address update" do
  let(:new_zip_code) { Faker::Address.zip_code }
  before { params[:zip_code] = new_zip_code }

  context "when user has address and want to change something in their address" do
    let!(:address) { create(:address, owner: user) }

    it "will succeed" do
      expect(result.success?).to eq(true)
      ...

この before は上の方で定義してある let との合わせ技で、かなり厳しい。

このクラスでは、異なる結果が得られる主な入力は params である。

この入力がどう変わると出力がどう変わるかは明確にわかるはずだが、これを call に明示的に渡さない理由がわからない。

にまったく同意で、そのために describe で切り分けているのだから、params に何が渡るのかは describe 内で明示する。何をテストしているのかを明確にするというのを意識したい。今は params の違いをテストしているのだから、params が何なのかをそのまま書く。let で定義して subject に渡しているので、let で上書けば良い。

 let(:params) { { last_name: "something" } }
 subject(:result) { described_class.call(current_user: user, params: params) }
 ...
 describe "checking Address update" do
   let(:new_zip_code) { Faker::Address.zip_code }
-  before { params[:zip_code] = new_zip_code }
+  let(:params) { {
+    last_name: "something",
+    zip_code: new_zip_code,
+  } }

   context "when user has address and want to change something in their address" do
     let!(:address) { create(:address, owner: user) }

     it "will succeed" do
       expect(result.success?).to eq(true)
       ...

「他のところで作ったものを before でちょこっと書き換えよう」はだいたい悪手。あと多分ココは last_name: "something", を残す必要なくて、zip_code のテストをしたいので zip_code のみになっている方が望ましいと思う。

`it "will succeed"` の文言

describe "checking Address update" do
  ...
  context "when user has address and want to change something in their address" do
    ...
    it "will succeed" do
      ...
    end
  end

  context "when user has not have any address and want to change something in their address" do
    ...
    it "create address with given params" do
      ...
    end
  end
end

これは何のテストをしているのか読み解くのが難しいと思うけど、よくよく読むと、やりたいことはただの upsert である。おそらく実装側はこうなっている。

if params[:zip_code]
  address = current_user.address || current_user.build_address
  address.zip_code = params[:zip_code]
  address.save!
end

upsert のテストをしているというのが分かるように説明を書く。なお、テスト対象や事前条件と context の説明は一致していて、context 直下に before もしくは let が来るのが良い分割。テストの違いを context や before, let で表現するのだ。

ついでに何をテストしているのか分かりやすいように「params[:zip_code] を渡したとき」というのも説明文として表現するとより親切だろう。

context "with params[:zip_code]" do
  let(:params) { {
    zip_code: new_zip_code,
  } }
  ...
  context "when address exists" do
    before { create(:address, owner: user) }
    ...
    it "will be updated" do
      ...
    end
  end

  context "when address does not exists" do
    ...
    it "will be created" do
      ...
    end
  end
end

describe, context, it の説明文を駆使して何をテストしているのかを明らかにするのは、可読性の高いテストを書くための基本です。

`it { is_expected.to be_success }` と `expect(result.success?).to eq(true)` が混ざっている

context "when address exists" do
  ...
  it "will be updated" do
    expect(result.success?).to eq(true)
    expect(address.reload.zip_code).to eq(new_zip_code)
  end
end

context "when address does not exists" do
  ...
  it { is_expected.to be_success }
  it "will be created" do
    expect { result }.to change(Address.all, :count).from(0).to(1)
    expect(Address.last.zip_code).to eq(new_zip_code)
    expect(user.address).to eq(Address.last)
  end
end

update 側では result.success? と、更新されたことの 2 個の expect があるが、 create 側では result.success? は単独のテストとして切り出されているし、書き方も違う。

書き方が違うと「この違いに何か意味があるのか？」という疑問が湧いてしまい、テスト対象がぼやける。テストの粒度は揃える。

また、success? であることはあまり主眼ではないので、目立たないように定型文っぽくなっているとより良い。つまり eq(true) とハッキリと対象をテストするよりも、読み下して読み飛ばせる方が望ましい。

 context "when address exists" do
   ...
+  it { is_expected.to be_success }
   it "will be updated" do
-    expect(result.success?).to eq(true)
     expect(address.reload.zip_code).to eq(new_zip_code)
   end
 end
 
 context "when address does not exists" do
   ...
   it { is_expected.to be_success }
   it "will be created" do
     ...
   end
 end

こうすると、全 context で「result.success?」と「詳細な副作用」をテストしている、と整理できる。

`let` が不思議な順序で連発されていて事前条件を読み解けない

context "when user has parent and want to change something" do
  let(:parent) { create(:parent) }
  let(:user) { create(:user, profession: student) }
  let(:student) { create(:student, parent: parent) }

  it { is_expected.to be_success }
  ...

ここでは parent を upsert するときの update 側をテストしたい。そのため、テスト対象 (subject) である Api::Students::Update.call(current_user: user, params: params) の user 側に「parent を持っている User である」という事前条件がある。

最終的に欲しいのは user なので、せめてこの順に並んでいて欲しい。

  let(:parent) { create(:parent) }
  let(:student) { create(:student, parent: parent) }
  let(:user) { create(:user, profession: student) }

また、事前条件なんだから before で整える、というのも自然な考え方だろう。let と違い遅延評価されず、スコープが絞られるため、怖れずに上から順に読めるようになり、処理を追うのが簡単になる。

context "when user has parent and want to change something" do
  before {
    parent = create(:parent)
    student = create(:student, parent: parent)
    @user = create(:user, profession: student)
  }
  let(:user) { @user }

  it { is_expected.to be_success }
  ...

let(:user) { create(:user, profession: @student) } でも良いんだけど、私は before で作ったインスタンスを let にパスするだけが好きです。この方が事前条件を整えた後に context に受け渡している感がある。

僕なら最初の 5 分でこうリファクタリングする

RSpec.describe Api::Students::Update do
  let(:user) { create(:user) }
  let(:params) { { last_name: "something" } }
  subject(:result) { Api::Students::Update.call(current_user: user, params: params) }

  context "when user is a student" do
    it { should be_success }
  end

  context "when user is teacher" do
    let(:user) { create(:user, :teacher) }
    it { should be_failure }
  end

  context "with params[:zip_code]" do
    before { @new_zip_code = Faker::Address.zip_code }
    let(:params) {
      {
        last_name: "something",
        zip_code: @new_zip_code,
      }
    }

    context "when address exists" do
      before { @address = create(:address, owner: user) }

      it { should be_success }
      it "will be updated" do
        result
        expect(@address.reload.zip_code).to eq(@new_zip_code)
      end
    end

    context "when address does not exists" do
      it { should be_success }
      it "will be created" do
        expect { result }.to change(Address.all, :count).from(0).to(1)
        expect(Address.last.zip_code).to eq(@new_zip_code)
        expect(user.address).to eq(Address.last)
      end
    end
  end

  context "with params[:parent]" do
    before { @parent_first_name = Faker::Name.female_first_name }
    let(:params) {
      {
        parent: { first_name: @parent_first_name },
      }
    }

    context "when parent exists" do
      before {
        @parent = create(:parent)
        student = create(:student, parent: @parent)
        @user = create(:user, profession: student)
      }
      let(:user) { @user }

      it { should be_success }
      it "will be updated" do
        result
        expect(@parent.reload.first_name).to eq(@parent_first_name)
      end
    end

    context "when parent does not exists" do
      it { should be_success }
      it "will be created" do
        expect { result }.to change(Parent.all, :count).from(0).to(1)
        expect(Parent.last.reload.first_name).to eq(@parent_first_name)
        expect(user.profession.parent).to eq(Parent.last)
      end
    end
  end
end

こうしておくと -f d で実行したときにも読み解きやすい。

Api::Students::Update
  when user is a student
    is expected to be success
  when user is teacher
    is expected to be failure
  with params[:zip_code]
    when address exists
      is expected to be success
      will be updated
    when address does not exists
      is expected to be success
      will be created
  with params[:parent]
    when parent exists
      is expected to be success
      will be updated
    when parent does not exists
      is expected to be success
      will be created

change はあんまり使いたくないなー (expect { result }.to はちょっと読みづらくない？) とか、多分 create の方のテストを先に書くだろうな (条件が複雑な方をより後でテストしたいので、レコードが存在している場合は後ろに書きたい) とか、一つのテストの中で複数の expect を書くなら aggregate_failures を使えとかの思いはあるけど、シュッと直すならこうかなー

元記事のリファクタリング後と何が違うか

describe (context) のネストは維持している
- テスト対象が明確に違うので、ネストしておく方が自然
- params[:zip_code] や params[:parent] があるときの、それぞれの upsert を確認している
context の説明はより実装寄りに
- ユニットテストなので実装寄りの言葉で問題無いし、その方が実装を書き換えるときにテストの対応を取りやすい
subject を引き続き使う
- テストしたいのは params の違いなので、params に着目したい。
- 元記事のリファクタリング後だと params は 70 文字目辺りから始まっていて、どこに違いがあるのかを読み解くのが困難
- ```
    result = Api::Students::Update.call(current_user: user, params: { parent: { first_name: parent_first_name } })
```
- subject に執着しているわけではなく、テスト間の違いをもっと目立たせろと思っています
let は context 間の差を表現するために使う
- よりテスト間の違いに目を向けさせるためです
- let は遅延評価＆後勝ちなところが大きなメリットで、まさに context 内で上書きしたいときに使う語彙です
- そのため、私は積極的に before やローカル変数、インスタンス変数を使い、let を温存します
  - 今回の書き換えでも user, params のみが let になっていて、何をテストしているのか読み解きやすいんじゃないか？
  - テーブルテストと同じことを context と let でやっているのです

subject やインスタンス変数に反対の派閥があるのは知っていて、異論はあると思うけど、私は「テスト対象が分かりやすくあった上で Ruby/RSpec の表現力にあやかりたい」と考えているので、こう書いています。

合わせて読みたい：Rails Developers Meetup 2017 で RSpec しぐさについて話した - onk.ninja

*1:公開当初は「params を merge している」と書いていたのでこういう反応があった https://twitter.com/qsona/status/1629840312389742593

params を before で書き換えている

it "will succeed" の文言

it { is_expected.to be_success } と expect(result.success?).to eq(true) が混ざっている

let が不思議な順序で連発されていて事前条件を読み解けない