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

Add a process to look at the timestamp and reload the term if it is not latest #56

Merged
merged 10 commits into from Oct 7, 2022

Conversation

ishikawa999
Copy link
Collaborator

If requests are distributed between two web servers, such as ALB, there is a problem that only one of them reloads i18n and the terms are not updated.
In order to allow reloading on the other web server as well, we check each request to see if the redmine_message_customize_timestamp is out of date and reload the i18n if it is.

@yui-har yui-har self-requested a review October 5, 2022 23:46
Copy link
Collaborator

@yui-har yui-har left a comment

Choose a reason for hiding this comment

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

テスト test/functional/custom_message_settings_controller_test.rb が失敗します。

% RAILS_ENV=test bundle exec rake redmine:plugins:test NAME=redmine_message_customize
Run options: --seed 40894

# Running:

.....F

Failure:
CustomMessageSettingsControllerTest#test_update_with_custom_messages_yaml [./plugins/redmine_message_customize/test/functional/custom_message_settings_controller_test.rb:65]:
Expected: "Home3"
  Actual: "Home1"


rails test plugins/redmine_message_customize/test/functional/custom_message_settings_controller_test.rb:59

..F

Failure:
CustomMessageSettingsControllerTest#test_toggle_enabled [./plugins/redmine_message_customize/test/functional/custom_message_settings_controller_test.rb:86]:
Expected response to be a <3XX: redirect>, but was a <200: OK>


rails test plugins/redmine_message_customize/test/functional/custom_message_settings_controller_test.rb:84

.F

Failure:
CustomMessageSettingsControllerTest#test_default_messages [./plugins/redmine_message_customize/test/functional/custom_message_settings_controller_test.rb:37]:
<Confirm default messages(config/locales/ja.yml)> expected but was
<Confirm your password to continue>..
Expected 0 to be >= 1.


rails test plugins/redmine_message_customize/test/functional/custom_message_settings_controller_test.rb:33

.F

Failure:
CustomMessageSettingsControllerTest#test_update_with_custom_messages [./plugins/redmine_message_customize/test/functional/custom_message_settings_controller_test.rb:55]:
Expected: "Home3"
  Actual: "Home1"


rails test plugins/redmine_message_customize/test/functional/custom_message_settings_controller_test.rb:49

F

Failure:
CustomMessageSettingsControllerTest#test_update_with_invalid_params [./plugins/redmine_message_customize/test/functional/custom_message_settings_controller_test.rb:73]:
<Message customize> expected but was
<Confirm your password to continue>..
Expected 0 to be >= 1.


rails test plugins/redmine_message_customize/test/functional/custom_message_settings_controller_test.rb:69

F

Failure:
CustomMessageSettingsControllerTest#test_edit [./plugins/redmine_message_customize/test/functional/custom_message_settings_controller_test.rb:20]:
<Message customize> expected but was
<Confirm your password to continue>..
Expected 0 to be >= 1.


rails test plugins/redmine_message_customize/test/functional/custom_message_settings_controller_test.rb:16

........................

Finished in 3.080297s, 12.6611 runs/s, 34.0876 assertions/s.
39 runs, 105 assertions, 6 failures, 0 errors, 0 skips

@ishikawa999
Copy link
Collaborator Author

@yui-har
#56 (review) について、
RAILS_ENV=test bundle exec rake redmine:plugins:test NAME=redmine_message_customize TESTOPTS="--seed=40894" のようにseed値を固定したり環境変数TZを変えても再現できません。
何か心あたりなど無いでしょうか?

@yui-har
Copy link
Collaborator

yui-har commented Oct 6, 2022

#56 (review) について、 RAILS_ENV=test bundle exec rake redmine:plugins:test NAME=redmine_message_customize TESTOPTS="--seed=40894" のようにseed値を固定したり環境変数TZを変えても再現できません。 何か心あたりなど無いでしょうか?

f3c6219 のパッチを当てた状態でテストを実施しました。
違いがあるとしたら、私の環境は ruby 3.1.2 、bundle update で最新状態にしてRedmine-trunkを動かしています。

@ishikawa999
Copy link
Collaborator Author

ruby 3.1.2 、bundle update で最新状態にしてRedmine-trunk

という状態に合わせても再現できませんでした。
パッと見た感じだと、他のテストでlabel_homeをHome3に書き換えた後、fixturesで設定が戻る+MessageCustomize::Locale.reload!によってHome1に戻るはずがうまく戻っていないように見えます。
これは実行順番に影響を受ける問題でしょうか?(複数回実行しても同じように失敗するか)

@yui-har
Copy link
Collaborator

yui-har commented Oct 6, 2022

ruby 3.1.2 、bundle update で最新状態にしてRedmine-trunk

という状態に合わせても再現できませんでした。 パッと見た感じだと、他のテストでlabel_homeをHome3に書き換えた後、fixturesで設定が戻る+MessageCustomize::Locale.reload!によってHome1に戻るはずがうまく戻っていないように見えます。 これは実行順番に影響を受ける問題でしょうか?(複数回実行しても同じように失敗するか)

seed の値によって、テスト成功・失敗します。

  • 成功: 22729, 41775
  • 失敗: 26485, 40894

@ishikawa999
Copy link
Collaborator Author

@yui-har
試されているredmine_message_customizeのコードは f2e3c03 以降のものでしょうか
seedの設定を正しく行うため、コミット番号を伺いたいです

@yui-har
Copy link
Collaborator

yui-har commented Oct 7, 2022

試されているredmine_message_customizeのコードは f2e3c03 以降のものでしょうか seedの設定を正しく行うため、コミット番号を伺いたいです

@ishikawa999
f3c6219 を使用しています。

@yui-har
Copy link
Collaborator

yui-har commented Oct 7, 2022

@ishikawa999
テスト以外はOKです。
(Redmine-trunk: r21901 で動作確認済)

Copy link
Collaborator

@yui-har yui-har left a comment

Choose a reason for hiding this comment

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

(テストは残っていますけど)レビュー完了です、masterブランチにマージしてください。

@ishikawa999
Copy link
Collaborator Author

テストは再現ができなかった + テストが失敗した環境でも該当する機能の動作は問題無かったため、マージします

@ishikawa999 ishikawa999 merged commit 5a06813 into master Oct 7, 2022
@ishikawa999 ishikawa999 deleted the add_process_to_reload_messages branch October 7, 2022 04:26
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

2 participants