Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

test/helpers配下をテストファイルとして認識させた #8007

Merged
merged 2 commits into from
Aug 21, 2024

Conversation

thmz337
Copy link
Contributor

@thmz337 thmz337 commented Aug 10, 2024

Issue

概要

test/helpers配下のファイルが、_test.rbで終わっていないため、テストファイルとして認識されていない。
test/helpers配下のファイル名を、_test.rbの形にリネームした

変更確認方法

  1. bug/recognize-as-test-fileをローカルに取り込む
  2. bin/rails test test/helpersを実行し、test/helpers配下のファイル群がテストとして認識されていることを確認する。また、テストが通ることを確認する。

Screenshot

変更前

❯ bin/rails test test/helpers 
Running via Spring preloader in process 4982
Run options: --seed 30100

# Running:


[Minitest::CI] Generating test report in JUnit XML format...


Finished in 0.000670s, 0.0000 runs/s, 0.0000 assertions/s.
0 runs, 0 assertions, 0 failures, 0 errors, 0 skips

変更後

❯ bin/rails test test/helpers 
Running via Spring preloader in process 4712
Run options: --seed 7270

# Running:

....................
[Minitest::CI] Generating test report in JUnit XML format...


Finished in 2.178230s, 9.1818 runs/s, 25.7089 assertions/s.
20 runs, 56 assertions, 0 failures, 0 errors, 0 skips

@thmz337 thmz337 self-assigned this Aug 10, 2024
@thmz337 thmz337 marked this pull request as ready for review August 10, 2024 13:04
@thmz337 thmz337 requested a review from Judeeeee August 10, 2024 13:06
@thmz337
Copy link
Contributor Author

thmz337 commented Aug 10, 2024

@Judeeeee

お疲れ様です。レビューをお願いできますでしょうか。
難しい場合は遠慮なくおっしゃってください。

よろしくお願いします。

@Judeeeee
Copy link
Contributor

@thmz337
お疲れ様です!承知しました〜
1週間目処にレビューできるかと思います🙆
もしお急ぎであれば、他の方に依頼いただければと思います🙏

Copy link
Contributor

@Judeeeee Judeeeee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@thmz337

すぐ終わりそうだったので、早速レビューさせていただきました🙏

動作確認としてはOKです!
ただ、レビューをしていてPRの作り方が気になったので、コメントしますね👌

PRのタイトル

「Bug/recognize as test file」というタイトルだと、何を行ったPRかが分かりにくいと感じました😢
もし私なら、やっていることを一言で表したいので、「test/helpers配下をテストファイルとして認識させた」にする気がします👀
是非他の方のPRタイトルも参考にしてみてください〜!

PRの概要

「test/helpers配下のファイルが、_test.rbで終わっていないため、テストファイルとして認識されていない。」だけだと、「なるほど。それで、このPRでは何をしたわけ...?」と突っ込まれる恐れがあるため、commitメッセージだけではなくPR概要で補足してあげると親切だと思います🙆‍♀️

丁寧に書くなら、今回やったことは

  • test/helpers配下のファイル名を、_test.rbの形にリネームした
  • test/helpers配下のテストをパスさせるためにテストファイルを修正

と書けそうです💪

Screenshot

変更前後でどこが変わったか分かりづらく感じました🤔
もし画像で示すなら、変更箇所を赤枠で囲むなどして目立たせるとよりよく伝わると思います👍

また、ターミナルの出力を示すなら、コピペで貼り付けた方がより見やすいと思います〜
Markdownで、「```」を使ってテキストを囲むと良さそうです!

テキスト

変更確認方法

凄く細かいことなので恐縮なのですが、

bin/rails test test/helpersを実行し、

と記述してあるのに、Screenshotの画像ではPARALLEL_WORKERS= 1 bin/rails test test/helpersと矛盾しているのが気になりました💦

@thmz337 thmz337 changed the title Bug/recognize as test file test/helpers配下をテストファイルとして認識させた Aug 10, 2024
@thmz337
Copy link
Contributor Author

thmz337 commented Aug 11, 2024

@Judeeeee

早速の確認ありがとうございました。
ご指摘の箇所を修正しましたので、よろしくお願いします。

Copy link
Contributor

@Judeeeee Judeeeee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@thmz337
修正ありがとうございます🙏
Approveさせていただきます。

@thmz337 thmz337 requested a review from komagata August 11, 2024 04:09
@thmz337
Copy link
Contributor Author

thmz337 commented Aug 11, 2024

@komagata

お疲れ様です。確認よろしくお願いします。

Copy link
Member

@komagata komagata left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

確認させて頂きました。OKです〜🙆‍♂️

@komagata komagata merged commit 7149ee3 into main Aug 21, 2024
12 checks passed
@komagata komagata deleted the bug/recognize-as-test-file branch August 21, 2024 02:05
@github-actions github-actions bot mentioned this pull request Aug 21, 2024
17 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants