Edited at

【動画付き】Re: Tama.rb に行ってモブプロをやって feature spec をリファクタリングした話


はじめに

先日Qiitaでこちらの記事を拝見しました。

Tama.rb に行ってモブプロをやって feature spec をリファクタリングした話 - Qiita

僕が翻訳に携わっている電子書籍「Everyday Rails - RSpecによるRailsテスト入門」(以下Everyday Rails)を題材にしてTama.rbで勉強会が開かれたようで、そこで書籍内のコードをリファクタリングする例がこの記事に載っています。

Everyday Railsを題材にして勉強会を開いてもらったのは大変ありがたいですし、リファクタリングされたコードもこれはこれで興味深いのですが、個人的には「僕は普段こういう書き方はしないなー」と思いました。

というわけで、上の記事でリファクタリングされたコードを僕だったらどう書くか、という話をこの記事で紹介したいと思います。


動画があります

僕が具体的にどんなことを考えてコードを書き換えていったのかは以下の動画で詳しく説明しています。

こちらもご覧いただくと、より理解が進むと思います。

Re: Tama.rb に行ってモブプロをやって feature spec をリファクタリングした話 - YouTube


テストコードに関する僕の基本方針

僕はテストコード(特にフィーチャスペックやシステムスペック)を書くとき、こういったことを重視しています。


  • 上から下に自然に読み下せること

  • 特別な理由がない限り文字列や数値のベタ書きを用いること(変数を多用しないこと)

  • ループ処理やif文など、プログラムチックなテストコードにしないこと

もっとざっくりいうと、「あたかも人間が読む文書のようなテストコード」が好みです。

この場合、DRYなテストコードにはなりませんが、アプリケーション側のコードとは違い、テストコードは多少DRYを捨てても構わないと考えています。

また、テストとして必ず担保したい仕様をカバーできる(=テストコードがテストコードの目的を果たしている)のであれば、ラクをするために雑に書くことも許容することが多いです。


元記事のテストコードを見て感じたこと

上に書いた「僕の基本方針」に照らし合わせると、元記事のテストコードは以下のような点を修正したくなりました。


  • subjectは複雑さが増す(文書らしさを損ねる)ので使わない

  • 単純な文字列をletで定義するぐらいならベタ書きする

また、example(itscenario)は本来であれば細かく分ける(1 exampleにつき、1エクスペクテーションとする)方が望ましいですが、フィーチャスペックにおいては実行速度の低下にもつながりやすいので、1つのexampleにまとめてしまっても良いと思います。

あと、context "with empty project description"として project_description が未入力だったケースをテストしていますが、これは「登録に成功するケース」の別パターンなので、わざわざテストケースを追加する必要はないと思いました。

もし「○○が未入力なら登録に成功する/失敗する」を細かく検証したいのであれば、モデルスペックにテストを追加するのが良いと思います。


僕が修正したテストコードはこちら

上記のような点をふまえて、僕が書き直したテストコードはこちらになります。


spec/system/projects_spec.rb

require 'rails_helper'

RSpec.describe "Projects", type: :system do
describe "project create" do
let(:user) { FactoryBot.create(:user) }

before do
sign_in_as(user)
end

context "with project name and description" do
scenario 'user creates a new project' do
click_link "New Project"
fill_in "Name", with: "tama.rb"
fill_in "Description", with: "omotesando"
expect {
click_button "Create Project"
}.to change(user.projects, :count).by(1)
expect(page).to have_content "Project was successfully created"
expect(page).to have_content "tama.rb"
end
end

context "with empty project name" do
scenario "user can't create a new project" do
click_link "New Project"
fill_in "Name", with: ""
fill_in "Description", with: "omotesando"
expect {
click_button "Create Project"
}.to_not change(user.projects, :count)
expect(page).to have_content "Name can't be blank"
end
end
end
end


まあ、ほとんど「リファクタリング前のコードに戻した」と説明した方が正しいかも知れませんね😅

ちなみにbeforeブロック内で使っている sign_in_as メソッドは、spec/support/login_support.rbに元から定義されていたものです。


spec/support/login_support.rb

module LoginSupport

def sign_in_as(user)
visit root_path
click_link "Sign in"
fill_in "Email", with: user.email
fill_in "Password", with: user.password
click_button "Log in"
end
end

RSpec.configure do |config|
config.include LoginSupport
end



細かいテストはモデルスペックに書く

あと、以下のようなモデルスペックも追加して「descriptionが未入力だった場合」も検証するようにしています。


spec/models/project_spec.rb

require 'rails_helper'

RSpec.describe Project, type: :model do
# 省略

describe 'validation' do
let(:project) { FactoryBot.build(:project) }
context 'with empty name' do
it 'is invalid' do
project.name = ''
expect(project).to be_invalid
end
end
context 'with empty description' do
it 'is valid' do
project.description = ''
expect(project).to be_valid
end
end
end
end


コントローラやviewのロジックをテストするのでなければ、このようにモデルスペックでテストした方が、書きやすさの面でも実行速度の面でも有利です。


発展:さらに雑に書く

フィーチャスペックはDRYにしづらい&実行速度が遅い、という傾向があるので、「検証エラーが発生する」というような基本的なテストケースは、以下のようにcontextを分けずに1つのexampleにまとめてしまうことが多いです(雑)。


spec/system/projects_spec.rb

require 'rails_helper'

RSpec.describe "Projects", type: :system do
describe "project create" do
let(:user) { FactoryBot.create(:user, first_name: 'Alice', last_name: 'Ito') }

scenario 'user creates a new project' do
sign_in_as(user)
click_link "New Project"

# 検証エラーが発生する場合
fill_in "Name", with: ""
fill_in "Description", with: "omotesando"
expect {
click_button "Create Project"
}.to_not change(user.projects, :count)
expect(page).to have_content "Name can't be blank"

# 正常な値を入力した場合
fill_in "Name", with: "tama.rb"
expect {
click_button "Create Project"
}.to change(user.projects, :count).by(1)
expect(page).to have_content "Project was successfully created"
expect(page).to have_content "tama.rb"
expect(page).to have_content "omotesando"
expect(page).to have_content "Owner: Alice Ito"
end
end
end


僕が必ずcontextを分けるのは、たとえば「一般ユーザーでアクセスした場合(は○○ボタンが表示されない)」と「管理者ユーザーでアクセスした場合(は○○ボタンが表示される)」のように、システムの振る舞いが根本的に変わるケースですね。


まとめ

というわけで、この記事ではEveryday Railsのテストコードを題材にして、フィーチャスペック(システムスペック)の書き方を議論してみました。

こうした書き方の違いは「良し・悪し(or 正しい・間違っている)」というよりも、「好き・嫌い」の世界に近いです。

いろいろな流派(?)があることを知って、自分の好みの書き方は何か、自分の開発チームではどういった書き方を推奨するか、といったテーマについて考えてみましょう。


あわせて読みたい

僕のテストコードの好みについては、過去にもいろいろと書いています。

RSpecの書き方については以下の記事の「Q. シンプルなスタイルと複雑なスタイル、選ぶならどっち?」も参考になると思います。

もちろん、「Everyday Rails - RSpecによるRailsテスト入門」のコーディングスタイルにあわせて書くのもおすすめです。