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

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

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

Avatar for hachi (Hayao Kimura)

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 スレッタ・マーキュリー - 水星の魔女