リファクタリングで気づいたソフトウェアテストへの勘違い

ogp

はじめに

こんにちは。修士1年でTechouseに長期インターンとして入社し、3ヶ月になるWakiYuukuと申します。

弊社では、大企業向けのSaaSプロダクトを複数運営しており、安定した品質で日々のサービスを提供するために、プロダクトのコーディングにおいてテスト駆動開発（TDD）を推進しています。また、継続的インテグレーション（CI）を導入することで、テストに通らないコードがmainブランチにマージされないように管理しています。

そのため、インターン生たちはまず研修でテスト駆動開発（TDD）の基礎を学んだ後、各プロジェクトにアサインされます。本記事では、私自身があるプロジェクトでTDDを活用したリファクタリング作業中に、ソフトウェアテストに対する誤った思い込みから生じたミスについて振り返り、その教訓を共有します。

あらまし

大学の夏休み中、私はあるプロジェクトに取り組む機会がありました。そのプロジェクトでは、既存のクラスのリファクタリングを行う必要がありました。

具体的には、従業員の絞り込みを行うためのパラメータparamを受け取り、絞り込まれた従業員に対して処理を行うメソッドAが存在していました。このメソッドを他の機能でも汎用的に仕様できるように絞り込み条件paramではなく従業員employeeを直接渡すようにリファクタリングをする必要がありました。

以下に具体的なコードを示します。

# リファクタリング前
def method_A(param)
    # 省略
    employees = filter_employees(param) # 従業員の絞り込みを実行
    do_something_with(employees) # employeesを使用した処理
end

このコードを以下のようにメソッドAの引数にparamではなく対象のemployeeを直接渡すようにリファクタリングする必要がありました。

# リファクタリング後
def method_B(param)
    # 省略
    employees = filter_employees(param) # 従業員の絞り込みをメソッドBに切り出し
end

def method_A(employees)
    do_something_with(employees) # employeesを使用した処理
end

既存のメソッドAにはテストコードが書かれているため、私はTDDの基本である「既存のテストが通るようにリファクタリングを行う」を守りながら図1のように実装を進め、最終的にテストが全て通過していることを確認することで自信を持ってリファクタリングの実装を完了させました。

altテキスト
図1: リファクタリング前のテストの構造とリファクタリング後のテストの実装

・・・しかし、ローカル環境での動作チェックで、「すべての従業員」を選択したときに結果が表示されないことに気が付きました。

原因

テストが全て通過しているにもかかわらず、なぜ期待通りに動作しないのかと少々困惑しながら原因を探ってみると、「すべての従業員」が選択されたときにparamはnilになる仕様であり、リファクタリングの際に絞り込みの条件がnilになることはないだろうという勝手な思いこみからnilの場合は処理を中断するようなコードを追加していたことが原因でした。また、既存のテストにはparamがnilのケースに対するテストが書かれておらず、既存のテストでもこの問題を発見できなかったのです。

具体的には、以下のようなmethod_Bの実装が問題でした。

def method_B(param)
    return if param.nil? # この処理が原因で`nil`の場合に処理が中断されてしまう
    employees = filter_employees(param)
end

反省点

問題を発見した当初、「代表的なテストケースであるnilケースをカバーしきれていない既存のテストに問題がある」と責任転嫁していました。しかし、今回のリファクタリングの失敗を振り返り、単にメソッドの仕様の確認不足や既存テストのカバー率以外にも、自身のソフトウェアテストへの勘違いとリファクタリングの進め方にも問題があったことに気づきました。

1. ソフトウェアテストへの勘違い

駆け出しエンジニアである私は「全てのテストを通るコードは正しい」というソフトウェアテストへの勘違いをしていました、今回のミスを通して、この考えはソフトウェアテストの7原則に反した考え方であることに気づきました。

developers.techouse.com

今回のケースで特に重要だったのは以下の2つの原則です。

原則1: テストは欠陥があることを示せるが、欠陥がないことを示せない

この原則は、ソフトウェアテストの根幹とも言える考え方です。テストをどれだけ行っても、ソフトウェアが完全に欠陥のない状態であると保証はできません。それにもかかわらず、私はこの基本的な原則を忘れ、「すべてのテストが通ったから大丈夫だ」と安易に考えてコードの改変をしてしまったのです。

原則2: 全数テストは不可能

もう1つ見落としていたのが、この原則です。ソフトウェア開発において、すべての条件や組み合わせを網羅的にテストすることは現実的に不可能です。そのため、効率的で効果的なテスト設計が求められます。

今回のケースでは、特に nil 値の扱いに関するテストが不足していることが問題でした。全数テストが不可能である以上、重要なケースを見極め、リファクタリングによる欠陥の発生を防ぎやすいテスト設計が必要です。TDDはリファクタリングを前提としてテストを作成するアプローチであるため、nil 値や空値といった、バグが発生しやすいケースを優先的にテスト対象とすることが重要だと身をもって理解しました。

2. 切り分けたメソッドBに対するユニットテストを書かなかった

実装面での反省点は切り分けたメソッドBにテストを追加しなかったことと、既存のテストケースを流用したことにあります。私が実装したテストコードでは、メソッドBの機能を完全にテストしきれていませんでした。
図2のように、切り分けたメソッドごとにユニットテストを書くのが本来あるべき実装であり、図にしてみると、それぞれのメソッドに対してテストを書くほうがシンプルでわかりやすい方法であることがわかります。
さらに、メソッドBに対するテストを追加していれば、引数paramとそれによって絞り込まれる従業員との関係を改めて詳細に確認する機会が生まれるため、paramがnilの場合の処理に問題があることに気づけたはずです。

altテキスト
図2: リファクタリング後のテストの構造と理想の実装

以上の反省点を踏まえ、切り出したメソッドBに対して以下のテストを追加しました。このテストでは、従業員の在籍状況による絞り込み処理と、paramがnilの場合の動作を検証しています。

メソッドBに対して追加したテスト
※以下はテストコードを簡略化して記載しています。

describe '#method_B' do
  # 在籍中、休職中、退職中の従業員を作成
  let(:employee) do
    employee = create(:employee)
    employee.build_employee_enroll(enroll_type: 'enroll')
    employee.save!
    employee
  end
  let(:left_employee) do
    employee = create(:employee)
    employee.build_employee_enroll(enroll_type: 'left')
    employee.save!
    employee
  end
  let(:quit_employee) do
    employee = create(:employee)
    employee.build_employee_enroll(enroll_type: 'quit')
    employee.save!
    employee
  end

  before do
    employee
    left_employee
    quit_employee
  end

  describe '対象に全従業員が選択されparamがnilの場合' do
    let(:param) { nil }
    it '全従業員のデータが出力される' do
      expect(method_B(param)).to eq([employee, left_employee, quit_employee])
    end
  end
  
  describe '対象に在籍中の従業員が選択され他場合' do
    let(:param) { 'enroll_type' => 'enroll' }
    it '在籍中の従業員のデータが出力される' do
      expect(method_B(param)).to eq([employee])
    end
  end

  #=====================================
  #以下他のテストケース
  #=====================================
end