Conversation
Walkthroughシステムテストの日本語テスト名が英語に置換され、メンション対象ログイン名が1箇所変更されました。テスト支援ヘルパー群にページ表示の明示的アサーションと待機ロジック（通知ページヘッダー待ち、ボタン待ち、レポート作成完了待ち）が追加されました。既存の公開API署名は一部メソッドで微調整されましたが、通知本体の実装変更はありません。 Changes
Sequence Diagram(s)sequenceDiagram
participant Test as SystemTest
participant Helper as ReportHelper / NotificationHelper / LoginHelper
participant Browser as Browser/Page
Test->>Helper: 呼び出し (例: create_report / exists_unread_notification / login_user)
Helper->>Browser: visit ページ
Browser-->>Helper: ページレンダリング
Helper->>Browser: assert ヘッダー要素待機 (例: 日報作成 / 通知 / ログイン)
alt 要素出現
Browser-->>Helper: 要素存在
Helper->>Helper: 必要に応じて wait/poll (Report.count 等)
Helper-->>Test: 成功/戻り値
else タイムアウト
Browser-->>Helper: 要素未検出
Helper-->>Test: 失敗 (テストアサーション)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~15 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/system/notification/reports_test.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 UI 💡 Knowledge Base configuration:
You can enable these settings 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
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
test/system/notification/reports_test.rb (1)
126-149: 末尾の余分な空白を除去してくださいテストタイトルの末尾にスペースが 1 つ残っています。
IDE によってはテスト名の一意性判定や検索に影響する場合があるため、削除を推奨します。-test 'no notification if report already posted ' +test 'no notification if report already posted'
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
test/system/notification/reports_test.rb(5 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: hirokiej
PR: fjordllc/bootcamp#8740
File: app/helpers/reports_helper.rb:56-64
Timestamp: 2025-06-29T03:44:15.179Z
Learning: このプロジェクト（fjordllc/bootcamp）では、ja.ymlファイルで一部の単語や文章のみI18n対応されているが、多くのテキストは日本語でハードコーディングされており、完全な国際化対応は行われていない。新しい機能でもI18n対応は不要と判断される。
test/system/notification/reports_test.rb (1)
Learnt from: masyuko0222
PR: fjordllc/bootcamp#8277
File: test/models/searcher_test.rb:0-0
Timestamp: 2025-07-07T12:46:01.631Z
Learning: テストにおいて、`result.all? { |condition| }` でコレクションの全要素が条件を満たすことを確認できれば、その条件を満たさない要素が含まれていないことも論理的に保証されるため、追加のアサーションは冗長となる。
🔇 Additional comments (4)
test/system/notification/reports_test.rb (4)
183-198: 英語化 👍日本語タイトルからの置き換えのみでロジックへ影響なし。
命名も十分に意味が通るためこの変更は問題ありません。
200-216: 英語化 👍同上。テスト名は一意でわかりやすくなっています。
218-230: 英語化 👍同上。
特記事項はありません。
232-244: 英語化 👍同上。テスト名が簡潔で、一貫性も保たれています。
karlley
force-pushed
the
chore/fix-japanese-test-title
branch
from
6f62965 to
9c05c8d
Compare
|
お疲れさまです！ |
|
@karlley |
|
テストの動作確認ありがとうございます！
やはりエラーが出てしまうんですね💦 |
|
@karlley
了解です😊 |
色々と検証ありがとうございます！！ |
|
@karlley
CIのcheckが通らなくなってしまったので、原因解明します。 |
tyrrell-IH
force-pushed
the
chore/fix-japanese-test-title
branch
4 times, most recently
from
edf58c2 to
1676af4
Compare
|
@karlley 修正した箇所が他のテストに影響してしまい、その調整に手間取ってしまったので遅くなりました🙏 karlleyさんの環境でテストが通らないor通りづらい等ありましたら教えてください また、以下に修正の内容を簡単に書きました。 修正内容の説明パスワードマネジャーの警告回避commit: 2a4ae56 user: 警告メッセージ回避のため assert_selectorでDOMの構築を待つcommit: c77a917
等の処理が走ってエラーになっていました。 Reportの保存を待ってidを返す追記: メインの方で別の実装がマージされたので以下の変更は削除 commit: 508ba04 日報の提出ボタンを押した後すぐに
待機時間の調整commit: 1676af4 現在 待機時間をDOM構築に必要と思われる時間に調整しました。 2025/8/9 12:00 追記
なので私の修正は「単体でのテストは通るようにした」という修正になります🙏 |
|
お疲れさまです。 |
|
お疲れさまです。 |
|
@karlley |
matuaya
left a comment
matuaya
left a comment
There was a problem hiding this comment.
私の環境では一番最後のテストtest/system/notification/reports_test.rb:232のみFailしてしまいました。karlleyさんの環境では問題ないとのことなので、やっぱり私の環境がいつも通りテストと相性悪いんだと思います💦
ちょっと英語のニュアンスの部分でコメントさせていただきました🙏
検討していただけたら嬉しいです！
| end | ||
|
|
||
| test '研修生が日報を作成し提出した時、企業のアドバイザーに通知する' do | ||
| test 'notify company advisor only on first report posted' do |
There was a problem hiding this comment.
ちょっと細かい英語表現の話になってしまうのですが💦
この文脈での"on first report posted"は実は結構曖昧で、
- 「日報が初めて投稿された時だけ通知して、同じ日報が編集などで再投稿された場合は通知しない」という意味と、
- 「一番最初に投稿された日報だけ通知して、その後に投稿される他の日報は通知しない」
という意味のどっちにも解釈できてしまうんです。
テストの内容を見るまではどっちなのかちょっと判断つかない部分があるので、パッとテスト名だけで内容をわかるようにするには、例えば
- notify company advisor only when report is initially posted
だと一目でテストの内容を把握できるんじゃないかなと思います！ - notify company advisor only when report is first posted
なんかもいいと思います。
他のテスト名についても同様に検討していただければと思います🙏
There was a problem hiding this comment.
なるほどです！
on とwhen のニュアンスの違いを理解できていませんでした💦
以下のようなニュアンスで認識しましたが正しいでしょうか？
on: 出来事が起きたタイミング(条件は無関係)when: 状況/条件が揃ったタイミング
また、以下のように修正しましたのでご確認お願いします。
183:notify company advisor only on first report posted
183:notify company advisor only when report is initially posted
200:notify follower only on first report posted
200:notify follower only when report is initially posted
218:notify mention target only on first report posted
218:notify mention target only when report is initially posted
232:notify user only on first report posted
232:notify user only when report is initially posted
There was a problem hiding this comment.
修正ありがとうございます！
「onとwhenの違い」についてですが、ここでの文脈でのonとwhenはどちらも出来事のタイミングを指しているので、細かいニュアンスの違いはなくほとんど同じような感じがします。
なのでonでもいけそうな感じがするのですが、あまり使わないというか違和感が残ってしまう感じがします💦
もしonを使用するとしたら、"Notify only on initial posting of report"だといけるのですが、あまり自然じゃない気がします💦
There was a problem hiding this comment.
回答ありがとうございます！
onでもいけそうな感じがするのですが、あまり使わないというか違和感が残ってしまう感じがします💦
なるほどです。英語があまり得意でないことと感覚的な部分になりそうなので難しいですね😭
今後はon やwhen などは意識的に慎重に選ぶように気をつけようと思ます！
| end | ||
|
|
||
| test '初日報は初めて公開した時だけ通知する' do | ||
| test 'notify user only on first report posted' do |
There was a problem hiding this comment.
これも細かい話になってしまうのですが・・・
これもちょっと曖昧で、
確かに「初日報のみ通知する」とも読み取れるですが、その場合は「初投稿のみ通知して再投稿は通知しない」というニュアンスが伝わりにくくなってしまいそうです。
なので、「初日報の初投稿の時のみ通知する」という意味が伝わると、テスト名がより内容を正確に反映できるんじゃないかなと思います！
|
レビューありがとうございます！
承知しました！ |
matuaya
left a comment
matuaya
left a comment
There was a problem hiding this comment.
Approveさせていただきます😊
最後に一点だけ、”notify user only when report is initially posted”に関してですが、「”初日報”の初投稿」というニュアンスが消えてしまったので、
”notify user only when first report is initially posted”
のほうが、日本語のテスト名”初日報は初めて公開した時だけ通知する”により近くなるかもしれないです！
動作には関係ないので細かすぎるのかな・・とも思うのでもし余裕があればご検討いただけたら嬉しいです🙏
すみません、初日報の部分が抜けてました💦 |
|
@karlley |
tyrrell-IH
force-pushed
the
chore/fix-japanese-test-title
branch
2 times, most recently
from
3157735 to
f71722a
Compare
|
返信遅くなりすみませんでした！ |
karlley
force-pushed
the
chore/fix-japanese-test-title
branch
from
f71722a to
bf7efdf
Compare
|
@komagata |
test/supports/login_helper.rb
Outdated
| has_selector?('h2.page-header__title', text: 'ダッシュボード', wait: 3) || | ||
| has_selector?('h1.auth-form__title', text: 'ログイン', wait: 3) |
There was a problem hiding this comment.
wait: 3の部分ですが下記のところがデフォルトを15にしているので逆に短くなっちゃってるかもです。（それが狙いだったらすみません）
There was a problem hiding this comment.
@komagata
私の方でcommitした内容なので、karlleyさんに許可を得た上で回答させていただきます🙏
やり取り
例えば
has_selector?('h2.page-header__title', text: 'ダッシュボード'がfalseで
has_selector?('h1.auth-form__title', text: 'ログイン'がtrue
になるようなテスト（ログイン権限がないユーザーがログインしようとして確実にはじかれることの確認、等）の場合に
has_selector?('h2.page-header__title', text: 'ダッシュボード'の確認で15秒待つのは長すぎるように感じました。
テストの試行を繰り返すうちにwait: 3程度ならテストが問題なく通ることを確認したので、wait: 3に設定しました。
There was a problem hiding this comment.
@tyrrell-IH なるほどです。意図的に設定されていたんですね。
必ず15秒待つわけではなく、要素が見つかった場合はすぐ終わるはずです。
（見つからない場合にMAXで15秒待つ）
そして、MAXでもそこまで待たなくて良いという意図があるのであれば、そういったものは他にも膨大にあると思うのでここだけ変更するとわかりづらいように思いました。
15秒の部分を変えることを検討するか、例えば短い版の秒数を定義して設定するかなど（あんまりやらない方がいい気がしますが）の方がいいかもと思いました。
tyrrell-IH
force-pushed
the
chore/fix-japanese-test-title
branch
from
d5d0dcc to
f494f2a
Compare
|
@karlley 3806358 でパスワードマネージャのモーダルが表示されないよう対応していますが、根本的な解決策ではないので別の方法を提案します。 class ApplicationSystemTestCase < ActionDispatch::SystemTestCase
# ...
driven_by(:selenium, using: :headless_chrome) do |driver_option|
driver_option.add_argument('--no-sandbox')
driver_option.add_argument('--disable-dev-shm-usage')
driver_option.add_argument('enable-blink-features=Clipboard')
driver_option.add_preference('profile.password_manager_leak_detection', false) # この行を追加
end
# ...私の方で別PRで対応しても良いのですが、軽微ですし、動作確認手順が無駄に面倒になる（本PRがマージ後に上記のコミットをrevertしてもテストがパスすることを確認する）ため、こちらのPRで対応をお願いしたいと思っています🙏 あと、本PRはもとのIssueの範囲を逸脱しているので、PRのタイトルと概要を変更した方が良さそうですー |
test/supports/login_helper.rb
Outdated
| assert( | ||
| has_selector?('h2.page-header__title', text: 'ダッシュボード') || | ||
| has_selector?('h1.auth-form__title', text: 'ログイン') | ||
| ) |
There was a problem hiding this comment.
assertをifのように使っているのがちょっとトリッキーに見えてこの場所が気になりました。
ログインしてログインページに遷移してしまう場合は何か問題が起きてると思うので、ログイン自体のテスト以外ではログインページに飛んでしまうのはエラーにすべきかなと思いました。
|
情報の共有ありがとうございます！
承知しました！ 進め方の結果次第にはなりますが、少なくとも私かtyrrellさんが対応することになるとは思っています。
ご指摘ありがとうございます！ |
|
@komagata 今後の本PRの方針について本PRは現在
が混在したPRになっています。 このPRについて
という方針で進めさせいただきたいですがよろしいでしょうか？ なぜその方針にしたいのか当初私の見込みでは軽微な修正で済む予定でしたので、本PRの中でテスト修正のコミットを積ませていただいておりました、。 今回のコメントの対応を本PR上で行うと「テストを通るようにする修正」の項目がさらに多くなる見込みなので、上記コメントを機に別途Issueとして対応した方が良いと考えました。 karlleyさんには以上の方針について相談のうえ了承を得ていますので、ご検討のほどよろしくお願いいたします🙏 |
|
@tyrrell-IH はい、そちらの方向で大丈夫です〜 |
tyrrell-IH
force-pushed
the
chore/fix-japanese-test-title
branch
from
f494f2a to
e2a7318
Compare
tyrrell-IH
force-pushed
the
chore/fix-japanese-test-title
branch
from
e2a7318 to
95f31c3
Compare
5 participants
Issue
概要
日本語のテスト名を英語に変更
変更確認方法
chore/fix-japanese-test-titleをローカルに取り込むtest/system/notification/reports_test.rb内の以下のテスト名が英語になっていることを確認Summary by CodeRabbit