「もうやめて！レビュワーのライフは0よ！」と言いたくなるRSpecの書き方

RSpecのレビュー大変問題

RSpecって本当に色々な書き方ができますよね。

mockを盛り盛りに書く人、DRYを追求したspecを書く人、itを細かく分ける人 etc...

個人的に、specの書き方は開発チーム内で良しとされているならその書き方で良いと思います。

ただ、新しくチームに入った人や、自分のように普段は違うチームで開発している人が見ると、理解しづらい、レビューしづらい、テストコードを追加、削除しづらい書き方ってあるよな〜と思ったので、まとめてみました。

あくまで個人の意見なので、温かい目で見てください🙇

！のついていないlet

👩「あなた!が付いてないようだけど、どこから呼ばれるのかしら？？」

RSpec.describe User do
  let(:user) { create(:user) }
  let(:book) { create(:book, user: user, store: store) }
  let(:store) { create(:store) }
  
  .......
end

この様にletが無限に続くような記述がされていると、このテストコードを読む時に「え、どのletがいつ呼ばれて、データがcreateされるんですか？？？」となるのでしんどい。

テスト実行前に作成するべきデータは全てlet! or before do ~ endで記述した方が分かりやすいテストコードになると思います。

AAA（Arrange, Act, Assert）を意識するだけでかなり読みやすい。

https://tech.pepabo.com/2021/08/23/writing-unit-test-with-aaa/

RSpecは expect { user.update! }.to change { .... }のように記述できるので、ActとAssertは1行で記述することに関して、個人的には良いと思います。

改善案

素直にlet!を使おう〜

もし、後続の処理でlet!で定義したデータが呼び出されないなら、before do ~ endで定義しよ〜

RSpec.describe User do
  let!(:user) { create(:user) }
  let!(:book) { create(:book, user: user, store: store) }
  let!(:store) { create(:store) }
  
  .......
end

上書きされたlet

🙍‍♂️「え、let君、上書きされてしまうの？？、あなたの本当の姿は、今何になっているの？？」

RSpec.describe User do
  let(:params) { { title: 'hoge', author: 'fuga', price: 1_000 } }
  
  describe '#register!' do
    context 'priceがnilの場合' do
      let(:params) { { title: 'hoge', author: 'fuga', price: nil } }
      
      ...
    end

    context 'titleがnilの場合' do
      let(:params) do
        super().tap { _1[:title] = nil }
      end
      
      ...
    end
  end
end

こんな感じで、各contextでletで定義したコードが上書きされていくような書き方は、シンプルに読むのがしんどい。。。

まだ、context 'priceがnilの場合'の方は、新しく定義し直して上書きしているような書き方なので少しは大丈夫な気もするが、context 'titleがnilの場合'の方はsuper(), tap等のメソッドが出てくるのが個人的に好きじゃないな〜という気持ち。

このコードをレビューすると仮定したら、super().tapの記述を見つけた瞬間に「元のletはどこや！？」と、コードを遡らなければいけなくなるため、このような書き方は極力（しなければいけない理由がある場合）辞めるべき。

改善案

多少DRYじゃなくなっても良いので、各describe, contextに一つずつ定義しよう〜

RSpec.describe User do
  describe '#register!' do
    context 'priceがnilの場合' do
      let(:params) { { title: 'hoge', author: 'fuga', price: nil } }
      
      ...
    end

    context 'titleがnilの場合' do
      let(:params) { { title: nil, author: 'fuga', price: 1_000 } }
      
      ...
    end
  end
end

user1, user2...と謎のナンバリングがされたlet

『メソッド、変数、クラスの命名はしっかりとやれ！！』と口酸っぱく言われたが、RSpecでは適当に書きがちな人、正直に手を上げてください。。。🙋‍♂️

RSpec.describe User do
  describe '#register_all_user!' do
    let!(:user1) { create(:user, registered: true) }
    let!(:user2) { create(:user, registered: false) }
  
    it 'user1は登録ずみのまま、user2は登録されること' do
      expect { User.register_all_user! }.to change ....
    end
  end
end

こんな感じでuser1, user2とナンバリングされたデータを作成するのは辞めたい。
せっかく好きな名前でletを定義できるので、意味のある名前にした方が分かりやすいと個人的に思います。

