Introduction

これはなに

可読性の高いテストコードを書くためのお作法集です。

みんなの意見を取り入れることでより良いガイドにしていきたいと思っているので、次に当てはまる場合はどんどんIssuesやPull Requestを送ってください。

• 疑問に思う点がある

• ここに書かれていないお作法がある

• 内容はいいけど表現がおかしい

• もっといいサンプルコードがある

English Version

前提

• RSpec

• FactoryBot

describeとcontext

describeとcontextは同じメソッドだが、次のように使い分けることで何をテストしているのかをわかりやすくできる。

• describe の引数にはテストの対象を書く

• context の引数にはテストが実行される際に前提になる条件や状態を書く

例

RSpec.describe Stack, type: :model do
  let!(:stack) { Stack.new }

  describe '#push' do
    context '文字列をpushしたとき' do
      it '返り値がpushした値であること' do
        expect(stack.push('value')).to eq 'value'
      end
    end

    context 'nilをpushした場合' do
      it 'ArgumentErrorになること' do
        expect { stack.push(nil) }.to raise_error(ArgumentError)
      end
    end
  end

  describe '#pop' do
    context 'スタックが空の場合' do
      it '返り値はnilであること' do
        expect(stack.pop).to be_nil
      end
    end

    context 'スタックに値があるとき' do
      before do
        stack.push 'value1'
        stack.push 'value2'
      end

      it '最後の値を取得すること' do
        expect(stack.pop).to eq 'value2'
      end
    end
  end
end

FactoryBotのデフォルト値

FactoryBotを利用した場合、各モデルのデフォルトのカラム値を設定することになる。このとき、各カラムの値がすべてランダム値となるように設定を書くとよい。その上で、必要な値のみをテスト中で明示的に指定することにより、「このテストで重要な値はなにか」がわかりやすくなる。

よくない例

アカウントが有効化されているかどうかをactiveカラムで管理しているとする。このような、有効／無効を表すカラムが固定されているケースはよく見かける。

FactoryBot.define do
  factory :user do
    name { 'willnet' }
    active { true }
  end
end

RSpec.describe User, type: :model do
  describe '#send_message' do
    let!(:sender) { create :user, name: 'maeshima' }
    let!(:receiver) { create :user, name: 'kamiya' }

    it 'メッセージが正しく送られること' do
      expect { sender.send_message(receiver: receiver, body: 'hello!') }
        .to change { Message.count }.by(1)
    end
  end
end

このテストはUser#activeがtrueであることが暗黙的な条件になってしまっている。sender.active #=> falseのときやreceiver.active #=> falseのときにどう振る舞うかを伝えることができていない。

さらに、このテストではnameを明示的に指定しているがこれは必要な指定なのだろうか？テストを読む人に余計なリソースを消費させてしまう、無駄なデータ指定はなるべく避けるのが好ましい。

よい例

FactoryBot.define do
  factory :user do
    sequence(:name) { |i| "test#{i}" }
    active { [true, false].sample }
  end
end

RSpec.describe User, type: :model do
  describe '#send_message' do
    let!(:sender) { create :user }
    let!(:receiver) { create :user }

    it 'メッセージが正しく送られること' do
      expect { sender.send_message(receiver: receiver, body: 'hello!') }
        .to change { Message.count }.by(1)
    end
  end
end

このテストだと「User#activeの戻り値がUser#send_messageの動作に影響しない」ということが(暗黙的にであるが)伝わる。もしUser#activeが影響するような修正が加えられた場合、CIで時々テストが失敗することによって、テストが壊れたことに気付けるはずだ。

大事なのは「テストに依存している値をすべてテスト中に明示している」ということなので、それが守られているのであればFactoryBotのデフォルト値を固定にしてもかまわない。

ランダム値が原因でテストが失敗したときの再現方法

「各カラムの値をランダム値にしてしまうと、CIでテストが失敗した時に再現できないのでは？」という意見があるが、RSpecはデフォルトの設定で次のようにseed値を外から注入できるようになっている。

spec/spec_helper.rb

Kernel.srand config.seed

