あんパン

こしあんこそ至高

Minami Aoyama Night #6で「よりよいレビュー環境を求めて」という発表をしました

先週金曜日に開催されたMinami Aoyama Night #6で「よりよいレビュー環境を求めて」という発表をしたので資料を公開します。以下、発表内容を文字に書き起こしたのでよろしければお読みください。

tl;dr

  • レビューはソフトウェアの質を担保する重要なファクターといえる
  • Pull Requestの作り方を工夫してレビュアーの負担を減らそう
  • 細かな改善を重ねてレビュアーが疲弊しないような環境作りをしていこう

Disclaimer

本稿は、はてなブログを開発しているチームが日々取り組んでいることを紹介するものです。まるっと他のプロダクトの開発に転用することは難しかったり、必ずしも効果を上げられるものではないでしょう。この内容がみなさんのチームのレビューにまつわる諸々を見つめ直すきっかけになれば幸いです。

なぜコードレビューをするのか

レビューはなんのためにするのでしょうか? 理由は様々ありそうですが、ソフトウェアの質を担保するため、という点を欠かす人はいないと思います。レビューの質が下がれば必然的にソフトウェアの質が下がります。これを回避するために私たちレビュイーができることは、レビュアーをもてなすことです。様々な手段を駆使してレビュアーの負担を軽減させることがソフトウェアの質につながるのです。

ひとつ注意したいのは、レビュアーが優位だとか、レビュイーが格下だとか、そういうことを言いたい訳ではないということです。レビューとはすなわち対話であり、ソフトウェアの質を良くするための手段でしかありません。ソフトウェアの質はチームで協力しあって保持するものです。そのための努力を惜しんではいけないというのが本稿の本質です。

はてなブログの開発フロー

本題に入る前に、はてなブログの開発フローについて紹介します。これから紹介する内容の前提です。

現在のはてなブログは、エンジニアが6名、デザイナが2名で開発を行っています。実際にはさらに細分化され、C向け(はてなブログ)チームとB向け(はてなブログMedia)チームにそれぞれ半数ずつ所属しています。はてなブログとはてなブログMediaは、Perlベースのモノリシックなアプリケーションで、チームに関わらず1つのリポジトリのみで開発を行っています*1

新機能を作る際は、masterブランチからトピックブランチを切って変更を加え、レビューを経てmasterブランチにmergeします。masterブランチにある程度変更がマージされたらreleaseブランチにマージしてリリースします。1日2回、14:30と17:30にレビュータイムがあり、基本的にはレビュー待ちのタスクをゼロにすることになっています。リリースは1日0回〜2回程度で、ほぼ毎日リリースを行っています。

レビュータイムでは、エンジニアによるエンジニアレビューとデザイナによるデザインレビューが行われます。はてなブログチームでは主にデザイナがHTMLやCSSを書くため、デザイナもPull Requestを作りますし、レビューも行います。

避けたいPull Requestのパターンと、それを回避するための戦略

さて、ここからが本題です。個人的に、またはチームとして、コードレビューの負荷軽減のために行っている取り組みをいくつかご紹介します。まずは避けたいPull Requestのパターンと、それを回避する戦略のの話をします。レビューはPull Request単位で行うものですから、この話を避けては通れないでしょう。今回紹介するパターンは以下の4つです。

  • 一発入魂パターン
  • 大会場パターン
  • 数珠繋ぎパターン
  • バザールパターン

時と場合によってはこれらのパターンを採らざるを得ないこともあります。そのため、バッドパターンではなく避けることが望ましいものとして提示しています。

なお、このパターン名は自分が勝手に考案したもので、特にチームで共通の語というわけではありません。

一発入魂パターン

一発入魂パターンは、新しい機能をすべて1つのトピックブランチで作りきるパターンです。

f:id:masawada:20180628175220p:plain
一発入魂パターン

ごく小さな変更であればさほど困ることはないかもしれませんが、変更の行数が肥大化するにつれて以下のような問題が発生します。

  • レビューするコード量が多く、レビュアーが疲弊する
    • 問題点を見落としやすくなり、不具合の温床となる
  • 問題点の数が多くなりがちで、レビュイーも疲弊する
  • レビューで問題点を指摘した後修正完了まで時間がかかった場合、再レビューで経緯を思い出すのに時間がかかる
  • masterにマージする際にコンフリクトしやすくなる

