Rails: RSpecが好きでないことを思い出したテスト（翻訳）

Rails: RSpecが好きでないことを思い出したテスト（翻訳）

最近、別のソフトウェア会社にいる友人がmutantのセットアップについて助けを求めてきました。問題点を発見するためにサンプルテストを共有しながら作業を進めました。Slackチャンネルで以下のスニペットを目にした瞬間、その場で以下のレスを書き込みました。

私: このexampleを見てると自分がRSpecが好きじゃないことを思い出してしまうよ

友人の返事: 好きにレビューしていいから。若手が書いたコードなんで早いとこフィードバックしたい。

念のため申し上げますが、私はRSpecというライブラリそのものは問題だと思っていません。RSpecは素晴らしいツールですし、多くのプロジェクトで使ってきましたが、RSpecを使う人によくある書き方が好きではないのです。それではテストを見てみましょう。

元のexampleはこうでした。

# frozen_string_literal: true

require "rails_helper"

RSpec.describe Api::Students::Update do
  let(:user) { create(:user) }
  let(:params) { { last_name: "something" } }
  subject(:result) { described_class.call(current_user: user, params: params) }

  context "when user is a student" do
    it { is_expected.to be_success }
  end

  context "when user is teacher" do
    let(:user) { create(:user, :teacher) }
    it { is_expected.to be_failure }
  end

  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)
        expect(address.reload.zip_code).to eq(new_zip_code)
      end
    end

    context "when user has not have any address and want to change something in their address" do
      let(:new_zip_code) { Faker::Address.zip_code }
      before { params[:zip_code] = new_zip_code }
      it { is_expected.to be_success }
      it "create address with given params" 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

  describe "checking parent update" do
    let(:parent_first_name) { Faker::Name.female_first_name }
    let(:params) { { parent: { first_name: parent_first_name } } }

    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 }
      it "will succeed" do
        result
        expect(parent.reload.first_name).to eq(parent_first_name)
      end
    end

    context "when user has not have any parent and want to change something in their parent" do
      it { is_expected.to be_success }
      it "will create parent with given params" 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

以下は、これを読んだ私の感想です。

• RSpec固有のシンタックスシュガーが実際のテストコードに比べてやたらと多い
• テストのsubjectは詳しく書くべきだが、その周辺はごちゃごちゃ書くべきではない
• これが何のテストなのかがわからない。contextの中にitがネストしていて意図が読み取れない。

友人: まあまあ、IDE使ってるからコードブロックを折り畳めばいいでしょ

私: そうなんだけど、Slackじゃ折り畳めないし、普段のコードレビューはGitHubで行っているそうだけどGitHubでも折り畳めないし。サービスの呼び出しに何が入力されているのかを知るのにファイルのあちこちにジャンプしたくないし

• セットアップがFactoryBotで行われている。
  これはデータベースのステートに何らかの生成物を設定するが、ビジネスルールに沿わないことがちょくちょくある（これはビジネスルールがActiveRecordにある場合の話だけど、自分らは何年も前にそういうことをやめた）。
  初期ステートのセットアップはドメインサービスで行う方がよい。FactoryBotの使い方が原因でデータベースオブジェクトが大量に作成され、テストの実行が激遅になったコードベースを山ほど見てきた。
• contextがletを上書きするためにしか使われていないので、exampleが変わるとセットアップも変わってしまう。
  構造をフラットにしてすべてのexampleを明示的にセットアップすればいいのでは？letで宣言されるものを何か別のものにする必要があるなら、exampleの中でローカル変数を使えば済む。letが有用なのは、依存関係を指定する場合や、テストケースごとに変わらないものを指定する場合。
• このクラスでは、異なる結果が得られる主な入力はparamsである。
  この入力がどう変わると出力がどう変わるかは明確にわかるはずだが、これをcallに明示的に渡さない理由がわからない。
• このテストで期待するスコープでParent.allのアサーションを行っても、そのサービスが望むParentオブジェクトにデータが割り当てられることは保証されない。

話を先に進めるために、取り急ぎ5分で以下のようにリファクタリングしました。

# frozen_string_literal: true

require "rails_helper"

RSpec.describe Api::Students::Update do
  specify "when user is a student" do
    expect(Api::Students::Update.call(current_user: user, params: { last_name: "something" })).to be_success
  end

  specify "when user is a teacher" do
    expect(Api::Students::Update.call(current_user: teacher, params: { last_name: "something" })).to be_failure
  end

  specify "when user has address and want to change something in their address will succeed" do
    address = create(:address, owner: user)

    result = Api::Students::Update.call(current_user: user, params: { last_name: "something", zip_code: new_zip_code })

    expect(result.success?).to eq(true)
    expect(address.reload.zip_code).to eq(new_zip_code)
  end

  specify "when user has not have any address and want to change something in their address create address with given params" do
    result =
      expect {
        Api::Students::Update.call(current_user: user, params: { last_name: "something", zip_code: new_zip_code })
      }.to change(Address.all, :count).from(0).to(1)

    expect(result.success?).to eq(true)
    expect(Address.last.zip_code).to eq(new_zip_code)
    expect(user.address).to eq(Address.last)
  end

  specify "when user has parent and want to change something" do
    user = create(:user, profession: student)

    result = Api::Students::Update.call(current_user: user, params: { parent: { first_name: parent_first_name } })

    expect(result).to be_success
    expect(parent.reload.first_name).to eq(parent_first_name)
  end

  specify "when user has not have any parent and want to change something in their parent" do
    user = create(:user, profession: student)

    result =
      expect {
        Api::Students::Update.call(current_user: user, params: { parent: { first_name: parent_first_name } })
      }.to change(Parent.all, :count).from(0).to(1)

    expect(result).to be_success
    expect(Parent.last.reload.first_name).to eq(parent_first_name)
    expect(user.profession.parent).to eq(Parent.last)
  end

  let(:user) { create(:user) }
  let(:teacher) { create(:user, :teacher) }
  let(:student) { create(:student, parent: parent) }
  let(:parent) { create(:parent) }
  let(:new_zip_code) { Faker::Address.zip_code }
  let(:parent_first_name) { Fake::Name.female_first_name }
end

皆さんのご感想はいかがでしょうか？どちらの方が読みやすいですか？

関連記事

RSpecの作者が振り返る歴史（翻訳）

Rails: 5年前のアドバイザリーロック実装が突然おかしくなった話（翻訳）