なので、次のようにCIで失敗したときのseed値をrspecコマンドの--seedオプションに指定することで同じ状況を再現できる。

rspec --seed 1234

FactoryBotでbelongs_to以外の関連をデフォルトで作成しない

FactoryBotでモデルを作成する際に、関連しているモデルも同時に作成することができる。

対象の関連がbelongs_toであれば特に問題はないが、has_manyの関連を扱う場合には注意が必要になる。

例として、UserとPostが一対多だったとしてFactoryBotでの定義を書いてみる。

FactoryBot.define do
  factory :user do
    sequence(:name) { |i| "username#{i}" }

    after(:create) do |user, evaluator|
      create_list(:post, 2, user: user)
    end
  end
end

after(:create)を使い、Userが作成された際に関連するPostも作成されるようにした。この定義を用いてUserが投稿したPostのうち、人気順で返すUser#posts_ordered_by_popularityのテストを書いてみる。

RSpec.describe User, type: :model do
  describe '#posts_ordered_by_popularity' do
    let!(:user) { create(:user) }
    let!(:post_popular) do
      post = user.posts[0]
      post.update(popularity: 5)
      post
    end
    let!(:post_not_popular) do
      post = user.posts[1]
      post.update(popularity: 1)
      post
    end

    it 'return posts ordered by popularity' do
      expect(user.posts_ordered_by_popularity).to eq [post_popular, post_not_popular]
    end
  end
end

理解しづらいテストコードになった。このテストはUserのレコードを作成した時に、関連するPostを2つ作成することに依存している。また、updateを利用してデータを変更しているため、最終的なレコードの状態を把握しづらくなっている。

これを避けるには、まず、デフォルトで一対多の関連レコードを作成することをやめるとよい。

FactoryBot.define do
  factory :user do
    sequence(:name) { |i| "username#{i}" }

    trait(:with_posts) do
      after(:create) do |user, evaluator|
        create_list(:post, 2, user: user)
      end
    end
  end
end

traitを利用し、デフォルトではPostを作成しないようにした。どんな値でも良いので関連先のPostがほしいときにはtraitを指定しUserを作成すれば良い。

関連先はテスト中で明示的に作成するようにする。

RSpec.describe User, type: :model do
  describe '#posts_ordered_by_popularity' do
    let!(:user) { create(:user) }
    let!(:post_popular) { create :post, user: user, popularity: 5 }
    let!(:post_not_popular) { create :post, user: user, popularity: 1 }

    it 'return posts ordered by popularity' do
      expect(user.posts_ordered_by_popularity).to eq [post_popular, post_not_popular]
    end
  end
end

これで、最初の例よりだいぶ見やすくなった。

日時を取り扱うテストを書く

注意: ただしく境界値分析を行いテストデータを生成すれば以下の手法は必要なくなる。以下の記述は補助的に使うのが良い。

日時を取り扱うテストを書く場合、絶対時間を使う必要がないケースであればなるべく現在日時からの相対時間を利用するのが良い。その方が実装の不具合に気づける可能性が増すからだ。

例として、先月に公開された投稿を取得するscopeと、そのテストを絶対時間を用いて記述する。

class Post < ApplicationRecord
  scope :last_month_published, -> { where(publish_at: (Time.zone.now - 31.days).all_month) }
end

require 'rails_helper'

RSpec.describe Post, type: :model do
  describe '.last_month_published' do
    let!(:april_1st) { create :post, publish_at: Time.zone.local(2017, 4, 1) }
    let!(:april_30th) { create :post, publish_at: Time.zone.local(2017, 4, 30) }

    before do
      create :post, publish_at: Time.zone.local(2017, 5, 1)
      create :post, publish_at: Time.zone.local(2017, 3, 31)
    end

    it 'return published posts in last month' do
      Timecop.travel(2017, 5, 6) do
        expect(Post.last_month_published).to contain_exactly(april_1st, april_30th)
      end
    end
  end
end

このテストは常に成功するが、実装にはバグが含まれている。

テストを相対日時に変更してみる。

require 'rails_helper'

