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

と、いうことで最近噂の Tama.rb が前々から気になったので初めて参加してモブプロをやってきました！

• Tama.rb #17 Everyday Rails - RSpecによるRailsテスト入門 - connpass

今回の Tama.rb の内容は Everyday Rails - RSpecによるRailsテスト入門 を読みながらグループに分かれてモブプロを行う、という内容でした。
ぶっちゃけモブプロはやったことがなかったのでどうなるのか不安でしたが、Vim グループというとても居心地がいいグループに入った事でもう好き勝手言いまくってました！！

そしてその結果、書籍の内容そっちのけで最初から最後までひたすら feature spec のコードをリファクタリングしていただけという結果になり…。
もう途中からモブプロはそっちのけで全員であーだこーだ言いながらみんなでわいわいいいながらコードを書く会になっていましたね…。
はやモブプロとは…という感じだったんですが、結果的には RSpec の知識を共有したりわいわい言いながらみんなでコードを書くことが出来てめっちゃ楽しかったです！

最終的には個人的にかなりいい感じのリファクタリングになったので、そこに至るまでの流れを簡単にまとめてみたいと思います。
結構うろ覚えなので話の流れとか間違ってたらごめんなさい！

元の feature spec

書籍に載っていたのは元々以下のような rspec でした。

require 'rails_helper'

RSpec.feature "Projects", type: :feature do
  # ユーザーは新しいプロジェクトを作成する
  scenario "user creates a new project" do
    user = FactoryBot.create(:user)

    visit root_path
    click_link "Sign in"
    fill_in "Email", with: user.email
    fill_in "Password", with: user.password
    click_button "Log in"

    expect {
      click_link "New Project"
      fill_in "Name", with: "tama.rb"
      fill_in "Description", with: "omotesando"
      click_button "Create Project"

      # MEMO: click_button とはまだ処理が完了していない可能性がある
      #       そこで以下のようなテストを実行することで「次のページがレンダリングされている」ことを保証する
      expect(page).to have_content "Project was successfully created"
      expect(page).to have_content "tama.rb"
      expect(page).to have_content "Owner: #{user.name}"
    }.to change(user.projects, :count).by(1)
  end
end

このテスト自体はそこまで複雑ではなくて以下のような流れになっています。

1． テストで使用する User を生成する
2． visit root_path で root にアクセスして生成した User で email と password を入力して "Log in" ボタンをクリックする
3． (expect のブロック内に入る)
4． "New Project" をクリックして New Project のページに移動する
5． "Name" と "Description" のフォームを入力して "Create Project" をクリックする
6． クリック後に "Project was successfully created" 等が表示されているかどうかのテストを行う
7． 最後に user.projects が増えていることを確認する

これはこれで流れがわかりやすくて悪くはないと思うんですが、最初にコードを見たときに以下の点が気になりました。

• scenario の中身が長い
  • 別 context のテストを追加しようとするとつらい
• expect {} の中で expect している…
  • これは待ち処理を行っているのでしょうがない部分もあるがうーん…

と、いうような話をモブプロ内で行っていたら「じゃあ、リファクタリングするべ」という流れになり書籍を読むのはやめてひたすらこのコードのリファクタリングを行っていました。
このときの共通の意識としては、

• フォームに変な入力を行った場合のテストを書きたいよね
• 失敗したときのテストを書きたいよね
• expect {} の中で expect してるのがきもい

っていうのがあったと思うのでこのあたりを中心にリファクタリングしていきました。

user を let 化する

まず最初は、

「user.name が空の場合のテストとかしたいよねー」
「じゃあ、let(:name) とかに切り出すべー」

みたいな話になり、まずは user を let に切り出しました。

require 'rails_helper'

