テストコードが読みづらかったのでRSpecをリファクタリングした話

こんにちは。プロダクトエンジニアのkuritaです。普段はSmartHRの届出書類機能の開発をしています。2023年の2月に入社したのですが、いつのまにか半年が経っていました。半年って体感短いですね。今回は、RSpecをリファクタリングした際の取り組みについてまとめました。テストコードの読みづらさに課題を感じ、まさに今リファクタリングに取り組んでいる方の参考になれば幸いです。

RSpecを読んでいくなかで感じた課題

チームに入って少し時間が経ったころにアプリケーションの仕様がとても難しいことに気づきました。また、テストコードが読みづらいことで時間が掛かってしまい、仕様の把握も大変になっていました。そこで、仕様をキャッチアップするだけでなく、テストコードをより良い状態にすることも目的として、リファクタリングを進めることにしました。

Example groupが長すぎる

私たちが開発している届出書類機能は、大きく分けて書類と従業員のデータを扱います。前者はアプリケーションのDBに保持していて、後者はGraphQLを利用してSmartHR 基本機能から取得します。そのため、テストの種類にかかわらず、書類と従業員のデータが必要な場合は、テストデータを準備するコードが肥大化してしまいます。確認（Assert）を行なっているコードにたどり着くころには、何をテストしているのか、そのためにはどんな前提が必要なのかといったことが脳内から揮発してしまうことが多かったです。何度かスクロールして確認する、エディタの画面を分割するといった方法で負担を軽減することはできますが、認知負荷が高い状態となっています。

letが多すぎる

基本的な方針として、letで値を定義することに問題はないと思うのですが、letを多用することで起きる問題もあると思います。前提となるデータとテスト対象の入力値がletを使用してどちらも並列で定義されている場合、テストコードを注意深く読み解かないと両者の違いをハッキリと認識することが難しくなってしまいます。

大胆にモックが使われている

外部サービスAPIを利用しているため、モックを利用している箇所がたくさん存在しています。また、アプリケーション内のモジュールを利用するときにも、モックを利用してテストを書いていることがあります。そのため、テストが壊れやすくなっている、リファクタリングをしづらくなっている状態です。

RSpecを読みやすくするために行なった取り組み

ここまで書いてきた課題は、いずれもRSpecの機能やテストコードの書き方を修正することで解決できそうでした。そのため、FactoryBotのtraitを使用したり、letの使い方を見直したり、モックの使い方を修正することで可読性の改善を行いました。

テストデータの作成コードをリファクタリングする

一般的に、AAAパターンの準備（Arrange）フェーズはテストを構成する中でもっともコードが大きくなると思います。私が所属するチームでもテストをするための前提となるテストデータの準備に多くの行数を割いている状態でした。ここでのつらみは、何をテストしているかではなく、テストするために必要なものを理解することにマインドシェアが奪われてしまうことでした。例えば、あるメソッドをテストする以下のようなテストコードはRSpecだと多く見られるのではないでしょうか。

describe "#hoge" do
  let(:foo) { create(:foo, bar:, ...) }
  let(:bar) { create(:bar, ...) }
  … # 以下50行くらいデータを作成する処理が続く
end

この問題を解決するために、describe内のテストデータを準備しているコードを改善する必要があります。チームではテストデータの作成にFactoryBotを使用していたので、FactoryBotの機能である trait を使用してリファクタリングを行ないました。

FactoryBot.define do
  ...
  factory :foo do
    trait :with_bar do
      bar
    end
  end
end

describe "#hoge" do
  let(:foo) { create(:foo, :with_bar) }
  …
end

しかし、このリファクタリングの方法にはデメリットもあります。それは、テストデータの準備をテストファイルから別ファイルに切り出したことで、テストファイルを見ただけではテストに必要なデータをどうやって作っているかがわからないことです。つまり、可読性とメソッドを実行する前提となるデータの作成方法の把握がトレードオフの状態になっています。そのため、常にtraitあるいは何かしらの方法を使ってテストデータの作成コードをテストファイルから追い出すことが良いとは限りません。チームでは作成したtraitが他のテストでも再利用可能なことが多く、traitを使ったほうが受けられる恩恵が大きかったため、traitを使った修正を行ないました。

FactoryBotを利用する際は、なるべく汎用性の高いfactoryを実装するように心がけることが大事です。特に、FactoryBotでモデルを作成する際、関連しているモデルを同時に作成する方法次第では汎用性が失われてしまいます。例えば、has_manyの関連を扱うデータを作成する際は注意が必要です。素直にfactoryのデフォルト値としてhas_many関連のデータを作成してしまうと、その歪みをテストコード内で吸収しなければならなくなり、テストコードが実装しづらい、理解しづらいといった問題が発生します。そのため、has_many関連のデータを作成する際はtraitを使用してデフォルト値の対象から外してあげるのがよいです。関連の数まで指定する必要がある場合は、transientという機能とhooksの機能を合わせて使うと良いです。例えば、以下のように実装すると、呼び出し側で作成する関連データの数を指定できます。

factory :request_with_crews do
  transient do
    crews_count { 1 } 
  end

  after(:create) do |request, evaluator|
    create_list(:request_crew, evaluator.crews_count, request:)
  end
end

呼び出し側では以下のようにcrews_countに任意の数を指定することで、任意の数分のデータを作成できます。

create(:request_with_crews, crews_count: 10)

上記のようにtraitやtransientを使って、全体的にデータの作成処理をFactoryBotに任せる方法で修正を行ないました。

