【アンチパターン】Arrange、Act、Assert(AAA)を意識できていないRSpecのコード例とその対処法

  • 61
    Like
  • 9
    Comment

はじめに

先日、コードレビューをしていると、「むむ、これは!?」と思うようなRailsのテストコードに遭遇しました。
イメージ的にはこんなテストコード(RSpec)です。

# トレーニングジムの予約システムを開発していると仮定してください
describe 'キャンセル処理' do
  let(:user) { create :user }
  let(:reservation) {
    create :reservation,
      user: user,
      start_at: '2017-08-10 10:00'.in_time_zone
  }
  context '24時間前をすぎるとキャンセル料が発生する' do
    before do
      travel_to '2017-08-09 10:00'.in_time_zone
      reservation.cancel!
    end
    after { travel_back }
    let(:billing) { user.billings.first }

    it { expect(user.billings.count).to eq 1 }
    it { expect(billing.amount).to eq 500 }
  end
end

上のコードは僕から見ると、いろいろとよろしくないテストコードになっています。
みなさんはどこに問題があるか気づけるでしょうか?

この記事では上のテストコードの問題点を指摘し、望ましいテストコードに修正する方法を説明します。

なお、この記事を読むためにはRSpecの基礎知識が必要になります。
RSpecについて詳しくない方はこちらの入門記事を先に読んでおいてください。

テストコードのAAA(Arrange、Act、Assert)を理解する

テストコードの基本ステップはArrange、Act、Assert(準備、実行、検証、以下AAA)の3ステップに分かれます。

