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

## はじめに

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

[Tama\.rb に行ってモブプロをやって feature spec をリファクタリングした話 \- Qiita](https://qiita.com/pink_bangbi/items/2390eb5f2252aa6d6f0a)

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

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

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

## 動画があります

僕が具体的にどんなことを考えてコードを書き換えていったのかは以下の動画で詳しく説明しています。
こちらもご覧いただくと、より理解が進むと思います。

[Re: Tama\.rb に行ってモブプロをやって feature spec をリファクタリングした話 \- YouTube](https://www.youtube.com/watch?v=6V-WU_TFacY&feature=youtu.be)

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

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

- 上から下に自然に読み下せること
- 特別な理由がない限り文字列や数値のベタ書きを用いること（変数を多用しないこと）
- ループ処理やif文など、プログラムチックなテストコードにしないこと

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

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

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

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

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

- subjectは複雑さが増す（文書らしさを損ねる）ので使わない
- 単純な文字列をletで定義するぐらいならベタ書きする

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

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

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

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

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

```ruby: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`に元から定義されていたものです。

```ruby: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が未入力だった場合」も検証するようにしています。

```ruby: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にまとめてしまうことが多いです（雑）。

```ruby: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 正しい・間違っている）」というよりも、「好き・嫌い」の世界に近いです。

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

### あわせて読みたい

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

- [テストコードの期待値はDRYを捨ててベタ書きする ～テストコードの重要な役割とは？～ \- Qiita](https://qiita.com/jnchito/items/eb3cfa9f7db752dcb796)
- [サヨナラBetter Specs\!? 雑で気楽なRSpecのススメ \- Qiita](https://qiita.com/jnchito/items/a90b3b09d008227d3d60)

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

- [【翻訳】RSpecのリードメンテナだけど何か質問ある？ \- Qiita](https://qiita.com/jnchito/items/3a8d19fd9a30468cafd4)

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

- [Everyday Rails - RSpecによるRailsテスト入門](https://leanpub.com/everydayrailsrspec-jp)