RSpec - 'users validation'のテストコードをレビューしてもらった結果

はじめに

みなさん、こんにちわ！Qiitaの投稿、ruby、rails、rspec初心者の J です！
つい先日、「プロから見た僕のspecはどうなの？」というのが気になり、
RSpec - 'users validation'のテスト を質問として、投稿しました。
そして、rubyやrspecについての動画をyoutubeで配信されていて、
僕の先生である@jnchito さんにコードレビューをしてもらいました！
テストコードは、そこそこ長かったのですが、一つ一つ徹底的に指摘してもらえて、
「僕ならこう書く！」を丁寧に解説してくださいました！
おかげさまで、

「ルールに従ったテストコードがなんなのか」と迷子になり、
なかなかcommitできず、地獄を彷徨っています。

から開放されて今は天にも上ような気分です！、、レビュー頂いて本当にありがとうございます。。泣

@jnchito さんについて

有名なので、みなさん知ってるかもですが、、

• プロを目指す人のためのRuby入門
  rubyについての書籍を出していたり、、
• Everyday Rails - RSpecによるRailsテスト入門
  rspecについての書籍の和訳をしたり、、
• 正規表現でテキスト中のTwitterアカウントだけを抜き出してみる
  youtubeで動画配信したり、、勉強会を開催している動画もありました！
• （1/2）Qiitaに投稿されていたRSpecのテストコードをレビューしてみた
  今回のレビューしてもらった動画はこちら！

めっちゃかっこいいダンディな先生です！！

という訳でこの記事は、僕自信がスペッカーになるためのアウトプットでもありますが、
まだ僕と同じくらいのスキル感の同志たちの参考になるべく、
指摘・解説してもらった内容から再度、自分自身のコードを見返してダメなところをガチガチに直してみました！
よかったら参考にしてみてください！

修正前の僕のゴミコード

前回のコードとのbefore、afterテストコードを載せるとQiitaのデータベースがパンクしてしまうかも　←(・・。)
なので、 修正前の僕のゴミコード を比較する場合は、タブを複製してどうぞ！

Specのネストイメージ

「ネストが深すぎなくていいんじゃないか？」って訳で、なるべく浅く修正！

RSpec.describe User, type: :model do
  describe 'attribute: name'
    context 'when present'
      it 'is valid'
        # ...省略
      end
    end
  end
end

上の内容は、、User > name > when present > it is valid
英訳的にはおかしな感じになりますが、どのネストも、、
「ユーザー名が存在する場合は有効です」的なニュアンスでわかりやすくネストし直しました。
これはあくまでイメージです！

修正後

Userモデル

app/models/user.rb

class User < ApplicationRecord
  before_save :downcase_email

  validates :name,
            presence: true,
            length: { maximum: 50 }
  VALID_EMAIL_REGEX = /\A[\w+\-.]+@[a-z\d\-]+(\.[a-z\d\-]+)*\.[a-z]+\z/i
  validates :email,
            presence: true,
            length: { maximum: 255 },
            format: { with: VALID_EMAIL_REGEX },
            uniqueness: { case_sensitive: false }

  private

    def downcase_email
      self.email = self.email.downcase
    end
end

補足

• VALID_EMAIL_REGEX = /\A[\w+\-.]+@[a-z\d\-]+(\.[a-z\d\-]+)*\.[a-z]+\z/i
  railstutorialより
  「２つの連続したドットはマッチさせないようにする」こちらの正規表現に修正しました。
• self.email.downcase!と、破壊的メソッドはあまり使用しない。
  将来的にいつどこで参照されるか不明なので、破壊的メソッドを使用しない方が無難。
  、、、たしかに。（ということで修正済みが以下。）

def downcase_email
  self.email = self.email.downcase
end

FactoryBot

spec/factories/users.rb

FactoryBot.define do
  factory :user do
    name { 'Tony Stark' }
    email { '[e-mail]' }
  end
end

補足

• 修正前: name { 'hoge' }、 email { "[e-mail]" }
  ありえない思っていたメールアドレスのドメイン部分。。あ、あれ？
  bar.com　ポチッ！
  、、、@example.comを使用すると誓います。

Spec

spec/models/user_spec.rb

require 'rails_helper'

