Fix flaky spec: Budget Investments Show milestones

[javierv]

References

• Issue Flaky spec: Budget Investments Show milestones #2718

Objectives

Fix the flaky spec that appeared in spec/features/budgets/investments_spec.rb:994 ("Budget Investments Show milestones").

Explain why the test is flaky, or under which conditions/scenario it fails randomly

The test fails if Globalize.locale != I18n.locale when the test starts. AFAIK there's only one test which can modify that locale: spec/features/translations_spec.rb:76 ("Translations Milestones Globalize javascript interface Highlight current locale"). So, to reproduce it locally, run those tests making sure the translations test runs first. On my machine, this command makes the test fail every time:

bin/rspec spec/features/budgets/investments_spec.rb:994 spec/features/translations_spec.rb:76 --seed 6700

Here's what happens during those two tests:

1. The translations test sets Globalize.locale = :es.
2. The milestones test config.before block sets I18n.locale = :en
3. The test creates data like first_milestone, which will get assigned description_es (as set by Globalize).
4. The visit budget_investment_path renders the milestones, which include the line globalize(neutral_locale(locale)).
5. Since locale is I18n.locale (that is, :en), the block will call first_milestone.description_en, which will return nil.
6. The test fails because it expects a description and finds nil instead.

Explain why your PR fixes it

By setting Globalize.locale = I18n.locale before each test, the changes one test might cause don't affect any other test.

Notes

• Changing code in ApplicationController and Management::BaseController isn't necessary to make the test pass. However, since having different values in Globalize.locale and I18n.locale has proven to be an issue on the test enviroment, I've also changed application code in order to avoid similar situations on production. Different opinions are welcome!

[aitbw]

Hello again @javierv !

For this PR, I'll only leave one comment as it's a pretty short patch.

I know you mentioned on the Notes section this is not necessary to get the test green but I'd like some in-depth details on why do you think the actual code can cause problems on a production environment

Thanks a lot once again. We appreciate your constant collaboration with CONSUL ❤️

[aitbw]

What will happen if we don't delete this callback? (as well as its related method)

[javierv]

With these changes, this callback would be redundant, since the same thing is now done in ApplicationController.

[aitbw]

You mean ApplicationController#set_locale does the exact same thing as Translatable#set_translation_locale (?)

[javierv]

Before these changes, ApplicationController#set_locale configured I18n.locale and Translatable#set_translation_locale configured Globalize.locale.

After these changes, ApplicationController#set_locale configures both I18n.locale and Globalize.locale. So there's no need for Translatable#set_translation_locale anymore, since Globalize.locale is already configured.

At least that's what I think 😅.

[javierv]

@aitbw Funny thing: I don't think the code can cause problems on production. But I didn't think it could cause problems while testing, and it did, so I don't trust myself that much 😆.

Without these changes, on production there are moments where I18n.locale and Globalize.locale are different. I'm afraid I don't know whether that could cause problems, at least with the current code 🤔.

However, I see an advantage while developing, particularly for developers who aren't so familiar with the code like myself. Setting Globalize.locale after I18n.locale makes the code easier to follow and easier to debug.

Needless to say, I'm not familiar enough with CONSUL to be sure one option is better than the other one, so whatever you think is fine for me 👍!

[voodoorai2000]

Looks good to me 👌