Clean Test Code Revisedの発表内容について

willnet · 2019 年 3 月 24 日午後 3:22

昨日のRails Developers Meetup 2019で可読性の高いテストコードの書き方について発表しました。発表を聞いた人や資料を読んだ人からの感想を聞くと「総論としては賛成なんだけど各論については違う意見を持っている」という人が結構いるな、という印象です。

みなさんが具体的にどういう視点でどういう選択をしているのか知りたいので「自分ならここはこう書く、なぜなら〜」みたいなコメントをいただけるととてもうれしいです(\( ⁰⊖⁰)/)!!!

Clean Test Code Revised - Speaker Deck

sinsoku · 2019 年 3 月 24 日午後 3:33

Twitterに書いた後にDiscourseのスレに気づいたので、こちらにも投稿しておきます。

ただ、個人的にはFactoryBotのデフォルトはランダム値じゃなくて、DBのデフォルト値が好きかな。ユニットテストで品質的な事をテストする必要は無いと思うし、微妙なランダム値だとFlaky Testになったりするので。

fujimura · 2019 年 3 月 24 日午後 4:41

自分の発表の裏番組だったので残念ながら聞けなかったんですが、資料読みました。ほぼ同意です。
差分はshared_context, shared_examples`は使わない、bangなしのletも使わない、くらいですかね。letは呼ぶ・呼ばないで事前条件を変えるので、脳のメモリを無駄遣いする＆事故の元と思ってます。

微妙なランダム値といえば、ランダムデータ生成はFakerを使っていたのですが、衝突してテストが落ちることがあるので、文字列の属性にはSecureRandom.urlsafe_base64 を使うようになりました。結果のdiffが読みづらいことがありますが、まあ誤差の範囲という印象。

willnet · 2019 年 3 月 25 日午前 3:29

差分はshared_context, shared_examples`は使わない、bangなしのletも使わない、くらいですかね。letは呼ぶ・呼ばないで事前条件を変えるので、脳のメモリを無駄遣いする＆事故の元と思ってます。

なるほどなるほど。チームで開発するならこれくらい縛った方が安全な気もしますね…

微妙なランダム値といえば、ランダムデータ生成はFakerを使っていたのですが、衝突してテストが落ちることがあるので、文字列の属性にはSecureRandom.urlsafe_base64 を使うようになりました。結果のdiffが読みづらいことがありますが、まあ誤差の範囲という印象。

Fakerで衝突を防ぎたい場合はFaker::Name.unique.nameみたいにできる、ということをこないだ知りました(まだ試してない)

stympy/faker: A library for generating fake data such as names, addresses, and phone numbers.

sinsoku · 2019 年 3 月 25 日午前 4:21

微妙なランダム値だとFlaky Testになったりする

ちょっと説明が足りていなかったので、補足を書きます。

Flaky Testの例

例えば、「チームにユーザーを追加できる機能」をテストしようとして、以下のようなテストを書くとする。

let(:owner) { create :user }
let!(:team) { create :team, owner: owner }
let!(:outsider) { create :user }

it '画面にチームメンバーの名前が表示されること' do
  visit team_path(team)

  within "#team_#{team.id} .member-list" do
    expect(page).to have_text owner.name
    expect(page).not_to have_text outsider.name
  end
end

このテストはownerとoutsiderの名前が偶然一緒になると、テストが落ちてしまう。
ランダム値を使ったテストはこういうテストが生まれやすく・・・

とここまで書いて思ったけど、これ単に「テストコード内に書かれていない暗黙的な条件をExpectationに使うのは」という話な気がしてきました。

やっぱりFlaky Testで困る

資料を読み直して気づきましたが、資料に記載されていた「rspec --seed で再現できる」では解決できないです。
seedで再現できるのはテストの実行順序だけなので、Factoryのランダム値までは再現できないはず。
一番の問題はこれかな。

bluerabbit · 2019 年 3 月 25 日午前 5:23

FactoryBotのデフォルトはランダム値じゃなくて、DBのデフォルト値が好きかな。ユニットテストで品質的な事をテストする必要は無いと思うし、微妙なランダム値だとFlaky Testになったりするので。

私はFactoryBotのデフォルトはランダム値派です。

Flaky Testの例
「チームにユーザーを追加できる機能」をテストしようとして、以下のようなテストを書くとする。

このテストで同一の名前だと落ちると気付けるのはメリットになりませんか？

このテストが落ちた時に同一の名前だと落ちるけど別に気にしなくていい場合は名前が違うという前提条件をcreate :user, name: 'owner'にすることになると思います。見えていない前提条件が足りなかったということになります。

それはFlaky Testというよりはテストが適切ではなかったということになりませんか？

この例でわかるランダムのメリットは意図していない前提条件に気付けること / 前提条件があるならばそのテストに書いてある状態になるということです。

willnet · 2019 年 3 月 25 日午後 1:53

