Add unit test for l10n.js

Add unit test for l10n.js

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=284959

[kelvinp, 2014-07-18 00:57:05]

PTAL

[Jamie, 2014-07-18 02:04:51]

LGTM once my comments are addressed.

https://codereview.chromium.org/399273003/diff/60001/remoting/webapp/unittest...
File remoting/webapp/unittests/l10n_unittest.js (right):

https://codereview.chromium.org/399273003/diff/60001/remoting/webapp/unittest...
remoting/webapp/unittests/l10n_unittest.js:12: chrome.i18n.getMessage =
chrome.i18n.getMessage || function() {};
The use of || here doesn't feel right. It's generally used in places where you
can't be sure what the execution order is, and it's only useful if it's used
consistently in all locations (otherwise anything added to the namespace risks
being overwritten by the one location that doesn't use ||). You mentioned that
you're pulling in the _proto.js files; in that case, it might be better to
assert that the namespace is defined here.

https://codereview.chromium.org/399273003/diff/60001/remoting/webapp/unittest...
remoting/webapp/unittests/l10n_unittest.js:21: var translation =
l10n.getTranslationOrError('not_existing_tag');
s/no_existing/non_existent/?

https://codereview.chromium.org/399273003/diff/60001/remoting/webapp/unittest...
remoting/webapp/unittests/l10n_unittest.js:33: equal(element.innerHTML,
'<b>Hello World</b>');
Why test both of these? Doesn't the first test imply the second? I think testing
innerHTML in all these test cases will suffice.

https://codereview.chromium.org/399273003/diff/60001/remoting/webapp/unittest...
remoting/webapp/unittests/l10n_unittest.js:76: var fixture =
document.getElementById('qunit-fixture');
Nit: indentation.

https://codereview.chromium.org/399273003/diff/60001/remoting/webapp/unittest...
remoting/webapp/unittests/l10n_unittest.js:78: ' i18n-value-1="param1"
i18n-value-2="param2"></div>';
For inline HTML that's too long for one line, I think it's more readable to
format it as you would in an HTML file. See
https://codereview.chromium.org/401623004/diff/20001/remoting/webapp/unittest...,
for example.

[kelvinp, 2014-07-21 17:46:22]

https://codereview.chromium.org/399273003/diff/60001/remoting/webapp/unittest...
File remoting/webapp/unittests/l10n_unittest.js (right):

https://codereview.chromium.org/399273003/diff/60001/remoting/webapp/unittest...
remoting/webapp/unittests/l10n_unittest.js:12: chrome.i18n.getMessage =
chrome.i18n.getMessage || function() {};
On 2014/07/18 02:04:50, Jamie wrote:
> The use of || here doesn't feel right. It's generally used in places where you
> can't be sure what the execution order is, and it's only useful if it's used
> consistently in all locations (otherwise anything added to the namespace risks
> being overwritten by the one location that doesn't use ||). You mentioned that
> you're pulling in the _proto.js files; in that case, it might be better to
> assert that the namespace is defined here.

Done.

https://codereview.chromium.org/399273003/diff/60001/remoting/webapp/unittest...
remoting/webapp/unittests/l10n_unittest.js:21: var translation =
l10n.getTranslationOrError('not_existing_tag');
On 2014/07/18 02:04:51, Jamie wrote:
> s/no_existing/non_existent/?

Done.

https://codereview.chromium.org/399273003/diff/60001/remoting/webapp/unittest...
remoting/webapp/unittests/l10n_unittest.js:33: equal(element.innerHTML,
'<b>Hello World</b>');
On 2014/07/18 02:04:51, Jamie wrote:
> Why test both of these? Doesn't the first test imply the second? I think
testing
> innerHTML in all these test cases will suffice.

Done.

https://codereview.chromium.org/399273003/diff/60001/remoting/webapp/unittest...
remoting/webapp/unittests/l10n_unittest.js:76: var fixture =
document.getElementById('qunit-fixture');
On 2014/07/18 02:04:51, Jamie wrote:
> Nit: indentation.

Done.

https://codereview.chromium.org/399273003/diff/60001/remoting/webapp/unittest...
remoting/webapp/unittests/l10n_unittest.js:78: ' i18n-value-1="param1"
i18n-value-2="param2"></div>';
On 2014/07/18 02:04:50, Jamie wrote:
> For inline HTML that's too long for one line, I think it's more readable to
> format it as you would in an HTML file. See
>
https://codereview.chromium.org/401623004/diff/20001/remoting/webapp/unittest...,
> for example.

Done.

[kelvinp, 2014-07-21 17:46:26]

The CQ bit was checked by kelvinp@chromium.org

[commit-bot: I haz the power, 2014-07-21 17:47:19]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/kelvinp@chromium.org/399273003/100001

[commit-bot: I haz the power, 2014-07-21 19:20:02]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)

[commit-bot: I haz the power, 2014-07-21 21:00:12]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-07-21 21:00:12]

Try jobs failed on following builders:
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)

[kelvinp, 2014-07-23 14:11:46]

The CQ bit was checked by kelvinp@chromium.org

[commit-bot: I haz the power, 2014-07-23 14:12:20]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/kelvinp@chromium.org/399273003/100001

[commit-bot: I haz the power, 2014-07-23 16:54:29]

Change committed as 284959