大会場パターン

大会場パターンは、新機能リリース用のブランチを作成して、そこから作業用のブランチを切って戻すPull Requestを作るパターンです。

f:id:masawada:20180629151423p:plain
大会場パターン

各Pull Request自体は細かくなっておりレビューがしやすく保たれているため一見良いように見えますが、以下のような問題があります。

  • 一発入魂パターンと同じく、新機能リリース用ブランチをmasterにマージする際にコンフリクトしやすくなる
  • 機能全体はレビューされているが、それをmasterにマージしたときに動くことの保証が薄い
    • 統合のレビューがないため、masterが大会場ブランチを作成した時点から乖離すればするほどこの傾向が顕著になる
  • レビューを受けないと先に進められないタスクでブロックされることがある

数珠繋ぎパターン

数珠繋ぎパターンは、大会場パターンの亜種です。レビュー待ち状態のトピックブランチから新たにブランチを切って、作業を先行するパターンです。

f:id:masawada:20180628175235p:plain
数珠繋ぎパターン

大規模な改修で見かけることはあまりありませんが、Pull Requestが2,3つ程度で完成する中小規模な改修で時々目にすることがあります。このパターンのメリットは、Pull Requestを細かく保ちつつもレビュー完了に先立って実装を進めることができるため大会場パターンで発生する作業のブロックを回避するという点です。

しかし、基本的には大会場パターンの亜種のため同様の問題があり、加えて以下のような問題も発生します。

  • 親のブランチで後にコミットが追加されると、その変更を子孫に反映して回る必要がある
    • 親ブランチのコミットが子孫のPull Requestに出現してログが見づらくことがある
  • マージの順序に気をつける必要がある
    • GitHubのWeb上でマージする際には特に気をつける必要がある
    • ひとつひとつさかのぼってマージするか、最後のPull Requestをmasterに向けてマージするか、どちらかをとる必要がある

バザールパターン

バザールパターンは、1つのブランチで複数人が作業するパターンです。

f:id:masawada:20180628175242p:plain
バザールパターン

通常、複数のエンジニアが1つのブランチで作業することは極稀ですが、エンジニアとデザイナが同じブランチで作業することがあります。この場合、以下のような問題が発生します。

  • レビュアーがどのコミットについてレビューをすべきかが不明瞭になる
  • エンジニアレビューとデザインレビューが同一のPull Requestで行われコメントが入り混じる

レビューしやすいPull Requestを作る戦略

ここまで4つの避けたいPull Requestのパターンを紹介しました。では、レビューがしやすいPull Requestとはどういうものでしょうか。僕は以下の2点を満たすものがそれだと考えています。

  • レビューすべきコードの量が少ない
  • レビュアーの負うべき責務がはっきりしている

レビューすべきコードの量が少ないというのは当然その通りでしょう。見るコードの量が少ないほど変更の影響範囲も狭くなるため考慮すべき事項もそれに応じて減少し、レビュアーの負担が軽減されます。

レビュアーの負うべき責務がはっきりしている状態とは、Pull Requestにその趣旨に沿ったコードのみが含まれている状態のことです。例えば、バザールパターンのPull Requestをレビューしようとすると、どのコードがレビューの対象でありどのコードがそうでないかを意識的に選びとる必要があります。この作業は意外とレビュアーの負担になりうるものです。前に挙げた例の他にも、直接Pull Requestとは関係ない便利ツールが追加されていたり、別のブランチからのcherry-pickが多かったりするのもレビュアーの負うべき責務がはっきりしていない状態といえます。

この2点を満たすためにはどのような戦略でPull Requestを組み立てていくと良いのでしょうか? これについて、僕は以下の点について留意すると良いと考えています。

  • 実装にあたって必要なタスクを事前に細かく分割しておく
  • Pull Requestは細かくmasterにマージする

実装にあたって必要なタスクを事前に細かく分割してまとめておく

まず重要なのは事前に必要なタスクを洗い出してまとめておくということです。大分当たり前なことかもしれませんが、慣れてくると中小規模の機能では意外とスキップしがちなことがあります。

