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

ユーザー一覧のローディングをプレースホルダに変更 #6307

Merged
merged 4 commits into from
Apr 19, 2023

Conversation

uchihiro04
Copy link
Contributor

@uchihiro04 uchihiro04 commented Mar 8, 2023

Issue

概要

ユーザー一覧にアクセスした際のローディングを、「ロード中」からプレースホルダを使用した形に変更しました。
プレースホルダとして、読み込み中用のユーザーブロックを12個表示しています。

変更確認方法

  1. feature/change-user-list-loading-to-placeholderをローカルに取り込む
  2. bin/rails sでローカル環境を立ち上げる
  3. 任意のアカウントでログインし、/usersにアクセスする
  4. ローディング中の画面に読み込み中用のユーザーブロックが12個表示されているか確認する
    表示されるユーザーブロックについては、Screenshotの変更後を参照。画面の表示を縮小すると確認がしやすくなります。

Screenshot

変更前

ローディング時に「ロード中」と表示される。
image

変更後

ローディング時に読み込み中用のユーザーブロックが表示される。
image

@uchihiro04 uchihiro04 changed the title ユーザー一覧のローディングをプレースホルダーに変更 ユーザー一覧のローディングをプレースホルダに変更 Mar 8, 2023
@uchihiro04
Copy link
Contributor Author

@machida
isseuに記載されている要件を満たす形で実装を行いましたので、プレースホルダのデザインをお願いいたします🙇‍♂️

@machida
Copy link
Member

machida commented Mar 15, 2023

@uchihiro04 デザイン了解ですー💪

@machida machida force-pushed the feature/change-user-list-loading-to-placeholder branch from 6ae9295 to c40273b Compare March 23, 2023 04:44
@machida
Copy link
Member

machida commented Mar 23, 2023

@uchihiro04 お待たせしました🙇‍♂️こちらデザインの調整をしたのでレビューに進めてくださいー

@machida machida removed their assignment Mar 23, 2023
@uchihiro04
Copy link
Contributor Author

@machida
デザインを調整していただき、ありがとうございます🙇‍♂️
コードレビューに進みたいと思います!

@uchihiro04 uchihiro04 marked this pull request as ready for review March 23, 2023 11:32
@uchihiro04
Copy link
Contributor Author

@monyatto
お疲れ様です!
ご都合がよろしければ、こちらのレビューをお願いできればと思うのですがいかがでしょうか?

@monyatto
Copy link
Contributor

@uchihiro04
レビュー依頼ありがとうございます! 今週の日曜までには確認できるかと思います💪

@uchihiro04
Copy link
Contributor Author

@monyatto
ありがとうございます! よろしくお願いいたします🙇‍♂️

@@ -124,7 +124,7 @@ class UsersTest < ApplicationSystemTestCase

visit_with_auth '/users', 'komagata'
assert_no_selector 'div.users-item.inactive'
assert_text '1ヶ月以上ログインがありません'
assert_text('1ヶ月以上ログインがありません', wait: 10)
Copy link
Contributor

Choose a reason for hiding this comment

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

assert_textassert_selectorなどのCapybaraの検証メソッドを使用している場合、assert_equalとは違い自動的にwaitしてくるのでwaitの指定は不要なのではと思いました。

こちらで試してみたところwaitの指定を抜いた状態でテストが通ったのでご確認お願いします🙏

Copy link
Contributor

Choose a reason for hiding this comment

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

参考にした伊藤さんの動画を貼り忘れてました🙇🏻
wait_for_vuejs を使いたくなったときに見る動画 - YouTube

Copy link
Contributor Author

Choose a reason for hiding this comment

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

wait: 10を設定している理由としましては、Capybaraが定めている待ち時間(5秒)を過ぎても、ユーザー一覧がプレースホルダーのままになっていて失敗するテストがいくつかあるためとなります。

