FLAKYなテストってなーに？

この記事は食べログアドベントカレンダー2022 の10日目の記事です🎅🎄

こんにちは。食べログでサーバーサイドエンジニアをやっている四宮です。
普段はシステムの設計課題解決やパフォーマンスチューニングなど、システムそのものの改善活動を中心に行なっています。
今日は単体テストに関するお話をしたいと思います。

単体テストは大事。でも自動で繰り返し実行されることがもっと大事。

みなさん、単体テスト書いてますか？単体テストって作るの結構しんどいですよね。
でも単体テストがあったことで、リリース前に正しく障害を検知でき、事無きを得た修正もまたたくさんあるのではないでしょうか？

単体テストというものは、作っただけで役目を終えるということはほとんどありません。
JenkinsやCircleCIなどのCI環境で自動実行させて何度も何度も繰り返すことで、
その真価が発揮されるのが単体テストというものです。

食べログはRuby on Railsアプリケーションです。
そしてRSpecで単体テストが書かれており、開発ブランチごとにCIによるテストが行われています。
なので、単体テストによって品質を確保している側面もとても大きいです。

さて、今日はそんな単体テストによる品質担保の大きな敵である「FLAKY」なテストについてお話したいと思います。

単体テストが動いた、そのあとのお話

テスト書いてるぜ！自動で実行してるぜ！という方。素敵！
そういう方に質問です。みなさまのプロジェクトにこんな単体テスト、ありませんか？

CIでテストがときどき落ちるが、リランしたら解消する
単発実行では成功するんだけど、まとめて複数のテストを実行したら失敗することがある
手元では成功するんだけど、CI環境に持っていったら成功しないことがある

いやですよね。こういうの。再現しない不具合ほど恐ろしいものはありません。

こういうテストのことをFLAKYなテスト、とよびます。
ちなみに"flaky"という単語ですが、アメリカのスラングで「信用できない」「信頼できない」みたいな意味になるそうです。

CircleCIでは、Flakyなテストの検出結果をまとめてくれるテストインサイト機能があったりするので、知っている方もいらっしゃるかもしれません。
https://circleci.com/ja/blog/introducing-test-insights-with-flaky-test-detection/

FLAKYなテストで困ること

FLAKYなテストというのは、思った以上に開発者の心をすり減らすものです。

Pull Request に紐づくCIでRSpecがNGになったけど、全く思い当たる節がないところで落ちている。
何か追加pushしたら触ってないのに直った
→ 本当に壊していたのか、たまたま失敗しただけなのかがわからない
急いでリリースしたいのに、そんなときに限ってFLAKYなテストが失敗する、リランしたら直った
→ RSpecのリラン実行待ちという本来不要な時間を取られてしまう

などなど、これを放置してもロクなことがありません。とてもストレスフルで、安心して快適に開発できません。

以下ではFLAKYなテストはなぜ起こるのか、どうすれば防げるのか、を考えてみます。

FLAKYなテストが起こってしまう原因

よくあるケース(過去に実際にあったケース)を紹介していきます。皆さんの周りにも似たようなケースがあるはず。
あくまで、これだ！ということはなく、これらはFLAKYなテストを生み出していた一例に過ぎないことをご留意ください。

ケース1 : 現在時刻がテスト結果に影響するテストで、なおかつ時刻の設定が不適切

「今月のお店の具体的な定休日がちゃんとセットされているかチェックする」などの処理を考えてみます。
この場合、現在時刻(現在日時)がテスト結果に影響します。このため、何らかの手段で現在時刻をセットして対応しようとするのですが、これが環境/実行方法に依存するケースが該当します。

以下では、hoge.holidays というメソッドが正しく動くか、というのをRSpecで判定しています。

before(:each) do
  allow(Time).to receive_message_chain(:now).and_return(Time.new(2022,11,1,6,10,0, "+09:00"))
  # データを現在時刻に応じて生成
  OfficeHourCreator.create_initial_records!(hoge.id)
end

it "休日チェック"
  Timecop.travel(Time.new(2022, 12, 1, 06, 10, 0, "+09:00")) do # 便宜的な"今日"
    date_expected = %w|20221204 20221211 20221218 20221225| # 日曜日
    holidays = hoge.holidays(Date.today..Date.today.end_of_year) # 今日以降の年末までの休みの日
    expect(holidays).to eq(date_expected.map{|d| Date.parse(d)}) # あってるか確認
  end
end

一見問題なさそうです。ですが、ここには罠がありました。

この処理はitの中でTimecopを使って時間を制御していたのですが、itは複数並列で実行されるケースがあり、
他のテストに影響を与える or 与えられるなどで不安定なケースになってしまいました。