これを行うことで、以下のような効果を得ることができます。

  • タスク間の関係を把握することができる
    • 並行して進められるもの、ブロッカーになりうるもの、などを把握できる
  • Pull Requestがプロジェクト中のどのフェーズのものかをレビュアーに視覚的に提示できる

さらに、ここで留意すべきは以下の2点です。

  • 分割したタスクをすぐさまIssueに落とし込まない
  • 作業時間がなるべく一定になるように分割する
  • 1つのタスクで作業する開発者はなるべく1人に留める

分割を終えたタスクは、実際に作業してみると共通化できる部分があったり、またはコード量が増えてしまうので一旦区切りをつけたくなったりと、更に分割したり結合したりをしたくなるタイミングがままあります。そのためすぐさまIssueに落とし込むのではなく、ひとつのまとめIssueを作ってタスクの進行度合を示すチェックリストを書いておくと効果的です。GitHubでProjectsを利用している場合はレーンにカードを追加することができるので、これを利用しても良いでしょう。こうすることでIssueを破棄する手間がなく自由にタスクを分割したり結合したりすることができます。issueを作ってしまうと心理的に分割や結合がしづらく、どうしてもPull Requestが肥大化することがあります。このような心理的障壁を取り除くのは意外と効果があるものです。

作業時間がなるべく一定になるよう心がけて分割するのも効果があります。特に、Pull Requestのレビューを行う時間がチームで固定されているときに効果的です。レビュータイムを終えてから新たなタスクに着手し、次のレビュータイムまでに終えられる程度の粒度に分割しておくことで、ブロッカーとなるタスクの影響度を最小限に抑えることができます。しかし、この点については見積もりの精度の高さが要求されますし、正確に見積もるためにはある程度の経験と慣れが必要になるため、あまり拘りすぎないことも大切です。

1つのタスクで作業する開発者はなるべく1人に留めるという点も留意して分割できると良いでしょう。無意識的に作業を進めると、UIの追加とアプリケーションとの結合、つまりデザインタスクとエンジニアリングタスクを1つのPull Requestで行ってしまうバザールパターンに陥りがちです。タスク分割時点でこの点にも留意すると、レビュアーの負担を軽減することができます。

Pull Requestは細かくmasterにマージする

前項は、Pull Reequestを細かくする、つまりレビューすべきコードの量をへらすための留意点でした。本項で紹介するPull Requestを細かくmasterにマージする手法は、レビュアーが負うべき責務をはっきりとさせるためのものです。

masterがトピックブランチを作成した時点から乖離すればするほど、トピックブランチをmasterにマージした際に正常に動く保証がなくなります。また、トピックブランチで作成した機能をmasterにマージする前に別のトピックブランチでも使いたい場合そのコミットをcherry-pickをする必要があり、それらのコードをレビューする責任の所在が不明瞭になります。これらの問題を解決するのにPull Requestを細かくmasterにマージする手法が有効です。基本戦略は簡単で、ユーザ影響の無い部分から作り、順次masterにマージしていくという手法をとります。

例えば、ブログサービスで記事にコメントをつけられる機能を作る場合、以下のようにタスク分割を行い順次masterにマージしていくことで機能を完成させることができそうです。

  • コメントを保存するためのテーブルを作成する
  • モデルクラスを作る
  • コメント作成/表示周辺の手続きを抽象化したメソッドを作成する
  • UIモックを作る
  • UIとロジックを結合する

Pull Requestを細かく分けてmasterにマージしていく戦略の詳しい解説についてはHatena Developer Blogにて解説されていますので、こちらも参考にしてみてください。

developer.hatenastaff.com

レビューを取り巻く環境を工夫する

ここからはレビューに関連するいくつかの改善の取り組みについてご紹介します。

レビュアーの偏りを可視化する

はてなブログチームでは毎日のレビュータイムにベストエフォートでPull Requestをレビューして回り、レビュー待ちのPull Requestをゼロにする活動を行なっています。しかし、あくまでベストエフォートであるため特定のエンジニアにレビュアーが偏ることがままありました。レビュアーに偏りがあるということはそのエンジニアが持っているタスクの進みがレビューで律速されるということであり、チームとしては不健全な状態といえます。そこで、これに気付いた同僚のid:aerealさんがレビュアーの偏りを可視化するツールを作成し、チームに導入しました。