単体でテストを実行すると成功するのですが、test/system/users_test.rbでファイルにあるテストをまるごと実行するといくつか失敗してしまいます😵
今回wait: 10を全て抜いた状態で試したところ、以下のような結果になりました。

1回目
Finished in 69.916451s, 0.9726 runs/s, 2.3886 assertions/s.
68 runs, 167 assertions, 2 failures, 1 errors, 0 skips

2回目
Finished in 78.522521s, 0.8660 runs/s, 2.0122 assertions/s.
68 runs, 158 assertions, 2 failures, 2 errors, 0 skips

3回目
Finished in 71.045198s, 0.9571 runs/s, 2.3084 assertions/s.
68 runs, 164 assertions, 3 failures, 3 errors, 0 skips

4回目
Finished in 77.764665s, 0.8744 runs/s, 2.0703 assertions/s.
68 runs, 161 assertions, 2 failures, 3 errors, 0 skips

5回目
Finished in 67.857615s, 1.0021 runs/s, 2.4905 assertions/s.
68 runs, 169 assertions, 1 failures, 0 errors, 0 skips

毎回必ず失敗するテストは無かったため、Flakyなテストになっていると言えそうです。

対してwait: 10を入れた状態の場合、以下のような結果となりました。

1回目
Finished in 70.778555s, 0.9607 runs/s, 2.4019 assertions/s.
68 runs, 170 assertions, 0 failures, 0 errors, 0 skips

2回目
Finished in 74.823446s, 0.9088 runs/s, 2.2720 assertions/s.
68 runs, 170 assertions, 0 failures, 0 errors, 0 skips

3回目
Finished in 66.484297s, 1.0228 runs/s, 2.5570 assertions/s.
68 runs, 170 assertions, 0 failures, 0 errors, 0 skips

4回目
Finished in 69.208383s, 0.9825 runs/s, 2.4563 assertions/s.
68 runs, 170 assertions, 0 failures, 0 errors, 0 skips

5回目
Finished in 69.053913s, 0.9847 runs/s, 2.4618 assertions/s.
68 runs, 170 assertions, 0 failures, 0 errors, 0 skips

環境によってはwaitを付けなくてもテストが落ちずに成功する可能性はありそうですが、Flakyなテストになってしまうことを防ぐため、wait: 10をテストコードに付けている形となります。

※ちなみに、Capybaraのメソッドの最大待ち時間は以下で設定されていました。

Capybara.default_max_wait_time = 5

Copy link
Contributor

@monyatto monyatto Mar 31, 2023

Choose a reason for hiding this comment

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

こちら再度ご確認お願いします🙏

テストにかかる最大時間を可能な限り短縮するために、以下を試していただけますでしょうか。

今回の変更でテストが長引いているのか、あるいはローカルの環境に寄るものなのかを確かめたいので、mainブランチでテストを実行して落ちるのかご確認お願いします。

  • mainで実行しても落ちる場合
    さらにcircleCIでご確認お願いします。circleCIで通るならばローカルよりもそちらの結果を優先していいかと思います。

  • mainでは落ちない場合
    今回のコードでテストが長引いてしまっていることになるため、waitが本当に必要なのかコードの見直しを今一度お願いいたします。

Copy link
Contributor Author

@uchihiro04 uchihiro04 Mar 31, 2023

Choose a reason for hiding this comment

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

レビューありがとうございます🙇‍♂️

こちらで確認する内容について2点質問させていただければと思います。

1. circleCIでテストする内容について
mainで実行してもテストが落ちる場合、circleCIで実行するテストは次のどちらになりますでしょうか。

  • mainブランチのテスト
  • feature/change-user-list-loading-to-placeholderブランチの、wait: 10をつけた状態のテスト
  • feature/change-user-list-loading-to-placeholderブランチの、wait: 10をつけていない状態のテスト

