Test which reminded me why I don't really like RSpec | Arkency Blog (日本語訳：Rails: RSpecが好きでないことを思い出したテスト（翻訳）｜TechRacho by BPS株式会社) を見ての感想。
元のコードのイマイチなところは 4 つあって、
paramsをbeforeで書き換えている *1it "will succeed"の文言it { is_expected.to be_success }とexpect(result.success?).to eq(true)が混ざっているletが不思議な順序で連発されていて事前条件を読み解けない
すべて、これによって何をテストしているのかが分かりづらくなっているという問題を引き起こす。
params を before で書き換えている
let(:params) { { last_name: "something" } } subject(:result) { described_class.call(current_user: user, params: params) } ... describe "checking Address update" do let(:new_zip_code) { Faker::Address.zip_code } before { params[:zip_code] = new_zip_code } context "when user has address and want to change something in their address" do let!(:address) { create(:address, owner: user) } it "will succeed" do expect(result.success?).to eq(true) ...
この before は上の方で定義してある let との合わせ技で、かなり厳しい。
このクラスでは、異なる結果が得られる主な入力は
paramsである。この入力がどう変わると出力がどう変わるかは明確にわかるはずだが、これを
callに明示的に渡さない理由がわからない。
にまったく同意で、そのために describe で切り分けているのだから、params に何が渡るのかは describe 内で明示する。何をテストしているのかを明確にするというのを意識したい。今は params の違いをテストしているのだから、params が何なのかをそのまま書く。let で定義して subject に渡しているので、let で上書けば良い。
let(:params) { { last_name: "something" } }
subject(:result) { described_class.call(current_user: user, params: params) }
...
describe "checking Address update" do
let(:new_zip_code) { Faker::Address.zip_code }
- before { params[:zip_code] = new_zip_code }
+ let(:params) { {
+ last_name: "something",
+ zip_code: new_zip_code,
+ } }
context "when user has address and want to change something in their address" do
let!(:address) { create(:address, owner: user) }
it "will succeed" do
expect(result.success?).to eq(true)
...
「他のところで作ったものを before でちょこっと書き換えよう」はだいたい悪手。あと多分ココは last_name: "something", を残す必要なくて、zip_code のテストをしたいので zip_code のみになっている方が望ましいと思う。
it "will succeed" の文言
describe "checking Address update" do ... context "when user has address and want to change something in their address" do ... it "will succeed" do ... end end context "when user has not have any address and want to change something in their address" do ... it "create address with given params" do ... end end end
これは何のテストをしているのか読み解くのが難しいと思うけど、よくよく読むと、やりたいことはただの upsert である。おそらく実装側はこうなっている。
if params[:zip_code] address = current_user.address || current_user.build_address address.zip_code = params[:zip_code] address.save! end
upsert のテストをしているというのが分かるように説明を書く。なお、テスト対象や事前条件と context の説明は一致していて、context 直下に before もしくは let が来るのが良い分割。テストの違いを context や before, let で表現するのだ。
ついでに何をテストしているのか分かりやすいように「params[:zip_code] を渡したとき」というのも説明文として表現するとより親切だろう。
context "with params[:zip_code]" do let(:params) { { zip_code: new_zip_code, } } ... context "when address exists" do before { create(:address, owner: user) } ... it "will be updated" do ... end end context "when address does not exists" do ... it "will be created" do ... end end end
describe, context, it の説明文を駆使して何をテストしているのかを明らかにするのは、可読性の高いテストを書くための基本です。
it { is_expected.to be_success } と expect(result.success?).to eq(true) が混ざっている
context "when address exists" do ... it "will be updated" do expect(result.success?).to eq(true) expect(address.reload.zip_code).to eq(new_zip_code) end end context "when address does not exists" do ... it { is_expected.to be_success } it "will be created" do expect { result }.to change(Address.all, :count).from(0).to(1) expect(Address.last.zip_code).to eq(new_zip_code) expect(user.address).to eq(Address.last) end end
update 側では result.success? と、更新されたことの 2 個の expect があるが、
create 側では result.success? は単独のテストとして切り出されているし、書き方も違う。
書き方が違うと「この違いに何か意味があるのか？」という疑問が湧いてしまい、テスト対象がぼやける。テストの粒度は揃える。
また、success? であることはあまり主眼ではないので、目立たないように定型文っぽくなっているとより良い。つまり eq(true) とハッキリと対象をテストするよりも、読み下して読み飛ばせる方が望ましい。
context "when address exists" do ... + it { is_expected.to be_success } it "will be updated" do - expect(result.success?).to eq(true) expect(address.reload.zip_code).to eq(new_zip_code) end end context "when address does not exists" do ... it { is_expected.to be_success } it "will be created" do ... end end
こうすると、全 context で「result.success?」と「詳細な副作用」をテストしている、と整理できる。
let が不思議な順序で連発されていて事前条件を読み解けない
context "when user has parent and want to change something" do let(:parent) { create(:parent) } let(:user) { create(:user, profession: student) } let(:student) { create(:student, parent: parent) } it { is_expected.to be_success } ...
ここでは parent を upsert するときの update 側をテストしたい。そのため、 テスト対象 (subject) である Api::Students::Update.call(current_user: user, params: params) の user 側に「parent を持っている User である」という事前条件がある。
最終的に欲しいのは user なので、せめてこの順に並んでいて欲しい。
let(:parent) { create(:parent) } let(:student) { create(:student, parent: parent) } let(:user) { create(:user, profession: student) }
また、事前条件なんだから before で整える、というのも自然な考え方だろう。let と違い遅延評価されず、スコープが絞られるため、怖れずに上から順に読めるようになり、処理を追うのが簡単になる。
context "when user has parent and want to change something" do before { parent = create(:parent) student = create(:student, parent: parent) @user = create(:user, profession: student) } let(:user) { @user } it { is_expected.to be_success } ...
let(:user) { create(:user, profession: @student) } でも良いんだけど、私は before で作ったインスタンスを let にパスするだけが好きです。この方が事前条件を整えた後に context に受け渡している感がある。
僕なら最初の 5 分でこうリファクタリングする
RSpec.describe Api::Students::Update do let(:user) { create(:user) } let(:params) { { last_name: "something" } } subject(:result) { Api::Students::Update.call(current_user: user, params: params) } context "when user is a student" do it { should be_success } end context "when user is teacher" do let(:user) { create(:user, :teacher) } it { should be_failure } end context "with params[:zip_code]" do before { @new_zip_code = Faker::Address.zip_code } let(:params) { { last_name: "something", zip_code: @new_zip_code, } } context "when address exists" do before { @address = create(:address, owner: user) } it { should be_success } it "will be updated" do result expect(@address.reload.zip_code).to eq(@new_zip_code) end end context "when address does not exists" do it { should be_success } it "will be created" do expect { result }.to change(Address.all, :count).from(0).to(1) expect(Address.last.zip_code).to eq(@new_zip_code) expect(user.address).to eq(Address.last) end end end context "with params[:parent]" do before { @parent_first_name = Faker::Name.female_first_name } let(:params) { { parent: { first_name: @parent_first_name }, } } context "when parent exists" do before { @parent = create(:parent) student = create(:student, parent: @parent) @user = create(:user, profession: student) } let(:user) { @user } it { should be_success } it "will be updated" do result expect(@parent.reload.first_name).to eq(@parent_first_name) end end context "when parent does not exists" do it { should be_success } it "will be created" do expect { result }.to change(Parent.all, :count).from(0).to(1) expect(Parent.last.reload.first_name).to eq(@parent_first_name) expect(user.profession.parent).to eq(Parent.last) end end end end
こうしておくと -f d で実行したときにも読み解きやすい。
Api::Students::Update
when user is a student
is expected to be success
when user is teacher
is expected to be failure
with params[:zip_code]
when address exists
is expected to be success
will be updated
when address does not exists
is expected to be success
will be created
with params[:parent]
when parent exists
is expected to be success
will be updated
when parent does not exists
is expected to be success
will be created
change はあんまり使いたくないなー (expect { result }.to はちょっと読みづらくない？) とか、多分 create の方のテストを先に書くだろうな (条件が複雑な方をより後でテストしたいので、レコードが存在している場合は後ろに書きたい) とか、一つのテストの中で複数の expect を書くなら aggregate_failures を使えとかの思いはあるけど、シュッと直すならこうかなー
元記事のリファクタリング後と何が違うか
describe(context) のネストは維持している- テスト対象が明確に違うので、ネストしておく方が自然
params[:zip_code]やparams[:parent]があるときの、それぞれのupsertを確認している
contextの説明はより実装寄りに- ユニットテストなので実装寄りの言葉で問題無いし、その方が実装を書き換えるときにテストの対応を取りやすい
subjectを引き続き使う- テストしたいのは
paramsの違いなので、paramsに着目したい。 - 元記事のリファクタリング後だと
paramsは 70 文字目辺りから始まっていて、どこに違いがあるのかを読み解くのが困難 result = Api::Students::Update.call(current_user: user, params: { parent: { first_name: parent_first_name } })subjectに執着しているわけではなく、テスト間の違いをもっと目立たせろと思っています
- テストしたいのは
letはcontext間の差を表現するために使う- よりテスト間の違いに目を向けさせるためです
letは遅延評価＆後勝ちなところが大きなメリットで、まさにcontext内で上書きしたいときに使う語彙です- そのため、私は積極的に
beforeやローカル変数、インスタンス変数を使い、letを温存します- 今回の書き換えでも
user,paramsのみがletになっていて、何をテストしているのか読み解きやすいんじゃないか？ - テーブルテストと同じことを
contextとletでやっているのです
- 今回の書き換えでも
subject やインスタンス変数に反対の派閥があるのは知っていて、異論はあると思うけど、私は「テスト対象が分かりやすくあった上で Ruby/RSpec の表現力にあやかりたい」と考えているので、こう書いています。
合わせて読みたい：Rails Developers Meetup 2017 で RSpec しぐさについて話した - onk.ninja
*1:公開当初は「params を merge している」と書いていたのでこういう反応があった https://twitter.com/qsona/status/1629840312389742593
id:stefafafan
id:onk
id:rokoucha
id:tokizuoh
id:sue445
id:nagayama
