Skip to content

Fix test flakes#86

Merged
mrcasals merged 2 commits intocodegram:masterfrom
PlayerData:fix-test-flakes
Feb 1, 2021
Merged

Fix test flakes#86
mrcasals merged 2 commits intocodegram:masterfrom
PlayerData:fix-test-flakes

Conversation

@ball-hayden
Copy link
Copy Markdown
Contributor

@ball-hayden ball-hayden commented Jan 30, 2021

✍️ Description

The current test suite is order dependent. Specifically, the following test fails if it is not run in the correct order:

ActiveModel::Validations::DateValidator::when value does not match validation requirements#test_0012_allows custom validation message to be handled by I18n [/home/runner/work/date_validator/date_validator/test/date_validator_test.rb:93]

The cause of this seems to be related to lazy loading of translations by the I18n gem.

If I18n.backend.store_translations is called before the I18n backend has loaded translations, the "overridden" translation is lost and the default is returned instead. This would only be an issue for ActiveModel versions depending on i18n >= 1.6.0, which appears to have only been required since Rails 6.

Also pulls in 4df9431 to fix issues if Time.zone hasn't been required yet.

✅ Fixes

This should fix the flakes currently blocking #85

✅ QA

Before requesting a review, please make sure that:

  • Preview is working on Netlify, Heroku or similar.
  • Manually check that it's working.
  • Add documentation and tests if needed.

Documentation

I18n release notes for lazy/eager loading: https://github.com/ruby-i18n/i18n/releases/tag/v1.6.0

Otherwise, the custom translation may be overwritten when the
backend loads translations as part of validation
@ball-hayden ball-hayden marked this pull request as ready for review January 30, 2021 12:15
@ball-hayden
Copy link
Copy Markdown
Contributor Author

Worth commenting that the I18n fix is only really an issue in the test suite.

There are contrived cases where a user has used store_translations in a Rails app which hasn't eager loaded translations (eager loading is done by default for the production env), but that isn't isolated to this project. Arguably, thinking about it, this is a bug in I18n.

@mrcasals
Copy link
Copy Markdown
Contributor

mrcasals commented Feb 1, 2021

Hi @ball-hayden! Thanks for taking the time and looking into it! Much appreciated!

@mrcasals mrcasals merged commit 849f45a into codegram:master Feb 1, 2021
@ball-hayden ball-hayden deleted the fix-test-flakes branch February 1, 2021 08:49
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.

2 participants