2. CircleCI確認後のコードについて
質問1の答えを実行後、CircleCIでテストが通った場合、test/system/users_test.rb内のテストコードは以下のどちらの形になることを想定されていらっしゃいますでしょうか。

  • wait: 10をつけた状態のテストコード
  • wait: 10をつけない状態のテストコード

それぞれご教示いただけますと助かります🙇‍♂️


また、mainブランチでtest/system/users_test.rb内にあるテストを10回実行してみました。失敗したテストを再度やり直してくれるCI=1はつけずにテストを実行しています。

1回目
Finished in 97.541523s, 0.6971 runs/s, 1.7634 assertions/s.
68 runs, 172 assertions, 0 failures, 0 errors, 0 skips

2回目
Finished in 69.345855s, 0.9806 runs/s, 2.4515 assertions/s.
68 runs, 170 assertions, 2 failures, 0 errors, 0 skips

3回目
Finished in 66.784197s, 1.0182 runs/s, 2.5755 assertions/s.
68 runs, 172 assertions, 0 failures, 0 errors, 0 skips

4回目
Finished in 66.380129s, 1.0244 runs/s, 2.5911 assertions/s.
68 runs, 172 assertions, 0 failures, 0 errors, 0 skips

5回目
Finished in 68.479079s, 0.9930 runs/s, 2.5117 assertions/s.
68 runs, 172 assertions, 0 failures, 0 errors, 0 skips

6回目
Finished in 72.296399s, 0.9406 runs/s, 2.2823 assertions/s.
68 runs, 165 assertions, 4 failures, 0 errors, 0 skips

7回目
Finished in 87.295875s, 0.7790 runs/s, 1.7756 assertions/s.
68 runs, 155 assertions, 8 failures, 0 errors, 0 skips

8回目
Finished in 82.727729s, 0.8220 runs/s, 1.9945 assertions/s.
68 runs, 165 assertions, 6 failures, 0 errors, 0 skips

9回目
Finished in 71.707318s, 0.9483 runs/s, 2.3847 assertions/s.
68 runs, 171 assertions, 1 failures, 0 errors, 0 skips

10回目
Finished in 80.977581s, 0.8397 runs/s, 1.9882 assertions/s.
68 runs, 161 assertions, 3 failures, 0 errors, 0 skips

10回中6回テストが失敗しました😵 そのため、「mainで実行しても落ちる場合」が該当しているかと思います。

何卒よろしくお願いいたします🙇‍♂️

Copy link
Contributor

Choose a reason for hiding this comment

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

@uchihiro04
ご確認ありがとうございます!
テストがmainでも落ちるということは、その原因はローカルの環境に寄るものの可能性が高いのかもしれないですねー。
ローカル環境で落ちてもcircleCIで通れば大丈夫みたいなので、circleCIでの結果をまた確認させてもらえればと思います。

以下ご質問に回答します。

1. circleCIでテストする内容について
mainで実行してもテストが落ちる場合、circleCIで実行するテストは次のどちらになりますでしょうか。

  • mainブランチのテスト
    →プレースホルダが表示されるタイミングでテストが実行され、待ち時間もないため失敗する。

  • feature/change-user-list-loading-to-placeholderブランチの、wait: 10をつけた状態のテスト
    →待ち時間があるのでテストはおそらく通る。最大10秒待つことになる。

  • feature/change-user-list-loading-to-placeholderブランチの、wait: 10をつけていない状態のテスト
    →待ち時間があるのでテストはおそらく通る。最大5秒待つことになる。

以上の結果から一番下の「feature/change-user-list-loading-to-placeholderブランチの、wait: 10をつけていない状態のテスト」がcircleCIで通ればそれが一番いいかと思います!

2. CircleCI確認後のコードについて
質問1の答えを実行後、CircleCIでテストが通った場合、test/system/users_test.rb内のテストコードは以下のどちらの形になることを想定されていらっしゃいますでしょうか。

こちらも上記と同じ理由からwait: 10をつけない状態のテストコードがいいかと思います!

