【初心者向け】レビュワーをイライラさせるRSpec集と解決方法

こんにちは。メドピアにjoinして3ヶ月目の保立です。
毎週のように新しい開発が進むため、毎日楽しくソースコードを書かせてもらっています。

テストコードを制するものは、Railsを制す!!!
ということで、今回はメドピアのRSpecについてです。
メドピアでは、RSpecを用いてテストコードを書いており、
- 1) models配下に記載するビジネスロジックに対するUnitTest
- 2) 機能ごとのEndToEndTest (E2E Test)
の2種類のテストコードを書いています。

RSpecについて、書こうと思ったきっかけ

RSpecは（というかRuby自体が）様々な書き方で動かすことができるため、統一したルールがないと、書いた人によってバラバラなテストコードになります。
私も初めてRSpecを書いた際に、参考書やソースコードによって書き方がバラバラで、どのRSpecを参考にすればよいのか迷いました。
RSpecでソースコードの仕様を理解できると、レビュワーや後から開発する人のストレスが軽減できますが、テストコードごとのフォーマットがバラバラだと、テストコードを理解するのに、時間がかかってしまいます。（イライラする人が出てきちゃいます）
そこで、今回は、メドピア内部のルールや、今まで指摘を受けたこと、私が気をつけていることをまとめていきます。

describe/context/itのフォーマットが統一されていない
インスタンス変数を使用している
letで定義した変数名が、何を表しているかわからない
beforeブロック内でテストを行う
テスト対象が同じ、複数のテストケースで、subjectが使われていない
環境や時間によって失敗する

1. describe/context/itのフォーマットが統一されていない

テスト対象、条件、振る舞いが決まったところに決まったフォーマットで記載されていれば、そのテストコードが何を表しており、実際のソースコードがどのような仕様なのかを簡単につかむことができます。
下記は、フォーマットが統一されていない例です。

# contextが日本語だったり英語だったり式だったり
describe 'valid?' do
  context '非公開のとき'
  context 'publish'
  context 'expired < Time.now'  
end

# itに条件が書かれている
describe 'valid?' do
  it '◯◯で△△のとき、回答が登録されること'
end

テストの条件や結果のフォーマットが異ならないように、メドピアでは、以下のように記載しています。

	内容	書き方
describe	テストの対象	#インスタンスメソッド名 .クラスメソッド名〜画面〜処理（E2Eテスト）
context	テストの前提条件	〜のとき
it	テストの結果	〜されること〜となること

また、なるべくcontextやitでは主語を書くようにしています。
context '非公開のとき' ではなく、context '質問が非公開のとき'のように書いています。

2. インスタンス変数を使用している

RSpecでは「インスタンス変数」（@から始まる変数）を使わず、「let / let!」を使って、テストで使用する変数を定義します。

# インスタンス変数を使用する
describe 'valid?' do
  before do
    @question = create(:question)
  end

  it 'is valid' do
    expect(@question).to be_valid
  end
end

「let / let!」を使うと、以下のように書き換えることができます。

describe 'valid?' do
  let(:question) { create(:question) }

  it '質問が登録されること' do
    expect(question).to be_valid
  end
end

インスタンス変数を使わない方がいい理由については、以下のリンクをご参考ください。

RSpecのletを使うのはどんなときか？（翻訳）| Qiita

3. letで定義した変数名が、何を表しているかわからない

letで「インスタンス変数」の代わりに、テストを行う変数を定義できます。しかし、定義した変数名が何を表すかが一目でわからないと、わかりやすいテストコードとは言えません。

describe 'valid?' do
  let(:question_1) { create(:question, title: 'タイトル') }
  let(:question_2) { create(:question, title: nil) }
  
  it 'タイトルが設定されている質問が、登録できること' do
    expect(question_1).to be_valid
  end

  it 'タイトルが設定されていない質問が、登録できないこと' do
    expect(question_2).to be_invalid
  end
end

このように、モデル名_1, 2 … というように変数名を設定すると、後から読んだ人は、どの変数が何を表しているのか理解するのに時間がかかってしまいます。
letで宣言する変数名には、以下のように何を表しているのかわかりやすい変数名を選ぶことを心がけています。

