[No QA] Add test for matching Translations keys #4090

parasharrajat · 2021-07-15T21:22:47Z

Details

Added Unit test that matches the keys from each Language file to the Main EN.js translations. If any key is missing, the test will fail. You will see a log stating the path of the key.

Fixed Issues

$ #4082

parasharrajat · 2021-07-15T21:31:32Z

@iwiznia this PR is itself a test for the PR. Check this https://github.com/Expensify/Expensify.cash/pull/4090/checks?check_run_id=3080986765#step:6:722.

parasharrajat · 2021-07-17T21:12:19Z

Please review the translations before merging. I don't speak Spanish but took help from google.

iwiznia

Sorry that I did not realize about this when creating the issue, but can we also add the opposite check? Basically to ensure we don't have tons of unused keys in the non base english files, we would also alert for any key present in non base english files that are not present on the english files.

iwiznia · 2021-07-19T15:03:57Z

tests/unit/TranslateTest.js

+        const excludeLanguages = ['en', 'es-ES'];
+        const languages = _.without(_.keys(originalTranslations.default), ...excludeLanguages);


Can we make this smarter and instead of having to add es-ES here make it skip any file that has 2 components? ie: pick up es but not es-ES

That was not the original Idea. After Sometime, we can decide to add more Translations. and Two component locale would be completely valid in that case. So to skip regardless of specification will lead to code change. So I think its better, not to make it smarter. It is sufficient to fulfill its purpose.

iwiznia · 2021-07-19T15:04:38Z

tests/unit/TranslateTest.js

@@ -52,3 +54,38 @@ describe('translate', () => {
        expect(translate.translate('en', ['testKeyGroup', 'testFunction'], {testVariable})).toBe(expectedValue);
    });
 });
+
+describe('Translation Keys', () => {
+    let activeLanguage;


Why is this "global"? Can't we pass it as a regular argument to matchKeys?

This is convenient.

iwiznia · 2021-07-19T15:05:27Z

tests/unit/TranslateTest.js

+        path += key ? `${key}.` : '';
+        const pathLevel = path;
+        if (key && !_.has(target, key)) {
+            console.debug(`🏹 ${path.slice(0, -1)} is missing from ${activeLanguage}.js`);


Instead of a debug log line, can we make it so that the test itself fails? Ideally it would show one failed test per missing key if that's possible, but if that's hard we can have one test fail that prints all missing keys.

Test will fail, if key don't match. This log is just to show what is missing.

Ideally it would show one failed test per missing key if that's possible,

I don't think its a good idea to create multiple tests that basically test the same thing.

if that's hard we can have one test fail that prints all missing keys.

This can be done. We can log all the missing keys.

ok cool, my main point is that it's better to have the test failure show what keys failed instead of having to look at the logs.

I tried to find way to add the missing key as failure message itself but I don't think its possible AFAIK. So we have to stick with logs. Could you please give me a hint about how to do that if possible?

hmmmm, I think that if you change the expect from toBeTruthy to something like arrayContaining would make jest to print the diff of the arrays and thus the missing keys?

parasharrajat · 2021-07-19T15:20:00Z

Basically to ensure we don't have tons of unused keys in the non base english files, we would also alert for any key present in non base english files that are not present on the english files.

Hmm, let me think about it.

iwiznia · 2021-07-26T11:18:07Z

Any update here @parasharrajat? I see we are missing translations in several places and this would prevent all those errors.

parasharrajat · 2021-07-26T12:23:18Z

@iwiznia I am thinking to rewrite the tests with a simpler and robust implementation. I will post multiple test clauses for each language file...etc. Maybe I'll finish it today.

parasharrajat · 2021-07-29T07:41:27Z

@iwiznia It's ready. Expect.arrayContaining is not useful as it does not give the required output and does not tell us which keys are missing. WRT, it outputs huge string on the logs which basically make it non-readable. So I have created our on Logs using console.log.

iwiznia

Looks great, but you got conflicts. Can you:

Resolve the conflicts
Make some tests fail in one of your commits, so I can look at the output of the failing tests in the GH actions
Please?

tests/unit/TranslateTest.js

parasharrajat · 2021-07-29T13:30:47Z

@iwiznia I have made few changes to es.js which will result in a test failure. Please check https://github.com/Expensify/App/pull/4090/checks?check_run_id=3192256522#step:6:595.

iwiznia · 2021-07-29T13:42:48Z

Ah damn, that's not that great as the exact failures are mixed in with other non useful logs. Making some suggestions in the PR to see if we can make it better.

iwiznia · 2021-07-29T13:44:46Z

tests/unit/TranslateTest.js

+            if (hasAllKeys.length) {
+                console.debug(`🏹 [ ${hasAllKeys.join(', ')} ] are missing from ${ln}.js`);
+            }
+            expect(hasAllKeys.length).toBe(0);


So, I was thinking, maybe we could do this and get a better error message?

Suggested change

expect(hasAllKeys.length).toBe(0);

expect(hasAllKeys).toBeEqual([]);

I think this will show in the test result exactly which keys are missing, no?
(same applies to the one below)

It does not show the keys that are not correct. It just shows Expected and produced output. So all the correct and wrong keys would be shown as a big string which is like 50 lines long.

Expected: 0 <-- would be all the keys on main language in green color. Received: 2 <-- would be all the keys on matching language in red color.

I tested with expect.objectContaining and expect.arrayContaining.

hmmmm, can we change the code to do _.without(mainLagunageKeys, ...languageKeys)? That would return only the missing keys, right?

It currently only gives the missing keys. Again _.without(mainLagunageKeys, ...languageKeys) will go in console.log. @iwiznia

The console log is not good enough as it is super hard to find the right message. In any case I don't think you understood what I meant, because I just tried and it works. Here's the test code:

it(`Does ${ln} locale has all the keys`, () => { const hasAllKeys = _.without(mainLanguageKeys, ...languageKeys); expect(hasAllKeys).toEqual([]); }); it(`Does ${ln} locale has unused keys`, () => { const hasAllKeys = _.without(languageKeys, ...mainLanguageKeys); expect(hasAllKeys).toEqual([]); });

And this is the output:

● Translation Keys › Does es locale has all the keys expect(received).toEqual(expected) // deep equality - Expected - 1 + Received + 4 - Array [] + Array [ + "preferencesPage.mostRecent", + "passwordForm.error.incorrectLoginOrPassword", + ] 79 | it(`Does ${ln} locale has all the keys`, () => { 80 | const hasAllKeys = _.without(mainLanguageKeys, ...languageKeys); > 81 | expect(hasAllKeys).toEqual([]); | ^ 82 | }); 83 | 84 | it(`Does ${ln} locale has unused keys`, () => { at Object.<anonymous> (tests/unit/TranslateTest.js:81:32) ● Translation Keys › Does es locale has unused keys expect(received).toEqual(expected) // deep equality - Expected - 1 + Received + 4 - Array [] + Array [ + "initialSettingsPage.appDownloadLinks.window.label", + "initialSettingsPage.appDownloadLinks.Rajat.label", + ] 84 | it(`Does ${ln} locale has unused keys`, () => { 85 | const hasAllKeys = _.without(languageKeys, ...mainLanguageKeys); > 86 | expect(hasAllKeys).toEqual([]); | ^ 87 | }); 88 | }); 89 | }); at Object.<anonymous> (tests/unit/TranslateTest.js:86:32)

oh Yeah. This way I got it now.

@iwiznia Check this out https://github.com/Expensify/App/actions/runs/1079637864. I added annotations.

parasharrajat · 2021-07-29T18:33:40Z

@iwiznia Thanks for the hint. I have updated the PR with the following changes:

Your suggestion of toBeEqual([]).
Added Annotations to quickly see what is the cause of failure.

Separate tests (Missing key and unused key) for each translation file.
Fixed all the translations and Finalized the PR.

It's now ready to be merged.

iwiznia

Ah lovely!

OSBotify · 2021-07-29T19:11:40Z

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

OSBotify · 2021-07-30T20:10:08Z

🚀 Deployed to staging in version: 1.0.81-5🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	failure ❌
🕸 web 🕸	success ✅

OSBotify · 2021-08-06T02:44:01Z

🚀 Deployed to production in version: 1.0.82-7🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

add test for Translations keys match

f31e802

parasharrajat requested a review from a team as a code owner July 15, 2021 21:22

MelvinBot requested review from iwiznia and removed request for a team July 15, 2021 21:22

added translations

9e0fe2a

iwiznia requested changes Jul 19, 2021

View reviewed changes

parasharrajat mentioned this pull request Jul 26, 2021

[Multi-language Support] No way of correctly translating time when messages were sent. #4219

Closed

parasharrajat added 4 commits July 29, 2021 12:57

fix: translation tests

6ad65b7

fix translation files

26626ec

Pull latest changes

b1e7dc2

added missing key

5551926

parasharrajat requested a review from iwiznia July 29, 2021 07:39

iwiznia reviewed Jul 29, 2021

View reviewed changes

tests/unit/TranslateTest.js Outdated Show resolved Hide resolved

tests/unit/TranslateTest.js Outdated Show resolved Hide resolved

parasharrajat added 2 commits July 29, 2021 18:52

refactor tests

158dfb3

pulled latest changes and Show test failure

72a3280

iwiznia reviewed Jul 29, 2021

View reviewed changes

fix: tests

f9baac6

parasharrajat force-pushed the tests branch from 2c6426c to f9baac6 Compare July 29, 2021 18:31

iwiznia approved these changes Jul 29, 2021

View reviewed changes

iwiznia merged commit 039ec01 into Expensify:main Jul 29, 2021

This was referenced Jul 30, 2021

Deploy Checklist: Expensify.cash 2021-07-28 #4279

Closed

Deploy Checklist: Expensify.cash 2021-07-30 #4333

Closed

parasharrajat deleted the tests branch November 20, 2023 13:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[No QA] Add test for matching Translations keys #4090

[No QA] Add test for matching Translations keys #4090

parasharrajat commented Jul 15, 2021 •

edited

Loading

parasharrajat commented Jul 15, 2021 •

edited

Loading

parasharrajat commented Jul 17, 2021

iwiznia left a comment

iwiznia Jul 19, 2021

parasharrajat Jul 19, 2021

iwiznia Jul 19, 2021

parasharrajat Jul 19, 2021

iwiznia Jul 19, 2021

parasharrajat Jul 19, 2021

iwiznia Jul 19, 2021

parasharrajat Jul 19, 2021 •

edited

Loading

iwiznia Jul 19, 2021

parasharrajat commented Jul 19, 2021

iwiznia commented Jul 26, 2021

parasharrajat commented Jul 26, 2021

parasharrajat commented Jul 29, 2021

iwiznia left a comment

parasharrajat commented Jul 29, 2021

iwiznia commented Jul 29, 2021

iwiznia Jul 29, 2021

parasharrajat Jul 29, 2021 •

edited

Loading

parasharrajat Jul 29, 2021

iwiznia Jul 29, 2021

parasharrajat Jul 29, 2021 •

edited

Loading

iwiznia Jul 29, 2021

parasharrajat Jul 29, 2021

parasharrajat Jul 29, 2021

parasharrajat commented Jul 29, 2021 •

edited

Loading

iwiznia left a comment

OSBotify commented Jul 29, 2021

OSBotify commented Jul 30, 2021

OSBotify commented Aug 6, 2021

		const excludeLanguages = ['en', 'es-ES'];
		const languages = _.without(_.keys(originalTranslations.default), ...excludeLanguages);

	expect(hasAllKeys.length).toBe(0);
	expect(hasAllKeys).toBeEqual([]);

[No QA] Add test for matching Translations keys #4090

[No QA] Add test for matching Translations keys #4090

Conversation

parasharrajat commented Jul 15, 2021 • edited Loading

Details

Fixed Issues

parasharrajat commented Jul 15, 2021 • edited Loading

parasharrajat commented Jul 17, 2021

iwiznia left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

parasharrajat Jul 19, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

parasharrajat commented Jul 19, 2021

iwiznia commented Jul 26, 2021

parasharrajat commented Jul 26, 2021

parasharrajat commented Jul 29, 2021

iwiznia left a comment

Choose a reason for hiding this comment

parasharrajat commented Jul 29, 2021

iwiznia commented Jul 29, 2021

Choose a reason for hiding this comment

parasharrajat Jul 29, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

parasharrajat Jul 29, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

parasharrajat commented Jul 29, 2021 • edited Loading

iwiznia left a comment

Choose a reason for hiding this comment

OSBotify commented Jul 29, 2021

OSBotify commented Jul 30, 2021

OSBotify commented Aug 6, 2021

parasharrajat commented Jul 15, 2021 •

edited

Loading

parasharrajat commented Jul 15, 2021 •

edited

Loading

parasharrajat Jul 19, 2021 •

edited

Loading

parasharrajat Jul 29, 2021 •

edited

Loading

parasharrajat Jul 29, 2021 •

edited

Loading

parasharrajat commented Jul 29, 2021 •

edited

Loading