ここではbefore句でテスト前に時間を変えて、after句でテスト後に時間を戻す、などで
it句の内部では時刻を行き戻りしない＋他のテストに影響を与えない条件で判定すべきでした。

このようにTimecop.travelで時刻をいじったテストをするのは若干クセがあるので、
これを使わずにRSpecを書けるなら、その方が安定的なテストを作りやすい可能性が高いので、可能であればそちらをお勧めします。
ここではいっそTimecop.travelを使わない方式に変えることで安定しました。
こちらの方が現在時刻などを歪ませる必要もないですし、かえって処理がスッキリしますね。

before(:each) do
  allow(Time).to receive_message_chain(:now).and_return(Time.new(2022,11,1,6,10,0, "+09:00"))
  # データを現在時刻に応じて生成
  OfficeHourCreator.create_initial_records!(hoge.id)
end

it "休日チェック"
  start_date = Date.parse(Time.new(2022, 12, 1, 06, 10, 0, "+09:00")) # 便宜的な"今日"
  end_date = start_date.end_of_year
  date_expected = %w|20221204 20221211 20221218 20221225| # 日曜日
  holidays = hoge.holidays(start_date..end_date) # 今日以降の年末までの休みの日
  expect(holidays).to eq(date_expected.map{|d| Date.parse(d)}) # あってるか確認
end

これは hoge.holidays のメソッドが引数として開始日/終了日を受け取れる構造になっていることがポイントになっています。
これがテスト容易性(テストしやすさ : テスタビリティ)につながっています。
オブジェクトのテスト容易性、という観点が、FLAKYなテストの削減という側面でも重要だということがお分かりいただけますでしょうか？

教訓1 : 日時/時刻に依存する処理は、処理に必要な日時情報を外部から渡せるようにしよう。

ケース2 : 順番を厳密チェックすべきでない要素に対して、順番のチェックを過剰に行なっている

「特定の要素に含まれていること」をチェックすればいいだけのはずなのに、
期待結果を順番こみで定義していて、完全一致を求めているケースです。
要素の順番が不定な場合、順番が違う、としてエラーになることがあります。

it "openにhoge,piyoが, privateにfooが含まれていること" do
  # もろもろreviewsのDBをお膳立てしておいた上で検証, user_id = 123456 の口コミを持ってくる
  reviews = Review.where(user_id: 123456)
  expect(described_class.new.__send__(:status, reviews)).to eq [{open: "hoge"\n"piyo"}, {private: "foo"}]
end

元々の処理はこんな感じでした(処理は簡略化してます)

def status(reviews)
  result_subjects = {open: '', private: ''}
  not_deleted_status = [1,3,5,7,9] # 削除してない口コミ状態を表すreview_status(値は仮です)
  # 削除されてない口コミをスコア順に並べて処理
  reviews.where(review_status: not_deleted_status).order(:score).each do |review|
    status = case 
              when review.published?
                :open
              when review.private?
                :private
              end

    if review.subject.present? < TEXT_MAX_LENGTH
      result_subjects[status] << CR unless result_subjects[status].empty?
      result_subjects[status] << review.subject
    end
  end
  result_subjects
end

ループの実行順序はorder(:score)に従うためscoreの値に依存するのですが、テストデータの方を確認するとscoreの値を明示的に定義していませんでした。
つまり、デフォルトの値が設定されるため、同じ値となります。ということは、実行順序は不定となります。

不定、ということは、:private のstatusが先に実行されるか、:openのstatusが先に実行されるかは確定しないということです。
FLAKYなテストの香りがしますね。

これ、テスト的にはhoge,piyoの要素とfooの要素が含まれているかをテストすればよかったのですが、
結果を順番必須の配列で定義してしまったがゆえに、順番が変わった場合に失敗テストとなるようになっています。
順番をチェックする必要がないのであれば、そのようにテストを書き直すべきです。

it "openにhoge,piyoが, privateにfooが含まれていること" do
  # もろもろreviewsのDBをお膳立てしておいた上で検証, user_id = 123456 の口コミを持ってくる
  reviews = Review.where(user_id: 123456)
  hoge_result = described_class.new.__send__(:status, reviews)
  expect(hoge_result[:open].include?("hoge\npiyo"})).to true
  expect(hoge_result[:private].include?("foo")).to true
end

ところで、順序を気にしない、ということは集合(Set)として扱って判定する、でもよいですね。
集合は配列と違い順番は関係ありません。このため、順番が違っても許されます。
簡単なコードで確かめてみましょう。

# array:配列 とset:集合 の違い
test1_arr = ["hoge","piyo"]
test2_arr = ["piyo","hoge"]
test1_set = Set.new(["hoge","piyo"])
test2_set = Set.new(["piyo","hoge"])