RSpec.describe Post, type: :model do
  describe '.last_month_published' do
    let!(:now) { Time.zone.now }
    let!(:last_beginning_of_month) { create :post, publish_at: 1.month.ago(now).beginning_of_month }
    let!(:last_end_of_month) { create :post, publish_at: 1.month.ago(now).end_of_month  }

    before do
      create :post, publish_at: now
      create :post, publish_at: 2.months.ago(now)
    end

    it 'return published posts in last month' do
      expect(Post.last_month_published).to contain_exactly(last_beginning_of_month, last_end_of_month)
    end
  end
end

このテストは、例えば3月1日に実行すると失敗する。常にバグを検知できるわけではないが、CIを利用することで、ずっと不具合が残り続ける可能性を減らすことができるだろう。

日付を外部から注入する

timecopやtravel_toなどを使い現在時刻を変更しテストを実行するよりも、時刻を外部入力できるようにしたほうが堅牢なコードになる。詳細は次のブログエントリにまとまっている。

「現在時刻」を外部入力とする設計と、その実装のこと - クックパッド開発者ブログ

beforeとlet(let!)の使い分け

テストの前提となるオブジェクト(やレコード)を生成する場合、let(let!)やbeforeを使う。このとき、生成後に参照するものをlet(let!)で生成し、それ以外をbeforeで生成すると可読性が増す。

次のようなscopeを持つUserモデルがあるとする。

class User < ApplicationRecord
  scope :active, -> { where(deleted: false).where.not(confirmed_at: nil) }
end

このテストをlet!のみを用いて書くと次のようになる。

require 'rails_helper'

RSpec.describe User, type: :model do
  describe '.active' do
    let!(:active) { create :user, deleted: false, confirmed_at: Time.zone.now }
    let!(:deleted_but_confirmed) { create :user, deleted: true, confirmed_at: Time.zone.now }
    let!(:deleted_and_not_confirmed) { create :user, deleted: true, confirmed_at: nil }
    let!(:not_deleted_but_not_confirmed) { create :user, deleted: false, confirmed_at: nil }

    it 'return active users' do
      expect(User.active).to eq [active]
    end
  end
end

let!とbeforeを併用して書くと次のようになる。

require 'rails_helper'

RSpec.describe User, type: :model do
  describe '.active' do
    let!(:active) { create :user, deleted: false, confirmed_at: Time.zone.now }

    before do
      create :user, deleted: true, confirmed_at: Time.zone.now
      create :user, deleted: true, confirmed_at: nil
      create :user, deleted: false, confirmed_at: nil
    end

    it 'return active users' do
      expect(User.active).to eq [active]
    end
  end
end

後者のほうが「メソッドの戻り値となるオブジェクト」と「それ以外」を区別しやすく、見やすいコードとなる。

※let!(:deleted_but_confirmed)のように名前をつけることで、どんなレコードなのか理解しやすくなると感じる人もいるかもしれない。しかしレコードに名前付けが必要であれば、単純にコメントとして補足してやればよいだろう

letとlet!の使い分け

基本的にlet!を推奨する。letの特性である遅延評価が有効に働く次のようなケースではletを使っても良い。

let!(:user) { create :user, enabled: enabled }

context 'when user is enabled' do
  let(:enabled) { true }
  it { ... }
end

context 'when user is disabled' do
  let(:enabled) { false }
  it { ... }
end

letで定義した値は呼ばれない限り実行されないのでlet!よりコストが低い、ということを利点として挙げている人もいるが、これを鵜呑みにすると「letで定義した値がテストケースによって使われたり使われなかったりする」状態になってしまう。これはテストの前提条件を理解するコストを上げ可読性を落とす。

控えめなDRY

DRYにする行為は常に善だと思われているかもしれないが、そうではない。例えばコードの重複を一つにまとめるということは処理の抽象化をするということで、状況や抽象化の仕方によってはDRYによって減らされたコストを上回るコストが発生することもある。

shared_examplesはよく考えて使う

shared_examplesを利用するとコードの重複を削除できるが、書き方によってはかえって可読性を落とすことがある。

例として、引数として渡された曜日に対応した分だけポイントを増やすメソッドPoint#increase_by_day_of_the_weekのテストをshared_examplesを利用して書いてみる。shard_exampleの定義は別ファイルに書かれているとして、先にshared_examplesを利用する側だけのコードを見てみる。