RSpec.describe User, type: :model do
  let(:user) { FactoryBot.build(:user) }

  describe 'attribute: name' do
    context 'when present' do
      # nameが存在している場合は、有効であること
      it 'is valid' do
        user.name = 'Tony Stark'
        expect(user).to be_valid
      end
    end

    context 'when blank' do
      # nameが空白の場合は、無効であること
      it 'is invalid' do
        user.name = ' '
        expect(user).to be_invalid
        expect(user.errors[:name]).to include("can't be blank")
      end
    end

    context 'when empty' do
      # nameが空の場合は、無効であること
      it 'is invalid' do
        user.name = ''
        expect(user).to be_invalid
        expect(user.errors[:name]).to include("can't be blank")
      end
    end

    context 'when nil' do
      # nameが存在していない場合は、無効であること
      it 'is invalid' do
        user.name = nil
        expect(user).to be_invalid
        expect(user.errors[:name]).to include("can't be blank")
      end
    end

    context 'when length is 50 characters or less' do
      # nameが50文字以下の場合は、有効であること
      it 'is valid' do
        user.name = 'a' * 50
        expect(user).to be_valid
      end
    end

    context 'when length is more than 50 characters' do
      # nameが50文字を超える場合は、無効であること
      it 'is invalid' do
        user.name = 'a' * 51
        expect(user).to be_invalid
        expect(user.errors[:name]).to include('is too long (maximum is 50 characters)')
      end
    end
  end

  describe 'attribute: email' do
    context 'when present' do
      # emailが存在している場合は、有効であること
      it 'is invalid' do
        user.email = '[e-mail]'
        expect(user).to be_valid
      end
    end

    context 'when blank' do
      # emailが空白の場合は、無効であること
      it 'is invalid' do
        user.email = ' '
        expect(user).to be_invalid
        expect(user.errors[:email]).to include("can't be blank")
      end
    end

    context 'when empty' do
      # emailが空の場合は、無効であること
      it 'is invalid' do
        user.email = ''
        expect(user).to be_invalid
        expect(user.errors[:email]).to include("can't be blank")
      end
    end

    context 'when nil' do
      # emailが存在していない場合は、無効であること
      it 'is invalid' do
        user.email = nil
        expect(user).to be_invalid
        expect(user.errors[:email]).to include("can't be blank")
      end
    end

    context 'when length is 50 characters or less' do
      # emailが255文字以下の場合は、有効であること
      it 'is valid' do
        user.email = 'a' * 243 + '@example.com'
        expect(user).to be_valid
      end
    end

    context 'when length is more than 50 characters' do
      # emailが255文字を超える場合は、無効であること
      it 'is invalid' do
        user.email = 'a' * 244 + '@example.com'
        expect(user).to be_invalid
        expect(user.errors[:email]).to include('is too long (maximum is 255 characters)')
      end
    end

    context 'when correct format' do
      # emailの形式が正しい場合は、有効であること
      it 'is valid' do
        user.email = '[e-mail]'
        expect(user).to be_valid

        user.email = '[e-mail]'
        expect(user).to be_valid

        user.email = '[e-mail]'
        expect(user).to be_valid

        user.email = '[e-mail]'
        expect(user).to be_valid

        user.email = '[e-mail]'
        expect(user).to be_valid
      end
    end

    context 'when is incorrect format' do
      # emailの形式が正しくない場合は、無効であること
      it 'is invalid' do
        user.email = 'user@example,com'
        expect(user).to be_invalid

        user.email = 'user_at_foo.org'
        expect(user).to be_invalid

        user.email = 'user.name@example.'
        expect(user).to be_invalid

        user.email = 'foo@bar_baz.com'
        expect(user).to be_invalid

        user.email = 'foo@bar+baz.com'
        expect(user).to be_invalid
      end
    end

    context 'when already taken' do
      # 同一のemailが既に登録されている場合は、無効であること
      it 'is invalid' do
        FactoryBot.create(:user, email: '[e-mail]')
        user.email = '[e-mail]'
        expect(user).to be_invalid
        expect(user.errors[:email]).to include('has already been taken')
      end
    end

    context 'when case insensitive and not unipue' do
      # emailの大文字と小文字を区別せず、一意ではない場合は、無効であること
      it 'is invalid' do
        FactoryBot.create(:user, email: '[e-mail]')
        user.email = '[e-mail]'
        expect(user).to be_invalid
        expect(user.errors[:email]).to include('has already been taken')
      end
    end

    # emailが小文字で保存されていること
    it 'is saved in lowercase' do
      user.email = '[e-mail]'
      user.save!
      expect(user.reload.email).to eq '[e-mail]'
    end
  end
end

補足

• 先生：必要ないデバックコードは消しましょう。
  僕：あ、はい！でもテストしてるときにpryは便利です！