context間の違いを意識してletを使用する

describeブロックの中にcontextを複数記述する場合、context間に共通のデータが出現することがあると思います。このような場合に、letを使ってデータをまとめることがRSpecでよく見られるテストの書き方だと思います。例えば、以下のようなコードです。

describe "#hoge" do
  subject { request.hoge(foo) }

  let(:foo) { "data" }

  context "A の場合" do
    it "..." do
      ...
    end
  end

  context "B の場合" do
    let(:foo) { "data2" }

    it "..." do
      ...      
    end
  end
end

let は遅延評価かつ後に書かれたものが使用されるため、context内で値を上書きしたい場合に使用するときに適したものと言えそうです。この性質により、context間、つまりテスト間の違いを目立たせることができます。違いを目立たせると、入力によって出力が変わるということをletから読み取れるようになります。

この考え方は指針とはなるものの、常に必ず守れるかというとそうでもありません。なぜなら、業務で扱っているアプリケーションは複雑な入力値を受け取ることも多く、入力値が複雑な場合はletの定義自体も複雑になる傾向があるからです。そういった場合、letからテスト間の違いを目立たせることが難しくなったり、テスト自体がわかりづらくなったりしてしまう可能性があります。データを作成するためのプライベートなメソッドをテストファイル内に作成したり、FactoryBotのようなフレームワークが備えている機能をうまく使ってあげたりすることでコード量を抑えることは可能です。しかし、先ほども言及したように、テストコードとテストデータを準備するコードの物理的距離が離れることによって逆にテストの可読性が落ちてしまうという問題も発生してしまいます。この問題に関する最適解は見つけられていないのですが、準備フェーズのコード量が多すぎて何をテストしているのかに関する理解の障害になるのであれば、準備フェーズのコードはテストコードから切り離したり、DRYにするという選択もアリだと思います。要はバランス...

モックの対象を絞る

ここからテストにおけるモックについても、お話をしたいと思います。テストにおけるモックって難しいですよね。個人的にはいつどういうケースでモックを使ったほうがよいのか悩むことがよくあります。モックは非常に便利なので、テスト対象のコードをすべてモックに置き換えてテストを書くことも可能です。一般的には、そのような手法でテストを書いていくスタイルをロンドン学派といい、なるべくモックを使わずにテストを書いていくスタイルを古典学派といいます。両者の考え方の違いを説明すると長くなってしまうので割愛しますが、簡単に言うとテストからなにを隔離するかが両者の本質的な違いです。どちらが良い悪いという話ではなく、実装するテストにおいていずれの考え方を採用したほうがテストの質が良くなるのかという観点で考えるのが良いと思います。問題になるのは、いつモックを使えば良いのかということです。モックを使うタイミングは、アプリケーションの境界を超えて行われる処理、かつその処理によって生じる副作用を外部から見ることができる場合です。例えば、メールサービスや外部サービスAPIなどを利用する場合が該当します。

RSpecでモックを使う場合、rspec/rspec-mocksというライブラリがテストダブルのフレームワークを提供してくれます。モックを使う際に注意したほうが良い点は、モックする対象を明確に絞ってあげることです。rspec-mocksには allow_any_instance_of , expect_any_instance_of という便利なメソッドが用意されています。これらを使用すると、引数に指定したクラスのインスタンスをなんでもモックすることが可能です。

allow_any_instance_of(Book).to receive(:title).and_return("The RSpec Book")
expect_any_instance_of(Book).to receive(:title).and_return("The RSpec Book")

これらは非常に便利なメソッドではあるのですが、テスト対象のコード内でスタブしたメソッドを呼び出している場合、常に一定の値が返ることになるため、使い方を間違えてしまうと思わぬ実装ミスに繋がってしまいます。rspec-mocksにはallow_any_instance_of , expect_any_instance_of の利用を推奨していないと記載されており、基本的には愚直にモックを使っていくことが好ましそうです。愚直に書いていく場合は以下のようになります。

テストでモックする例

describe ".fetch" do
  it "..." do
    result = { title: "The RSpec Book", … }
    allow(BookClient).to receive(:fetch).with(any_args).and_return(result)
    book = instance_double("Book", title: result[:title])
    allow(Book).to receive(:fetch).with(any_args).and_return(book)
    ...
  end
end

テスト対象のコード例

class Book
  …
  def self.fetch(id)
    result = BookClient.fetch(id)
    new(title: result[:title], …)
  end
  …
end

上記のコード例で見たように、モックを利用する場合、テストの対象となるアプリケーションコードの実装内容をテストコード内で再現しなければなりません。そのため、リファクタリングの方法次第ではテストが壊れてしまいます。また、モックと実装が密結合してしまうため、リファクタリングするたびに振る舞いが変わらないことを担保するためのテストを修正しなければならなくなります。これはツラいですね。アプリケーション内部のコードをモックしてテストを書くと、テストが壊れやすくなったり、リファクタリングが行ないにくくなったりする傾向にあると思います。一方で、外部サービスAPIのように仕様が定まっていて、後方互換性が保たれる可能性が高いものを対象にするのが良さそうです。

まとめ

以上、RSpecをリファクタリングした際の取り組みに関する紹介でした。テストコードを修正することによって劇的に効果を感じるということはないですが、可読性を向上させることによって長期的に効果が得られるのではないかと考えています。今後も品質と開発効率を向上させる価値の高いテストを実装し続けていきたいと思っています。