RSpecの気になる挙動を見つけてからrspec-coreにPRがマージされるまでの話
文中で挙げているソースコードは2024年10月18日時点のmasterブランチのものです。
きっかけ
きっかけは2024年2月29日のことでした。
閏日に必ず失敗するRSpecのexampleがありmasterブランチのCIが失敗していたので、
pendingして一時的にCIを通したくて以下のような修正を行いました。
it pending: true do
expect(hoge).to eq fuga
end
しかし上記の example は保留されず失敗してしまいました。
何故そのような挙動をするのか理解できないまま試行錯誤し、
結局 xit として example の成功を棚上げしてCIを通しました。
今思うとCIが通らない事に焦り自分が想像していた挙動をしていなかったことに苛立っていただけなのかもしれないですが、この仕様に違和感をもちソースコードを読んでみることにしました。
RSpecの現行バージョンの仕様
RSpecはその役割毎に複数のリポジトリに分かれていますが、今回のような example の処理が定義されているのは rspec-core というリポジトリでした。
因みに今後はモノレポに移行するらしいです。（https://github.com/rspec/rspec-core/issues/3088#issuecomment-2144008326）
該当箇所は lib/rspec/core/example_group.rb の以下のコードでした。
def self.define_example_method(name, extra_options={})
idempotently_define_singleton_method(name) do |*all_args, &block|
desc, *args = *all_args
options = Metadata.build_hash_from(args)
options.update(:skip => RSpec::Core::Pending::NOT_YET_IMPLEMENTED) unless block
options.update(extra_options)
RSpec::Core::Example.new(self, desc, options, block)
end
end
all_args は example の引数なので、例えば it の第一引数が example の説明（以下 doc_string と呼ぶ） をするStringオブジェクトでなくても desc に格納されます。要は pending: true というハッシュオブジェクトが doc_string として認識されていたのです。私はこの場合 pending のオプションが正しく認識されて example が保留になるべきと考えてこのPullRequestを作成しました。
最初のPR
#3071 のPullRequestは example の第一引数がStringオブジェクトの場合のみ第一引数を変数 desc に格納し、残りは args に格納するという修正をしています。
def self.define_example_method(name, extra_options={})
idempotently_define_singleton_method(name) do |*all_args, &block|
desc = all_args.shift if all_args.first.is_a? String
args = all_args
options = Metadata.build_hash_from(args)
options.update(:skip => RSpec::Core::Pending::NOT_YET_IMPLEMENTED) unless block
options.update(extra_options)
RSpec::Core::Example.new(self, desc, options, block)
end
end
rspec-coreのメンテナーからは一定の理解を得られましたが、同時に後述のコメントをしていただきこのPullRequestはクローズしました。コメントの最初の要点としては、この修正がexampleの第一引数はStringであることがドキュメントに明記されているにもかかわらずString以外を許容することになることです。次の要点としてはこの修正がユーザーにどのような影響を与えるかが不透明であるということです。仮に第一引数でexampleの説明としてSynmolオブジェクトを利用している人がいれば、それは説明として認識されなくなってしまいます。とはいえ過去に第一引数をStringに強制する試みを検討したことはあるようで、私の修正に対してはかなりポジティブに捉えていただきました。最終的に 3.9.9 でString以外のオブジェクトを第一引数で指定した場合に非推奨警告を出力し 4.x.x で第一引数をStringオブジェクトに強制する仕様変更を行うこととなりました。
改めてIssueとPullRequestを作成
ということで先ずはIssueを作成しました。
タイトルは doc_string which is the first argument of example, must be string object 。直訳すると exampleの第一引数に来るdoc_stringはStingオブジェクトでなければならない です。
そしてv3.9.9でリリースする為のmainブランチへ向けた非推奨警告を出力するPullRequestと、
v4.x.xでリリースする為の本丸のものを作成しました。
ただ、 #3074 を作成した後のメンテナー同士のコミュニケーションで自分が誤った、矛盾した修正を入れていたことに気づきます。コメントはこちら。要は must be string object なのにStringオブジェクト以外を渡してもよしなに動くような修正するのは違うんじゃない？ってことです。ここの所を何も考えず、一発目と同じ修正でいいや！と思ってPullRequest作ってしまってました。ということでRSpecとしてどのような挙動をすべきかを考えて（コメントも読み返して）改めてPullRequestを作成しました。新しい方ではStringオブジェクト以外をexampleの第一引数に渡したらArgumentErrorになるようにしてます。
2024/11/07現在
この記事を執筆している時点ではIssueには次のメジャーバージョンである 4.0 のマイルストーンが付けられていて、非推奨警告のPullRequestには 3.9.9 、ArgumentErrorの方は 4.0 開発用の親ブランチにマージされている状態です。ただこのPullRequestは修正依頼来てたのに見落としていて結局メンテナーの方が手を加えてマージされていました。
今回の貢献で学んだこと
正直些細な修正ではあるものの、今回かなりメジャーなライブラリに対してIssueを作り貢献できたことはかなり嬉しいです正直。ただ反省点や勉強になった点はあり、先ずは英語表現を正確に行えるようにしなくてはと思いました。ちょっと前までは、いや母語ではないからしょうがないじゃんって思ってましたが、メンテナーからすると言ってることとやってること違うと迷惑でしかないし、英語はプログラマーの共通言語みたいな側面もあると感じることもあるのでもっと勉強して英語の能力を向上させねばと思いました。また、OSSをやる際は（そうでない場合でも）仕様を変更する際の影響には敏感になるべきだと思いました。今回の修正は気に食わない挙動がきっかけでしたが、最初の修正を混ぜた際のユーザーへの影響までは考えられていませんでした。破壊的な仕様変更がある場合は非推奨の警告を出しつつ次のメジャーバージョンで変更する、という方法は一般的ではあるかもですが勉強になりました。
おわりに
ここまで読んでくれた方、ありがとうございます。
こんな感じで緩く久しぶりのOSS活動を振り返ってみました。
別の記事でも書きましたが最近OSSのコードを読む機会がちらほらありOSSへの貢献はやっていきたいと考えてます。C言語勉強してOSSやると宣言して久しい・・・
次回Issue起票したりPullRequest作成する際は今回の経験を活かせればと思ってます！
また、あまり中身ないですがこの記事が誰かのソフトウェア開発やOSS活動の役に立てたら幸いです。
Discussion