先ほどのテストコードであれば、それぞれ次のように分けられます。

  • Arrange(準備)= 予約オブジェクト(reservation)を作成し、システム日時を24時間前を過ぎた状態に変更する(travel_to ...
  • Act(実行)= 予約のキャンセルを実行する(reservation.cancel!
  • Assert(検証)= 適切な金額のキャンセル料が発生していることを確認する(expect...

RSpecのコードであれば、これら3つのステップが以下の場所に分散していることが望ましいです。

  • Arrange = let/let!、もしくはbeforeの中(場合によってはitの中でも可)
  • Act = itの中
  • Assert = itの中

この点を確認した上で先ほどのコードをもう一度確認してみましょう。

# トレーニングジムの予約システムを開発していると仮定してください
describe 'キャンセル処理' do
  # Arrange
  let(:user) { create :user }
  let(:reservation) {
    create :reservation,
      user: user,
      start_at: '2017-08-10 10:00'.in_time_zone
  }
  context '24時間前をすぎるとキャンセル料が発生する' do
    before do
      # Arrange
      travel_to '2017-08-09 10:00'.in_time_zone
      # Act
      reservation.cancel!
    end
    after { travel_back }
    # Arrangeではなく、Actの結果を取得している
    let(:billing) { user.billings.first }

    # Assert
    it { expect(user.billings.count).to eq 1 }
    it { expect(billing.amount).to eq 500 }
  end
end

ご覧のように、Actがbeforeの中に入っています。
また、let(:billing)はArrangeのためではなく、Actの結果を確認するために存在しています。

AAAの切り分けが適切でないと何をテストしているのかわかりづらい

先ほどのテストコードを見た瞬間、僕は「ん?このテストは何の振る舞い(どのメソッド)をテストしてるんだ??」と不思議に思いました。

これは、Actにあたる処理がbeforeの中に書かれ、itの中には書かれていなかったためです。

このようにAAAが不適切にテストコード内に散らばっていると、テストしたい振る舞いの「何をどうする(その結果、何がどうなる)」が不明確になり、コードの読み手を混乱させる原因になります。

AAAを適切に切り分けたテストコードに書き直す

というわけで、このテストコードのAAAを適切に切り分けましょう。
僕が書くなら、次のようなテストコードに書き直します。

describe 'キャンセル処理' do
  # Arrange
  let(:user) { create :user }
  let(:reservation) {
    create :reservation,
      user: user,
      start_at: '2017-08-10 10:00'.in_time_zone
  }
  context '24時間前をすぎた場合' do
    # Arrange
    before do
      travel_to '2017-08-09 10:00'.in_time_zone
    end
    after { travel_back }

    it 'キャンセルするとキャンセル料が発生する' do
      # Act
      reservation.cancel!

      # Assert
      expect(user.billings.count).to eq 1

      billing = user.billings.first
      expect(billing.amount).to eq 500
    end
  end
end

このように書くとitの中を見たときに、「ああ、キャンセルを実行したから支払いが発生したんだな」ということが明確にわかるはずです。

contextの中の文言にも注目!

また、contextで書いていた文言が変わっている点にも注目してください。

  • 修正前 = context '24時間前をすぎるとキャンセル料が発生する'
  • 修正後 = context '24時間前をすぎた場合' + it 'キャンセルするとキャンセル料が発生する'

修正前はcontextの中に条件と振る舞いが両方入っていました。

一方、修正後はcontextの中には条件が、itの中には期待する振る舞いが書かれています。

RSpecはBDD(Behavior Driven Development、振る舞い駆動開発)向けのテスティングフレームワークです。
つまり振る舞いを検証するが目的なので、itの引数には振る舞いに関する説明を記述し、ブロックの内部にはそれを検証するコードを書くのが望ましいです。

it '(振る舞いに関する説明)' do
  # 対象のメソッドが上の説明のとおりに振る舞うことを検証する
end

このようにcontextとitの文言をきちんと書くことで、AAAが正しく切り分けられているかどうかの確認ができます。

Tips:先に中身のないitを書くとAAAをきれいに切り出しやすい

AAAがちゃんと切り出せているかどうか自信がない人は、テストコードを書き始める前に大枠だけ固めてしまいましょう。
具体的には次のように「中身のないit」から書き始めると、AAAをうまく切り出しやすくなります。

describe 'キャンセル処理' do
  context '24時間前をすぎた場合' do
    it 'キャンセルするとキャンセル料が発生する'
  end
  context '24時間前よりも前の場合' do
    it 'キャンセルしてもキャンセル料が発生しない'
  end
end

RSpecは中身のないitを「未着手(pending)のテスト」して扱うので、この状態でテストを実行することもできます。

【Special thanks】
このテクニックを提案してくれた @mah_lab に感謝!

ソニックガーデンのメンバーもよくやってるようです。
(というか、僕もよくやります!)

Screen Shot 2017-09-14 at 10.38.22.png

beforeやletを乱用して「一石二鳥でDRYなコード」を目指すのはNG

ところで、最初に紹介した問題のコードがどうして生まれてきたのか、僕にはなんとなく予想がつきます。
おそらくこのコードを書いた人は「記述効率のよいテストコード」を目指していたんだと思います。

beforeやletなど、RSpecに用意された各種機能を駆使すると、重複を排除して効率よくテストコードを書くことができます。
そしてプログラマは「DRYなコードこそ善」と信じているので、「テストコードもDRYに書きたい」という心理が働きがちです。

しかし、プロダクションコードとテストコードはそもそもの目的が異なることを理解しましょう。

「DRYであることが常に善」であるプロダクションコードとは異なり、テストコードの場合は「対象の処理がちゃんと動いていることが一目でわかる、ドキュメントのようなコードこそが善」です。

「beforeやletを駆使した無駄にカッコいいコード」よりも、「愚直にベタ書きした読みやすいテストコード」を重視しましょう。

余談:subjectは無理に使わない方がよい

ちょっと話が脱線しますが、もしかするとみなさんの中には「subjectは?ねえ、subjectは使わないの??」と思っている人がいるかもしれません。

TwitterとかでRSpecに関するツイートを見ていたりすると、ときどき「subjectをどう書くか」で悩んでいる人を見かけます。
また、「RSpecのテストコードはsubjectをうまく切り出して、it { is_expected.to ... }の形式で書くのが一番スマートだ」と考えている人も結構多いように思います。

ですが、僕のポリシーは「subjectは原則使わない。subjectを使った方が明らかに効率がよく、可読性も落ちないときだけ例外的に使う」です。

ためしに先ほどのコードをsubjectを使うように直してみましょう。

describe 'キャンセル処理' do
  let(:user) { create :user }
  let(:reservation) {
    create :reservation,
      user: user,
      start_at: '2017-08-10 10:00'.in_time_zone
  }
  context '24時間前をすぎるとキャンセル料が発生する' do
    before do
      travel_to '2017-08-09 10:00'.in_time_zone
    end
    after { travel_back }
    # subjectを定義する
    subject { reservation.cancel!; user.billings }

    # subjectを使って検証する
    it { expect(subject.count).to eq 1 }
    it { expect(subject.first.amount).to eq 500 }
  end
end

うーん、やるとすればこんな感じでしょうか。。

もしかすると「いやいや、subject(:billings)って書いた方がいいよ!」とか「こんなふうにsubjectを切り出せば、itの中をもっとスマートに書けるよ!」みたいに思っている人もいるかもしれません。

ですが、そもそもsubjectをあらゆる場面で適用しようとすると、「どうやってsubjectをうまく切り出すか」ということに頭を使いすぎてしまいます。

時間をかけてでもうまく切り出せたならまだ良いですが、subjectとして扱いづらいオブジェクトをむりやりsubjectに切り出すと、いびつで読みにくいテストコードになります(実際、上のコードも可読性が悪いと思います)。

なので、僕個人の意見としては、「subjectはいったん窓から投げ捨ててitの中にベタ書きした方が、読みやすいテストコードを短時間で書けるよ」と考えています。

まとめ

というわけで、この記事では「Arrange、Act、Assert(AAA)を意識できていないRSpecのコード例」と、その対処法を説明してみました。

これまでAAAを意識してこなかった人は、自分のテストコードを見直してAAAが適切な場所に切り分けられているかチェックしてみましょう。

beforeやletを駆使しまくって「やった、DRYになった!」と喜んでいると、テストの可読性を落とす原因になりますよ!

あわせて読みたい

AAAの話は先日発売された書籍「Effective Testing with RSpec 3(洋書)」にも載っています。
他にも「テストコードはかくあるべし」という話が豊富に載っているので、この本を読めばRSpecの書き方とテストコードの基礎知識をしっかり学べます。

DRYでカッコいいテストコードは無理に書かなくていいよ、という話は過去にもいくつか書いています。
こちらもあわせてどうぞ。

可読性の高いRSpecのテストコードを書くためには、こちらのサイトも参考になると思います。

133contribution

自分ならこんなふうに書くと思います。

describe 'キャンセル料' do
  let(:user) { create :user }
  let(:reservation) do
    create :reservation,
      user: user,
      start_at: '2017-08-10 10:00'.in_time_zone
  end

  subject { user.billings.first }

  context '24時間前をすぎた場合' do
    before do
      travel_to '2017-08-09 10:00'.in_time_zone
    end
    after { travel_back }

    context 'キャンセルすると' do
      before { reservation.cancel! }

      it { is_expected.to be_present }
      its(:amount) { is_expected.to eq 500 }
    end
  end
end

@jnchitoさんのコードでは、最初にdescribe 'キャンセル処理'としていますが、ここに違和感があります。
describeはテスト対象を表すものであり、簡単に言えばモノだと思っています。
例)

  • #hoge(メソッド)
  • Foo(クラス)
  • foo.bar(属性値)
  • GET /api/companies (API(のレスポンス))
  • など

キャンセル処理はモノではないので、ここをキャンセル料に変更しました。
おそさく@jnchitoさんがsubjectをうまく切り出せなかったのはこれが原因ではないでしょうか?
describe 'キャンセル料'とすることで素直にsubject { user.billings.first }と切り出せます。

ちなみに自分はsubjectはコードを読む人に「これがテスト対象だよ」と示すのが大きな目的で、
無理にis_expectedを使う必要はないと思っています。

あとit 'キャンセルするとキャンセル料が発生する'ですが、キャンセルするとは振る舞いなのでこれもcontextに切り出しました。
自分はActはcontextだと思うんですよね。

「Effective Testing with RSpec 3」をまだ読んでいないので、読んだら考えが変わるかもしれませんが、
現状自分はこんな風に考えています。

493contribution
  • describeの下にsubject
  • 副作用はchangeで書く
  • できるだけitに説明テキストを渡さない

というポリシーが好みなのですがどうでしょうか?

describe 'キャンセル処理' do
  subject { reservation.cancel! }

  let(:user) { create :user }
  let(:reservation) {
    create :reservation,
      user: user,
      start_at: '2017-08-10 10:00'.in_time_zone
  }
  context 'は24時間前をすぎた場合' do
    before do
      travel_to '2017-08-09 10:00'.in_time_zone
    end
    after { travel_back }

    it { expect { subject }.to change { user.billings.count }.from(0).to(1) }
    it { expect { subject }.to change { user.billings.first&.amount }.from(nil).to(500) }
  end
end

ボッチを使わなければ行けないのがアレですが、failしたときの文章が説明的でかつ、テスト内容と同期するので、
テスト内容が間違っていたり、テスト内容を変更する必要がでた場合に、すぐに気付けるのが良いと思っています。

たぶん上のケースだと

キャンセル処理 は24時間前をすぎた場合 should change `user.billings.count` from 0 to 1
キャンセル処理 は24時間前をすぎた場合 should change `user.billings.first&.amount` from nil to 500

って出ると思います。

あともう一点なのですが、
itの中に手続き的な処理を書いて、複数回expectしているのが気になります。
単純な手続きだから問題ないってことでしょうか?
itの中に破壊的な処理があった場合に、正しくテスト仕切れない気がしていて、テストの独立性(って言葉があっているかわかりませんが)が保てているのか気になっています。
ただ、テスト実行時間とのトレードオフになるようにも思いますので、メリデメばなしではある?

31960contribution

@isuke さん、こんにちは。
なるほど、そのコードだとsubjectがうまくはまっているように見えますね。

ただ、ここに「キャンセルしたらbillingが1つだけ増えること」を担保したい、と考えたときはどうなるのでしょうか?
(billingが2つ増えるバグがあとから見つかった場合など)

「そのときは、これをこうしてああすればうまくsubjectを・・・」となるのかもしれませんが、subjectをうまく切り出すために四苦八苦するのはあまり時間の有効な使い方じゃないなあ、という気がしています。

あまり突き詰めると宗教論争みたくなってしまいますが:sweat:

ちなみに「Effective Testing with RSpec 3」では第10章にsubjectに関する説明が載っています。

31960contribution

@193 さん、こんにちは。
うーん、このコードは僕にいわせるとちょっと技巧的で何をテストしているのか、ぱっと見ではよくわからないですね。。
subjectは処理ではなく、値を返す「モノ(オブジェクト)」にした方が通常コードの可読性がよくなると思います。

itの中に手続き的な処理を書いて、複数回expectしているのが気になります。

これはあえてやっています。
もちろん、原則は「1つのitの中に1つのexpect」です。
ただ、最近ではそこまで無理に守る必要はないと考えるようになりました。
詳しい背景は「サヨナラBetter Specs!? 雑で気楽なRSpecのススメ」を参照してください。

もし、手前のexpectがコケて、次のexpectが走らないケースがどうしても気になるのであれば、aggregate_failuresを使うと良いと思います。

668contribution

自分なら1 expectでこうですかねー。travel_toはafter書き忘れちゃうことが多くてblock渡して使うようにしてます。

describe 'キャンセル処理' do
  # Arrange
  let(:user) { create :user }
  let!(:reservation) { create(:reservation, user: user, start_at: '2017-08-10 10:00'.in_time_zone }

  context '24時間前をすぎた場合' do
    around {|example| travel_to('2017-08-09 10:00'.in_time_zone) { example.run } }

    it 'キャンセルするとキャンセル料が発生する' do
      # Act and Assert
      expect { reservation.cancel! }.to change { user.billings.count }.by(1).and change { user.billings.sum(:amount) }.by(500)
    end
  end
end
31960contribution

@hanachin_ さん、こんにちは。

なるほど、changeを使ったわけですね。

実は僕もexpect { reservation.cancel! }.to change { user.billings.count }.by(1)のように書いた方がこの場合はテストの意図が明確になるなーと思ってはいました。
ただ、こうするとActとAssertが混ざって説明がしづらくなるので採用を見送りました。

一方、change { user.billings.sum(:amount) }.by(500)の方は、一瞬「なんでsumするの?」という気がしました。
「支払い額の合計が増えること」みたいな要求仕様になっていたならリーダブルなんですが。

とはいえ、1 expectの誘惑もわからなくはないですし、sumを使っても可読性がそこまで落ちてるわけではないので、ありっちゃありかな~と思います。

2558contribution

私もsubject使う派ですが、@jnchitoさんの元コードを流用して書くと

describe 'キャンセル処理' do
  # Arrange
  let(:user) { create :user }
  let(:reservation) {
    create :reservation,
      user: user,
      start_at: '2017-08-10 10:00'.in_time_zone
  }
  subject { reservation.cancel! }

  context '24時間前をすぎた場合' do
    # Arrange
    before do
      travel_to '2017-08-09 10:00'.in_time_zone
    end
    after { travel_back }

    it 'キャンセルするとキャンセル料が発生する' do
      subject  

      # Assert
      expect(user.billings.count).to eq 1

      billing = user.billings.first
      expect(billing.amount).to eq 500
    end
  end
end

こんな感じに、itの中でsubjectを呼び出して使う、というようにしています。subjectを使うことで、「このテストの主題はこれだな」というのが見つけやすいのかなと。

ちなみにcontextの中でsubjectを呼び出してしまうと、subjectをbefore { A }の後に実行できなくなるので、実行順序の調整ができないという事例もあったため、今はitの中で呼び出すようにしています。

describe 'キャンセル処理' do
  subject { reservation.cancel! }
  context 'ケース1' do
    before { subject } # この位置でsubjectを呼び出してしまうと実行順序を調整できなくなる。
    context 'ケース1-1' do
      before { A }
      it do
        ...
      end
    end
  end
end
668contribution

@jnchito お返事ありがとうございます!

一方、change { user.billings.sum(:amount) }.by(500)の方は、一瞬「なんでsumするの?」という気がしました。
「支払い額の合計が増えること」みたいな要求仕様になっていたならリーダブルなんですが。

確かにそこは分かりづらいですね。

やはり発生したキャンセル料のbillingについて個別で調べたほうがリーダブルそうです。

単に請求額がなかったのが500になることを確認するだけならこんな感じでもよさそうですが

expect { reservation.cancel! }.to change { user.billings.last&.amount }.to(500).from(nil)

発生したキャンセル料について調べたいことが増えれば増えるほどchangeでは確認しづらくなりそうなので2つに分けたほうがいいのかも

# 請求が1増えること
expect { reservation.cancel! }.to change { user.billings.count }.to(1)

# 発生したキャンセル料について色々詳しくexpectしていく
billing = user.billings.last
expect(billing.amount).to eq(500)
expect(billing.).to eq()

副作用が目的の(procedural)メソッドと、返り値が目的の(functional)メソッド。
返り値が目的のメソッドは、subjectがうまくハマる。

今回のcancel!はproceduralメソッド。

user.billings.firstをsubjectにするって発想は微妙に本題とずれてるし、
reservation.cancel!をsubjectと呼ぶのは英語的にもキツイと思うし、その副作用を及ぼすためにitのブロックの中で呼ぶってのも何だか。

proceduralメソッドは素直にsubjectを使わんのがよいのでは。