不明点などございましたらまたご連絡ください〜! よろしくお願いします🙏

Copy link
Contributor Author

Choose a reason for hiding this comment

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

コメントを参考に、feature/change-user-list-loading-to-placeholderブランチのコードにつけていたwait: 10を削除し、circleCIでテストを5回実行しました。

1回目:成功
image

2回目:失敗(test/system/admin/users_test.rbの102行目のテストが失敗。今回のプルリクで変更した箇所とは関係なし)
image

3回目:成功
image

4回目:成功
image

5回目:成功
image

そのため、wait: 10をつけなくてもテストは通過すると判断できそうです。

Copy link
Contributor

@monyatto monyatto left a comment

Choose a reason for hiding this comment

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

@uchihiro04
テストに関して気になったところを一点コメントしましたのでご確認お願いします〜🙏

Copy link
Contributor Author

@uchihiro04 uchihiro04 left a comment

Choose a reason for hiding this comment

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

@monyatto
レビューありがとうございます!

いただいたレビューにつきましてコメントしましたので、お手数ですがご確認いただけますと幸いです🙇‍♂️

@@ -124,7 +124,7 @@ class UsersTest < ApplicationSystemTestCase

visit_with_auth '/users', 'komagata'
assert_no_selector 'div.users-item.inactive'
assert_text '1ヶ月以上ログインがありません'
assert_text('1ヶ月以上ログインがありません', wait: 10)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

wait: 10を設定している理由としましては、Capybaraが定めている待ち時間(5秒)を過ぎても、ユーザー一覧がプレースホルダーのままになっていて失敗するテストがいくつかあるためとなります。

単体でテストを実行すると成功するのですが、test/system/users_test.rbでファイルにあるテストをまるごと実行するといくつか失敗してしまいます😵
今回wait: 10を全て抜いた状態で試したところ、以下のような結果になりました。

1回目
Finished in 69.916451s, 0.9726 runs/s, 2.3886 assertions/s.
68 runs, 167 assertions, 2 failures, 1 errors, 0 skips

2回目
Finished in 78.522521s, 0.8660 runs/s, 2.0122 assertions/s.
68 runs, 158 assertions, 2 failures, 2 errors, 0 skips

3回目
Finished in 71.045198s, 0.9571 runs/s, 2.3084 assertions/s.
68 runs, 164 assertions, 3 failures, 3 errors, 0 skips

4回目
Finished in 77.764665s, 0.8744 runs/s, 2.0703 assertions/s.
68 runs, 161 assertions, 2 failures, 3 errors, 0 skips

5回目
Finished in 67.857615s, 1.0021 runs/s, 2.4905 assertions/s.
68 runs, 169 assertions, 1 failures, 0 errors, 0 skips

毎回必ず失敗するテストは無かったため、Flakyなテストになっていると言えそうです。

対してwait: 10を入れた状態の場合、以下のような結果となりました。

1回目
Finished in 70.778555s, 0.9607 runs/s, 2.4019 assertions/s.
68 runs, 170 assertions, 0 failures, 0 errors, 0 skips

2回目
Finished in 74.823446s, 0.9088 runs/s, 2.2720 assertions/s.
68 runs, 170 assertions, 0 failures, 0 errors, 0 skips

3回目
Finished in 66.484297s, 1.0228 runs/s, 2.5570 assertions/s.
68 runs, 170 assertions, 0 failures, 0 errors, 0 skips

4回目
Finished in 69.208383s, 0.9825 runs/s, 2.4563 assertions/s.
68 runs, 170 assertions, 0 failures, 0 errors, 0 skips

5回目
Finished in 69.053913s, 0.9847 runs/s, 2.4618 assertions/s.
68 runs, 170 assertions, 0 failures, 0 errors, 0 skips

環境によってはwaitを付けなくてもテストが落ちずに成功する可能性はありそうですが、Flakyなテストになってしまうことを防ぐため、wait: 10をテストコードに付けている形となります。

