Factorybot 改善ツール作成失敗と学び 2023.02.18

　 2 ● ruby 歴４年ぐらい ● 関西在住 ● freee の関西拠点で freee会計 を作っています ● 福岡の思い出 ○ 博多-大阪間をヒッチハイク ○ 博多で6時間拾っていただけず ○ 拾って頂いたご家族に博多うどんをおごっていた だいた ○ 博多よいとこ hachi @hachiblog 木村駿生/ Kimura Hayao

今日の話は去年 freee 社の開発合宿での失敗談 3

自分の失敗を超えてこれからを切り開いてほしい 4

5 01　課題 02　アプローチ 03　作ってみた結果 04　学んだこと 目次

6 01　課題 02　アプローチ 03　作ってみた結果 04　学んだこと 目次

テストが遅い

8 前提 ● 自分が普段開発している Rails アプリケーションは常に 100 人ほどのエンジニアが関わっている ● 全員が速度を意識したテストを書いている保証はないし、何らかの理由で遅いテストを書いてしまうことはある ● テスト自体はかなり書かれていて、カバレッジも高い ● 当時、テストに 30min ほどかかっていた（今は10minほどに改善されている）

9 テストが遅くなる原因 ● そもそも対象の処理自体が遅い ● テストデータ作成時に N+1 が発生している ● DBを読み書きしなくてもいいテストで読み書きしている ● 必要以上に何度も同じレコードを書き込んでいる ● 上記のようなテストコードを書いてしまっても気づかない

10 テストが遅くなる原因 ● そもそも対象の処理自体が遅い ● テストデータ作成時に N+1 が発生している ● DBを読み書きしなくてもいいテストで読み書きしている ● 必要以上に何度も同じレコードを書き込んでいる ● 上記のようなテストコードを書いてしまっても気づかない

テストが遅くならない仕組みを作りたい

12 01　課題 02　アプローチ 03　作ってみた結果 04　学んだこと 目次

13 今回着目した原因 ● そもそも対象の処理自体が遅い ● テストデータ作成時に N+1 が発生している ● DBを読み書きしなくてもいいテストで読み書きしている ● 必要以上に何度も同じレコードを書き込んでいる ● 上記のようなテストコードを書いてしまっても気づかない

14 今回の登場人物 ( gems ) ● RSpec ○ Ruby のテストフレームワーク ● ActiveRecord ○ Rails の Model 部分。Database にリクエストする ● Factorybot ○ テスト用データをかんたんに作れるgem。今回の主人公 ● TestProf ○ テストのパフォーマンス計測やパフォーマンス向上のための便利ツール gem

