review-testing

機能の実装やバグ修正と併せて作成されたテストコードをレビューしています。テストが単に通るだけでなく、適切に設計され、十分な網羅性があり、保守しやすいことを確認してください。実装の詳細をそのままなぞるだけのテストは、誤った安心感を生み、リファクタリング時の保守負担になります。

レビューの対象範囲

レビュー対象を特定します。

1. 現在のブランチで変更されたテストファイルを、ベースブランチと比較して見つけます。
2. そのテストが対象とする本番コードを見つけます。インポート、関数呼び出し、ファイル名の規則をたどって、テストと対象コードを対応付けます。
3. 同じモジュールや関数に関する既存の関連テストのうち、変更されていないものを見つけます。更新が必要な場合や、テストの不足を明らかにする場合があります。

本番コードより先にテストファイルを読みます。テストだけから機能の要件やエッジケースを推測できるなら、それはテストが適切に記述されている証拠です。テストの内容を理解するために実装を読む必要がある場合は、報告すべき指摘事項です。

価値のあるテストとは

各テストを、次の4つの観点で評価します。

回帰防止 — このテストは実際にバグを検出できますか？単純なコードを実行するだけのテストや、複雑な分岐を通らないテストでは、何も防げません。確認事項：テストは重要なビジネスロジックに触れていますか？それとも、単純なゲッターの正常系だけを確認していますか？

リファクタリング耐性 — 振る舞いを変えずにコードを再構成したとき、このテストは壊れますか？内部のメソッド名、呼び出し順序、プライベートな状態に密結合したテストは、整理のたびに誤った失敗を引き起こし、テストスイートへの信頼を損ないます。

迅速なフィードバック — ユニットテストはミリ秒単位で実行されるべきです。テストが不必要にファイルシステム、ネットワーク、データベースへアクセスするなら、設計上の問題です。ただし、速さと価値を混同しないでください。すべてをモックして何も検証しない高速なユニットテストより、実際のデータベースクエリを検証する統合テストのほうが優れています。

保守性 — このコードに詳しくない人でも、テストを読んで何を、なぜ検証しているのか理解できますか？セットアップが冗長なテスト、名前が分かりにくいテスト、モックが深くネストしたテストは、この基準を満たしません。

レビュー項目

1. アサーションの完全性

生成されたテストに最もよく見られる弱点は、明白な出力だけを検証し、状態変更による「影響範囲全体」を見落とすことです。

テストでアクションを実行したとき、ほかに何が変わったかを確認します。たとえば、テストでカートに商品を追加する場合、商品の個数だけを確認していますか？それとも、価格計算、小計の更新、ほかの商品に影響がないことも検証していますか？

次の場合は指摘してください：

• テストが戻り値を検証している一方、副作用を無視している
• テストが処理の成功を確認している一方、正しい結果が得られたかを確認していない
• テストが正常系を検証している一方、境界条件（空の入力、null、最大値、オフバイワン）を省略している
• エラー系のテストで、エラーがスローされたことだけを確認し、エラーメッセージ、型、クリーンアップの動作を確認していない

2. テストの構造

各テストは、Arrange-Act-Assert のサイクルを正確に1回だけ含める必要があります。テストが複数回続けて操作とアサーションを行う場合、複数の振る舞いをテストしているため、分割するべきです。

次の場合は指摘してください：

• テストに複数の操作フェーズがある（1つの振る舞いではなく、ワークフローをテストしている）
• Arrange フェーズが大きすぎて、テスト対象の振る舞いが埋もれている
• セットアップヘルパーやユーティリティ関数の中にアサーションがある（何を検証しているのかが分かりにくくなる）
• テストロジックに条件分岐やループが含まれている — テストは単純に上から下へ実行されるコードであるべきです

3. フィクスチャと状態の管理

テストデータの作成方法によって、テストスイートを大規模になっても保守できるかどうかが決まります。インラインセットアップ（テスト本体でデータを作成する方法）は、シンプルなテストには適していますが、多くのテストでコンストラクターのシグネチャが変わると、修正が大変になります。暗黙的なセットアップ（共有のbeforeEach／setUpブロック）は重複をなくしますが、各テストが実際に何に依存しているかが見えにくくなります。委譲型セットアップ（明示的に呼び出すファクトリー関数やビルダー）なら、構築ロジックを集約しながら、テストの読みやすさを保てます。

次の場合に指摘してください：

• 共有セットアップブロックで、ほとんどのテストが使わないオブジェクトを作成している
• テストが外部ファイル、データベースの状態、または別の場所で定義されたグローバル変数に依存している（「ミステリーゲスト」— テストに必要なものは、テスト本体から確認できるか、1回の関数呼び出しで参照できるようにする必要があります）
• リソースを作成せずに存在すると仮定している（ファイル、データベースのレコード、環境変数など）
• テストがクリーンアップせずに共有状態を変更し、実行順序に依存する失敗を引き起こしている

4. モックの境界

モックは分離に不可欠ですが、使いすぎるとテストが実装の鏡になってしまいます。

重要な原則： すべての関数呼び出しではなく、アーキテクチャ上の境界でモックしてください。クエリ型の依存関係にはスタブ（あらかじめ用意したデータ）を使い、メールの送信や外部システムへの書き込みなど、副作用を伴うコマンドに限ってモック（やり取りの検証）を使ってください。