RSpec.describe Point, type: :model do
  describe '#increase_by_day_of_the_week' do
    let(:point) { create :point, point: 0 }

    it_behaves_like 'point increasing by day of the week', 100 do
      let(:wday) { 0 }
    end

    it_behaves_like 'point increasing by day of the week', 50 do
      let(:wday) { 1 }
    end

    it_behaves_like 'point increasing by day of the week', 30 do
      let(:wday) { 2 }
    end

    # ...
  end
end

どんな前提条件で結果として何を期待しているのか、これだけを見て理解できるだろうか。

定義は次のようになる。

RSpec.shared_examples 'point increasing by day of the week' do |expected_point|
  it "increase by #{expected_point}" do
    expect(point.point).to eq 0
    point.increase_by_day_of_the_week(wday)
    expect(point.point).to eq expected_point
  end
end

このテストが読みづらいのは、shared_examplesを成立させるために必要な前提条件が多いことが一つ挙げられる。

• テストされる主体であるpoint

• メソッドに渡される引数であるwday

• 期待する結果であるexpected_point

また、それぞれの定義が分散していることが挙げられる

• 外側のlet(point)

• it_behaves_likeブロック内のlet(wday)

• it_behaves_likeの第二引数(expected_point)

可読性を上げるには、まず、適切に名前をつけることが考えられるだろう。

RSpec.shared_examples 'point increasing by day of the week' do |expected_point:|
  it "increase by #{expected_point}" do
    expect(point.point).to eq 0
    point.increase_by_day_of_the_week(wday)
    expect(point.point).to eq expected_point
  end
end

RSpec.describe Point, type: :model do
  describe '#increase_by_day_of_the_week' do
    let(:point) { create :point, point: 0 }

    context 'on sunday' do
      let(:wday) { 0 }
      it_behaves_like 'point increasing by day of the week', expected_point: 100
    end

    context 'on monday' do
      let(:wday) { 1 }
      it_behaves_like 'point increasing by day of the week', expected_point: 50
    end

    context 'on tuesday' do
      let(:wday) { 2 }
      it_behaves_like 'point increasing by day of the week', expected_point: 30
    end

    # ...
  end
end

新しくcontextを作り、wdayについての説明を加えた。そして、期待する結果であるexpected_pointをキーワード引数として利用することで、実引数側に名前がつき、数値が何を表すのかすぐに理解できるようになった。

しかし、そもそもこれはshared_examplesを利用すべきケースなのだろうか？shared_examplesを利用せずに書くと次のようになる。

RSpec.describe Point, type: :model do
  describe '#increase_by_day_of_the_week' do
    let(:point) { create :point, point: 0 }

    context 'on sunday' do
      let(:wday) { 0 }

      it "increase by 100" do
        expect(point.point).to eq 0
        point.increase_by_day_of_the_week(wday)
        expect(point.point).to eq 100
      end
    end

    context 'on monday' do
      let(:wday) { 1 }

      it "increase by 50" do
        expect(point.point).to eq 0
        point.increase_by_day_of_the_week(wday)
        expect(point.point).to eq 50
      end
    end

    context 'on tuesday' do
      let(:wday) { 2 }

      it "increase by 30" do
        expect(point.point).to eq 0
        point.increase_by_day_of_the_week(wday)
        expect(point.point).to eq 30
      end
    end

    # ...
  end
end

it_behaves_likeでの前提条件や引数が多くなればなるほど複雑度が増す。DRYにするメリットが複雑度を上回るかどうか、慎重に考えるべきだ。

スコープを考慮する

describe 外にテストデータを置かない

例えば、次のような spec があるとする

describe 'sample specs' do
  context 'a' do
    # ...
  end

  context 'b' do
    let!(:need_in_b_and_c) { ... }
    # ...
  end

  context 'c' do
    let!(:need_in_b_and_c) { ... }
    # ...
  end
end

この場合、b と c で同じ前提条件を利用しているので、一つ上のレベルに移動してDRYにしようと考える人もいるかもしれない。