github.com

以下のグラフは、実際に5月頃に計測した、直近100件のPull Requestのレビュアーを示すグラフです。

f:id:masawada:20180629145313p:plain:w400
5月のレビュアーの偏り具合

これを見たところ、50%以上のPull Requestが2人のエンジニアによって行われているということがわかりました。また、あまりレビュアー率の高くないエンジニアはC向けチームのエンジニアに寄っており、レビュータイムのリマインダがC向けチームの日例と近いことが原因という予想が立てられ、リマインダの発火時間をずらすことになりました。この後リマインダの時間変更を含めいくつかの施策を行った結果、実際に偏りをある程度均一化することができました。現在のグラフは以下のような状態になっています。

f:id:masawada:20180629145325p:plain:w400
6月のレビュアーの偏り具合

これでもまだ偏りが存在しているため、更に均一化を進めるのが今後の課題ということになりそうです。

もちろんこのグラフだけでエンジニアのチームへの貢献度を測ることはできません。Pull Requestの粒度が全く同一に揃っているわけではありませんし、レビュアーのコンテキスト理解の有無などによってもレビューにかかる時間や負担の具合などが変わってくるため、このグラフをエンジニアの評価に直接転用することはできないという点はチームでも共通認識として持っています。しかし、レビュアーの偏りの傾向などはこのグラフから十分に読み取れるので、それを均一化するための対策を立てるための情報源として非常に有益といえそうです。

アルバイトスタッフにもレビューをしてもらう

冒頭でレビューはソフトウェアの質を担保するために行うと述べましたが、その他にも以下のような効能があると考えています。

  • 触れたことがない領域の知見を深めることができる
    • 言語特有の知識
    • プロダクト特有の知識
  • 適度なPull Requestの粒度、分割の手法等を学ぶことができる

もともとアルバイトスタッフはレビューをしない運用でしたが、この考えをもとに最近はアルバイトスタッフにもレビューを行なってもらっています。ただしプロパーのエンジニアと同様にレビューするのではなく、以下のような制限を設けてこの範囲で自由にレビューしてもらうことにしています。

  • レビュータイムは1時間に制限する
  • アルバイトスタッフがapproveしてもmasterにマージはできない

時間制限については、単純にレビューに時間をとりすぎることを防ぐために設けています。当然プロパー社員よりも就業時間が短いためその多くをレビューに割いてしまうと本来お願いしたかった業務に滞りがでてしまうためです。

アルバイトスタッフがapproveしてもマージできないのは、コードをリリースする責任を負わせないための仕組みです。レビューを行うということは、そのコードをリリースする責任を実装者とともに負うということでもあります。責任をアルバイトスタッフが負う必要がないようにこの制限を設けています。

これは単に運用上のルールというだけでなく仕組み化もされています。Pull Requestをレビュー依頼にする際はプロパー社員だけが参加しているグループに対してReview Requestを行うフローになっています。そしてPull Requestはapproveされない限りmasterにマージできないようにプロテクトを掛けています。こうすることで、このグループに参加していないエンジニアがapproveしてもmasterへのマージはできないような仕組みを作ることができます。チームに新たなプロパー社員が参加したてのときも同様の方法で、approveは可能だがマージはブロックする、という運用を行うことができます。

これを実践してみて実際に効果が上がっているかについては、取り組みを始めてから1ヶ月程度ということもあり正直なところまだわかっていません。現在はこの取り組みと平行して、大きなタスクに着手する際は事前にタスク分割のレビューも行う、といった取り組みも行っています。そのため少なくともPull Requestの粒度については心がけてもらえている状態です。

まとめ

以上、レビュアーの負担を減らす取り組みについてご紹介しました。レビューはソフトウェアの質を担保する重要なファクターですが、意外とレビュアーの負担というものは軽視されがちなものです。細かな改善を重ねてこの負担を軽減することで、ソフトウェアの質の向上にも貢献できるのではないかと思います。これを機に本稿をお読みのみなさんもレビュアーが疲弊しないような環境づくりに取り組んでみてはいかがでしょうか。

*1:実際には、ScalaやGoで書かれたサブシステムもあります