test: compareTimeStringsのゴールデンテストを追加(フェーズ1お手本PR) - #419
Conversation
フェーズ1(依存ゼロの純関数テスト)の最初の実装。api配下で初めての _test.go ファイルとして、ファイル配置・命名・テーブル駆動テストの書き方の実例にする。 設計書(docs/development/test-design/phase1-pure-functions.md)の10ケースをそのまま実装。要判断2件(空文字列→等価扱い、非数値だがコロンありの入力→0:00扱い)は現状挙動をそのまま固定し、issue #418で判断する。
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthrough
Changes時刻文字列比較テスト
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related issues
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
api/lib/usecase/shift_usecase_test.go (1)
5-9: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winGoDoc形式のコメントはガイドラインで禁止されています。
行5のコメントが関数名
TestCompareTimeStringsで始まっており、GoDoc形式に該当します。コーディングガイドラインではapi/**/*.goに対して GoDoc形式を禁止しています。本PRは後続テスト実装の例と位置付けられているため、ガイドラインに準拠した形式に修正することを推奨します。As per coding guidelines:
api/**/*.go: Use Japanese for code comments; do not use GoDoc format.♻️ 提案する修正
-// TestCompareTimeStrings は compareTimeStrings のゴールデンテスト。 +// compareTimeStrings のゴールデンテスト。 // 期待値は docs/development/test-design/phase1-pure-functions.md の // compareTimeStrings セクションに記載のケース表と、実行による裏取り結果に基づく。 // 「要判断」の付いたケースは現状の挙動をそのまま固定したもので、 // 挙動の是非は issue `#418` で判断する。🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@api/lib/usecase/shift_usecase_test.go` around lines 5 - 9, TestCompareTimeStrings のコメントをGoDoc形式にならないよう更新し、コメント冒頭を関数名以外の日本語表現に変更してください。既存のテスト目的、参照ドキュメント、判断保留ケースに関する説明は維持してください。Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@api/lib/usecase/shift_usecase_test.go`:
- Around line 5-9: TestCompareTimeStrings
のコメントをGoDoc形式にならないよう更新し、コメント冒頭を関数名以外の日本語表現に変更してください。既存のテスト目的、参照ドキュメント、判断保留ケースに関する説明は維持してください。
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: fcd96b8e-6b76-4f6a-8045-f0acaf3bdcb3
📒 Files selected for processing (1)
api/lib/usecase/shift_usecase_test.go
CodeRabbitの指摘どおりTestCompareTimeStringsコメントの関数名始まりを解消(AGENTS.mdのGoDoc形式禁止規約に準拠)。 また「コロンが2個以上」ケースが空文字列ケースと同じガード節(issue #418の論点1)を通るにもかかわらずissue参照が漏れていたため追加した。
概要
フェーズ1(依存ゼロの純関数テスト)の最初の実装。
compareTimeStrings(api/lib/usecase/shift_usecase.go:509)にゴールデンテスト(今のコードが実際に返している値を、そのまま「正解」としてテストに固定する手法。仕様書ではなく現状の動作を基準にする)を追加する。api配下で初めての_test.goファイルであり、以降メンバーが残り7関数(#420〜#426)を実装する際のお手本を兼ねる。背景(なぜこの実装なのか)
このPRはお手本を兼ねるため、機械的な変更内容だけでなく設計判断の理由も残す。
なぜこの関数を最初の実装対象に選んだか
フェーズ1(純関数、単体テスト)は依存先を偽装する必要が無く、誰の環境で実行しても結果が変わらない。一方フェーズ2(repository、実DB統合)は環境に結果が左右されやすい(このPRに先立つ調査で、ローカルに残っていた古いDBボリュームのせいで誤った挙動を観測しかけたことがあった)。メンバーへの最初の割り振りには環境依存の無いフェーズ1が適しており、その中でも戻り値が
intのみで比較しやすく、要判断も少ないcompareTimeStringsを選んだ。なぜ期待値がこの数字なのか
wantの値は、コードを読んで「こう動くべき」と推測したものではない。設計書(docs/development/test-design/phase1-pure-functions.md)作成時に、実際に使い捨てテストを実行し、今のコードが実際に返す値を観察してから記録したもの(ゴールデンテストの定義どおり)。たとえば"aa:bb"のケースが-1(0:00扱い)になるのは、strconv.Atoiのエラーが_で握りつぶされる現状のコードをそのまま反映した結果であり、「本来こうあるべき」という設計上の意図ではない。なぜ要判断2件をこのPRの中で直さなかったか
要判断とは、実装時に見つかった「これは仕様として固定してよい挙動か、それともバグとして直すべきか」がまだ決まっていない箇所のこと。「異常な入力を0扱いにすべきか、エラーにすべきか」は、
compareTimeStrings単体を読むだけでは決められず、呼び出し元のsort.Slice(shift_usecase.go:423-425)や、その先のシフトデータの整合性まで調査が要る。テストを書くPRとバグを直すPRを混ぜると、後から見た人が「これは現状を記録しただけか、意図的に挙動を変えたのか」をdiffから判断できなくなる。そのため、判断が必要な部分は現状の挙動をそのまま固定した上で、判断そのものを別issue(#418)に切り出した。変更内容
api/lib/usecase/shift_usecase_test.goを新規追加。docs/development/test-design/phase1-pure-functions.mdのcompareTimeStringsセクションに載っている10ケースを、そのままテーブル駆動テスト(入力と期待値の組み合わせをスライスで列挙し、1つのループで全パターンを検証するGoの定番の書き方)として実装した。<対象ファイル名>_test.gopackage usecase(テスト対象と同一パッケージ)。compareTimeStringsは小文字始まりのunexportedな関数で、Goでは同じパッケージ内のコードからしか呼び出せない。そのためテストファイルもpackage usecaseにして、直接呼べるようにしているcompareTimeStringsはfunc (a *shiftUseCase) compareTimeStrings(...)という、構造体のメソッドとして定義されている(この構造体をレシーバと呼ぶ)。ただし関数の中身はレシーバaのフィールドを一切参照していない純関数なので、テストでは中身が空のゼロ値&shiftUseCase{}を作ってメソッドを呼び出すだけで済む(DBやSlackへの接続を用意する必要が無い)name/time1/time2/wantをそのまま構造体スライスにし、t.Runでサブテスト化要判断2件は、設計書の記載どおり現状挙動をそのまま期待値にし、コメントで issue #418 にリンクした。
動作確認
go vet ./lib/usecase/...・golangci-lint run ./lib/usecase/...ともに指摘なし。関連
docs/development/test-design/phase1-pure-functions.mdSummary by CodeRabbit