Improve 'LanguageTest::test_locales_config_key_set_properly'#3490
Improve 'LanguageTest::test_locales_config_key_set_properly'#3490andrii-bodnar wants to merge 2 commits intoBookStackApp:developmentfrom
Conversation
|
Thanks for offering this PR @andrii-bodnar but I'm not really sure I understand the point of this change.
I would expect this test to fail, since the locales config no longer matches the available languages. This test is designed to check when there's a difference between the language files and the locales config. With the provided change, the test would no longer fail on mis-match, instead only failing when the config contains more options which is a rarer scenario. Let me know if I've mis-understood this change though. |
|
@ssddanbrown yes, you understood me correctly. The test will fail if the config has some language that does not exist in the langs directory, but if the lang directory contains some language missing in the config - it will pass. This is ok in my opinion. Languages could be merged and the config can be updated later (for example, if we don't want to add some language to the config until it will not be fully translated) - in this case, the application will work correctly, all tests will pass, and will not distract other developers. |
The trouble is that this test is supposed to fail in the scenario of new languages being added, that's it's entire purpose, as a reminder that a new language has been added and that the config needs to be updated to align, hence this PR invalidates the reason I wrote the test. It shouldn't really distract other developers since the I10n branch won't be under their responsibility. |
|
@ssddanbrown okay, as a reminder - it makes sense. |
Hey everyone!
In this PR I've updated the
LanguageTest::test_locales_config_key_set_properlyUnit test to better handle such cases when a new locale is added in Crowdin.As we can see here, a new locale was added in Crowdin and the test fails. The strict comparison is unnecessary. It should be enough to check if there are corresponding language folders to the config locales. Nothing happens if the lang folder will contain extra language.
In addition, I've made a minor fix of Crowdin name in the issue template 🙂