15 DBを読み書きしなくてもいいテストで読み書きしている ● テストデータの作成には Factorybot gem を用いている ● Factorybot では、create method を使うとDBに書き込みをし、build or build_stubbed method を使うとインスタン スの生成のみになる。 ● 例えば以下のようなテストでDBへの書込は必要ない Rspec.describe SampleClass do describe '#sample' do it 'return sample text' do expect( described_class.create( # ここ は build でいい hoge: 'hoge' ).sample ).to eq 'sample: hoge' end end end class Sample < ActiveRecord::Base def sample "sample: #{hoge}" end end

16 DBを読み書きしなくてもいいテストで読み書きしている ● テストデータの作成には Factorybot gem を用いている ● Factorybot では、create method を使うとDBに書き込みをし、build or build_stubbed method を使うとインスタン スの生成のみになる。 ● 例えば以下のようなテストでDBへの書込は必要ない class Sample < ActiveRecord::Base def sample "sample: #{hoge}" end end Rspec.describe SampleClass do describe '#sample' do it 'return sample text' do expect( described_class.build( # これで DBへの書込みがなくなった hoge: 'hoge' ).sample ).to eq 'sample: hoge' end end end

17 必要以上に何度も同じレコードを書き込んでいる ● DB書込みが必要なテストでも、テストケースごとに書き 込み直す必要はない場合がある ● 検索用エンドポイントなどはそういう場合が多い ● 右の GET /samples のテストは create_list は１回でいい Rspec.describe SampleController, type: :requests do describe 'GET /samples' do subject { get samples_path, params: {hoge: hoge, huga: huga} response } let(:hoge) { nil } let(:huga) { nil } context 'hoge による検索' do before do create_list(:sample, 3) end let(:hoge) { 'hoge' } it 'すべて hoge レコード' do expect(subject.all? { |sample| sample[:hoge] == 'hoge' }).to eq true end end context 'huga による検索' do before do create_list(:sample, 3) # 2回 create しているのが無駄 end let(:huga) { 'huga' } it 'すべて huga レコード' do expect(subject.all? { |sample| sample[:huga] == 'huga' }).to eq true end

18 必要以上に何度も同じレコードを書き込んでいる ● DB書込みが必要なテストでも、テストケースごと に書き込み直す必要はない場合がある ● 検索用エンドポイントなどはそういう場合が多い ● 右の GET /samples のテストは create_list は１回 でいい context 'hoge による検索' do before do create_list(:sample, 3) end let(:hoge) { 'hoge' } it 'すべて hoge レコード' do expect(subject.all? { |sample| sample[:hoge] == 'hoge' }).to eq true end end context 'huga による検索' do before do create_list(:sample, 3) # 2回 create しているのが無駄 end let(:huga) { 'huga' } it 'すべて huga レコード' do expect(subject.all? { |sample| sample[:huga] == 'huga' }).to eq true end end

19 必要以上に何度も同じレコードを書き込んでいる ● 2つの context の前に行う ● before_all method ( 後述 ) を使うことで、 ２つの context で使うレコードを１回で作 成している before_all do # ここでまとめて行う create_list(:sample, 3) end context 'hoge による検索' do let(:hoge) { 'hoge' } it 'すべて hoge レコード' do expect(subject.all? { |sample| sample[:hoge] == 'hoge' }).to eq true end end context 'huga による検索' do let(:huga) { 'huga' } it 'すべて huga レコード' do expect(subject.all? { |sample| sample[:huga] == 'huga' }).to eq true end end end

20 before_all method とは ● rails でテストを書く際、基本的には transactional tests という形式で行われる ○ 各テストで作成されたレコードはテスト終了時にロールバックされ、消える ● しかし、 describe や context で before(:all) で作成したレコードはロールバックされず消えない ○ ゴミレコードが残り、後のテストに影響を与えてしまう ○ after(:all) で明示的に消す必要があり、扱いが難しい ● TestProf gem によって使えるようになる before_all method は before(:all) のように context 内で一度しか実行し ないが、後片付けもしてくれるというメソッド。便利

21 ２つの課題にどうアプローチするか ● ① DBを読み書きしなくてもいいテストで読み書きしている ○ テスト中で作成されているレコードのうち、select, update, delete されていないレコードがある場合に検知す る仕組みを作る ● ② 必要以上に何度も同じレコードを書き込んでいる ○ 復数のテストケースで create されているレコードのうち、 update されていないレコードがあるとき検知する仕 組みを作る

それぞれのアプローチの詳細説明 22

23 DBを読み書きしなくてもいいテストで読み書きしている ● テスト中で作成されているレコードのうち、select, update, delete されていないレコードがある場合に検知する仕 組みを作る ○ つまり create 以降に db アクセスが発生していないレコードを検知するということ Database create record その後のリクエスト無し 課題解消前 テストプロセス

24 DBを読み書きしなくてもいいテストで読み書きしている ● テスト中で作成されているレコードのうち、select, update, delete されていないレコードがある場合に検知する仕 組みを作る ○ つまり create 以降に db アクセスが発生していないレコードを検知するということ Database create record 解消できない例 テストプロセス select record

25 DBを読み書きしなくてもいいテストで読み書きしている ● テスト中で作成されているレコードのうち、select, update, delete されていないレコードがある場合に検知する仕 組みを作る ○ つまり create 以降に db アクセスが発生していないレコードを検知するということ テストプロセス Database リクエスト無し 課題解消後 リクエストが 無く高速

26 必要以上に何度も同じレコードを書き込んでいる ● 復数のテストケースで create されているレコードのうち、 update されていないレコードがあるとき検知する仕組みを作る ○ update されていた場合 create を共通化すると、テストAとテストBでレコードの値が変わってしまう Database create record create record テストプロセス 課題解消前 テストA テストB

27 必要以上に何度も同じレコードを書き込んでいる ● 復数のテストケースで create されているレコードのうち、 update されていないレコードがあるとき検知する仕組みを作る ○ update されていた場合 create を共通化すると、テストAとテストBでレコードの値が変わってしまう Database create record create record テストプロセス 解消できない例 update record テストA テストB

28 テストA 取得したレコードを使用するだけ 必要以上に何度も同じレコードを書き込んでいる ● 復数のテストケースで create されているレコードのうち、 update されていないレコードがあるとき検知する仕組みを作る ○ update されていた場合 create を共通化すると、テストAとテストBでレコードの値が変わってしまう Database create record テストプロセス 課題解消後 テストB 取得したレコードを使用するだけ

具体的にはどう書くか 29

30 具体的にどう書くか ● ① DBを読み書きしなくてもいいテストで読み書きしている ○ テスト中で作成されているレコードのうち、select, update, delete されていないレコードがある場合に検知す る仕組みを作る ● ② 必要以上に何度も同じレコードを書き込んでいる ○ 復数のテストケースで create されているレコードのうち、 update されていないレコードがあるとき検知する仕 組みを作る

31 DBを読み書きしなくてもいいテストで読み書きしている ● create 以降に db アクセスが発生していないレコードを検知するため、まず create, updateを検知する ● ActiveRecord::Transaction.with_transaction_returning_status をオーバーライドする ○ destroy, create, update などのメソッドで使われている共通 method def destroy #:nodoc: with_transaction_returning_status { super } end def save(**) #:nodoc: with_transaction_returning_status { super } end def save!(**) #:nodoc: with_transaction_returning_status { super } end def touch(*, **) #:nodoc: with_transaction_returning_status { super } end ActiveRecord::Transaction より引用 def with_transaction_returning_status status = super BuildableDetector.insert_created_or_updated_list(self) status end

32 def insert_created_or_updated_list(instance) @created_or_updated_list.each do |location, hash| if hash[:class] == instance.class && instance.id.in?(hash[:ids]) # すでに hash にレコードがある 場合は update とみなす @created_or_updated_list[location][:updated] = true end end bt = caller # backtrace を見て、factory による create でない場合はスキップする return if bt.find {|str| str.include?('/app/') && str.exclude?('app/models/shard/common/base') } （省略） specs = bt.filter {|str| str.include?('/spec/') && str.exclude?('spec/rails_helper') && str.exclude?('/spec/support') } location = bt.reverse.find {|str| str.include?('/spec/') && str.exclude?('spec/factories/') && str.exclude?('spec/rails_helper') && str.exclude?('/spec/support') } return unless location if @created_or_updated_list[location] # backtrace のそれっぽいものを key にして保存 @created_or_updated_list[location][:ids] << instance.id else @created_or_updated_list[location] = { # create したことを hash に記録する class: instance.class, ids: [instance.id], updated: false, }

33 DBを読み書きしなくてもいいテストで読み書きしている ● select の検知 ● ActiveRecord::Relation.load, ActiveRecord::Querying.find_by_sql をオーバーライドする ○ select 時に呼ぶ共通メソッド def load(&block) unless loaded? @records = exec_queries(&block) ::BuildableDetector.insert_selected_list(klass, @records) @loaded = true end self end def insert_selected_list(klass, records) if @selected_list[klass] @selected_list[klass] += records.map(&:id) else @selected_list[klass] = records.map(&:id) end end

34 DBを読み書きしなくてもいいテストで読み書きしている ● 最終結果の出力 ○ dump_buildable メソッドを、テスト実行の最後に呼び出して検知した class とその位置を出力 def dump_buildable @created_or_updated_list.each do |key, hash| klass = hash[:class] next if hash[:updated] unless @selected_list[klass] || (@selected_list[klass] & hash[:ids]) p "#{klass} は create する必要がないかもしれません at: #{key}" end end end config.after(:suite) do BuildableDetector.dump_buildable end

35 必要以上に何度も同じレコードを書き込んでいる ● 復数のテストケースで create されているレコードのうち、 update されていないレコードがあるとき検知する ● 先程と同じ仕組みを使い、 create されている class, update されている class を検知 ● そのあと、「２回以上 create されていて、 update されていないもの」があれば before_all を使うよう促す

36 01　課題 02　アプローチ 03　作ってみた結果 04　学んだこと 目次

テストの改善ポイントが検知できた！🎉🎉 37

が…… 38

39 作ってみた結果 ● 一定役には立った ○ 実際、このツールを使って 4min かかってたあるテストが 20sec になった ○ 他にもいくつか改善はできた ● 一方で、CIで回して使えるかと言われると、前者は偽陽性、後者は偽陰性が多すぎて厳しそうだった

どのような場合に偽陽性/偽陰性になるか 40

41 どのような場合に偽陽性になるか ● factory 内で trait に含まれているもの ○ trait でまとめて作成しているが、使用していない場合 ● 検索のテストなどで、あるレコードが検索に当てはまらないことをテストしたいとき ○ レコードの存在自体は必要だが参照はされない ● 作成したレコードの after_save で作られたりするレコードにテストが依存しているとき ○ build では callback が発火しないのでテストが落ちてしまう ● 検索においてサブクエリで参照して検索対象を絞っているとき ○ 絞り込みに利用しているだけで、レコードとしてはつかってないので引っかかってしまう

42 どのような場合に偽陰性になるか ● テストケースによって生成するデータを変えている場合 ○ id は違うため、同じレコードかどうかは他のカラムを見て判定する必要がある ○ そのため少しでも違うデータを作成されると検知できない

なぜこの施策が失敗したか 43

44 なぜこの施策が失敗したか ● 本質的にレコード作成の意図はレコード作成の事実からは読み取れない ○ after_save のテストをしたいのかもしれない ○ SELECTで参照されずレコード存在のみが重要かもしれない ● 事実のみを検知する仕組みは意図を読み取れないのでゲームセット

45 01　課題 02　アプローチ 03　作ってみた結果 04　学んだこと 目次

46 学んだこと ● 素早い仮説検証が重要 ● 使えない場合をあらかじめもっと考えたほうがよかった ● とはいえ実装してみることは大事で得られるものも多い

47 素早い仮説検証が重要 ● 開発合宿の2日間で雑に実装するというのをやってみてよかった ● 仮説をたててミニマムな実装をして検証したのでダメージが小さい

48 使えない場合をあらかじめもっと考えたほうがよかった ● 偽陽性が多くなってしまうのをもっと早く気付けなかったか ● CI で回してよい施策かをもっと詰めておくべきだった

49 とはいえ実装してみることは大事で得られるものも多い ● 多くのものを得た ○ Factorybot, ActiveRecordの内部実装を知れた ○ このアプローチが失敗であるということが身を持ってわかった ○ テストを少しでも早くできた ○ この場に立てた！！！ ● 「戻れば一つ、進めば二つ」by スレッタ・マーキュリー - 水星の魔女

ご清聴ありがとうございました 50