RSpec.feature "Projects", type: :feature do
  # コンテキストで切り分けそうな値を切り出す
  let(:user) { FactoryBot.create(:user) }
  let(:email){ user.email }
  let(:password){ user.password }
  let(:name){ user.name }

  scenario "user creates a new project" do
    visit root_path
    click_link "Sign in"
    fill_in "Email", with: email
    fill_in "Password", with: password
    click_button "Log in"

    expect {
      click_link "New Project"
      fill_in "Name", with: "tama.rb"
      fill_in "Description", with: "omotesando"
      click_button "Create Project"

      # MEMO: click_button とはまだ処理が完了していない可能性がある
      #       そこで以下のようなテストを実行することで「次のページがレンダリングされている」ことを保証する
      expect(page).to have_content "Project was successfully created"
      expect(page).to have_content "tama.rb"
      expect(page).to have_content "Owner: #{name}"
    }.to change(user.projects, :count).by(1)
  end
end

割と妥当なやり方ですね。

ログイン処理を before に切り出す

今回テストするのは『New Project からプロジェクトを生成した場合にどうなるのか』を検証するテストになります。
なので最初の方にある『ログイン処理』は scenario から切り出して before で定義する事になりました。
また、ここで気づいたんですが

「あれ、ログイン処理が今回のテストに関係ないなら別に email とか password は let で切り出さなくてよくね」
「じゃあ、無駄な let は定義しないで user.email とかで参照するべ」

という流れになり、今回必要がない let は消すことにしました。

require 'rails_helper'

RSpec.feature "Projects", type: :feature do
  let(:user) { FactoryBot.create(:user) }
  let(:name){ user.name }

  # ログイン処理を before に切り出し
  before do
    visit root_path
    click_link "Sign in"
    # let 経由ではなくて user 経由で参照する
    fill_in "Email", with: user.email
    fill_in "Password", with: user.password
    click_button "Log in"
  end

  scenario "user creates a new project" do
    expect {
      click_link "New Project"
      fill_in "Name", with: "tama.rb"
      fill_in "Description", with: "omotesando"
      click_button "Create Project"

      # MEMO: click_button とはまだ処理が完了していない可能性がある
      #       そこで以下のようなテストを実行することで「次のページがレンダリングされている」ことを保証する
      expect(page).to have_content "Project was successfully created"
      expect(page).to have_content "tama.rb"
      expect(page).to have_content "Owner: #{name}"
    }.to change(user.projects, :count).by(1)
  end
end

めっちゃライブ感がありますね！

プロジェクトの生成 を subject に切り出す

これはわたしが強い気持ちで、

「scenario の中身が長いので subject に切り出そう！！！」

と言って『プロジェクトを生成する処理』をまるっと subject に切り出しました。
ここでポイントなのは『subject が lambda を返している点』です。
これは subject が lambda を返すことで

scenario "user creates a new project" do
  expect { subject.call }.to change(user.projects, :count).by(1)
end

を

scenario "user creates a new project" do
  is_expected.to change(user.projects, :count).by(1)
end

と言う風に is_expected で記述することが出来るからです。
また、

「もう scenario の中身が1行しかないから it のほうがよくない？」

みたいな話になり全員が特に強い気持ちがなかったので scenario から it に変更し、更に引数も省略しました。
「あーみんなもそういう意識があるんだー」と「自分の書き方もそこまで間違ってないんだなー」と思いました。
一人で書いていると何が正しいのかがわからなくなるのでこういう『共通の認識』みたいなのを共有できるとだいぶ安心感があっていいですねー。
で、以下のようになりました。

it { is_expected.to change(user.projects, :count).by(1) }

結果的に出来上がったのは以下のような subject になります。

require 'rails_helper'

RSpec.feature "Projects", type: :feature do
  let(:user) { FactoryBot.create(:user) }
  let(:name){ user.name }

  before do
    visit root_path
    click_link "Sign in"
    fill_in "Email", with: user.email
    fill_in "Password", with: user.password
    click_button "Log in"
  end

  # 『プロジェクトを生成する』という処理を subject に切り出し
  subject {
    -> {
      click_link "New Project"
      fill_in "Name", with: "tama.rb"
      fill_in "Description", with: "omotesando"
      click_button "Create Project"

      # MEMO: click_button とはまだ処理が完了していない可能性がある
      #       そこで以下のようなテストを実行することで「次のページがレンダリングされている」ことを保証する
      expect(page).to have_content "Project was successfully created"
      expect(page).to have_content "tama.rb"
      expect(page).to have_content "Owner: #{name}"
    }
  }

  # その結果、 scenario はやめて it 1行だけに
  it { is_expected.to change(user.projects, :count).by(1) }