これは僕も発表前に「本当に大丈夫なんだっけ？」となったので手元で確認済みです。FactoryBotのランダム値を例えば発表資料にあるようにstatus { %i[ready doing dome].sample } とした場合でも–seedで結果を固定できます。

なぜ固定できるかというと、rails g rspec:installしたときに生成されるconfig/spec_helper.rbに次のような設定がデフォルトで生成され、srandが–seedで指定された値になるからです。

Kernel.srand config.seed

↑が消されたりコメントアウトされていたりすると固定できないですが、ここをいじる人はきっとそれほどいないはず…。

sinsoku · 2019 年 3 月 25 日午後 2:31

このメリットはすごく分かります。
ただ、稀に落ちるテストに遭遇した時に、人はリトライせずに前提条件をちゃんと直すのか…というところが少し気になるところです。
みんな気軽にリトライしないで…と。

たしかに！気づきませんでした
そうなると、個人的に一番の問題が無くなったので、ランダム値で良さそうと思いました

willnet · 2019 年 3 月 27 日午前 12:06

めっちゃわかる。「テストがコケたときに原因を探らずにとりあえず再実行」みたいなのをどうにかしないといけないですね…

JunichiIto · 2019 年 6 月 4 日午前 1:00

https://blog.willnet.in/entry/2019/03/27/092642 にコメントを書こうとしましたが、こちらの存在に気づいたのでこっちにコメントしておきます↓

スライドと動画を改めて拝見させてもらいました。
「そうそう、そういうケース、よくあるよね〜」という「RSpecあるある」を見事にピックアップしていて素晴らしい内容だと思いました！

ところで、スライドを見ていて気づいたのですが、64枚目のlet!(:sender)はもしかするとlet!(:todo)が正ではないでしょうか？
スライドの修正漏れか何かかな？と思ったので、念のためご報告しておきます。

あとは、個人的に僕はこう考えるかな〜と思った点がいくつかあるので以下にメモしておきます。
あくまで個人の一意見ですが、ご参考までに〜。

56、57枚目
エディタをスクロールしている間に脳内メモリが揮発しがち
この例はうまい解決策がない

テスト全体で当たり前に使われるレコード（userを束ねるcompanyとか）や当たり前の前処理（単純なログインとか）であれば、ファイルの冒頭にあってもあまり脳に負担をかけないはず
スクロールしなくても、エディタ上で上下分割しながらテストコードを読むことはできる
あまりにも脳に負担がかかるようであれば、ネストをやめるのはアリ
もしくはspecファイルそのものをわける（user_spec.rbと、user_for_special_case_spec.rbのように）のも有効

59枚目
別ファイルの定義を行ったり来たりしている間に脳内メモリが揮発しがち

ヘルパーメソッドにいい名前が付けられていれば（うまく抽象化できていれば）比較的脳内の負担は少ない（1回実装を見たら、あとは見なくて済む）
もちろん、他のファイルで再利用しなければ、同じファイルに定義した方がよい

willnet · 2019 年 6 月 5 日午前 7:19

報告ありがとうございますー。スライド修正しました！

JunichiIto:

56、57枚目
エディタをスクロールしている間に脳内メモリが揮発しがち
この例はうまい解決策がない

テスト全体で当たり前に使われるレコード（userを束ねるcompanyとか）や当たり前の前処理（単純なログインとか）であれば、ファイルの冒頭にあってもあまり脳に負担をかけないはず

スクロールしなくても、エディタ上で上下分割しながらテストコードを読むことはできる

あまりにも脳に負担がかかるようであれば、ネストをやめるのはアリ

もしくはspecファイルそのものをわける（user_spec.rbと、user_for_special_case_spec.rbのように）のも有効

59枚目
別ファイルの定義を行ったり来たりしている間に脳内メモリが揮発しがち

ヘルパーメソッドにいい名前が付けられていれば（うまく抽象化できていれば）比較的脳内の負担は少ない（1回実装を見たら、あとは見なくて済む）

もちろん、他のファイルで再利用しなければ、同じファイルに定義した方がよい

ご意見に同意です(( ⁰⊖⁰)/)。ちゃんとした書き方で書いていればそこまで負担ではないのですが、ときどきお仕事で極端な書き方をしている(例: ネストがめちゃめちゃ深い、メソッドの名前付けが下手で何をしているのかすぐにわからないなど)のを見かけるのでした。テストコードで冗長さを減らして簡潔に書こうとすると可読性が下がることがおおいので、よいバランスで書いていきたいですね…。

willnet · 2023 年 11 月 22 日午前 9:08

thoughtbot社はlet(let!)でもインスタンス変数でもなくて、都度ヘルパメソッドを定義しようという方針で、これは新しい派閥だな〜と感じました

guides/testing-rspec at main · thoughtbot/guides

Avoid let (or let!) in RSpec. Prefer extracting helper methods, but do not re-implement the functionality of let. Example.

willnet · 2023 年 11 月 24 日午前 10:01