コードベースで既に採用されているモックの規約を尊重してください。普遍的なルールからの逸脱ではなく、プロジェクト内の不整合を指摘してください。

次の場合に指摘してください：

• テストが所有していない型（サードパーティーライブラリ、フレームワークの内部機能など）をモックしている — ライブラリが更新されると、古い前提に基づくモックが通ってしまいます。代わりに、薄いアダプター、統合テストでの実際の実装、または公式のテストユーティリティ（MemoryRouter、テスト用データベース、インメモリーキャッシュなど）を使ってください。
• モックの密度が高い — モックの設定が4つ以上ある場合、本番コードが多くの責務を抱えすぎている可能性があります。
• テストが内部メソッドの呼び出し回数や引数の順序を検証している（コードが「何をするか」ではなく、「どのように動作するか」にテストを結びつけてしまいます）
• スタブが検証されている（スタブが呼び出されたことを確認しても、振る舞いではなく実装をテストすることになります）

5. テストの匂い

匂い	どのような状態か	問題となる理由
アサーション・ルーレット	失敗メッセージのないアサーションが複数ある	デバッグしないと、どのアサーションが失敗したのか分かりません
欲張りなテスト	1つのテストで、関連のない複数のメソッドを実行している	失敗したとき、どの振る舞いに問題があるのか判断できません
怠惰なテスト	複数のテストが、同じメソッドを同じ入力で呼び出している	保守コストが重複する一方で、カバレッジは向上しません
眠たいテスト	ハードコードされたsleep()／Sys.sleep()／setTimeout()	CIでは不安定になり、どこでも実行が遅くなります。ポーリングまたは明示的な待機を使ってください。
腐ったグリーン	try／tryCatchまたは条件分岐内にアサーションがある	アサーションに到達しないため、テストが常に成功します
過敏な等価性判定	toString()／print()の出力を検証している	書式の変更で失敗します。代わりに、構造上のプロパティを検証してください
プリント文	アサーションの代わりにprint()／console.log()を使っている	デバッグ用の残骸であり、何も検証していません
スナップショットの乱用	振る舞いのアサーションの代わりにスナップショットを使っている	どんな変更でも失敗し、開発者が盲目的にスナップショットを更新してしまいます。適切な用途：HTMLコンポーネントの構造（すべてのプロパティの組み合わせではなく）を1つスナップショットにする、エラーメッセージのテキスト、CLIの出力。不適切な用途：オブジェクト全体のスナップショットを取る、またはすべてのバリエーションをレンダリングする。
実装の鏡像	本番コードと同じロジックで期待値を計算している	どちらも間違っている場合でさえ、テストコードと本番コードは必ず一致してしまいます。信頼できる情報源から得た期待値をハードコードしてください。

6. 命名と可読性

テスト名は実装ではなく、動作を説明する必要があります。適切に命名されたテストスイートは、機能仕様書のように読めます。

次の場合に指摘してください：

• テスト名が内部メソッド名を参照している（例：test_processData_returns_true）— 名前を変更するとテストが失敗します
• テスト名が汎用的である（test1、test_it_works、test_basic など）
• シナリオや期待される結果が名前から伝わらない

動作を表す名前を優先してください：test_expired_subscription_blocks_access、delivery_with_past_date_is_invalid、empty_cart_shows_zero_total。

7. カバレッジの漏れ

本番コードの変更とテストスイートを照合してください。

• テストで実行されていない分岐や条件はありませんか？
• エラー経路はテストされていますか？（単に「例外が発生するか」だけでなく、「適切な例外が発生し、正しくクリーンアップされるか」も確認してください）
• エッジケースはカバーされていますか？（空のコレクション、null/NA/None/undefined、境界値、該当する場合は並行アクセス）
• 本番コードで既存の動作を変更した場合、既存のテストは更新されていますか？

実装をたどり、判断が必要な箇所をすべて書き出してください。それぞれの if、match、switch、エラーハンドラー、または早期リターンについて、テストスイートで判断の両方の結果が検証されているか確認してください。

testthat を使った R のテストをレビューする際は、r-testthat スキルが利用可能か確認し、R 固有の規約やパターンに活用してください。

回答形式

## Summary
[Overall assessment: How well do these tests protect the codebase?]

## Critical Issues (Blocking)
[Tests that provide false confidence or will cause real problems.]

## Required Changes
[Design problems that weaken the test suite.]

## Strong Suggestions
[Improvements to test quality and maintainability.]

## Noted
[Minor style or convention issues. Mention once, then move on.]

## Verdict
Request Changes | Needs Discussion | Approve

## Next Steps
[Options for proceeding]

指摘ごとに file:line 形式で参照を記載してください。問題を示す具体的なテストコードを引用し、より適切なコード例を示してください。

次のステップ

レビューの最後に、次の選択肢をユーザーに提示してください。

指摘事項について話し合い、対応する： AskUserQuestion ツールを使って問題点を順に確認してください。重要度またはトピックごとにまとめ、解決方法を提示し、推奨する選択肢を明示してください。

問題を修正する： 優先度の高い順に、ブロッキングの問題、必須の変更、提案の順で直接修正することを提案してください。各グループの対応後、続行する前に確認してください。

プルリクエストに追加する： プルリクエストのレビューの場合は、レビューコメントを投稿することを提案してください。次の帰属表示を含めてください："Review assisted by the review-testing skill."

サブエージェントとして動作している場合は、次のステップを省略し、レビューの指摘事項のみを出力してください。