end

各々の処理の役割が明確になってきましたね。
it (scenario)が1行だけになりだいぶスッキリしてきました。

describe で別スコープ化

ここまで来ると 「上位スコープにあんまり let とか定義したくないよねー」
「describe でテストの意味を切り分けよう！」
ということで describe でテストを切り分けました。

require 'rails_helper'

RSpec.feature "Projects", type: :feature do
  # 『プロジェクトを生成した場合』のテストとして切り分ける
  describe "project create" do
    let(:user) { FactoryBot.create(:user) }
    let(:name){ user.name }

    before do
      visit root_path
      click_link "Sign in"
      fill_in "Email", with: user.email
      fill_in "Password", with: user.password
      click_button "Log in"
    end

    subject {
      -> {
        click_link "New Project"
        fill_in "Name", with: "tama.rb"
        fill_in "Description", with: "omotesando"
        click_button "Create Project"

        # MEMO: click_button とはまだ処理が完了していない可能性がある
        #       そこで以下のようなテストを実行することで「次のページがレンダリングされている」ことを保証する
        expect(page).to have_content "Project was successfully created"
        expect(page).to have_content "tama.rb"
        expect(page).to have_content "Owner: #{name}"
      }
    }

    it { is_expected.to change(user.projects, :count).by(1) }
  end
end

こうすることで今後『プロジェクトを削除した場合のテスト』などを追加する場合に let が切り分けることが出来るので便利です。

click_button "Create Project" したあとの待ち処理をどうするか

さて、今回一番悩んだ点です。
click_button "Create Project" した後にどうやって『ページ遷移が終了したのか』の判定を行うのか、です。
現状は expect(page).to have_content "Project was successfully created" を行い『"Project was successfully created" が content に表示されるまで待つ』という処理を行っています。

「現状は "Project was successfully created" で判定しているけどバリデーションエラーになった場合とかに必ずしも "Project was successfully created" が表示されるとは限らない」
「失敗した時のページ遷移の条件をどうするか…」
「今回の程度ならもう sleep 1 でいいんじゃね？」
「待ち処理の為に expect 使いたくなりなり〜」

みたいな話をしていたんですが、結果的には『have_content に渡す文字列をコンテキストによって変える( :let で定義しておく)』という事にしました。
また、

「expect {} 内で expect を行っているのはあくまでも『待ち処理を行う』という目的じゃん？」
「expect {} 内でテストを行う必要性はないんじゃなかろうか」

と、言うことで一旦『 expect {} 内で行う expect は1回だけ』にすることにしました。
ついでに fill_in "Name" に渡す文字列もコンテキストによって変えたいので let で定義することに。

require 'rails_helper'

RSpec.feature "Projects", type: :feature do
  describe "project create" do
    let(:user) { FactoryBot.create(:user) }
    let(:name){ user.name }
    # コンテキストで変わりそうな値を let で定義する
    let(:have_content_notice) { "Project was successfully created" }
    let(:project_name) { "tama.rb" }
    let(:project_description) { "omotesando" }

    before do
      visit root_path
      click_link "Sign in"
      fill_in "Email", with: user.email
      fill_in "Password", with: user.password
      click_button "Log in"
    end

    subject {
      -> {
        click_link "New Project"
        fill_in "Name", with: project_name
        fill_in "Description", with: project_description
        click_button "Create Project"

        # MEMO: click_button とはまだ処理が完了していない可能性がある
        #       そこで以下のようなテストを実行することで「次のページがレンダリングされている」ことを保証する
        expect(page).to have_content have_content_notice
      }
    }

    it { is_expected.to change(user.projects, :count).by(1) }
  end
end

なかなか整ってきましたね

context を追加する

もう少しです！

• project_name が空の場合
• project_description が空の場合

のテストを追加していきたいと思います！

require 'rails_helper'

