PullReqに動作確認を書こう
自分がどう動作確認しているかを書きましょう
コードだけでなく、動作確認方法もレビューしてもらいましょう
動作確認方法がレビューできれば、手元で動かさなくてもLGTMできる、かもしれない
こちらに詳しい話があるので読みましょう
例
https://gyazo.com/8abb545d73f6c9930f8d4fa39007b70f
rakusai.icon コメント
このPRでは、cssのファイルの配置を変えているだけなので、比較的簡単なPRである。
ここで、「project設定の見た目が崩れていない」という項目があることに注目。つまり、本人がここが変わる可能性があると認識して、確認したことを示している。（実際 sassのimport順が変わっているのでその恐れは若干ある）
また、レビューアーも、この点を再度確認すると共に、さらに漏れていないところがないか考える。
rakusai.icon 的には、scrapboxのshokaiの「動作確認」の項目が非常にレビューしやすくて毎回すごいと思っている
が、案外自分で書いてみると難しいということに気づいている。
Railsのほうのレビューをするとき、同じことを書いて欲しいと思うので、みんなマネして欲しい。
GyazoRailsのPullReq templateにHow to check itという項目があって、同じことを意図しているのですが、それとは別の話？akix.icon
もっと細かく書くべし、という話なのか、書いてない人がいるので書いてください、という話なのか
shokaiさんのブログには同意akix.icon
rakusaiさんがRailsの方のレビューで同じことを書いて欲しいと思う時があれば、その都度このページにリンクを貼りつつ指摘して行くと良さそうですねakix.icon
ホントに説明が書かれていなかったら書けよで済むんだけど、説明が不十分とかだと気づけないので、都度都度指摘してちゃんと書くような文化をつくるべきだと思うのでakix.icon
rakusai.icon How to check itとも大まかな趣旨は同じだと思います。
shokai.icon PRは非常に読みやすいし、ミスが少なくてよいのでお手本にしましょう。という話。
↑に例をあげてみました。
how to check itの書き方のコツと言い換えてもよいです。
言いたかったことshokai.icon
自分の所に来るOSSのpull requsetにレビュー方法とか一切書いて無くてmergeできない
もうコードが頭に入ってないので、その変更の影響範囲を想像できない
じゃあpull request送る側の頭にある影響範囲を教えてくれよというのがきっかけ
相談.icon 例えばさっき作ったこのPullReq、How to check itの欄で課金ページに影響があることが分かるけど、それとは別に影響範囲も明示した方が分かりやすいのかなakix.icon
adblockインストールしていない時は表示されない事を確認してますよね？それも書くといいと思うshokai.icon
すごく自明っぽいけど影響範囲を漏れなくカバーする具体的な確認フローを書いておくといい気がしてる
お、なるほどakix.icon
実装した部分だけ書きがちなので参考になったakix.icon
レビュアーと変更者が互いに、「あのへんまで影響してそうだけど◯◯氏ならまあチェックしてるだろ、たぶん」とか考え出すとつらい
もう一度書き直すと更に良いshokai.icon
pull requestのレビューというのは、結局ダブルチェックなのだから まずチェック項目に漏れが無い事を確認するべき
ある人が「この操作で全てを確認できてる」と判断した「判断そのもの」を、別の人が確認する
設定画面に項目追加したけど、これログイン画面にも影響しているよね、とか
（必要があれば）チェック項目を2人とも実行してみて、本当にokか確認する
実は同じ確認操作を二人でやっても、ほぼ意味がない
コンピュータでは、2人が同じ操作やったら基本的には同じ結果しか得られない
各人のローカルのデータセットが違う等、色々な前提条件が違う環境でtestしてみると見つかるbugはある。そういう場合だけやればいい
悪いレビュー依頼
確認項目を明かさずレビューを依頼すると
両者がランダムに確認するだけになる
両者が「相手なら自分の想定外の影響範囲もチェックしてくれてるだろ・・たぶん・・」という適当なLGTMを出す
LGTM（見た感じ良いんじゃね？）
どこを見たのかが重要
バグを探す時に、どこを見ていないかで推理できる