QAメンバーである私は「コードレビュー」でなにを見ているか

2023年12月28日 14:23

私は品質管理部の部長をしつつ、開発チームの中で品管メンバーとして動いています。

その中で私が入っている「お会計チーム」でどういったことをおこなっているかを次のブログに書きました。
お会計チームの扱っているものの特性もあり、主にSETスキルを活用しながら案件をリリースまで進めています。

上記の記事に書いているのですが、私がおこなっていることの流れとして（大まかに書くと）次のようなことをおこなっています。

この中で「コードレビュー」としてどういったことをおこなっているのかについてあまり書いていないなと思ったので、もう少し詳しく書いてみようかと思います。

なお、今は運用コストなどを考えて、（一般的に言われる）E2Eテストは導入していません。
なので、後述する話で出てくるテストコードは全てそれを除くテストレベル/テストサイズの自動テストの話になります。

コードレビュー時のチェックポイント

私がコードレビューをおこなう上でやりたいことは上述した（２）になります。

守り方もいろいろとあり自動テストで事前に守れるようにするのもあれば、ログなどによってリリース後に検知しやすいようにするなどもあります。
自動テストで守れないのであれば手動テストで守る必要があります。

そういった総合的な観点をもってコードレビューを実施します。

とはいえ、現実としては時間との兼ね合いもあるためチェックする内容にも優先順位があります。

その中でも「どう守るか」というのは必須事項となるため最優先です。
守り方の中でも「自動テスト」でどこが守られているか「自動テスト」で守れない・守りづらいところはどこかを強く見ています。

これによって「手動テスト」によって守る必要がある箇所が分かるようになります。

見ているポイントとして次の３つに分類して説明しますが、実際のコードレビュー時にはこの３つを行き来したりしています。

どのようなインターフェースか
仕様から見て考慮漏れがないかどうか
処理の流れがどうなっているか、わかりやすいか
ロジックが複雑になってないかどうか
- 当然直しづらいケースもあるので、その場合は他のテストレベルを厚くし守るようにするといった判断もします
手動テストで動作確認しづらい箇所があるかどうか
- たとえば例外時の処理がUI/UX的にどうかというのを見たいケースもあります、その場合は専用のPRを用意してもらってそのPRで見るという選択もします

自身がどれだけ全体を把握しているかもありますが、PRのdiffだけでは処理の流れがわかりづらいケースもあります。

そのときはPRではなく変更後のコード全般を見るようにしています。

今、関わっているチームは基本的にコードドキュメントを書くようになっています（無意味に書きまくる必要はないです）。

その中で主に見ているポイントとしては次のとおりです。

プロダクトコードを見つつ、対応するテストコードを見ています。

（今だと）コードレビューの際に一番力を入れてみているのはここになります。
その中で見ている主にポイントとしては次のとおりです。

テストケースが足りているか、多くないか
- ただし、現時点でテストケースの追加が困難な箇所もあります。その場合は、他でどう守るかを考えます（他の箇所での自動テスト or 手動テストで担保など）
テストケース名が中身とあっているかどうか、分かりづらくないか
- よくある「期待どおり」といった文言みたいなものでなく「具体であるかどうか」等
テストコードが冗長でないか
- パラメタライズテストのほうできれいに出来るパターンであればその旨をFB
テストコードでの事前準備（setup/arrange）が適切かどうか
- テストデータに適切な値を使っているかどうか（flakyな原因にもなる）
テストダブルがどこで使われているか、無駄に使われていないか

なんでもかんんでもテストコードを書けばいいわけではないので、テストケースが足りているかについては「チームの状況」「プロダクトコードの状況」「対象の機能の内容」によって濃淡を決めています。

私が所属するお会計チームは当初テストコードを厚め厚めにしていました。
これは「メンバーのテストコードに対する習熟度」と「プロダクトコードの複雑さ」と「求められる堅牢性」などによって判断したものです

しかし、今では上記も改善されており以前に比べて「厚さ」を少し薄くできるようになっています。

なお、テストコードの書き方という意味では一緒のチームで働くメンバーが以前ブログ記事に「テストコード規約」としても書いてくれています。

私がどのようにコードレビューをしているかについては上述したとおりです。

ただ見ている範囲は上述した範囲だけとは限りませんし、上述したことを常に全ておこなっているわけでもありません。

次のようないろいろな要素があり、見るべきポイントの濃淡が出てきます。

これらを元に濃淡を決めながらコードレビューをしつつ「どこでどう守るか」を考えながら常に進めています。