RSpec.feature "Projects", type: :feature do
  describe "project create" do
    let(:user) { FactoryBot.create(:user) }
    let(:name){ user.name }
    let(:have_content_notice) { "Project was successfully created" }
    let(:project_name) { "tama.rb" }
    let(:project_description) { "omotesando" }

    before do
      visit root_path
      click_link "Sign in"
      fill_in "Email", with: user.email
      fill_in "Password", with: user.password
      click_button "Log in"
    end

    subject {
      -> {
        click_link "New Project"
        fill_in "Name", with: project_name
        fill_in "Description", with: project_description
        click_button "Create Project"

        # MEMO: click_button とはまだ処理が完了していない可能性がある
        #       そこで以下のようなテストを実行することで「次のページがレンダリングされている」ことを保証する
        expect(page).to have_content have_content_notice
      }
    }

    # project_name と description が存在する場合のテスト
    context "with project name and description" do
      it { is_expected.to change(user.projects, :count).by(1) }
    end

    # project_name が空の場合のテスト
    context "with empty project name" do
      let(:have_content_notice) { "Name can't be blank" }
      let(:project_name) { "" }

      it { is_expected.not_to change(user.projects, :count) }
    end

    # project_description が空の場合のテスト
    context "with empty project description" do
      let(:project_description) { "" }

      it { is_expected.to change(user.projects, :count).by(1) }
    end
  end
end

おおーだいぶ簡単にテストが追加できましたね。
余談ですが、アプリ側の挙動が全然わかってなくて

「project_description が空だとエラーになるよねー」
「いや、でも description が空でも別に大丈夫なんじゃない？」
「じゃあ、試しにやってみよう！」

と、言う感じでやってみたら実は description が空でもプロジェクトが生成されるということがわかりました。
テストからアプリ側の挙動がわかるのがちょっとおもしろかったです。

let は DRY にしない

これも RSpec あるあるかもしれませんが let はあまり DRY にしたくありません。
と、いうのも

「let はデフォルトで定義するんじゃなくて各 context で明示的に定義したいよね」
「context の外に let があると目線が上に言ったりきたりしてつらい」
「コードが重複してもいいのでなるべく狭い範囲(context)で let を定義しよう！」

と、言うことで let(:have_content_notice) などは各 context 内で明示的に定義するようにしました。
これも強い反対意見などなくて割とみんな共通の意識なのかなーと思いました。

require 'rails_helper'

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

    before do
      visit root_path
      click_link "Sign in"
      fill_in "Email", with: user.email
      fill_in "Password", with: user.password
      click_button "Log in"
    end

    subject {
      -> {
        click_link "New Project"
        fill_in "Name", with: project_name
        fill_in "Description", with: project_description
        click_button "Create Project"

        # MEMO: click_button とはまだ処理が完了していない可能性がある
        #       そこで以下のようなテストを実行することで「次のページがレンダリングされている」ことを保証する
        expect(page).to have_content have_content_notice
      }
    }

    context "with project name and description" do
      let(:have_content_notice) { "Project was successfully created" }
      let(:project_name) { "tama.rb" }
      let(:project_description) { "omotesando" }

      it { is_expected.to change(user.projects, :count).by(1) }
    end

    context "with empty project name" do
      let(:have_content_notice) { "Name can't be blank" }
      let(:project_name) { "" }
      let(:project_description) { "omotesando" }

      it { is_expected.not_to change(user.projects, :count) }
    end

    context "with empty project description" do
      let(:have_content_notice) { "Project was successfully created" }
      let(:project_name) { "tama.rb" }
      let(:project_description) { "" }

      it { is_expected.to change(user.projects, :count).by(1) }
    end
  end
end

have_content のテストを追加する

最後に先程削除した expect(page).to have_content "tama.rb" のテストをどうするのか考えました。
これ、expect(page) を呼び出しているのでちょっとテストの仕方に工夫が必要になります。
最初は、

it { is_expected.to change { page }.to have_content "tama.rb" }

みたいに書いてみたんですが、これだとうまく行きませんでした。

