1103385 - Warning about Plural form unknown for locale "null" breaks unit tests when using localization

Status: RESOLVED FIXED

(Add-on SDK Graveyard :: General, defect, P2)

Description

[Qeole]

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:35.0) Gecko/20100101 Firefox/35.0
Build ID: 20141110194511

Steps to reproduce:

I'm developping an add-on with the add-on SDK, using localization, and implementing unit tests.

For info / to reproduce, my add-on source is available there:
https://github.com/Qeole/Enlight

My add-on SDK version is 1.17.
I obtained the same behavior with Firefox 33.1.1 and 35.0a2.


Actual results:

When launching unit tests with |cfx test|, I'm getting following warning:
console.warn: <module_name>: Plural form unknown for locale "null"

This warning is then caught by the harness clean-up function in [1] and causes a supplementary test to fail (see [2] for example output).

It seems it is caused by the code in [3], which fails to find plural form for "null" locale; it looks like the add-on SDK does not set any locale for the browser when running tests with |cfx test|?


Expected results:

Localization should not break unit tests.
Possible solutions could be:
- somehow setting a plural form when locale is "null"
- setting a locale for unit tests
- changing log level from "warning" to "info" (but it was already downgraded from "error" level in bug 746137)
- …



[1] http://hg.mozilla.org/mozilla-central/annotate/7ab92d922d19/addon-sdk/source/lib/sdk/test/harness.js#l235
[2] https://stackoverflow.com/questions/26264807/2-of-3-tests-passed-when-only-2-test-methods-were-exported
[3] http://hg.mozilla.org/mozilla-central/annotate/7ab92d922d19/addon-sdk/source/lib/sdk/l10n/plural-rules.js#l398

Comment 1

[Qeole]

Note: exporting LANG|LANGUAGE env variables (e.g. |export LANG=en_US.UTF-8| in bash) does not help.

Comment 2

[Adrian Aichner [:anaran]]

This might be the culprit:

https://github.com/mozilla/addon-sdk/blob/712a4874e0d1600cbb9d3311e79073dcf4ac14e1/lib/sdk/l10n/json/core.js#L35

Anyway, these are the searchable sources if you want to try your luck on these.

Comment 3

[Qeole]

Yes, replacing |null| with |"en"| at this line seems to do the trick. I don't know if this is what we want to do here? Or if changing it might have some impact on something else in the SDK?

Comment 4

[Adrian Aichner [:anaran]]

I made these changes for debugging:

diff --git a/lib/main.js b/lib/main.js
index e13e177..a32108b 100644
--- a/lib/main.js
+++ b/lib/main.js
@@ -5,6 +5,9 @@ var buttons  = require('sdk/ui/button/toggle');
 var tabs     = require("sdk/tabs");
 var panels   = require("sdk/panel");
 var spref    = require('sdk/simple-prefs');
