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

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

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.  
    Factorybot 改善ツール作成失敗と学び
    2023.02.18

    View Slide

  2.  
    2
    ● ruby 歴4年ぐらい
    ● 関西在住
    ● 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 は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

    View Slide

  18. 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

    View Slide

  19. 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

    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
    2つの課題にどうアプローチするか
    ● ① 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 を検知
    ● そのあと、「2回以上 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