Zenn
🔨

テストの可読性を下げてしまう3つのアンチパターン

2024/03/13に公開
Rails
RSpec
Test
factorybot
アンチパターン
tech

はじめに

そもそもテストの可読性が下がるとどうなるのか。何がまずいのか。
筆者が過去に担当したプロジェクトでは下記のようなことが起こりました。

まずテストコードのメンテナンスコストが増加して、新たなロジックにテストが追加されなかったり、CIが落ちた際にテストコードをコメントアウトしてしまったりと、どんどんテストが軽視されるようになってしまいます。

こうなると、テストコードが本来の役割を果たさなくなり、バグが発生しやすくなったり、実装者以外に仕様が伝わりにくかったりと、さまざまな不都合が発生してしまいます。

上記のようにならないために、普段テストコードを書くときに意識している避けるべきことと改善策を、サンプルコードと一緒にまとめてみました。

1. テストデータの名前に連番や意味のない記号を用いる

下のBadの例のようなやつです。let で宣言したデータに _1 や _a とネーミングしています。

なぜダメなのかというと、どのようなデータなのかが書いた本人以外には分かりづらいですし、本人も1ヶ月後には分からなくなっているでしょう。

Goodの例に示した _published や _deleted のように推測しやすい意味のある名前をつけてあげることで、どのようなデータなのか一目でわかるようになります。

  • Bad
article_spec.rb
let(:article_1) { build(:article, published_at: 3.days.ago) }
let(:article_a) { build(:article, deleted_at: 5.days.ago) }
  • Good
article_spec.rb
let(:article_published) { build(:article, published_at: 3.days.ago) }
let(:article_deleted) { build(:article, deleted_at: 5.days.ago) }

2. データのユースケースをSpecファイルで直接指定している

タイトルが少し分かりづらいですが、Badの例の verified_at: 15.days.ago が "条件を直接セット" に該当します。

今回のサンプルコードは、ユーザーが申込可能かどうかを判定する applyable? メソッドのテストです。
例えば、複数箇所で user_applyable が宣言されていたとします。そこで仮に申込可能の条件に「ユーザーが成人していること」を追加するというような仕様変更があった場合、全ての user_applyable に対して修正が必要になります。

改善策としてはGoodの例のように、Factoryファイルで trait を用いてデータのユースケースを用意して、Specファイル側では直接値を指定しない形にすることです。
こうすることで、前述のような前述のような仕様変更の際もFactoryファイルの trait 一箇所を修正すれば良くなっただけではなく、データのユースケースの知識をSpecファイルからFactoryファイルに移行でき、Specファイルの見通しが良くなりました。

  • Bad
spec/models/user_spec.rb
describe '#applyable?' do
  context 'ユーザーが本人確認済みの場合' do
    # ↓この記述が複数箇所に存在する
    let(:user_applyable) { build(:user, verified_at: 15.days.ago) }
    it 'trueを返す' do
      expect(user_applyable.applyable?).to eq true
    end
  end
end
  • Good
spec/factories/user_spec.rb
FactoryBot.define do
  factory :user do
    # 〜 省略 〜
    trait :applyable do
      verified_at { 15.days.ago }
    end
  end
end
spec/models/user_spec.rb
describe '#applyable?' do
  context 'ユーザーが本人確認済みの場合' do
    let(:user_applyable) { build(:user, :applyable) }
    it 'trueを返す' do
      expect(user_applyable.applyable?).to eq true
    end
  end
end

3. Specファイル冒頭に大量のlet

Specファイル冒頭で大量の let でデータを宣言するのも、可読性を損なうのでNGです。

1つのテストケース（context）を読むのに、いちいちファイル冒頭を見に行ったり、使いたい条件に当てはまるデータを探したりするのは、面倒ですし時間がかかります。

Goodの例のように、1つのテストケースに対して必要なデータのみをcontext内で宣言するのが良いです。

  • Bad
spec/models/user_spec.rb
RSpec.describe User, type: :model do
  let(:user_applyable) { build(:user, :applyable) }
  let(:user_verified) { build(:user, :verified) }
  let(:user_not_verified) { build(:user, :not_verified) }
  let(:user_adult) { build(:user, :adult) }
  let(:user_declined) { build(:user, :declined) }
  let(:user_banned) { build(:user, :banned) }
  # 〜 大量のデータの宣言が続く 〜

  describe '#declined?' do
    context 'ユーザーが退会済みの場合' do
      # ↓実際に必要なのは user_declined のみ
      it 'trueを返す' do
        expect(user_declined.declined?).to eq true
      end
    end
  end
  # 〜 省略 〜
end
  • Good
spec/models/user_spec.rb
RSpec.describe User, type: :model do
  describe '#declined?' do
    context 'ユーザーが退会済みの場合' do
      # ↓テストケースに必要なデータのみを宣言
      let(:user_declined) { build(:user, :declined) }
      it 'trueを返す' do
        expect(user_declined.declined?).to eq true
      end
    end
  end
  # 〜 省略 〜
end

まとめ

ここまでで、以下の3つのアンチパターンと改善策を挙げてきました。

  • テストデータの名前に連番や意味のない記号を用いる
    → 一目でどんなデータかわかるような命名にする
  • データのユースケースをSpecファイルで直接指定している
    → Factoryファイルに移行して trait を使用する
  • Specファイル冒頭に大量のlet
    → テストケースごとに必要なデータのみをcontext内で定義する

どれもデータを適切に用意しておくという点で共通しているかと思います。
適切なデータの用意は、テストコードの可読性の向上やメンテナンスのハードルを下げることに役立ちます。

記事の内容は以上です。

Discussion