Factorybot 改善ツール作成失敗と学び/ Factorybot improvement tool creation failure and learning

Transcript

1. 　 Factorybot 改善ツール作成失敗と学び 2023.02.18
2. 　 2 • ruby 歴４年ぐらい • 関西在住 • freee の関西拠点で freee会計 を作っています • 福岡の思い出 ◦ 博多-大阪間をヒッチハイク ◦ 博多で6時間拾っていただけず ◦ 拾って頂いたご家族に博多うどんをおごっていた だいた ◦ 博多よいとこ hachi @hachiblog 木村駿生/ Kimura Hayao
3. 今日の話は去年 freee 社の開発合宿での失敗談 3
4. 自分の失敗を超えてこれからを切り開いてほしい 4
5. 5 01　課題 02　アプローチ 03　作ってみた結果 04　学んだこと 目次
6. 6 01　課題 02　アプローチ 03　作ってみた結果 04　学んだこと 目次
7. テストが遅い
8. 8 前提 • 自分が普段開発している Rails アプリケーションは常に 100 人ほどのエンジニアが関わっている • 全員が速度を意識したテストを書いている保証はないし、何らかの理由で遅いテストを書いてしまうことはある • テスト自体はかなり書かれていて、カバレッジも高い • 当時、テストに 30min ほどかかっていた（今は10minほどに改善されている）
9. 9 テストが遅くなる原因 • そもそも対象の処理自体が遅い • テストデータ作成時に N+1 が発生している • DBを読み書きしなくてもいいテストで読み書きしている • 必要以上に何度も同じレコードを書き込んでいる • 上記のようなテストコードを書いてしまっても気づかない
10. 10 テストが遅くなる原因 • そもそも対象の処理自体が遅い • テストデータ作成時に N+1 が発生している • DBを読み書きしなくてもいいテストで読み書きしている • 必要以上に何度も同じレコードを書き込んでいる • 上記のようなテストコードを書いてしまっても気づかない
11. テストが遅くならない仕組みを作りたい
12. 12 01　課題 02　アプローチ 03　作ってみた結果 04　学んだこと 目次
13. 13 今回着目した原因 • そもそも対象の処理自体が遅い • テストデータ作成時に N+1 が発生している • DBを読み書きしなくてもいいテストで読み書きしている • 必要以上に何度も同じレコードを書き込んでいる • 上記のようなテストコードを書いてしまっても気づかない
14. 14 今回の登場人物 ( gems ) • RSpec ◦ Ruby のテストフレームワーク • ActiveRecord ◦ Rails の Model 部分。Database にリクエストする • Factorybot ◦ テスト用データをかんたんに作れるgem。今回の主人公 • TestProf ◦ テストのパフォーマンス計測やパフォーマンス向上のための便利ツール gem
15. 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. 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. 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. 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. 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. 20 before_all method とは • rails でテストを書く際、基本的には transactional tests という形式で行われる ◦ 各テストで作成されたレコードはテスト終了時にロールバックされ、消える • しかし、 describe や context で before(:all) で作成したレコードはロールバックされず消えない ◦ ゴミレコードが残り、後のテストに影響を与えてしまう ◦ after(:all) で明示的に消す必要があり、扱いが難しい • TestProf gem によって使えるようになる before_all method は before(:all) のように context 内で一度しか実行し ないが、後片付けもしてくれるというメソッド。便利
21. 21 ２つの課題にどうアプローチするか • ① DBを読み書きしなくてもいいテストで読み書きしている ◦ テスト中で作成されているレコードのうち、select, update, delete されていないレコードがある場合に検知す る仕組みを作る • ② 必要以上に何度も同じレコードを書き込んでいる ◦ 復数のテストケースで create されているレコードのうち、 update されていないレコードがあるとき検知する仕 組みを作る
22. それぞれのアプローチの詳細説明 22
23. 23 DBを読み書きしなくてもいいテストで読み書きしている • テスト中で作成されているレコードのうち、select, update, delete されていないレコードがある場合に検知する仕 組みを作る ◦ つまり create 以降に db アクセスが発生していないレコードを検知するということ Database create record その後のリクエスト無し 課題解消前 テストプロセス
24. 24 DBを読み書きしなくてもいいテストで読み書きしている • テスト中で作成されているレコードのうち、select, update, delete されていないレコードがある場合に検知する仕 組みを作る ◦ つまり create 以降に db アクセスが発生していないレコードを検知するということ Database create record 解消できない例 テストプロセス select record
25. 25 DBを読み書きしなくてもいいテストで読み書きしている • テスト中で作成されているレコードのうち、select, update, delete されていないレコードがある場合に検知する仕 組みを作る ◦ つまり create 以降に db アクセスが発生していないレコードを検知するということ テストプロセス Database リクエスト無し 課題解消後 リクエストが 無く高速
26. 26 必要以上に何度も同じレコードを書き込んでいる • 復数のテストケースで create されているレコードのうち、 update されていないレコードがあるとき検知する仕組みを作る ◦ update されていた場合 create を共通化すると、テストAとテストBでレコードの値が変わってしまう Database create record create record テストプロセス 課題解消前 テストA テストB
27. 27 必要以上に何度も同じレコードを書き込んでいる • 復数のテストケースで create されているレコードのうち、 update されていないレコードがあるとき検知する仕組みを作る ◦ update されていた場合 create を共通化すると、テストAとテストBでレコードの値が変わってしまう Database create record create record テストプロセス 解消できない例 update record テストA テストB
28. 28 テストA 取得したレコードを使用するだけ 必要以上に何度も同じレコードを書き込んでいる • 復数のテストケースで create されているレコードのうち、 update されていないレコードがあるとき検知する仕組みを作る ◦ update されていた場合 create を共通化すると、テストAとテストBでレコードの値が変わってしまう Database create record テストプロセス 課題解消後 テストB 取得したレコードを使用するだけ
29. 具体的にはどう書くか 29
30. 30 具体的にどう書くか • ① DBを読み書きしなくてもいいテストで読み書きしている ◦ テスト中で作成されているレコードのうち、select, update, delete されていないレコードがある場合に検知す る仕組みを作る • ② 必要以上に何度も同じレコードを書き込んでいる ◦ 復数のテストケースで create されているレコードのうち、 update されていないレコードがあるとき検知する仕 組みを作る
31. 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. 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. 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. 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. 35 必要以上に何度も同じレコードを書き込んでいる • 復数のテストケースで create されているレコードのうち、 update されていないレコードがあるとき検知する • 先程と同じ仕組みを使い、 create されている class, update されている class を検知 • そのあと、「２回以上 create されていて、 update されていないもの」があれば before_all を使うよう促す
36. 36 01　課題 02　アプローチ 03　作ってみた結果 04　学んだこと 目次
37. テストの改善ポイントが検知できた！🎉🎉 37
38. が…… 38
39. 39 作ってみた結果 • 一定役には立った ◦ 実際、このツールを使って 4min かかってたあるテストが 20sec になった ◦ 他にもいくつか改善はできた • 一方で、CIで回して使えるかと言われると、前者は偽陽性、後者は偽陰性が多すぎて厳しそうだった
40. どのような場合に偽陽性/偽陰性になるか 40
41. 41 どのような場合に偽陽性になるか • factory 内で trait に含まれているもの ◦ trait でまとめて作成しているが、使用していない場合 • 検索のテストなどで、あるレコードが検索に当てはまらないことをテストしたいとき ◦ レコードの存在自体は必要だが参照はされない • 作成したレコードの after_save で作られたりするレコードにテストが依存しているとき ◦ build では callback が発火しないのでテストが落ちてしまう • 検索においてサブクエリで参照して検索対象を絞っているとき ◦ 絞り込みに利用しているだけで、レコードとしてはつかってないので引っかかってしまう
42. 42 どのような場合に偽陰性になるか • テストケースによって生成するデータを変えている場合 ◦ id は違うため、同じレコードかどうかは他のカラムを見て判定する必要がある ◦ そのため少しでも違うデータを作成されると検知できない
43. なぜこの施策が失敗したか 43
44. 44 なぜこの施策が失敗したか • 本質的にレコード作成の意図はレコード作成の事実からは読み取れない ◦ after_save のテストをしたいのかもしれない ◦ SELECTで参照されずレコード存在のみが重要かもしれない • 事実のみを検知する仕組みは意図を読み取れないのでゲームセット
45. 45 01　課題 02　アプローチ 03　作ってみた結果 04　学んだこと 目次
46. 46 学んだこと • 素早い仮説検証が重要 • 使えない場合をあらかじめもっと考えたほうがよかった • とはいえ実装してみることは大事で得られるものも多い
47. 47 素早い仮説検証が重要 • 開発合宿の2日間で雑に実装するというのをやってみてよかった • 仮説をたててミニマムな実装をして検証したのでダメージが小さい
48. 48 使えない場合をあらかじめもっと考えたほうがよかった • 偽陽性が多くなってしまうのをもっと早く気付けなかったか • CI で回してよい施策かをもっと詰めておくべきだった
49. 49 とはいえ実装してみることは大事で得られるものも多い • 多くのものを得た ◦ Factorybot, ActiveRecordの内部実装を知れた ◦ このアプローチが失敗であるということが身を持ってわかった ◦ テストを少しでも早くできた ◦ この場に立てた！！！ • 「戻れば一つ、進めば二つ」by スレッタ・マーキュリー - 水星の魔女
50. ご清聴ありがとうございました 50