ActiveDecoratorにプルリクを投げるまでにやったこと

怪しい挙動を見つけたきっかけ

テストコードで以下のようなことをしてました。

Rails.application.eager_load!

testでもproductionと同じ環境になるように事前に読み込んでいたのですが、テストを個別に実行するときにも全ファイルを読み込んでしまうため消したいと思っていました。

試しに消してCIを回してみると、一部のテストがfailした。

どんな問題が起きたか?

再現するコード例は以下の通り。

form = Form::User.new
ActiveDecorator::Decorator.instance.decorate(form)
form.is_a?(UserDecorator)  #=> true

Form::UserDecorator を定義してないときに、なぜか UserDecorator が使われてしまう。

コードを読んで発生箇所を突き止める

ActiveDecorator のコード内で Object.const_get を使っている箇所が UserDecorator を返していた。

そして、モジュールの自動読み込みに関する処理は ActiveSupport に定義されている。

ここまで調べて、自動読み込み周りはどう直すべきかよく分からなかった。

OSSパッチ会で松田さんに聞く

ちょうど問題を見つけた2日後にパッチ会があったので、直接聞いてみることにした。

blog.agile.esm.co.jp

a_matsuda さんに聞いてみたところ、以下のことが分かった。

  • 最近、そのあたりのコードを変えた
    • v1.2.0、v1.3.0(最新)で挙動を確認してみて欲しい
  • 意図した挙動ではない(バグっぽい)

動作確認してみると、 v1.2.0 では問題が起きず v1.3.0 で問題が起きること が判明した!

再現するテストケースを書く

v1.2.0 では問題ない事が分かったら、すぐ amatsuda/active_decorator@73dcdd6 が原因だと分かった。

問題を再現させるテストケースを書き、意図通りに失敗させて、 73dcdd6 をリバートしてテストが通ることを確認した。

英語のコミットログを書く

自分が最初に書いた英語のコミットログがこれ。

Fix a wrong decoration for nested classes

There is a problem that unintended decorators are used for nested classes.
This commit reverts 73dcdd67ae146ee9e76b1da9ae9e9dbd55529193 to fix the problem.

英語が苦手なので、OSSパッチ会に参加していた @yahonda さんにレビューしてもらいました。フィードバックは以下の通り。

  • wrongunintended が何か分からない
    • actual と expected を書いた方が良い
  • 細かいけど a wrong decoration の a は要らない
  • unintended より unexpected の方が良い

レビュー頂いて気づいたけど、英語の問題の前にコミットログの内容が足りていなかった...。

直したコミットログ

どういう問題が起きていて、何を期待しているかが分かるようにコミットログを直してプルリクを作成した。

Fix wrong decoration for nested classes

ActiveDecorator decorates incorrectly for the nested class that do not define decorator.

comic = Foo::Comic.new
ActiveDecorator::Decorator.instance.decorate(comic)
comic.is_a?(ComicDecorator)  #=> true

If `Foo::ComicDecorator` is not defined, it should not use `ComicDecorator` instead of `Foo::ComicDecorator`.
The cause of the problem is 73dcdd6, so we will revert it.

github.com

まとめ

OSSパッチ会のおかげでプルリク作るところまで出来た。ありがたい。