describe 'valid?' do
  # 基本パターンの場合、model名をそのまま変数名にすることが多いです。
  let(:question) { create(:question, title: 'タイトル') }
  # 例外パターンの場合、model名の後に例外内容を表す変数名にします。
  let(:question_without_title) { create(:question, title: nil) }
  
  it 'タイトルが設定されている質問が、登録できること' do
    expect(question).to be_valid
  end

  it 'タイトルが設定されていない質問が、登録できないこと' do
    expect(question_without_title).to be_invalid
  end
end

4. beforeブロック内でテストを行う

beforeは、テストの前提条件を用意するためのブロックです。
しかし、beforeがやるべきでないテストの実施を、beforeブロックに書かれていることもあります。

context '登録ボタンをクリックしたとき' do
  # before内でテストしているケース
  before do
    within '#button' do
      expect(page).to have_css '.disabled' # 非活性であることをチェック
    end
    fill_in 'name', with: '山田太郎'
    within '#button' do
      expect(page).to have_css '.enabled' # 活性であることをチェック
    end
    click_on '登録する'
  end

  # 登録処理のテストが続く 
  it '登録されること'
  it '画面遷移すること'  
end

上記の例のように、beforeでテストをしてしまうと、テストの前提条件がどこで何をテストしているのかわかりづらいだけでなく、before内のテストで失敗した際に、後続のテストが行われなくなってしまいます。

5. テスト対象が同じ、複数のテストケースで、subjectが使われていない

subject を使用すると、そのメソッド内のテスト内容を一括で設定することができます。

# subjectを使わない
describe '#human_time_distance' do
  context '現在時刻と一致するとき' do
    let(:from_time) { now }
    it { expect(helper.human_time_distance(from_time)).to eq '今' }
  end

  context '現在時刻より前の時刻のとき' do
    let(:from_time) { now - 1.second }
    it { expect(helper.human_time_distance(from_time)).to eq '過去' }
  end

  context '現在時刻より後の時刻のとき' do
    let(:from_time) { now + 1.second }
    it { expect(helper.human_time_distance(from_time)).to eq '未来' }
  end
end

expect(helper.human_time_distance(from_time)) を subjectにまとめると以下のようになります。

describe '#human_time_distance' do
  subject { helper.human_time_distance(from_time) }
  
  context '現在時刻と一致するとき' do
    let(:from_time) { now }
    it { is_expected.to eq '今' }
  end

  context '現在時刻より前の時刻のとき' do
    let(:from_time) { now - 1.second }
    it { is_expected.to eq '過去' }
  end

  context '現在時刻より後の時刻のとき' do
    let(:from_time) { now + 1.second }
    it { is_expected.to eq '未来' }
  end
end

特に、メソッドの返却値のテストやバリデーションのテストでは、積極的にsubjectを用いて、DRYなテストコードを心がけています。

6. 環境や時間によって失敗する

ローカルでテストした時には動いたけど、特定の環境ではたまに上手くいかないテストケースもよくあると思います。
メドピアでも、ローカルではうまくいくのに、CI上では3, 4回に1回くらい失敗するテストがありました。以下の例は、メドピアでも比較的多かったケースです。

# let!でmodel作成後、テストまで時間がかかるケース
let!(:question_published) { create(:question, published_at: Time.current) }
let!(:question_non_published) { create(:question, published_at: Time.current + 1.second) }

before do
  # 色々処理が書かれる
  # 色々処理が書かれる
end

it '質問が表示されること' do
  # 色々テストが書かれる
  # 色々テストが書かれる

  expect(question_published).to be_valid
  expect(question_non_published).to be_invalid
end

上記の例では、published_atが現在時刻以降のquestionのみ表示するテストですが、1秒後に公開されるquestion_non_publishedを作成してからテストするまでにタイムラグが生じ、たまにうまくいかないことがあります。
このような場合は、テストを分割するか、1.secondをもっと大きな値に変更することで、解決できます。
常に成功するテストでないと、プロジェクト全体の生産性に影響を与えるので、現在の環境で常に動くテストコードを書く必要があります。