describe 'sample specs' do
  let!(:need_in_b_and_c) { ... }

  context 'a' do
    # ...
  end

  context 'b' do
    # ...
  end

  context 'c' do
    # ...
  end
end

しかし、これは良くない。'a' のコンテキストに、必要のない前提条件が含まれてしまうからだ。この例だけだとピンとこないかもしれない。このような let! が 10、context が 30 あって、どの let! がどの context に対応する前提条件なのかわからない状況を想像すると、下手に前提条件をまとめる怖さがわかるだろうか。

もちろん、すべての context において、共通で使う前提条件であれば、まとめてしまうのは問題ない。

各ブロックにおける前提条件の配置ルール

• 各ブロックの前提条件は、その配下のすべての expectation で利用するもののみ書く

• 特定の expectation のみで利用するものにおいては、その expectation に書く

落穂拾い

• 各ブロックにおける前提条件の配置ルール、の例外は次のようなケース

• だが、基本的には各ブロック内で宣言するほうが望ましい

let!(:user) { create :user, enabled: enabled }

context 'when user is enabled' do
  let(:enabled) { true }
  it { ... }
end

context 'when user is disabled' do
  let(:enabled) { false }
  it { ... }
end

必要ないレコードを作らない

パフォーマンスの観点から、レコードを作らなくてすむ場合は作らないようにしたい。

describe 'posts#index' do
  context 'when visit /posts' do
    let!(:posts) { create_list :post, 100 }

    before { visit posts_path }

    it 'display all post titles' do
      posts.each do |post|
        expect(page).to have_content post.title
      end
    end
  end
end

「100件の投稿タイトルが表示できること」をテストしたい場合は別だが、ただ投稿タイトルを表示できているかチェックできればいい場合、明らかに無駄なレコードを作っている。

この場合の最小限のレコード数は1件である。

describe 'posts#index' do
  context 'when visit /posts' do
    let!(:post) { create :post }

    before { visit posts_path }

    it 'display post title' do
      expect(page).to have_content post.title
    end
  end
end

モデルのユニットテストでも、作らなくてよいレコードを作っているケースはよくある。

RSpec.describe User, type: :model do
  describe '#fullname' do
    let!(:user) { create :user, first_name: 'Shinichi', last_name: 'Maeshima' }

    it 'return full name' do
      expect(user.fullname).to eq 'Shinichi Maeshima'
    end
  end
end

User#fullnameはレコードが保存されているか否かに影響しないメソッドである。この場合はcreateではなくbuild(もしくはbuild_stubbed)を使う。

RSpec.describe User, type: :model do
  describe '#fullname' do
    let!(:user) { build :user, first_name: 'Shinichi', last_name: 'Maeshima' }

    it 'return full name' do
      expect(user.fullname).to eq 'Shinichi Maeshima'
    end
  end
end

このような単純なケースではUser.newを利用しても良い。

updateでデータを変更しない

FactoryBotで作成したレコード中のカラムをupdateメソッドで変更すると、最終的なレコードの状態がわかりにくくなるし、テストに依存している属性もわかりにくくなるので避ける。

RSpec.describe Post, type: :model do
  let!(:post) { create :post }

  describe '#published?' do
    subject { post.published? }

    context 'when the post has already published' do
      it { is_expected.to eq true }
    end

    context 'when the post has not published' do
      before { post.update(publish_at: nil) }

      it { is_expected.to eq false }
    end

    context 'when the post is closed' do
      before { post.update(status: :close) }

      it { is_expected.to eq false }
    end

    context 'when the title includes "[WIP]"' do
      before { post.update(title: '[WIP]hello world') }

      it { is_expected.to eq false }
    end
  end
end

Post#published?メソッドに依存している属性をすぐに理解することができるだろうか？updateはたいていFactoryBotのデフォルト値を「一番データとして多い形」に設定し、それを少し変更して使うために使われる。

updateは使用せず、FactoryBotのデフォルト値に記載したようにデフォルト値をランダムに保つと良い。

letを上書きしない

letで定義したパラメータを内側のcontextで上書きすると、updateでデータを変更しないで説明した例と同様に、最終的なレコードの状態がわかりにくくなるので避ける。