+let ps = require("sdk/preferences/service");
+console.log('general.useragent.locale', ps.get('general.useragent.locale'));
+console.log('intl.accept_languages', ps.get('intl.accept_languages'));
 var _        = require("sdk/l10n").get; // Localization

 /*

diff --git a/lib/sdk/l10n/plural-rules.js b/lib/sdk/l10n/plural-rules.js
index 22e5b8b..a0410f1 100644
--- a/lib/sdk/l10n/plural-rules.js
+++ b/lib/sdk/l10n/plural-rules.js
@@ -396,7 +396,10 @@ exports.getRulesForLocale = function getRulesForLocale(loc\
ale) {
   let index = LOCALES_TO_RULES[locale];
   if (!(index in RULES)) {
     console.warn('Plural form unknown for locale "' + locale + '"');
+    console.trace();
     return function () { return "other"; };
+  } else {
+    console.log('Plural form at ' + index + ' for locale "' + locale + '"');
   }
   return RULES[index];
 }

I run
cmd /c "%HOME%/tmp/mozilla/addon-sdk/bin/activate && cfx test -o -b c:/programme/nightly/firefox.exe" -p ../cfx3 -v > cfx-test-`dtz`.txt 2>&1
and it reveals (see below) that
var _        = require("sdk/l10n").get; // Localization
(now at line 11 with my changes) is causing the warning.

This happens even though the firefox preferences I log before that show general.useragent.locale en-US.

This all turns out to be harmless.
If you want to run the plural tests you can just
cp ../addon-sdk/test/test-l10n-plural-rules.js test/

Running your tests as usual will show all the plural rules tests passing from Arabic to Japanese.

The warning in this context is still an annoyance and I hope to get some geuidance from the addon-sdk developers.

Adrian

Welcome to the Add-on SDK. For the docs, visit https://developer.mozilla.org/en\
-US/Add-ons/SDK
Warning: missing module: resource://gre/modules/Preferences.jsm
Warning: missing module: ../../framescript/FrameScriptManager.jsm
Using binary at 'c:/programme/nightly/firefox.exe'.
Using profile at 'C:\Users\AichnerAd\tmp\mozilla\cfx3'.
Running tests on Firefox 36.0a1/Gecko 36.0a1 ({ec8030f7-c20a-464f-9b0e-13a3a9e9\
7384}) under winnt/x86.
console.log: enlight: general.useragent.locale en-US
console.log: enlight: intl.accept_languages chrome://global/locale/intl.propert\
ies
console.warn: enlight: Plural form unknown for locale "null"
console.trace: enlight:
_10n/plural-rules.js 399 getRulesForLocale
_js.path/sdk/l10n.js 16
_h/toolkit/loader.js 257 evaluate
_h/toolkit/loader.js 308 load
_oader/cuddlefish.js 129 CuddlefishLoader/options<.load
_h/toolkit/loader.js 581 require
_enlight/lib/main.js 11
_h/toolkit/loader.js 257 evaluate
_h/toolkit/loader.js 308 load
_oader/cuddlefish.js 129 CuddlefishLoader/options<.load
_h/toolkit/loader.js 581 require
_/tests/test-main.js 5

Comment 5

[elinv.elinv]

Al intentar correr un ejemplo básico, me dice constantemente lo siguiente
-----------------------------------
JPM undefined Starting jpm run on dos
Creating XPI
JPM undefined XPI created at /tmp/@dos-0.0.1.xpi (119ms)
Created XPI at /tmp/@dos-0.0.1.xpi
JPM undefined Creating a new profile
console.warn: dos: Plural form unknown for locale "null"
-----------------------------------
En Ubuntu 15.04, Firefox 38.0
Con cfx run me dice exactamente lo mismo.
Si alguién me puede ayudar lo agradecería???

Comment 6

[Frédéric Wang (:fredw)]

Here is the description for JPM:

Steps to reproduce:

1) Create a new "test" add-on with the default values

fred@debian:~$ mkdir test && cd test && jpm init
title: (My Jetpack Addon) 
name: (test) 
version: (0.0.1) 
description: (A basic add-on) 
entry point: (index.js) 
author: 
engines (comma separated): (firefox,fennec) 
license: (MIT) 
JPM undefined About to write to /home/fred/test/package.json:

{
  "title": "My Jetpack Addon",
  "name": "test",
  "version": "0.0.1",
  "description": "A basic add-on",
  "main": "index.js",
  "author": "",
  "engines": {
    "firefox": ">=38.0a1",
    "fennec": ">=38.0a1"
  },
  "license": "MIT"
}

Is this ok? (yes) 

2) Open test/test-index.js and add the following line at the top of the file:

   var _    = require("sdk/l10n").get;

3) Run the tests with the command "jpm test -b iceweasel"

Actual result:

The tests fail with the following message:

JPM undefined Starting jpm test on My Jetpack Addon
Creating XPI
JPM undefined XPI created at /tmp/@test-0.0.1.xpi (22ms)
Created XPI at /tmp/@test-0.0.1.xpi
JPM undefined Creating a new profile
Running tests on Iceweasel 39.0/Gecko 39.0 ({ec8030f7-c20a-464f-9b0e-13a3a9e97384}) under linux/x86_64.
console.warn: test: Plural form unknown for locale "null"
...console.error: test: 
  warnings and/or errors were logged.

3 of 4 tests passed.
There were test failures...

Expected result:

The tests should pass.

Comment 7

[Frédéric Wang (:fredw)]

Attachment: Patch (first proposal)

Comment 8

[Frédéric Wang (:fredw)]

Attachment: Patch (second proposal)

Comment 9

[Frédéric Wang (:fredw)]

So I've submitted two pull requests with different proposals:

- Attachment 8642937 [details] is what was proposed in comment 3, that is just make json.language() defaults to "en" instead of "null". This function does not seem to be used elsewhere than in l10n.js, so that should be safe (but perhaps it can be used from outside the addon-sdk code)

- Attachment 8642936 [details] instead just changes the default of a local variable to "en" so it won't appear outside l10n.js ; this is consistent with what pluralMappingFunction already does (defaulting to "en").

Comment 10

[Frédéric Wang (:fredw)]

More remarks:

I'm using *.properties file to localize an add-on and indeed lib/sdk/l10n/json/core.js will not be able to detect the best matching locale in that case.

An "obvious" workaround would be to add dummy JSON localization files but that still does not seem to work. Some debugging shows that:

1) lib/sdk/addon/runner.js, calls ./lib/sdk/l10n/loader.js and correctly sets @l10n/data with the expected localization data.
2) Then lib/sdk/l10n/json/core.js is called once, with the expected localization data.
3) Then lib/sdk/deprecated/unit-test.js is called but ./lib/sdk/l10n/loader.js is not called and @l10n/data is probably not set.
4) Then lib/sdk/l10n/json/core.js is called a second time and the localization data are not set.
5) Finally the warning about null locale happens.

Probably, unit-test.js should sets @l10n/data

Comment 11

[Martin Giger [:freaktechnik]]

I don't think erikvold does reviews anymore (his bugzilla account is disabled).

Comment 12

[Frédéric Wang (:fredw)]

Who should I ask for reviews? (assuming someone is still maintaining the add-on SDK)

Comment 13

[Martin Giger [:freaktechnik]]

Sadly I have no idea either. I'd ask someone else who is on this list: https://github.com/mozilla/addon-sdk/blob/master/CONTRIBUTING.md#reviewers (I think that's also the suggested reviewers by bugzilla). But I have no idea of who is in charge.

Comment 14

[Frédéric Wang (:fredw)]

(In reply to Martin Giger [:freaktechnik] from comment #13)
> Sadly I have no idea either. I'd ask someone else who is on this list:
> https://github.com/mozilla/addon-sdk/blob/master/CONTRIBUTING.md#reviewers
> (I think that's also the suggested reviewers by bugzilla). But I have no
> idea of who is in charge.

@Dave: any idea?

Comment 15

[Dave Townsend [:mossop]]

(In reply to Frédéric Wang (:fredw) from comment #14)
> (In reply to Martin Giger [:freaktechnik] from comment #13)
> > Sadly I have no idea either. I'd ask someone else who is on this list:
> > https://github.com/mozilla/addon-sdk/blob/master/CONTRIBUTING.md#reviewers
> > (I think that's also the suggested reviewers by bugzilla). But I have no
> > idea of who is in charge.
> 
> @Dave: any idea?

No-one is actively maintaining the SDK at this point. You can try jsantell for review he might have more time available than me right now.

Comment 16

[Jordan Santell [:jsantell] [@jsantell]]

Comment on attachment 8642937 [details] [review]
Patch (second proposal)

I think either way looks good -- If this is the default for language, I'd assume all other things that return locales will also be non-falsy, returning a similar default ("en", "en-US").

Granted I'm very unfamiliar with the l10n code in SDK, but this seems like a good plan if all defaults are consistent and tests pass.

Comment 17

[Frédéric Wang (:fredw)]

Attachment: Patch

Comment 18

[Frédéric Wang (:fredw)]

Comment on attachment 8656540 [details] [review]
Patch

Travis seems to have some failures at the moment, so I didn't check whether this breaks tests...

https://travis-ci.org/mozilla/addon-sdk/branches

Comment 19

[Jordan Santell [:jsantell] [@jsantell]]

Comment on attachment 8656540 [details] [review]
Patch

SDK is now being merged via patches through bugzilla like the rest of mozilla-central -- can you add a patch for this?

Comment 20

[Frédéric Wang (:fredw)]

Attachment: Patch

https://treeherder.mozilla.org/#/jobs?repo=try&revision=fe2d0682f195

Comment 21

[Nigel Babu [:nigelb]]

Frédéric, Jordan isn't listed as a reviewer for the add-ons sdk module. Can you either update the Modules page or have someone who's listed as peer/owner give an r+?

Comment 22

[Frédéric Wang (:fredw)]

@Nigel: As I understand, Mozilla is moving away from the addon sdk and so I assume there have been some changes in the organization. See comment 11 and below where I asked about who is in charge of reviewing the patches now. I believe someone who is more knowledgeable than me about the internals should update the Modules page or forward the review to the appropriate person.

Comment 23

[Jordan Santell [:jsantell] [@jsantell]]

I've been a core maintainer on the SDK team since 2013, modules page should probably be uodated

Comment 24

[Nigel Babu [:nigelb]]

Thanks jsantell, I'm going to add checkin-needed flag back on. Wes should catch this one. If not, I'll land it when I wake up. Sorry about the confusion :(

Comment 25

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/0e6788084525

Comment 26

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/0e6788084525