• 先生：FactoryBotのユーザーを見にいかないといけないのはよくないなぁ
  僕：FactoryBotでユーザーを定義している場合でも、エクスペクテーションを書く前に明確にnameに値を入れます！
• 僕：context、itの文章を修正しました！
  • context: 'when ~' ~である時をグループ化
  • it: validパターンといinvalidパターンを検証
    イメージはネストイメージの部分の文脈のニュアンスです。
• 先生：saveの検証まではいらないかなぁ。。
  validationが通ってsaveで落ちることは、滅多にないし、もしあったらrailsのバグか何か。
  僕：、、、なるほど！（削除済み以下。）
  修正前のコードを抜粋: expect(user.save).to be_falsey
• 先生：ここのテストをまとめよう。
  「nameが存在していない場合は、無効であること」
  • name = ' '（スペースのみ）
  • name = ''（空）
  • name = nil（nil）

    context 'when name is blank' do
      # nameが存在しない場合は、無効であること
      it 'is invalid' do
        user.name = ' '
        expect(user).to be_invalid
        expect(user.errors[:name]).to include("can't be blank")

        user.name = ''
        expect(user).to be_invalid
        expect(user.errors[:name]).to include("can't be blank")

        user.name = nil
        expect(user).to be_invalid
        expect(user.errors[:name]).to include("can't be blank")
      end
    end

僕：ここはなんとなくまとめずに書きたいと思ったのでblank、empty、nilで区分けしました！

• 僕：user.valid?と聞いてあげないとエラーメッセージにmessagesが入ってこないと思ってました、、
  ということでuser.valid?と聞かずにexpectをそのまま記載！
• 先生：正規表現のテスト、ここはいいですね！
  僕：あざっす！
• 先生：dryを捨てて、変数に格納してあれやこれやしない！
  ロジックを組むと、そのロジックにミスが発生して思わぬ事故になる。。。
• 先生：uniqueness: { case_sensitive: false }をテストした方がいいのでは？
  僕：（2/2）Qiitaに投稿されていたRSpecのテストコードをレビューしてみたで解説してもらったように、、
  • emailアドレスが小文字の有効なuserを保存。
  • 上で登録したuserの大文字emailをuserのemailに入れ直す。
  • userは無効であること。
  • また、エラーメッセージにhas already been takenが含まれていること。
    が以下のコード。

context 'when case insensitive and not unipue' do
  # emailの大文字と小文字を区別せず、一意ではない場合は、無効であること
  it 'is invalid' do
    FactoryBot.create(:user, email: '[e-mail]')
    user.email = '[e-mail]'
    expect(user).to be_invalid
    expect(user.errors[:email]).to include('has already been taken')
  end
end

まとめ

もっとたくさんの指摘もらいましたが、まだプロの考えに及んでいない自分がいるので、
レビュー&解説して頂いた動画を見直して理解した部分をもっと明確に記載しようと思います！
テスト書き始めの方は、僕と同じで「どこまでテスト」していいのかわからないとなる同志も少なくなさそうです。
それぞれ意見は異なると思いますが、初心者的にズバリ！

'railsのバリデーションを信じて、自分や他の人の作ったロジックをテストしよう！'

ってことですね！
railsの検証をわざわざテストするのは、もったいないと思いました、、
また、先生も言っていた通り、「言い出したらキリがないもの」もあるということを考えれば、
テストポイントを絞る力の方が重要だと思います。
今回のバリデーションに対するテストは、ほぼ100%と言える正解のテストコードが存在すると思うので、
自分やチームメンバーが作ったロジックをテストする未来の練習にはなりそうです！

それからみなさん！

必要でない限り、テストはdryを捨てましょう！

みんながコード読みやすくてハッピーになれるし、
何より、、
TESTが間違ってたら意味ないですからね、、w

では、また！

テストのアウトプット

User
  attribute: name
    when present
      is valid
    when blank
      is invalid
    when empty
      is invalid
    when nil
      is invalid
    when length is 50 characters or less
      is valid
    when length is more than 50 characters
      is invalid
  attribute: email
    is saved in lowercase
    when present
      is invalid
    when blank
      is invalid
    when empty
      is invalid
    when nil
      is invalid
    when length is 50 characters or less
      is valid
    when length is more than 50 characters
      is invalid
    when correct format
      is valid
    when is incorrect format
      is invalid
    when already taken
      is invalid
    when case insensitive and not unipue
      is invalid