← 他の記事もよんでみる
Available languages:
日本語

最近やってるバグ修正パッチの書き方

目次

自分はバグ修正が好きです。最近バグ修正をしていたら、いいね〜と言われることが何度かありました!せっかくなのでバグ修正のパッチ(PR, CL, みなさんはなんて呼んでいますか?)を出す時にさいきん気をつけていることをメモしてみようと思います👶

ちなみに直近で出したパッチとそのコメントはこんな感じです。
違うなと思った方はここでタブを閉じてください!!

https://phabricator.services.mozilla.com/D299935#10401116

https://phabricator.services.mozilla.com/D285380#9910214

前提: 複雑な問題をシンプルに解決することに価値を見出す

まだエンジニアではなかった頃に叩き込まれた考え方として、複雑な問題を受け取って簡単な形で返せというものがあります。

自分は技術が何も関係ないビジネスコンサルタントをやっていたことがあるのですが、受け取る課題はどれも抽象的かつ複雑な制約がつくものばかりでした。教わったことの中で印象的だったのは、最終的なアウトプットは簡単に見えれば見えるほど良いというものです。もちろん時と場合にはよりますが、特に開発や意思決定の現場ではいまもそう思っています。

コンサルの場合はアウトプットが何らかの資料になることが多々ありますが、要するにその資料に書いてあることが難解だったらクライアントはコンサルにお願いした意味がないのです(時と場合にはよります)。特に経営の現場ではスピードが命ですから、パッと読んでスッと入らないスライドはダメです(はい、もちろん時と場合によります)。スライド1枚を理解するために要する時間を5秒までに圧縮しろという人もいました。そういう背景もあって「1スライド1メッセージ」だなんて言うのですね。

個人的にはパッチも同じだと思っています。複雑な問題に対して、なるべく認知負荷の低い方法で(もちろんmaintainabilityなど色々な要素も考慮された上で)解決できることが望ましいと思います。

ただしここで現実が立ちはだかります。解決策そのもの、つまりコード変更そのものについてシンプルであることが望ましいのですが、そうはいかないことも多いわけです。しかし、そのような場合にもパッチに対するドキュメンテーションで改善を図れると考えています。この記事では、そのような前提のもと、バグ修正パッチのドキュメンテーションで気をつけていることを書いていってみます。

気をつけていること

バグ修正のパッチは読み解くのが難しいという意識を持つ

機能追加とは違ってバグ修正のパッチは局所的であることが多くなります。そのため、私個人の意見ですが、バグ修正のパッチはレビューが難しくなりがちです。

機能追加であれば、ある種の流れにそってレビューを行うことができます。例えば新しく追加されたインターフェースから順に追ってレビューするだとか、各レイヤーで要件を満たしているのかだとか。
一方でバグ修正は下手したら変更がワンラインだけだったりして、レビュアーは修正されたワンラインがどのような影響をどこまで与えるのか自分で思いを馳せることになります。その一行にたどり着くパスが一通りなら困りませんが、大体の場合はその一行がいろんなところから呼ばれています。大変ですね!

ということで、バグ修正のパッチを出す時には、そもそも相手に多大な負担をかけているという意識を持つようにしています。冗談のようですが、いつもよりも相手への思いやりをもってレビューに臨むようにしています。これがけっこう効いている気がします。

かならずテストを追加してから修正する

あたりまえ体操なのですが、まずはバグを再現するテスト、つまり今のコードでは失敗するテストを追加するようにしています。これはエンジニアになったばかりの頃に@smith_30_さんから教わった大事な教えです。私の理解では、これが少なくとも二つの意味で効いてきます。

  1. いまバグが存在する、あとでバグが修正された、この二点が確実に確認される
  2. 何を直したいのか、相手と認識を揃えられる

二つ目がかなりいいと思っています。バグは複雑なものになると、再現条件と受け入れ条件ですら複雑になりがちです。ちゃんとテストケースをミニマムにしてflakinessをなくしておけば(これが難しい場合もあるけど)、クリアな議論ができますよね👶

