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

Transcript

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

   View Slide

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

   View Slide

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

   View Slide

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

   View Slide

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

   View Slide

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

   View Slide

7. テストが遅い
   

   View Slide

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

   View Slide

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

   View Slide

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

    View Slide

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

    View Slide

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

    View Slide

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

    View Slide

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

    View Slide

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
    

    View Slide

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
    

    View Slide

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
    

    View Slide

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
    

    View Slide

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
    

    View Slide

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

    View Slide

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

    View Slide

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

    View Slide

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

    View Slide

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

    View Slide

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

    View Slide

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

    View Slide

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

    View Slide

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

    View Slide

29. 具体的にはどう書くか
    29
    

    View Slide

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

    View Slide

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
    

    View Slide

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,
    }
    

    View Slide

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
    

    View Slide

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
    

    View Slide

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

    View Slide

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

    View Slide

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

    View Slide

38. が……
    38
    

    View Slide

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

    View Slide

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

    View Slide

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

    View Slide

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

    View Slide

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

    View Slide

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

    View Slide

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

    View Slide

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

    View Slide

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

    View Slide

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

    View Slide

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

    View Slide

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

    View Slide