test: CalendarControllerTest を実行日に依存しないよう修正する (#6390) - #7054
Open
ttokoro20240902 wants to merge 4 commits into
Open
test: CalendarControllerTest を実行日に依存しないよう修正する (#6390)#7054ttokoro20240902 wants to merge 4 commits into
ttokoro20240902 wants to merge 4 commits into
Conversation
testWeekendHolidaysStyle は当月の第一土曜日・第一日曜日のセルを #this-month-holiday-X で探していたが, その日が「今日」と重なると テンプレートが #today を優先するため要素が見つからず失敗していた. 毎月 2 日 (第一土曜・第一日曜) に発生する. - Carbon::setTestNow() と DataProvider で実行日を固定し, 月初の曜日と 今日が土日に重なるケースを網羅する - 対象日が今日の場合は #today を参照するようセレクタを切り替える - setTestNow() は tearDown() で戻す. メソッド末尾で戻すとアサーション 失敗時に到達せず, 後続のテストを巻き込むため - 月初からの曜日計算を firstOfMonth(曜日) に置き換え, 取得結果が 実際に土日かどうかをアサーションで担保する Refs #6390 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
CalendarController は「今日」の判定を Carbon::now() で行うが, テスト側は new \DateTime() で期待値を組み立てていた. 通常実行では一致するため表面化 しないが, Carbon::setTestNow() で時刻を固定すると描画側だけが固定日付に なり, 実時刻との差で必ず失敗する. 同クラスの他のテストは既に Carbon::now() を使っており, これだけが不揃い だった. 期待値を Carbon::now() に統一する. 2025-06 〜 2026-03 の各月について 1日 / 月末 / 月末前日 / 15日 の計 32 日で 時刻を固定して全テストを実行し, 全日で緑になることを確認した. Refs #6390 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
rector.php の YieldDataProviderRector / AttributeArgumentsOrderRector に追従する. - データプロバイダの return 配列を yield へ変更し戻り値を \Iterator に - #[DataProvider] の引数を名前付き引数へ Refs #6390 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughカレンダー表示テストに Carbon の時刻リセット処理を追加しました。週末休日テストを複数の日付ケースに対応させ、月初の土曜日・日曜日と当日表示を検証します。 Changesカレンダー表示テストの安定化
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: Poem
🚥 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@tests/Eccube/Tests/Web/Block/CalendarControllerTest.php`:
- Line 162: Update the testTodayStyle() method declaration to include the : void
return type, since the test does not return a value and must satisfy the
project’s PHP typing guidelines.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 5742b516-7b5e-4b5d-b563-d15866417deb
📒 Files selected for processing (1)
tests/Eccube/Tests/Web/Block/CalendarControllerTest.php
CodeRabbit の指摘に対応する. 本 PR で本文を変更したメソッドであり, 同じく書き換えた testWeekendHolidaysStyle には : void が付いているため 不揃いだった. 同クラスの他 5 メソッドは本 PR で触っていない既存コードのため対象外とする. Refs #6390 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## 4.4 #7054 +/- ##
==========================================
+ Coverage 77.66% 77.76% +0.10%
==========================================
Files 597 597
Lines 29339 29339
==========================================
+ Hits 22785 22816 +31
+ Misses 6554 6523 -31
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
概要(Overview・Refs Issue)
Refs #6390
CalendarControllerTestが 実行日に依存して失敗する 問題を修正します。testWeekendHolidaysStyleは当月の第一土曜日・第一日曜日のセルを#this-month-holiday-Xで探していますが、その日が「今日」と重なると
calendar.twigが#todayを優先するため要素が見つからず、InvalidArgumentException: The current node list is empty.で落ちます。Issue には「月初日が日曜日の場合の第一土曜日」とありますが、実測すると月初の曜日に関係なく
第一土曜日と第一日曜日の 2 日で発生します（毎月 2 日）。修正前のコードに時刻固定だけを入れて確認した結果:
方針(Policy)
Issue の「修正案」に沿いつつ、次の 4 点で構成しました。
Carbon::setTestNow()と#[DataProvider]で実行日を固定する — 月初の曜日 × 今日が土日と重なるか、の組み合わせを 7 パターン網羅します。CalendarControllerにloopCount === 28の専用分岐があるため、2 月がちょうど 28 日で日曜始まりのケースも含めています。#todayを参照する —calendar.twigの ID 優先順位（#today-and-holiday→#today→#this-month-holiday-X）に合わせます。setTestNow()のリセットはtearDown()で行う — Issue の修正案にある「メソッド末尾でリセット」は、途中のアサーションが失敗すると到達せず、同一プロセスの後続テストを巻き込みます。実際に再現用パッチを当てた状態でクラス全体を流したところ、testWeekendHolidaysStyleではなくtestTodayStyleが失敗しました。firstOfMonth(曜日)に置き換える — 元のif/elseif7 分岐と「日曜の前日が今月かどうか」の判定が不要になります。実装に関する補足(Appendix)
ガード用のアサーションを追加しています。 このテストは期待値と CSS セレクタを同じ変数から組み立てているため、
日付の取得自体が誤っていてもアサーションが通ってしまいます（元のコードからある性質です）。
計算方法を置き換えた以上そのままにはできないので、取得結果が本当に土曜／日曜で 7 日以内かを検証しています。
testTodayStyleも併せて修正しています。 こちらは Issue の範囲外ですが、期待値をnew \DateTime()で組み立てており、描画側の
Carbon::now()と不揃いでした。通常実行では一致するため表面化しませんが、時刻を固定した瞬間に必ず壊れます。同クラスの他のテストは既に
Carbon::now()を使っており、これだけが例外でした。修正範囲は実測で決めました。
setUp()に一時的なハーネスを入れて外から時刻を与え、2025-06〜2026-03 の各月について 1 日 / 月末 / 月末前日 の計 30 日でクラス全体を実行したところ、
全日で落ちるのは
testTodayStyleだけでした。testHolidayStyleの土日回避ロジックは見た目こそ複雑ですが月末・年境界・2 月末を含む全日で正しく動いていたため、手を付けていません。
テスト（Test)
コードスタイル / 静的解析 / リファクタ規約 / ユニットテストの 4 ゲートをローカルで通しています。
src)YieldDataProviderRector/AttributeArgumentsOrderRectorに追従済み）tests/Eccube/Tests/Web/Block/修正が実際にゲートとして機能するかを、部分ごとに外して確認しました。
#today対応）を外すtearDown()のリセットを外すtestTodayStyleが失敗（setTestNowの漏れ）時刻を固定した通し確認も行っています。
tearDown()のリセットが効いていること--filter単体実行で緑 — 他テストへの依存がないことdtb_calendarは実行前後とも 0 件で汚染なし相談（Discussion）
testTodayStyleの整合修正」「rector 追従」の 3 本に分けています。まとめたい場合は squash してください。testHolidayStyleは上記のとおり実測では全日通るため今回は触っていません。日付固定に揃えるべきかはご判断ください。マイナーバージョン互換性保持のための制限事項チェックリスト
テストコードのみの変更で、
src/には一切手を入れていません。レビュワー確認項目
Summary by CodeRabbit