ローカル環境で通らなかったテストnotify comment and checkを通るように修正#9027
Conversation
Walkthroughテストヘルパーで作成後の画面ヘッダ検証を追加し、作成済レポートIDをDB参照から現在のパスの正規表現抽出へ変更。通知システムテストは Changes
Sequence Diagram(s)sequenceDiagram
actor Tester as テスト
participant Browser
participant App
Tester->>Browser: create_report(title, description, wip)
Browser->>App: POST /reports
App-->>Browser: Redirect /reports/{id} or /reports/{id}/edit
Browser-->>Tester: current_path を取得
Tester->>Tester: path から report_id を抽出
Tester->>Browser: Visit /reports/{report_id}
Browser->>App: GET /reports/{report_id}
App-->>Browser: レポートページ（ヘッダ/タイトル表示）
Tester->>Browser: Post comment
Browser->>App: POST /reports/{report_id}/comments
Tester->>Browser: Logout -> Visit /
Browser->>App: GET /
Tester->>Browser: Open notifications
Browser->>App: GET /notifications
App-->>Browser: 通知を表示
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Possibly related issues
Suggested reviewers
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 RuboCop (1.76.1)test/supports/report_helper.rbrubocop-minitest extension supports plugin, specify Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. 📜 Recent review detailsConfiguration used: .coderabbit.yaml 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
tyrrell-IH
force-pushed
the
chore/fix-notify_comment_and_check-test
branch
from
6b9b512 to
7929cb4
Compare
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
1 similar comment
✅ Actions performedReview triggered.
|
|
@tyrrell-IH 私の環境でもこのテストを10回以上実行しましたが、すべてパスしましたので、問題自体は修正できていると思います。 ただ、保守性や実行時間、テストの安定性の観点から 駒形さんに確認したところ、 修正案は考えつけていないのですが、私が試したこと、修正の未熟なアイデアを共有します。 試してダメだったこと
を に変更（日報提出ボタンをクリック後、その日報のページに遷移するのでそれを待機する）してみましたが、flakyなので却下 （追記） 
検討中の案実装は未着手でアイデア段階です🙇🏻♂️
懸念点は
|
|
@ryufuta 修正方針1
でDOM構築を待つ 修正方針2
試してダメだったことの件について私の環境で エラーが発生しないこと、 そもそもryufuta さんのエラー内容と 添付いただいた画像からも 以上より、なぜryufutaさんの環境でそのエラーが発生したのかが気になります🤔 report_idを取得する方法確かに 修正案として、当初
このやり方は結構いいなと思ったのですが、
あまりいい方法は思いつかなかったのですが、日報作成後に ただ、これも
という点でいい方法なのかどうかあまり自信はなく、場合によっては再度駒形さんに相談することになるかもしれません。 この辺もご意見いただきたいです〜 |
tyrrell-IH
changed the title
tyrrell-IH
force-pushed
the
chore/fix-notify_comment_and_check-test
branch
from
cae8f04 to
696e06a
Compare
|
@tyrrell-IH 結論から言うと、これで大丈夫だと思います。 試してダメだったことの件について最新のブランチをpullして私の環境でもテストを実行してみました。 エラーの内容と今回の修正が関係なさそうというのはご指摘の通りだと思います。 エラーメッセージを改めて見ると、 report_idを取得する方法こちら、pathからidを取得する今の方法の方が良いと思うので大丈夫です。（思い付かなかった。勉強になります）
リダイレクトする動作に依存する点について。 正規表現を使用している点について。 |
|
@ryufuta
なんとなくエラーの内容的に今回のwebp(avatar_url)に絡みそうかなと思ったので、再発したらまた教えてください🙏 不安な点については駒形さんにレビュー出す時に伝えたいと思います！ |
![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
🧹 Nitpick comments (1)
test/system/notifications_test.rb (1)
261-271: ジョブ実行範囲の最小化＋パスヘルパーの利用で可読性・意図の明確化
perform_enqueued_jobsは通知生成に関わる操作（コメント/確認OK）に限定すると意図が明確で副作用も減らせます。その後のログアウト・通知UIの確認はジョブ実行に関係ないためブロック外へ。- 文字列連結のパス指定は、
report_path/root_pathを使うとルーティング変更に強くなります。- perform_enqueued_jobs do - visit_with_auth "/reports/#{report_id}", 'komagata' - assert_selector 'h1.page-content-header__title', text: 'コメントと' - fill_in 'new_comment[description]', with: 'コメントと確認した' - click_button '確認OKにする' - logout - visit_with_auth '/', 'hatsuno' - assert_selector 'h2.page-header__title', text: 'ダッシュボード' - find('.header-links__link.test-show-notifications').click - assert_text 'hatsunoさんの日報「コメントと」にkomagataさんがコメントしました。' - end + perform_enqueued_jobs do + visit_with_auth report_path(report_id), 'komagata' + assert_selector 'h1.page-content-header__title', text: 'コメントと' + fill_in 'new_comment[description]', with: 'コメントと確認した' + click_button '確認OKにする' + end + logout + visit_with_auth root_path, 'hatsuno' + assert_selector 'h2.page-header__title', text: 'ダッシュボード' + find('.header-links__link.test-show-notifications').click + assert_text 'hatsunoさんの日報「コメントと」にkomagataさんがコメントしました。'
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
test/supports/report_helper.rb(2 hunks)test/system/notifications_test.rb(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.rb
⚙️ CodeRabbit Configuration File
**/*.rb: # refactoring
- まずFat Controllerを避け、次にFat Modelを避ける。
- Serviceクラスの乱用を避ける。
- controller concernを作ろうとしたらPORO(Plain Old Ruby Object)やActiveRecordモデルでの実装で代替できないか検討する。
Rails Patterns
- ViewHelperにメソッドを実装する時にはまずDecoratorパターンを使うことを検討する。（active_decorator gemを導入しているのでそれを使う）
- 複雑なActiveRecordクエリがあり、再利用できそうな場合はQueryObjectパターンを検討する。（rails-patterns gemを導入しているのでそれのQuery機能を使う）
- Viewにpartialを作る場合はViewComponentを使うことを検討する。
- 複数のActiveRecordモデルを操作する１つの責務がある時や外部APIとやりとりする処理がある場合にはInteractorオブジェクトパターンを検討する。（interactor gemを導入しているのでそれを使う）
- 複数のInteractorを実行するような処理がある場合Organizerオブジェクトパターンを検討する。（interactor gemを導入しており、その中にOrganizerの機能があるので使う）
Files:
test/supports/report_helper.rbtest/system/notifications_test.rb
test/**/*
⚙️ CodeRabbit Configuration File
test/**/*: # Test
- TestCase名は英語で書く。
- どうしても避けられない時以外にsystem testでsleepは使わない。
Unit Test
model, helper, decorator, view_componentについてはメソッドを追加した場合は必ず対応したUnit TestのTestCaseを1つは書く。
Files:
test/supports/report_helper.rbtest/system/notifications_test.rb
🧠 Learnings (1)
📚 Learning: 2025-07-23T20:31:13.856Z
Learnt from: jun-kondo
PR: fjordllc/bootcamp#8977
File: app/controllers/reports_controller.rb:63-63
Timestamp: 2025-07-23T20:31:13.856Z
Learning: fjordllc/bootcampプロジェクトの`app/controllers/reports_controller.rb`において、`create`と`update`アクションは両方とも`report.save_uniquely`を使用し、同じ`:report_save`イベントと`'report.save'`イベントを発行する。これは両方とも本質的に「レポートの保存」操作であり、作成と更新を区別する必要がないためである。
Applied to files:
test/supports/report_helper.rb
🧬 Code Graph Analysis (1)
test/system/notifications_test.rb (1)
test/supports/report_helper.rb (1)
create_report(4-23)
🔇 Additional comments (2)
test/supports/report_helper.rb (1)
6-6: 新規ページのDOM構築待ちの追加は妥当
visit new_report_path直後にヘッダの存在を待つことで、以降の操作の安定性が上がっています。test/system/notifications_test.rb (1)
258-259: ダッシュボード表示の待機追加は適切ログイン直後のヘッダを待つことで、その後の作業（レポート作成）を安定させています。意図に合致しており問題ありません。
tyrrell-IH
force-pushed
the
chore/fix-notify_comment_and_check-test
branch
from
696e06a to
27a87d0
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
test/supports/report_helper.rb (1)
17-21: OR待機の除去とwIP/提出での明示的なassert分岐はGood
has_selector?のOR待機からassert_selectorの分岐へ置き換えられており、失敗時の診断性と安定性が向上しています。過去コメントの提案どおりで問題ありません。
🧹 Nitpick comments (3)
test/supports/report_helper.rb (3)
20-20: 提出時の見出し待機にも明示的なwaitを付与してフレーク低減wip側に
wait: 5を指定しているのに対し、提出側は未指定です。環境差やCI負荷時の遅延を考慮して、提出側にも wait を付けておくと安定します。- assert_selector 'h1.page-content-header__title', text: title + assert_selector 'h1.page-content-header__title', text: title, wait: 5
6-6: 新規作成ページのヘッダ確認にもwaitを指定して一貫性を確保初期表示もDOM構築が遅れる可能性があるため、他の箇所と同様に
waitを付与するのが無難です。- assert_selector 'h2.page-header__title', text: '日報作成' + assert_selector 'h2.page-header__title', text: '日報作成', wait: 5
24-26: ID抽出は安全になっていてOK。変数名をMatchDataに即したものへ微調整処理自体は安全で正しいです。変数名が「id」を連想させるため、実体（MatchData）に即した名前へ変えると読みやすくなります。
- report_match_id = current_path.match(%r{^/reports/(\d+)(/edit)?$}) - assert report_match_id, "Unexpected path after creating report: #{current_path}" - report_match_id[1].to_i + path_match = current_path.match(%r{^/reports/(\d+)(/edit)?$}) + assert path_match, "Unexpected path after creating report: #{current_path}" + path_match[1].to_i
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
test/supports/report_helper.rb(2 hunks)test/system/notifications_test.rb(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- test/system/notifications_test.rb
🧰 Additional context used
📓 Path-based instructions (2)
**/*.rb
⚙️ CodeRabbit Configuration File
**/*.rb: # refactoring
- まずFat Controllerを避け、次にFat Modelを避ける。
- Serviceクラスの乱用を避ける。
- controller concernを作ろうとしたらPORO(Plain Old Ruby Object)やActiveRecordモデルでの実装で代替できないか検討する。
Rails Patterns
- ViewHelperにメソッドを実装する時にはまずDecoratorパターンを使うことを検討する。（active_decorator gemを導入しているのでそれを使う）
- 複雑なActiveRecordクエリがあり、再利用できそうな場合はQueryObjectパターンを検討する。（rails-patterns gemを導入しているのでそれのQuery機能を使う）
- Viewにpartialを作る場合はViewComponentを使うことを検討する。
- 複数のActiveRecordモデルを操作する１つの責務がある時や外部APIとやりとりする処理がある場合にはInteractorオブジェクトパターンを検討する。（interactor gemを導入しているのでそれを使う）
- 複数のInteractorを実行するような処理がある場合Organizerオブジェクトパターンを検討する。（interactor gemを導入しており、その中にOrganizerの機能があるので使う）
Files:
test/supports/report_helper.rb
test/**/*
⚙️ CodeRabbit Configuration File
test/**/*: # Test
- TestCase名は英語で書く。
- どうしても避けられない時以外にsystem testでsleepは使わない。
Unit Test
model, helper, decorator, view_componentについてはメソッドを追加した場合は必ず対応したUnit TestのTestCaseを1つは書く。
Files:
test/supports/report_helper.rb
🧠 Learnings (1)
📚 Learning: 2025-07-23T20:31:13.856Z
Learnt from: jun-kondo
PR: fjordllc/bootcamp#8977
File: app/controllers/reports_controller.rb:63-63
Timestamp: 2025-07-23T20:31:13.856Z
Learning: fjordllc/bootcampプロジェクトの`app/controllers/reports_controller.rb`において、`create`と`update`アクションは両方とも`report.save_uniquely`を使用し、同じ`:report_save`イベントと`'report.save'`イベントを発行する。これは両方とも本質的に「レポートの保存」操作であり、作成と更新を区別する必要がないためである。
Applied to files:
test/supports/report_helper.rb
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build_and_test
|
@tyrrell-IH @komagata |
test/supports/report_helper.rb
Outdated
| click_button(wip ? 'WIP' : '提出') | ||
|
|
||
| if wip | ||
| assert_selector 'h2.page-header__title', text: '日報編集', wait: 5 |
There was a problem hiding this comment.
下記でデフォルト15秒にしているので逆に短くしているように思いますが、こちらは意図通りでしょうか。
https://github.com/fjordllc/bootcamp/blob/main/test/test_helper.rb#L14
There was a problem hiding this comment.
@komagata
意図して書きました、
wipがfalseの場合
if wip
assert_selector 'h2.page-header__title', text: '日報編集', wait: 5
の判定（提出ボタンを押した場合には全てこの判定を通る）に15秒待つのは長すぎると思いました。
waitを3秒とか5秒とか10秒とかに変更しつつ、30回くらいテストを回してみて現状確実にテストが通る最低の時間が5秒という感じです。
tyrrell-IH
force-pushed
the
chore/fix-notify_comment_and_check-test
branch
from
27a87d0 to
ce8ce9d
Compare
|
@komagata とCodeRabbitよりNitPickで指摘のあった事項
に関する修正も行なっています。 また改めてこのPRを作成した経緯について説明します。 ただ、PRの内容の精査、descriptionの作成、レビューに出すタイミング、レビューへの対応が、私の時間的な制約上管理できない可能性があったため、ryufutaさんへ「自由にコミットしたり、好きにしていいです」という合意のもと叩き台としてのPRを作成しました。 結果的には今回のPRは私が実装の全てを担っているので、私がPR作成者、ryufutaさんがレビュアーとなっていますが、私としては共同でPRを作成しているという認識なので、私が対応できない時はryufutaさんがレビューの対応、再レビュー依頼をすることになると思いますので、その旨ご了承ください🙏 |
3 participants
Issue
概要
issueの通りです。テストが通るように修正しました。
修正のポイントは以下の通りです。
Report.last.idを実行すると違う日報のidを返してしまうので、日報の保存を待つ処理を追加visit_with_authを使ってログイン状態を切り替える際に、logoutを利用し明示的にログインセッションを破棄することでログイン状態を切り替えるvisit等で画面を移動する際に、assert_selectorを使って移動先のDOMが構築されるのを待つ変更確認方法
chore/fix-notify_comment_and_check-testをローカルに取り込み、ブランチを切り替える。Screenshot
内部的な修正のため無し。
Summary by CodeRabbit
Summary by CodeRabbit