RSpec.describe Post, type: :model do
  let!(:post) { create :post, title: title, status: status, publish_at: publish_at }
  let(:title) { 'hello world' }
  let(:status) { :open }
  let(:publish_at) { Time.zone.now }

  describe '#published?' do
    subject { post.published? }

    context 'when the post has already published' do
      it { is_expected.to eq true }
    end

    context 'when the post has not published' do
      let(:publish_at) { nil }

      it { is_expected.to eq false }
    end

    context 'when the post is closed' do
      let(:status) { :close }

      it { is_expected.to eq false }
    end

    context 'when the title includes "[WIP]"' do
      let(:title) { '[WIP]hello world'}

      it { is_expected.to eq false }
    end
  end
end

subjectを使うときの注意事項

subjectはis_expectedやshouldを使い一行でexpectationを書く場合は便利だが、逆に可読性を損なう使われ方をされる場合がある。

describe 'ApiClient#save_record_from_api' do
  let!(:client) { ApiClient.new }
  subject { client.save_record_from_api(params) }

  #
  # ...多くのexpectationを省略している...
  #

  context 'when pass  { limit: 10 }' do
    let(:params) { { limit: 10} }

    it 'return ApiResponse object' do
      is_expected.to be_an_instance_of ApiResponse
    end

    it 'save 10 items' do
      expect { subject }.to change { Item.count }.by(10)
    end
  end
end

このようなとき、expect { subject }のsubjectは一体何を実行しているのかすぐには判断できず、ファイルのはるか上方にあるsubjectの定義を確認しなければならない。

そもそも"subject"は名詞であり、副作用を期待する箇所で定義すると混乱を招く。

is_expectedを利用し暗黙のsubjectを利用する箇所と直接subjectを明示する箇所が混在しており、どうしてもsubjectを使いたい場合は、subjectに名前をつけて使うと良い。

describe 'ApiClient#save_record_from_api' do
  let!(:client) { ApiClient.new }
  subject(:execute_api_with_params) { client.save_record_from_api(params) }

  context 'when pass  { limit: 10 }' do
    let(:params) { { limit: 10} }

    it 'return ApiResponse object' do
      is_expected.to be_an_instance_of ApiResponse
    end

    it 'save 10 items' do
      expect { execute_api_with_params }.to change { Item.count }.by(10)
    end
  end
end

expect { subject }の時よりはわかりやすくなったはずだ。

is_expectedを利用していない場合は、subjectの利用をやめてclient.save_record_from_api(params)を各expectationにべた書きするのが良い。

allow_any_instance_ofを避ける

公式のドキュメントにも書かれているが、allow_any_instance_of(expect_any_instance_of)が必要な時点でテスト対象の設計がおかしい可能性がある。

例として、次のようなStatement#issueのテストを書いてみる。

class Statement
  def issue(body)
    client = TwitterClient.new
    client.issue(body)
  end
end

RSpec.describe Statement do
  describe '#issue' do
    let!(:statement) { Statement.new }

    it 'call TwitterClient#issue' do
      expect_any_instance_of(TwitterClient).to receive(:issue).with('hello')
      statement.issue('hello')
    end
  end
end

expect_any_instance_ofを使ってしまったのは、StatementクラスとTwitterClientクラスが密結合しているのが原因である。結合を弱めてみる。

class Statement
  def initialize(client: TwitterClient.new)
    @client = client
  end

  def issue(body)
    client.issue(body)
  end

  private

  def client
    @client
  end
end

RSpec.describe Statement do
  describe '#issue' do
    let!(:client) { double('client') }
    let!(:statement) { Statement.new(client: client) }

    it 'call TwitterClient#issue' do
      expect(client).to receive(:issue).with('hello')
      statement.issue('hello')
    end
  end
end

issueメソッドを持つオブジェクトであれば、どのクラスでもclientとして扱えるように修正した。外部からclientを指定できる作りにしたことで、将来的にFacebookClientなど別のクライアントにも対応できるようになった。結合が弱まり、単純なモックオブジェクトでテストが記述できるようになった。