Upgrade to Pro — share decks privately, control downloads, hide ads and more …

Factorybot 改善ツール作成失敗と学び/ Factorybot improvement...

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

hachi (Hayao Kimura)

February 18, 2023
Tweet

More Decks by hachi (Hayao Kimura)

Other Decks in Programming

Transcript

  1.   2 • ruby 歴4年ぐらい • 関西在住 • freee の関西拠点で

    freee会計 を作っています • 福岡の思い出 ◦ 博多-大阪間をヒッチハイク ◦ 博多で6時間拾っていただけず ◦ 拾って頂いたご家族に博多うどんをおごっていた だいた ◦ 博多よいとこ hachi @hachiblog 木村駿生/ Kimura Hayao
  2. 9 テストが遅くなる原因 • そもそも対象の処理自体が遅い • テストデータ作成時に N+1 が発生している • DBを読み書きしなくてもいいテストで読み書きしている

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

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

    • 必要以上に何度も同じレコードを書き込んでいる • 上記のようなテストコードを書いてしまっても気づかない
  5. 14 今回の登場人物 ( gems ) • RSpec ◦ Ruby のテストフレームワーク

    • ActiveRecord ◦ Rails の Model 部分。Database にリクエストする • Factorybot ◦ テスト用データをかんたんに作れるgem。今回の主人公 • TestProf ◦ テストのパフォーマンス計測やパフォーマンス向上のための便利ツール gem
  6. 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
  7. 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
  8. 17 必要以上に何度も同じレコードを書き込んでいる • DB書込みが必要なテストでも、テストケースごとに書き 込み直す必要はない場合がある • 検索用エンドポイントなどはそういう場合が多い • 右の GET

    /samples のテストは create_list は1回でいい 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
  9. 18 必要以上に何度も同じレコードを書き込んでいる • DB書込みが必要なテストでも、テストケースごと に書き込み直す必要はない場合がある • 検索用エンドポイントなどはそういう場合が多い • 右の GET

    /samples のテストは create_list は1回 でいい 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
  10. 19 必要以上に何度も同じレコードを書き込んでいる • 2つの context の前に行う • before_all method (

    後述 ) を使うことで、 2つの context で使うレコードを1回で作 成している 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
  11. 20 before_all method とは • rails でテストを書く際、基本的には transactional tests という形式で行われる

    ◦ 各テストで作成されたレコードはテスト終了時にロールバックされ、消える • しかし、 describe や context で before(:all) で作成したレコードはロールバックされず消えない ◦ ゴミレコードが残り、後のテストに影響を与えてしまう ◦ after(:all) で明示的に消す必要があり、扱いが難しい • TestProf gem によって使えるようになる before_all method は before(:all) のように context 内で一度しか実行し ないが、後片付けもしてくれるというメソッド。便利
  12. 21 2つの課題にどうアプローチするか • ① DBを読み書きしなくてもいいテストで読み書きしている ◦ テスト中で作成されているレコードのうち、select, update, delete されていないレコードがある場合に検知す

    る仕組みを作る • ② 必要以上に何度も同じレコードを書き込んでいる ◦ 復数のテストケースで create されているレコードのうち、 update されていないレコードがあるとき検知する仕 組みを作る
  13. 23 DBを読み書きしなくてもいいテストで読み書きしている • テスト中で作成されているレコードのうち、select, update, delete されていないレコードがある場合に検知する仕 組みを作る ◦ つまり

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

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

    create 以降に db アクセスが発生していないレコードを検知するということ テストプロセス Database リクエスト無し 課題解消後 リクエストが 無く高速
  16. 26 必要以上に何度も同じレコードを書き込んでいる • 復数のテストケースで create されているレコードのうち、 update されていないレコードがあるとき検知する仕組みを作る ◦ update

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

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

    ◦ update されていた場合 create を共通化すると、テストAとテストBでレコードの値が変わってしまう Database create record テストプロセス 課題解消後 テストB 取得したレコードを使用するだけ
  19. 30 具体的にどう書くか • ① DBを読み書きしなくてもいいテストで読み書きしている ◦ テスト中で作成されているレコードのうち、select, update, delete されていないレコードがある場合に検知す

    る仕組みを作る • ② 必要以上に何度も同じレコードを書き込んでいる ◦ 復数のテストケースで create されているレコードのうち、 update されていないレコードがあるとき検知する仕 組みを作る
  20. 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
  21. 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, }
  22. 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
  23. 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
  24. 35 必要以上に何度も同じレコードを書き込んでいる • 復数のテストケースで create されているレコードのうち、 update されていないレコードがあるとき検知する • 先程と同じ仕組みを使い、

    create されている class, update されている class を検知 • そのあと、「2回以上 create されていて、 update されていないもの」があれば before_all を使うよう促す
  25. 39 作ってみた結果 • 一定役には立った ◦ 実際、このツールを使って 4min かかってたあるテストが 20sec になった

    ◦ 他にもいくつか改善はできた • 一方で、CIで回して使えるかと言われると、前者は偽陽性、後者は偽陰性が多すぎて厳しそうだった
  26. 41 どのような場合に偽陽性になるか • factory 内で trait に含まれているもの ◦ trait でまとめて作成しているが、使用していない場合

    • 検索のテストなどで、あるレコードが検索に当てはまらないことをテストしたいとき ◦ レコードの存在自体は必要だが参照はされない • 作成したレコードの after_save で作られたりするレコードにテストが依存しているとき ◦ build では callback が発火しないのでテストが落ちてしまう • 検索においてサブクエリで参照して検索対象を絞っているとき ◦ 絞り込みに利用しているだけで、レコードとしてはつかってないので引っかかってしまう
  27. 49 とはいえ実装してみることは大事で得られるものも多い • 多くのものを得た ◦ Factorybot, ActiveRecordの内部実装を知れた ◦ このアプローチが失敗であるということが身を持ってわかった ◦

    テストを少しでも早くできた ◦ この場に立てた!!! • 「戻れば一つ、進めば二つ」by スレッタ・マーキュリー - 水星の魔女