$30 off During Our Annual Pro Sale. View Details »

バグから生まれたgem太郎

 バグから生まれたgem太郎

color_box

June 30, 2022
Tweet

More Decks by color_box

Other Decks in Technology

Transcript

  1. バグから生まれたgemの話
    失敗から学ぶ技術
    失敗から学ぶ技術
    @color_box

    View Slide

  2. rspec-all_records_validatorのはなし

    View Slide

  3. プロジェクトで出たバグの話
    プロジェクトで出たバグの話

    View Slide

  4. ある日突然の問い合わせ
    ある日突然の問い合わせ

    View Slide

  5. 「なんか更新機能動かないよ?」
    「なんか更新機能動かないよ?」

    View Slide

  6. なぜか特定レコードがinvalidになっていた

    View Slide

  7. モデル構造
    モデル構造
    class Binder < ApplicationRecord

    has_many :documents

    validates :documents, length: {minimum: 1}

    end

    class Document < ApplicationRecord

    belongs_to :binder

    end

    View Slide

  8. Binderからdocumentsに対する関連が消失
    has_many関連のlengthに対する

    最小値のバリデーションによりinvalidに

    View Slide

  9. バリデーションをすり抜けるはなし
    バリデーションをすり抜けるはなし

    View Slide

  10. 発生原因
    発生原因
    結論から書くとバグ

    View Slide

  11. 問題のバリデーション
    問題のバリデーション
    関連の個数に対するバリデーションは
    バグですり抜けうる
    validates :documents, length: {minimum: 1}

    View Slide

  12. 再現コード
    再現コード
    binderをそれが持つ書類ごとコピーしようとして

    下記のようなコードを書いてしまう
    original_binder = binder.find(XXX)

    copied_documents = original_binder.documents.each do |document
    document.dup

    end

    Binder.create!(documents: copied_documents)

    View Slide

  13. バグ
    バグ
    オリジナルのbinderのdocumentsをコピーする

    eachを使用したため
    コピー元のdocumentsが移動
    map等を使用すべき箇所だった
    copied_documents = original_binder.documents.each do |document
    document.dup

    end

    View Slide

  14. バリデーションがあるのに
    すり抜けてしまう

    View Slide

  15. 同じフローを実行するE2Eテストは存在

    検出できなかった

    View Slide

  16. テスト内でoriginal_binderに対する

    バリデーションは実行されていれば検出できた

    View Slide

  17. 新規に作製されたレコードについてのみ実行され
    古いレコードはバリデーションされない

    View Slide

  18. original_binderでバリデーションが

    実行されないため
    すり抜けが発生
    すり抜けが発生

    View Slide

  19. 自動で検出できると嬉しい

    View Slide

  20. こういうすり抜けをなんとかしたい
    こういうすり抜けをなんとかしたい
    テスト終了時にDB内のすべてのレコードに対して
    validationをかけることで検出できそう

    View Slide

  21. テスト終了時に
    全レコードの
    バリデーションチェック

    View Slide

  22. Q.
    Q.
    特定レコードが
    invalidになることを確認するテストはどうする?

    View Slide

  23. A. 無視する
    A. 無視する

    View Slide

  24. featureやsystemのような

    E2Eテストのみを対象とする
    バグでない限り
    ユーザー操作を模倣したテスト終了時に
    invalidなデータが発生しているのはおかしい

    View Slide

  25. 初手、プロトタイプ、検証
    初手、プロトタイプ、検証
    ひとまずざっくりRSpecのafterフックに仕込む
    config.after do

    ApplicationRecord.subclasses.all? {|klass|

    klass.all.each {|obj|

    expect(obj).to be_valid

    }

    }

    end

    View Slide

  26. 雑に仕事プロジェクトに導入
    雑に仕事プロジェクトに導入
    テスト用のコードなので本番環境に入らない
    本番には影響がなく開発/テスト環境に閉じる
    最悪取り外せば良い

    View Slide

  27. 判明した問題
    判明した問題
    default_scopeを使用していると正常なユーザー操
    作後でもinvalidになるレコードが
    大量発生する
    大量発生する
    CI実行時間が
    1.5倍に
    1.5倍に

    View Slide

  28. default_scope
    default_scopeの問題
    の問題
    default_scopeを使用していると

    そのモデルと関連するレコードがinvalidになるのは
    不可避
    不可避

    View Slide

  29. レコード総チェックの中で特定のモデルを
    避けたい
    避けたい

    View Slide

  30. 実行時間の問題
    実行時間の問題
    全モデルをチェックする上で
    不要なモデルまで処理してそう
    fixtureで投入するマスターデータなどは

    バリデーション不要のはず

    View Slide

  31. レコード総チェックの中で特定のモデルを
    避けたい
    避けたい

    View Slide

  32. モデルのフィルタ機能追加
    モデルのフィルタ機能追加
    default_scope絡みでinvalidになるレコードを除外

    バリデーション不要なレコードを除外して高速化

    View Slide

  33. リリースまで
    リリースまで
    概ね機能はできたのでgemとしてリリースしたい

    テストは絶対必要

    View Slide

  34. テストどうしよう
    テストどうしよう
    書きづらい

    View Slide

  35. feature specもしくはsystem spec終了時のみ

    実行したいコードのテスト
    RSpecのafterでのみ実行される

    コードをRSpecで実行する?
    expectが失敗することをテストする?
    expect in expect問題

    View Slide

  36. どうやってテストする?

    View Slide

  37. 分割して対処
    分割して対処
    本質でない箇所は一旦無視
    「全レコードに対してバリデーションを実行」
    という部分のみテストする

    View Slide

  38. レコードを用意して
    バリデーションをかけるための場が必要
    擬似的なRailsプロジェクトが必要

    View Slide

  39. 先人から学ぶ
    先人から学ぶ
    テストを実行する場としてのRailsアプリが必要
    DatabaseRewinder gemのテストを参考にする
    小さいRailsアプリをテスト用に追加

    View Slide

  40. expect in expect 問題

    View Slide

  41. テスト対象コード中にexpectがあるのは明らかな
    破綻
    破綻
    expectが失敗することを確認するテストは
    そもそも無理
    そもそも無理

    View Slide

  42. RSpecの中で「expectが失敗」したら

    その時点でテストは失敗する
    expectが失敗することによって
    成功するテストは書けなさそう

    View Slide

  43. gem内のコードとしてexpectではなく

    validation!を使用する
    「例外が発生することを確認するテスト」は
    「expectが失敗することを確認するテスト」
    の100倍書きやすい

    View Slide

  44. expect in expectを回避して対応

    View Slide

  45. リリース
    リリース
    そんなこんなで機能もテストも用意できたので
    リリース
    $ rake release

    View Slide

  46. 改良編
    改良編
    件のバグはhas_many関連を持つモデルでのみ発生
    has_manyを持つモデルのみを対象として高速化
    ActiveRecord::Reflection::ClassMethods
    を使用することで
    has_many関連を持つモデルを抽出できる

    View Slide

  47. まとめ
    まとめ
    バグによってはバリデーションをすり抜けうる
    2つのパターン
    1. default_scope設定されたモデルの周辺モデル
    2. has_many関連に対する個数のバリデーション

    View Slide

  48. 2 のパターンのすり抜けをテストで防止するため

    rspec-allrecords_validatorを是非どうぞ

    https://github.com/colorbox/rspec-
    all_records_validator

    View Slide