バグから生まれたgem太郎

https://esminc.doorkeeper.jp/events/138417
で使用したスライドです

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

Transcript

1. バグから生まれたgemの話 失敗から学ぶ技術 失敗から学ぶ技術 @color_box
2. rspec-all_records_validatorのはなし
3. プロジェクトで出たバグの話 プロジェクトで出たバグの話
4. ある日突然の問い合わせ ある日突然の問い合わせ
5. 「なんか更新機能動かないよ？」 「なんか更新機能動かないよ？」
6. なぜか特定レコードがinvalidになっていた
7. モデル構造 モデル構造 class Binder < ApplicationRecord has_many :documents validates :documents, length: {minimum: 1} end class Document < ApplicationRecord belongs_to :binder end
8. Binderからdocumentsに対する関連が消失 has_many関連のlengthに対する 最小値のバリデーションによりinvalidに
9. バリデーションをすり抜けるはなし バリデーションをすり抜けるはなし
10. 発生原因 発生原因 結論から書くとバグ
11. 問題のバリデーション 問題のバリデーション 関連の個数に対するバリデーションは バグですり抜けうる validates :documents, length: {minimum: 1}
12. 再現コード 再現コード binderをそれが持つ書類ごとコピーしようとして 下記のようなコードを書いてしまう original_binder = binder.find(XXX) copied_documents = original_binder.documents.each do |document document.dup end Binder.create!(documents: copied_documents)
13. バグ バグ オリジナルのbinderのdocumentsをコピーする eachを使用したため コピー元のdocumentsが移動 map等を使用すべき箇所だった copied_documents = original_binder.documents.each do |document document.dup end
14. バリデーションがあるのに すり抜けてしまう
15. 同じフローを実行するE2Eテストは存在 検出できなかった
16. テスト内でoriginal_binderに対する バリデーションは実行されていれば検出できた
17. 新規に作製されたレコードについてのみ実行され 古いレコードはバリデーションされない
18. original_binderでバリデーションが 実行されないため すり抜けが発生 すり抜けが発生
19. 自動で検出できると嬉しい
20. こういうすり抜けをなんとかしたい こういうすり抜けをなんとかしたい テスト終了時にDB内のすべてのレコードに対して validationをかけることで検出できそう
21. テスト終了時に 全レコードの バリデーションチェック
22. Q. Q. 特定レコードが invalidになることを確認するテストはどうする？
23. A. 無視する A. 無視する
24. featureやsystemのような E2Eテストのみを対象とする バグでない限り ユーザー操作を模倣したテスト終了時に invalidなデータが発生しているのはおかしい
25. 初手、プロトタイプ、検証 初手、プロトタイプ、検証 ひとまずざっくりRSpecのafterフックに仕込む config.after do ApplicationRecord.subclasses.all? {|klass| klass.all.each {|obj| expect(obj).to be_valid } } end
26. 雑に仕事プロジェクトに導入 雑に仕事プロジェクトに導入 テスト用のコードなので本番環境に入らない 本番には影響がなく開発/テスト環境に閉じる 最悪取り外せば良い
27. 判明した問題 判明した問題 default_scopeを使用していると正常なユーザー操 作後でもinvalidになるレコードが 大量発生する 大量発生する CI実行時間が 1.5倍に 1.5倍に
28. default_scope default_scopeの問題 の問題 default_scopeを使用していると そのモデルと関連するレコードがinvalidになるのは 不可避 不可避
29. レコード総チェックの中で特定のモデルを 避けたい 避けたい
30. 実行時間の問題 実行時間の問題 全モデルをチェックする上で 不要なモデルまで処理してそう fixtureで投入するマスターデータなどは バリデーション不要のはず
31. レコード総チェックの中で特定のモデルを 避けたい 避けたい
32. モデルのフィルタ機能追加 モデルのフィルタ機能追加 default_scope絡みでinvalidになるレコードを除外 バリデーション不要なレコードを除外して高速化
33. リリースまで リリースまで 概ね機能はできたのでgemとしてリリースしたい テストは絶対必要
34. テストどうしよう テストどうしよう 書きづらい
35. feature specもしくはsystem spec終了時のみ 実行したいコードのテスト RSpecのafterでのみ実行される コードをRSpecで実行する？ expectが失敗することをテストする？ expect in expect問題
36. どうやってテストする？
37. 分割して対処 分割して対処 本質でない箇所は一旦無視 「全レコードに対してバリデーションを実行」 という部分のみテストする
38. レコードを用意して バリデーションをかけるための場が必要 擬似的なRailsプロジェクトが必要
39. 先人から学ぶ 先人から学ぶ テストを実行する場としてのRailsアプリが必要 DatabaseRewinder gemのテストを参考にする 小さいRailsアプリをテスト用に追加
40. expect in expect 問題
41. テスト対象コード中にexpectがあるのは明らかな 破綻 破綻 expectが失敗することを確認するテストは そもそも無理 そもそも無理
42. RSpecの中で「expectが失敗」したら その時点でテストは失敗する expectが失敗することによって 成功するテストは書けなさそう
43. gem内のコードとしてexpectではなく validation!を使用する 「例外が発生することを確認するテスト」は 「expectが失敗することを確認するテスト」 の100倍書きやすい
44. expect in expectを回避して対応
45. リリース リリース そんなこんなで機能もテストも用意できたので リリース $ rake release
46. 改良編 改良編 件のバグはhas_many関連を持つモデルでのみ発生 has_manyを持つモデルのみを対象として高速化 ActiveRecord::Reflection::ClassMethods を使用することで has_many関連を持つモデルを抽出できる
47. まとめ まとめ バグによってはバリデーションをすり抜けうる 2つのパターン 1. default_scope設定されたモデルの周辺モデル 2. has_many関連に対する個数のバリデーション
48. 2 のパターンのすり抜けをテストで防止するため rspec-allrecords_validatorを是非どうぞ https://github.com/colorbox/rspec- all_records_validator