※ちなみに、Capybaraのメソッドの最大待ち時間は以下で設定されていました。

Capybara.default_max_wait_time = 5

@monyatto
Copy link
Contributor

@uchihiro04
なるほど環境によってはwait: 10がないと落ちてしまうんですね🤔 ご確認ありがとうございます!

以下は別件です。動作やテストは既に問題ないのですが、Vueの部分について私が理解できていない部分があるので、メンターさんにペアプロを依頼して一緒に見ていただこうかなと思っております。
ただ、その場合今週末くらいまでお待ちいただくことになってしまうと思うので、もし不都合でしたらお知らせいただけるとありがたいです(既にお待たせしてしまっているのに後から申し訳ございません🙇🏻)

@uchihiro04
Copy link
Contributor Author

@monyatto
ペアプロで確認される旨、承知しました!
急いでいるわけではないので大丈夫です👍

@@ -268,7 +268,7 @@ class UsersTest < ApplicationSystemTestCase

test 'only admin can see link to talk on user list page' do
visit_with_auth '/users', 'komagata'
assert_link '相談部屋'
assert_link('相談部屋', wait: 10)
Copy link
Contributor

Choose a reason for hiding this comment

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

()は無くても大丈夫なので、他の行に合わせて無しで統一したほうがいいかもです!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

確かに、他の行は()がついていないですね😵

()無しでテストが通過することを確認できたため、修正いたしました。該当するコミットは32313b7となります。

Copy link
Contributor

@monyatto monyatto left a comment

Choose a reason for hiding this comment

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

@uchihiro04
お待たせしました🙏
ペアプロでチェックしてもらい、2点ほど追記したのでご確認お願いします!

@uchihiro04 uchihiro04 force-pushed the feature/change-user-list-loading-to-placeholder branch from c40273b to 32313b7 Compare March 31, 2023 14:45
Copy link
Contributor Author

@uchihiro04 uchihiro04 left a comment

Choose a reason for hiding this comment

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

@monyatto
レビューありがとうございます!
修正および2点ほど質問させていただきました。ご回答いただけますと助かります🙏

何卒よろしくお願いします🙇‍♂️

@@ -124,7 +124,7 @@ class UsersTest < ApplicationSystemTestCase

visit_with_auth '/users', 'komagata'
assert_no_selector 'div.users-item.inactive'
assert_text '1ヶ月以上ログインがありません'
assert_text('1ヶ月以上ログインがありません', wait: 10)
Copy link
Contributor Author

@uchihiro04 uchihiro04 Mar 31, 2023

Choose a reason for hiding this comment

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

レビューありがとうございます🙇‍♂️

こちらで確認する内容について2点質問させていただければと思います。

1. circleCIでテストする内容について
mainで実行してもテストが落ちる場合、circleCIで実行するテストは次のどちらになりますでしょうか。

  • mainブランチのテスト
  • feature/change-user-list-loading-to-placeholderブランチの、wait: 10をつけた状態のテスト
  • feature/change-user-list-loading-to-placeholderブランチの、wait: 10をつけていない状態のテスト

2. CircleCI確認後のコードについて
質問1の答えを実行後、CircleCIでテストが通った場合、test/system/users_test.rb内のテストコードは以下のどちらの形になることを想定されていらっしゃいますでしょうか。

  • wait: 10をつけた状態のテストコード
  • wait: 10をつけない状態のテストコード

それぞれご教示いただけますと助かります🙇‍♂️


また、mainブランチでtest/system/users_test.rb内にあるテストを10回実行してみました。失敗したテストを再度やり直してくれるCI=1はつけずにテストを実行しています。

1回目
Finished in 97.541523s, 0.6971 runs/s, 1.7634 assertions/s.
68 runs, 172 assertions, 0 failures, 0 errors, 0 skips

2回目
Finished in 69.345855s, 0.9806 runs/s, 2.4515 assertions/s.
68 runs, 170 assertions, 2 failures, 0 errors, 0 skips

