テスト環境でユーザーとアイコン画像が紐付けされるようにした #5096
Conversation
tomonariha
force-pushed
the
bug/link-userdata-and-images-in-test-environment
branch
from
c3d4b35 to
8961855
Compare
|
@tksmasaki さん、お疲れ様です。 今回、ぜひmasakiさんにこのイシューのレビューをお願いしたいと思い、ご連絡させていただきました。 お忙しいとは存じますが、お引き受けいただけると嬉しいです🙏 |
|
@tomonariha |
|
@tksmasaki さん、ありがとうございます！ |
There was a problem hiding this comment.
@tomonariha
いくつかコメントしました！ご確認よろしくお願いします！
また、レビューとは別のことになるのですが、
システムテストをブラウザで目視しながら動かしたい場合は、bootcamp の README に HEADED=1 rails test のようにして HEADED を true にする方法が書いてあるので、そっちに合わせたほうが他の方に伝える場合などは都合がいいかも？と思いました。（やりたいことはコード上で値を直接 true にしても同じなので、どっちでもいいとは思います🙏）
README のテストの項目
test/test_helper.rb
Outdated
| @@ -34,3 +34,22 @@ class ActionDispatch::IntegrationTest | |||
| ActiveSupport.on_load(:action_dispatch_system_test_case) do | |||
| ActionDispatch::SystemTesting::Server.silence_puma = true | |||
| end | |||
|
|
|||
| # Rails 7 の ActiveStorage::FixtureSet.blob と同様の機能を実装 | |||
| # Pull Request #4182 「Update rails 7.0.2.2」の完了後に削除する | |||
There was a problem hiding this comment.
現在は 7.0.3 へのアップデートになっていますね。
Pull Request #4182 での Rails 7 へアップデート完了後に削除する などにした方がアップデートするマイナーバージョンが変わっても正確な内容になっていいかもしれないです🤔
また、PRのURLをコメントで貼っておいてもいいかもしれないです！（僕がサンプルで書いた時は貼ってなかったですね...🙏）
There was a problem hiding this comment.
いつの間にかマイナーバージョン上がってますね🤔
マイナーバージョンの記載をやめて、PRへのリンクを記載するようにしてみました。
2512d0d
| @@ -0,0 +1,24 @@ | |||
| # Pull Request #4182 Update rails 7.0.2.2 の完了後、ActiveStorage::FixtureSet.blob を使うように変更する | |||
There was a problem hiding this comment.
現在は 7.0.3 へのアップデートになっていますね。
Pull Request #4182 での Rails 7 へアップデート完了後に削除する などにした方がアップデートするマイナーバージョンが変わっても正確な内容になっていいかもしれないです🤔
また、PRのURLをコメントで貼っておいてもいいかもしれないです！（僕がサンプルで書いた時は貼ってなかったですね...🙏）
test_helper.rb に書いたコメントと同様です！
There was a problem hiding this comment.
同じくマイナーバージョンの記載をやめて、PRへのリンクを記載するようにしてみました。
2512d0d
| # frozen_string_literal: true | ||
|
|
||
| require 'application_system_test_case' | ||
|
|
There was a problem hiding this comment.
このテストは Rails 7 へのアップデートが完了後、デフォルトの機能で fixture の画像をアタッチするようになったら不要になるかなと思うので、他の変更箇所にあるコメントと同じようにその旨を書いておくといいかもなと思いました！
There was a problem hiding this comment.
確かにコメントがあった方がいいですね！
Rails7移行後のためのコメントを記載するようにしました。
2512d0d
There was a problem hiding this comment.
コメントの対応ありがとうございます！
他の変更箇所にあるコメントと同じようにその旨を書いておく
僕のこの書き方が悪くて申し訳ないのですが...🙇♂️
ActiveStorage::FixtureSet.blob を使うように変更する というコメントではなく、テスト自体（このファイルごと）不要になることを書いた方が適切かなと思いました。
現在は Rails 7 から使えるようになる機能を自作で書いているので、テストで確認するのは良いと思うのですが、Rails 7 へアップデートで ActiveStorage::FixtureSet.blob を使うようになると、gem の機能をわざわざテストすることになるので不要になるといった考えです（何か勘違いしていたらすみません...）
テストのプラクティスの説明にあった以下の内容のケースに当てはまるかなと思ってます。
modelのvalidationなど、railsの機能、gemの機能をテストしてしまう。（gemのテストはgemの中でそれぞれやってるので必要ない）
There was a problem hiding this comment.
rails7移行後でもアタッチのテストは必要かな？と思っていましたが、確かにActiveStorage::FixtureSet.blobでアタッチすればrailsの機能ということでテストはいらないですね。
理解が悪くて申し訳ないです🙇♂️
テストを削除するよう記述を変更しました。8512b43
|
|
||
| tanaka_avatar_blob: <%= ActiveStorage::Blob.fixture filename: "users/avatars/tanaka.jpg" %> | ||
|
|
||
| yameo_avatar_blob: <%= ActiveStorage::Blob.fixture filename: "users/avatars/yameo.jpg" %> |
There was a problem hiding this comment.
現在、test/fixture/files には companies と users 以外に、
- articles/ogp_images/test.jpg
- practices/ogp_images/1.jpg
がありますが、これらはセットアップ時にアタッチしなくてよい画像でしょうか？
これらの画像の用途を把握できていないのですが、test/fixture/files 以下に扱いが異なるファイルがあるのが気になったので、確認できたらいいなと思いました！
There was a problem hiding this comment.
#4249 の方で2つの画像ファイルについて質問しました。
komagataさんからご回答いただき、
アップロードのテストに使っているやつかもです。そうであればアタッチする必要はありません。（その辺りのテストを確認してみてください）
とのことです。
これらの画像の用途としては、
test/system/articles_test.rb
test/system/practices_test.rb
test/system/practice/completion_test.rb
で画像のアップロードのテストに使われているだけなので、アタッチの必要はなさそうです。
|
@tksmasaki さん、レビューありがとうございます😄 |
|
@tksmasaki さん、お待たせしました🙇♂️ また、#4249 にてkomagataさんより、
とのことなので、development環境でも画像がアタッチされるように変更しました。7b39699 あと、GUIのシステムテストを |
| @@ -0,0 +1,59 @@ | |||
| # Pull Request #4182(https://github.com/fjordllc/bootcamp/pull/4182) で Rails 7 への移行完了後、 | |||
| # ActiveStorage::FixtureSet.blob を使うように変更する | |||
There was a problem hiding this comment.
attachments.yml の内容に関しては、blobs.yml で ActiveStorage::FixtureSet.blob を使うようになった場合でも内容は変わらないので、このコメントは不要かもなという気がしました。
画像データを登録する方法が変わってもこの fixture の記述に影響はないはずなので、今回の修正についてよくわかっていない状態でこのコメントを読むと、むしろ混乱するかもと個人的に思いました🤔
There was a problem hiding this comment.
確かに attachments.yml については、 blobs.yml と違ってrails7移行後に変更するところはないですね。
ActiveStorage::FixtureSet.blob を使うように変更するってどういうこと？ってなりそうなので、コメントは削除しました。8512b43
| # frozen_string_literal: true | ||
|
|
||
| require 'application_system_test_case' | ||
|
|
There was a problem hiding this comment.
コメントの対応ありがとうございます！
他の変更箇所にあるコメントと同じようにその旨を書いておく
僕のこの書き方が悪くて申し訳ないのですが...🙇♂️
ActiveStorage::FixtureSet.blob を使うように変更する というコメントではなく、テスト自体（このファイルごと）不要になることを書いた方が適切かなと思いました。
現在は Rails 7 から使えるようになる機能を自作で書いているので、テストで確認するのは良いと思うのですが、Rails 7 へアップデートで ActiveStorage::FixtureSet.blob を使うようになると、gem の機能をわざわざテストすることになるので不要になるといった考えです（何か勘違いしていたらすみません...）
テストのプラクティスの説明にあった以下の内容のケースに当てはまるかなと思ってます。
modelのvalidationなど、railsの機能、gemの機能をテストしてしまう。（gemのテストはgemの中でそれぞれやってるので必要ない）
tomonariha
force-pushed
the
bug/link-userdata-and-images-in-test-environment
branch
from
7b39699 to
8512b43
Compare
|
@tksmasaki さん、コメントありがとうございます！ |
|
@tksmasaki さん、大変お待たせしました🙇♂️ |
There was a problem hiding this comment.
@tomonariha
修正ありがとうございます！LGTMです🙆
ちなみにCIが通りにくかったのは、このPRの修正の影響とは関係なかったですか？少し気になったのでよければ教えていただけるとありがたいです🙏
|
@tksmasaki さん、ご確認ありがとうございます！ 今回、15回連続CI落ちる→色々調べてわからなかったので日を置いてもう1度CI通す→1回で通るという流れでした。 Error:
Admin::CompaniesTest#test_delete_company:
ActionView::Template::Error: ActiveStorage::FileNotFoundError
app/models/company.rb:13:in `logo_url'
app/views/admin/companies/_form.html.slim:20
app/views/admin/companies/_form.html.slim:1
app/views/admin/companies/edit.html.slim:18
rails test test/system/admin/companies_test.rb:41このようにエラーログに # app>models>company.rb
def logo_url
if logo.attached?
logo.variant(resize: LOGO_SIZE).processed.url
else
image_url('/images/companies/logos/default.png')
end
endこれを見るに、今回のPRでcompanyのロゴのアタッチが有効になる→ ただ、確実なことは言えないので、この後issueかこのPRでkomagataさんにお知らせして判断を仰ごうと思っています。 |
|
@tomonariha
エラー内容が |
tomonariha
force-pushed
the
bug/link-userdata-and-images-in-test-environment
branch
from
8512b43 to
41b5b02
Compare
|
@tksmasaki さん、情報ありがとうございます！ def after_teardown
super
FileUtils.rm_rf(ActiveStorage::Blob.service.root)
endを組み込んで(41b5b02)、CIを走らせてみた結果、 Error:
AttachmentsTest#test_attachment_company_logo:
ActionView::Template::Error: ActiveStorage::FileNotFoundError
app/models/company.rb:13:in `logo_url'
app/views/companies/_profile.html.slim:7
app/views/companies/show.html.slim:21
rails test test/system/attachments_test.rb:132つのFailureはどちらもテスト名以外は同じで Failure:
CompaniesTest#test_GET_/companies:
--- expected
+++ actual
@@ -1 +1 @@
-"企業一覧 | FJORD BOOT CAMP（フィヨルドブートキャンプ）"
+""と言う内容でした。 また、この # app>models>company.rb
def logo_url
if logo.attached?
logo.variant(resize: LOGO_SIZE).processed.url
else
image_url('/images/companies/logos/default.png')
end
endリサイズ部分を消して # app>models>company.rb
def logo_url
image_url('/images/companies/logos/default.png')
endのようにしてテストをすると、エラーが出なくなりました。 それと、railsガイドにあったこちらのコードも組み込んでみましたが、 parallelize_setup do |i|
ActiveStorage::Blob.service.root = "#{ActiveStorage::Blob.service.root}-#{i}"
endローカルのテストの時点で次のエラーが出るようになってしまいました。 Error:
Admin::CompaniesTest#test_update_company:
NoMethodError: undefined method `root=' for #<ActiveStorage::Service::DiskService:0x0000000107af9d50パラレルライズというのがよくわからず使っているので、何か違うことをしてるかもしれません🙇♂️ 今回 CIを通りやすくするためには、今回のPRでのcompanyのロゴのアタッチは見送りユーザー画像のみにするということも考えましたが、tksmasakiさんのご意見をお伺いしたいです🙏 |
|
@tomonariha
僕としてもありなのかなと思っています。
という状況だと思うので、Issueを立ておいていうのもなんですが、companyのテスト環境のアタッチができてなくてもとりあえずは大丈夫だと思います...（作業してもらっておいて、なくてもいいかも、みたいな感じになったら申し訳ないです🙇） エラーの原因ははっきり分かったほうがいいとは思いますが、優先度は高くないのかな？と思うので、そのあたりを含めkomagataさんと相談していただけるとありがたいです！ |
|
@tksmasaki さん、貴重なご意見ありがとうございます🙏大変参考になります。 |
tomonariha
force-pushed
the
bug/link-userdata-and-images-in-test-environment
branch
2 times, most recently
from
3751749 to
0c72180
Compare
tomonariha
force-pushed
the
bug/link-userdata-and-images-in-test-environment
branch
from
4be293b to
ab75434
Compare
|
@tksmasaki さん、お疲れ様です！ それと、今回レビューをお引き受けいただいて、多大な時間と労力を割いて下さりありがとうございました！ |
なるほど、これなら問題が確実に改善してよさそうですね！ |
|
@komagata さん、レビューよろしくお願いします🙏 |
Issue
概要
テスト環境と開発環境でユーザーとアイコン画像が紐付けされるようにした
参考issue:#4305
変更確認方法
準備
ブランチ
bug/link-userdata-and-images-in-test-environmentをローカルに取り込む開発環境での確認
rails db:resetでDBの再生成をする。bin/setupでDBに初期データを投入する。rails sでサーバーを起動する。変更前
変更後
テスト環境での確認
HEADED=1 rails test test/system/attachments_test.rbを実行し、ブラウザでのシステムテストを起動する。ウィンドウが閉じるのが早くて確認しにくい場合は、
test/system/attachments_test.rbにあるテストメソッドのvisit_with_auth〜の次の行にsleep 10などを挟む。変更前
変更後