X で先日話題になってた内容で、PR のレビューに関するものがあった。
内容的には開発現場で CI と自動テストが機能しておらず、レビュアーが動作確認をしないといけないのはどうなんだろうという旨のものだった。
この話自体はまぁそういう事はある程度なんだけど、問題はそこに対して付けられている引用やリプで、大半がそれはレビュアーの仕事じゃないやら動作確認はテストフェーズの仕事で PR レビューでやる事じゃないやら、単体テストがないなんてありえないなど言われてた。
多分、レビュアーが自分だった場合に発生する不満をぶつけてるんだと思うんだけど、あるべき論になりすぎてて見てられなかった。
自動テストが導入されてないのは良くないのはわかる、テストをつけてないのはリジェクトしたいのもわかる (し、すれば良い) し、本来の範囲じゃないというのもわかる。
でも、テストフェーズでやるべきとか、テスト時にやれば良いからレビューではやらない方が良いというのは明らかにおかしい。
テストフェーズに回すべきではない理由
マネジメントの文脈になるけど、テストフェーズ (要するに後に回す) に任せてはいけない理由は明確にある。
機能開発や修正で発生した PR をその場で動作確認するケースと、テストに回したケースで比較して考えると明らかに良くない。
その場で動作確認するのは明確に大変で、PR のマージまでの時間が遅くなり、開発のアジリティが下がるのは間違いない。
コレは否定するところはなく事実。
ただ、だから後ろに回そうというのはおかしい。
開発速度を優先してテストを後回しにした場合、その実装が PR マージ時点で正しいものだったかを確定できない。
もちろん動作確認したからといって完全に大丈夫なんて言えないが、それは単体テストを付けたところで同じなので議論しない。
元の指摘では当人が確認しているのであればあとはテストに回すという話なんだけど、PR を出す本人は思い込みが存在する可能性があるので第三者レビューがないと仕様理解が不正確なままマージされる可能性が高くなる。またテストについても同様で、ランタイムで落ちるケースや単純なバグも他者の目を通さないと混入する可能性が高くなる。
そして、これは PR レビューする側がそういう目で見ないと何の効果も発揮しない確率が上がってしまう。
単純な話ではこういうことになり、PR 時の動作保証に労力を割かない決断をするとテストフェーズ時で発覚するバグの数が増える。
テストフェーズにやれ派の言い分でありそうなのは、先にやろうが後でやろうがバグやエラーの検知が後工程に回って開発がその分進むんだから問題ないだろという主張なんだと思う。
ここが大きく間違ってる点というのが自分の主張。
何が間違ってるのか？
まず、PR 時に発見できるはずのバグやエラーをテストまで放置された場合に何が起こるか。
バグやエラーを引き当てた人の作業が重くなる
本来よりも多く未発見バグやエラーの上に実装を重ねてしまう
テストフェーズ (後工程) まで発見が遅れる
マネジメントの観点ではそもそもどちらも発生させてはいけない類の問題。
1 は誰がボトルネックになっているのか、何が問題になっているのかを隠すためダメ。
2 はバグやエラーを前提としたコードを作ってしまうため、面倒なことになる可能性が高く、後から修正する際に場当たり的な修正を招きやすいためダメ。
3 は発見が遅れることによる改修コストが上がることや、発見が遅くなることでマネジメントレイヤーで解決できる手札が減るためダメとなる。
つまりマネジメントの観点が大きく、後肯定になるほどに訂正コストが大きくなるためなるべく早く発見して直そうというだけの話。
この辺 PMBOK にも書かれてるので読んでください。第 7 版には確実に書いてる。
最後に
PR のレビューを誰がどの責任でやるかなんてチームの状況によって変わるし、こんな面倒なことにやりたくねーよって自動化やレビューを楽にするテクニックが発展してるのでそれにうまく乗っかるべきなのは間違ってない。
ただ、この件で思った以上に教条的というか、あるべき論とかに取り憑かれてる人が多く、現実に発生した時にどうするべきかという話をしてない人が多くて心配になった。
レビューが大変なのは当たり前 (人の作ったものを確認して評価する事が楽なわけない) なので、大変さを理由に持ってくるのは間違いなく筋が悪いかな。
実際に動作させるのが毎回できないとか、レビュー専門とかになってくるとまた別ではあるけど、レビュー専門担当でやってる身としては仕様理解に努めてコードから危なそうな所を探すのは常にやってるので、口が裂けてもサボって良いなんて言えなかった。
レビューサボんなよ、後でしんどいぞ (戒め)。