なお、逆に捉えて、バグ修正のパッチをレビューするときはテストから見るようにもしています。

パッチの説明はわかりやすい構成にする

パッチに付与する説明はいつもこんな構成にしています。

# まとめ

ここに原因を3行、解決策を1行で書く

# 原因

順をおって、コードを引用して書くようにしています。詳しくはあとで。

# 解決策

なぜそのソリューションを選んだのか書くようにしています。
選ばなかったソリューションにも言及します。これが大事だと思っています。

# FAQ

相手が持ちそうな疑問で、上におさまらなかったものについてはここで対応しておきます。
自分はiterationを極限まで減らしたい、かつ未来に情報を残したいタイプなので厚めに書きがちです。

いわゆるADRやdesign docと同じだと思っています。解決したい事柄をきちんと分析して、どんな解決策を選んだのかを書く。あとは、未来の自分も含め文脈を失った人たちが困らないように補足情報を記しておく。

バグ修正のパッチも将来誰が読むかわかりません。あとから理由を理解するためにはもちろん、コードベースのキャッチアップにちょうど良い情報として扱われたり、似たようなバグの調査に使われたりするかもしれません。ということで、時間が許す範囲で詳しく書けばいいんじゃないの〜?と思っています。

原因がパッとよんでわかるようにする

原因の説明がかなり大事だと思っています。

そのために下記のようなことをやっています。

実際にどんな感じかということで、https://phabricator.services.mozilla.com/D299935#10401116 を一部引用してみます。この直前でmCanTriggerSwipemViewPortIsOverscrolledthis->mOverflowDeltaX != 0.0といった条件がtrueになりますよ〜という説明をしています。

So, `WidgetWheelEvent::TriggersSwipe()` becomes true.

https://searchfox.org/firefox-main/rev/69a6da6b/widget/MouseEvents.h#803-806

```cpp
bool TriggersSwipe() const {
  return mCanTriggerSwipe && mViewPortIsOverscrolled &&
         this->mOverflowDeltaX != 0.0;
}
```

This means that DispatchWheelEvent() sends a response with aStartSwipe=true and the history swipe is started.

https://searchfox.org/firefox-main/rev/69a6da6b/dom/ipc/BrowserChild.cpp#1884-1915

```cpp
void BrowserChild::DispatchWheelEvent(const WidgetWheelEvent& aEvent,
                                      const ScrollableLayerGuid& aGuid,
                                      const uint64_t& aInputBlockId) {
  WidgetWheelEvent localEvent(aEvent);
  [...]

  DispatchWidgetEventViaAPZ(localEvent);

  if (localEvent.mCanTriggerSwipe) {
    SendRespondStartSwipeEvent(aInputBlockId, localEvent.TriggersSwipe());
  }

  [...]
}
```

解決策の根拠を書く。特にregressionを起こさない根拠と他を選ばなかった理由

なぜその解決策で問題が解決するのか書いています。これは原因がきっちりと書けていればそう苦労しないことが多いと思います。

どちらかというと、下記2点の説明の方が大変だったり必要だったりします。

必要ならテストを追加したり表を駆使したりすることが求められるかもしれません。がんばってやりましょう。

実際のパッチ

ちなみに直近で出したパッチとそのコメントはこんな感じです。

https://phabricator.services.mozilla.com/D299935#10401116

https://phabricator.services.mozilla.com/D285380#9910214

おわりに

さて、なんか色々書きましたが、個人的には開発組織のカルチャーに合わせて柔軟にやるのが一番いいと思っています。スタートアップの初期段階でこんな細かくやってたらやばいと思いますし、巨大コードベースでワンライン変更がリグレッションを起こしまくるような現場ならちゃんとこのくらいやったほうがいいと思いますし。

こんな感じです。みなさんも気をつけていることがあれば教えてください!ではでは〜👋

← 他の記事もよんでみる