3回目
Finished in 66.784197s, 1.0182 runs/s, 2.5755 assertions/s.
68 runs, 172 assertions, 0 failures, 0 errors, 0 skips

4回目
Finished in 66.380129s, 1.0244 runs/s, 2.5911 assertions/s.
68 runs, 172 assertions, 0 failures, 0 errors, 0 skips

5回目
Finished in 68.479079s, 0.9930 runs/s, 2.5117 assertions/s.
68 runs, 172 assertions, 0 failures, 0 errors, 0 skips

6回目
Finished in 72.296399s, 0.9406 runs/s, 2.2823 assertions/s.
68 runs, 165 assertions, 4 failures, 0 errors, 0 skips

7回目
Finished in 87.295875s, 0.7790 runs/s, 1.7756 assertions/s.
68 runs, 155 assertions, 8 failures, 0 errors, 0 skips

8回目
Finished in 82.727729s, 0.8220 runs/s, 1.9945 assertions/s.
68 runs, 165 assertions, 6 failures, 0 errors, 0 skips

9回目
Finished in 71.707318s, 0.9483 runs/s, 2.3847 assertions/s.
68 runs, 171 assertions, 1 failures, 0 errors, 0 skips

10回目
Finished in 80.977581s, 0.8397 runs/s, 1.9882 assertions/s.
68 runs, 161 assertions, 3 failures, 0 errors, 0 skips

10回中6回テストが失敗しました😵 そのため、「mainで実行しても落ちる場合」が該当しているかと思います。

何卒よろしくお願いいたします🙇‍♂️

@@ -268,7 +268,7 @@ class UsersTest < ApplicationSystemTestCase

test 'only admin can see link to talk on user list page' do
visit_with_auth '/users', 'komagata'
assert_link '相談部屋'
assert_link('相談部屋', wait: 10)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

確かに、他の行は()がついていないですね😵

()無しでテストが通過することを確認できたため、修正いたしました。該当するコミットは32313b7となります。

ユーザー一覧のローディング時に表示されるプレースホルダをもとにテストを実行してしまうことでテストが失敗していたため、ローディングが完了する形にテストを修正しました。
CIでのテスト実行時間を短縮するため削除しました。
@uchihiro04 uchihiro04 force-pushed the feature/change-user-list-loading-to-placeholder branch from 32313b7 to 026a3a0 Compare April 11, 2023 14:47
@uchihiro04
Copy link
Contributor Author

@monyatto
コメントありがとうございます!

いただいたコメントをもとに、wait: 10を削除した状態でcircleCIにてテストを5回実施しました。
以下に結果を貼りましたので、ご確認いただけますと助かります🙇‍♂️
#6307 (comment)

Copy link
Contributor

@monyatto monyatto left a comment

Choose a reason for hiding this comment

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

こちらApproveさせていただきます〜😊
(テストの書き方や検証メソッドの違いなど調べていくうちに大変勉強になりました!ありがとうございます!)

@uchihiro04
Copy link
Contributor Author

@monyatto
Approveありがとうございます!

こちらこそレビューをいただいたことで、ローカルでのテストの状況だけでwaitをつける判断をしてはいけないことに気づけました🙂
丁寧にレビューいただき感謝です🙇‍♂️

@uchihiro04
Copy link
Contributor Author

@komagata
お疲れ様です!
チームのメンバーからApproveをいただきましたので、レビューをお願いいたします🙇‍♂️

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 74ca03f into main Apr 19, 2023
@komagata komagata deleted the feature/change-user-list-loading-to-placeholder branch April 19, 2023 12:20
@github-actions github-actions bot mentioned this pull request Apr 19, 2023
19 tasks
@uchihiro04
Copy link
Contributor Author

@komagata
ありがとうございます! 引き続きチーム開発に取り組んでいこうと思います!

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.

None yet

4 participants