例えば今回のuser2はnot_registered_user的な感じの名前にすれば「登録されていないuser」ということが一発で分かるので、その後のitの説明も分かりやすく記述できると思います。

あと、before do ~ endの中でそういったデータを作成するときは、コメントを残しとけば良いかな〜という気持ちです。

改善案

letの命名も、普段の命名みたいに分かりやすくしよ〜

RSpec.describe User do
  describe '#register_all_user!' do
    let!(:registered_user) { create(:user, registered: true) }
    let!(:not_registered_user) { create(:user, registered: false) }
  
    it 'registered_userは登録ずみのまま、not_registered_userは登録されること' do
      expect { User.register_all_user! }.to change ....
    end
  end
end

必要の無い値が入ったlet

let!君 「よし、名前はtakashiっと、え？名前の意味？特にないっすね〜〜」

RSpec.describe User do
  describe '#register!' do
    let!(:user) { create(:user, name: 'takashi', registered: false) }
    
    it 'userが登録されること' do
      expect { user.register! }.to change { user.registered }.from(false).to(true)
    end
  end
end

今回のサンプルコードのテストではlet!(:user)で定義されているname: takashiは関係がないはず（takashiっていう名前だけ登録できない等の理由がなければ。。。）

このテストではregisteredの値がtrueorfalseのみ指定しておけば良いので、余計な情報はletに含めない方が良い。

レビュワーによっては「なんでnameが指定されているの？？」と突っ込まれることもあるかも。

改善案

letで定義するデータには必要最低限の情報だけ付与しよ〜

RSpec.describe User do
  describe '#register!' do
    let!(:user) { create(:user, registered: false) }
    
    it 'userが登録されること' do
      expect { user.register! }.to change { user.registered }.from(false).to(true)
    end
  end
end

デフォルト値に依存したテスト

let!君「俺の名前？Factoryを見に行ってくれや」

FactoryBot.define do
  factory :user do
    name { 'takashi' }
  end
end

RSpec.describe User do
  describe '#takashi?' do
    let!(:user) { create(:user) }
    
    it { expect(user.takashi?).to eq true }
  end
end

FactoryBotで定義されているUserのnameがdefaultで'takashi'になっている状態で、takashi?(takashiならtrueを返すメソッド)をテストしているコード。

テストコードを見ただけでは、let!(:user)で作成されるuserにname: 'takashi'が入っていることを分からないため、非常に分かりづらい（これはちょっと極端なサンプルだが。）

必要の無い値が入ったlet

とは逆で、テストする際に対象となる値はテストコード中に登場させよう。

改善案

テスト対象の値は、テスト内で明示的に記述しよ〜

RSpec.describe User do
  describe '#takashi?' do
    let!(:user) { create(:user, name: 'takashi') }
    
    it { expect(user.takashi?).to eq true }
  end
end

subjectの返り値を検証するテスト

👩「人類の安寧のために、subjectへの鉄槌を」

RSpec.describe User do
  describe '#registered_hoge_service?' do
    subject { user.registered_hoge_service? }
    
    let!(:user) { create(:user, name: 'takashi', hoge_registered_at: Time.zone.now) }
    
    it 'trueが返ること' do
      expect(subject).to eq true
    end
    
    # もしくは
    it { is_expected.to eq true }
  end
end

個人的にsubject自体あまり使用したくないが、それに関しては荒れそうなので置いておく。

が、subjectを使用するなら、せめて返り値を検証するようなspecで使用しないで欲しい。

このテストをレビューするとしたら、絶対に「subjectって何だっけ？」といちいち定義位置まで見に行くことになるはず。
それが凄く手間。

あと、可読性の面でもexpect(subject).to eq ..よりexpect(user.registered_hoge_service?).to eqの方が読みやすい。（と個人的に思う）

というか今までの経験上、「subjectのおかげでめちゃくちゃ読みやすい！！！」となったことより「subjectどこに定義されてんねん？今何行目までみたっけ？？」となったことの方が多いので、基本的にsubjectは使わない or せめて返り値を検証するようなテストでは使わない方が良さそう

改善案

subject使うのやめよ〜

RSpec.describe User do
  describe '#registered_hoge_service?' do
    let!(:user) { create(:user, name: 'takashi', hoge_registered_at: Time.zone.now) }
    
    it 'trueが返ること' do
      expect(user.registered_hoge_service?).to eq true
    end
  end
end