「これで動くといいなー」 「あー動かなかったー」 「page が返す値が同じになっているのかなあ…」 「じゃあ、 subject を直接呼び出せばいいんじゃない？」

で、まずは以下のようなテストにしました。

it "shows project_name" do
  subject.call
  expect(page).to have_content project_name
end

これはこれでいいのですが、

「subject.call っていう呼び出しが意味不明過ぎる」 「これはつらい」 「じゃあ subject に名前つければいいんじゃね？」 「「それだ！」」

という事で subject(:new_project) と定義し、 new_project.call と呼び出すことに。 ぶっちゃけ subject に引数が渡せることは完全に失念していたいので「なるほどなー」とめっちゃ関心してました。 そして、最終的に出来上がったので次のコードになります！

require 'rails_helper'

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

    before do
      visit root_path
      click_link "Sign in"
      fill_in "Email", with: user.email
      fill_in "Password", with: user.password
      click_button "Log in"
    end

    subject(:new_project) {
      -> {
        click_link "New Project"
        fill_in "Name", with: project_name
        fill_in "Description", with: project_description
        click_button "Create Project"

        # MEMO: click_button とはまだ処理が完了していない可能性がある
        #       そこで以下のようなテストを実行することで「次のページがレンダリングされている」ことを保証する
        expect(page).to have_content have_content_notice
      }
    }

    context "with project name and description" do
      let(:have_content_notice) { "Project was successfully created" }
      let(:project_name) { "tama.rb" }
      let(:project_description) { "omotesando" }

      it { is_expected.to change(user.projects, :count).by(1) }
      it "shows project_name" do
        new_project.call
        expect(page).to have_content project_name
      end
    end

    context "with empty project name" do
      let(:have_content_notice) { "Name can't be blank" }
      let(:project_name) { "" }
      let(:project_description) { "omotesando" }

      it { is_expected.not_to change(user.projects, :count) }
    end

    context "with empty project description" do
      let(:have_content_notice) { "Project was successfully created" }
      let(:project_name) { "tama.rb" }
      let(:project_description) { "" }

      it { is_expected.to change(user.projects, :count).by(1) }
    end
  end
end

もう最初のコードのかけらもないですね！
ちょうどここでモブプロの時間も終了して個人的にはかなり満足したコードがかけたと思います。

まとめ

はじめてモブプロをやったんですが、これがモブプロとして正しいのかどうかは置いておいて多人数でコードを書くのはやっぱり楽しいですね。
普段は基本的に1人だけでコードを書いているのでこういう『他人のコードの書き方』や『コードを書く上での知見』みたいなのを得るのはとても貴重な機会でした。
意外というか意見を出しながらコードを書いていたんですが、あんまりコードの書き方の方向性で殴り合い揉めなかったことですね。

これは自分の声がデカかったのかもしれませんが、『あーみんなもそう思っているんだー』みたいな安心感の方が大きかった気がします。
1人でコードを書いていると何が正しいのかがわからなくなる事が多いんですが、こういう機会があると『自分のコードの書き方は間違ってなかったんや！！』みたいに思えるのでめっちゃいいですね。

あと今回は 1つの PC をみんなで使いまわしていたんですが、自分の Vim に慣れすぎてて他人の Vim がめっちゃつらいという事が体験できました…。
いかにしてに自分の Vim が自分の使いやすいようにカスタマイズされているのか改めて実感しました。

今回参加してみてモブプロ自体がかなりよくて、そのあともいろいろと Ruby について話が出来たので次もぜひぜひ参加してみたいですね！
これは Ruby の話に飢えているんだ…もっと Ruby の話がしてえんだ…。

そんな感じで簡単に初参加した Tama.rb の内容をまとめてみました。
同じグループでワイワイしていた方々、運営の方々ありがとうございましたー！

その後

で、いまブログ記事を書きながら先程動かねーと言っていた

it { is_expected.to change { page }.to have_content "tama.rb" }

をどうにか出来ないかいろいろと試していたんですが、page ではなくて page.html を参照することで解決出来ました。

it { is_expected.to change { page.html }.to include "tama.rb" }

これでかなりすっきりしましたね！