# 配列では順番が一致する必要がある : test1_arr != test2_arr
irb(main):011:0> test1_arr == test2_arr
=> false

# 集合では順番が一致する必要はない : test1_set == test2_set
irb(main):012:0> test1_set == test2_set
=> true

ということで、このように書き換えが可能です。

it "openにhoge,piyoが, privateにfooが含まれていること" do
  # もろもろreviewsのDBをお膳立てしておいた上で検証, user_id = 123456 の口コミを持ってくる
  reviews = Review.where(user_id: 123456)
  hoge_result = Set.new(described_class.new.__send__(:status, reviews))
  expect(hoge_result).to eq Set.new([{open:"hoge\npiyo"},{private:"foo"}])
end

データ構造的にはこちらの方が正しく、要素が増えてもそのままいけそうです。
そもそもの戻り値として「配列」が適切なのか「集合」が適切なのかを意識することで、テストから設計/仕様の見直しにも繋げられそうですね。

教訓2 : データ構造を考慮した設計やテストにしよう。

その3 : 処理待ちするためにテスト内にsleepを入れて辻褄を合わせている

たとえば何か時間のかかる処理を待たないと正しく判定できないとかの理由で、単体テストにsleepを使いたくなるかもしれません。

it "処理が終わった後、ちゃんと値がセットされていること" do
  before_modified_at = something.modified_at.strftime(timestamp_format)
  # 前処理(テストでは内部処理はほとんどないので爆速で終わる)
  something.pre_method
  # 終了待ちをする
  sleep(1)
  # 本処理(テストでは内部処理はほとんどないので爆速で終わる)
  result = something.process
  # 結果を判定する(sleepで最低1秒ずれるからnot_to equal)
  expect(result.modified_at).not_to eq(before_modified_at)
  ...
end

しかしこれはいけません。
単純に処理時間が長くなってしまうだけでなく、処理系によってはsleepが効かなかったり、自動テストを並列化する時にこれが悪さをする、などの理由でFLAKYになってしまうことがあります。
sleepを入れて自動テストをなんとか切り抜ける、はNGです。

時間に関わるテストをやりたい場合は、beforeやafterでtravel_to、travel_backを使って明示的に時間を制御しましょう。
ケース1でも述べた通り、it句を並列実行しても良いようにしておく必要があるので中でタイムリープが発生しないようにしましょう。before~afterでちゃんと括ることをお忘れずに。

before do
  # 実行時は 2022/9/25 11:59:58 にタイムスリップ
  travel_to(Time.local(2022, 9, 25, 11, 59, 58))
end
after do
  # 時を戻そう
  travel_back
end
let(:before_updated_timestamp) { Time.local(2022, 9, 25, 10, 0, 0) }
let(:timestamp_format) { '%Y%m%d%H%M%S' }
let(:something) {FactoryBot.create(:something, modified_at: before_updated_timestamp)}

it "処理が終わった後、ちゃんと値がセットされてること" do
  # before_modified_at は 2022/9/25 10:00:00 の時刻を表す値になる
  before_modified_at = something.modified_at.strftime(timestamp_format)
  # 前処理(テストでは内部処理はほとんどないので爆速で終わる)
  something.pre_method
  # 本処理(テストでは内部処理はほとんどないので爆速で終わる)
  result = something.process
  # 結果を判定する(travel_to でmodified_atは2022/9/25 11:59:58 になるので、必ずnot_to equal)
  expect(result.modified_at).not_to eq(before_modified_at)
end

これで結果がsleepの挙動に依存しなくなるので、安心ですね。おまけにテスト実行速度も速くなるので一石二鳥です。時間が解決するわけではなく、何かの処理の終了を待たないといけない場合は、処理完了のステータスを確認し、処理を行いましょう。
いずれにせよ、単体テスト内部でのsleepはご法度だと心得ましょう。

教訓3 : RSpec内部でsleepはイケてない。時間を見たいならtravel_to、処理待ちするなら別の方法をあたろう

まとめ

いかがでしたか？
FLAKYなテストは、開発者のSAN値をゴリゴリ削りますし、せっかくのテスト資産が負債となってしまうきっかけでもあります。
ただ一方で、FLAKYなテストの解決に銀の弾丸はなく、地道な積み重ねが必要な地味な取り組みです。
素敵な開発体験のために欠かせない作業だと心得て、エンジニアみんなで脱FLAKYに取り組んで行きましょう！

最後までお読みいただきありがとうございました。

明日は @osunu-k の「食べログノートでスキーマドリブン開発をやってみた」です。お楽しみに！