557024 - test_bug519468.js fails on non-English Linux systems

Status: RESOLVED FIXED

(Toolkit :: Startup and Profile System, defect)

Description

[Ben Bucksch (:BenB)]

As said in bug 519468 comment 23:

This fails for me on a German system:
TEST-UNEXPECTED-FAIL |
.../mozilla/_tests/xpcshell/test_chrome/unit/test_bug519468.js | en-US == de-DE

Can you please fix this?

Comment 1

[Ben Bucksch (:BenB)]

Setup:
- Ubuntu 9.10 with Gnome session
- env: LANG="de_DE.UTF-8"

Comment 2

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to comment #1)
> Setup:
> - Ubuntu 9.10 with Gnome session
> - env: LANG="de_DE.UTF-8"

Does it works now? (for info I'm using a non-english system too)

Comment 3

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

I am seeing the same issue with en-CA on OS X.

Comment 4

[Dave Townsend [:mossop]]

Anyone want to work up a patch for this?

Comment 5

[Ben Bucksch (:BenB)]

Dave, this makes tests fail on all non-english systems. It's by no means minor. This passes the definition of "Severity: blocker" by hindering dev.
The one to fix it is the one who wrote the test, or owns the code.

Comment 6

[Axel Hecht [:Pike]]

I looked at this test many moons ago, and IIRC, I tracked this down to an actual failure of the code path in specific combinations of installed locales and OS settings. In particular, the logic depends on a pref being explicitly set, which you cannot do if you need to set it to the default value.

The real fix, IMHO, would be to drop the matchOS pref, and use a special value in our pref that controls the selected locale. Talked about that with :bs way back when, too. Given how many people out there make assumptions on general.useragent.locale, it might be time to pick a different pref for the combined explicit and matching locale selection.

If my theory holds, which someone should verify, then disabling the test might be the right immediate measure?

Comment 7

[:Irving Reid (No longer working on Firefox)]

I'm also seeing this on Mac OS X, en-CA (same as :espindola), except I'm building the platform bits required by Thunderbird.

One question: What should the behaviour of matchOS be if the OS locale isn't available in the chrome? Fall back to the locale preference, or to en-US?

Comment 8

[Axel Hecht [:Pike]]

The chrome registry falls back to en-US in case it can't find a match for the preferred locale. That's independent on whether the preferred one is coming from general.useragent.locale or the OS with matchOS.

Comment 9

[:Irving Reid (No longer working on Firefox)]

Attachment: Refactor test case to pass on non en-US systems

Stubbed out the Locale Service so that the test can control what OS locale the preference handling sees, and adjusted the test cases to cover both when the OS locale is supported by the manifest, and when it isn't.

Not sure this is ready for check in. While fixing the test, I noticed that the observer callback may be called multiple times for each test iteration (when either of the preferences is changed) even if the resulting selected locale doesn't change. Is that the behaviour we want, or should the observer callback only happen when the externally visible locale changes?

Also, while I didn't see the logic error Axel referred to, the new test structure should make it easier to pin down if there really is something wrong in the code.

The feedback I'd like is whether to put this patch through, or to investigate the observer callback and possible logic issues first (along with any corrections needed in the patch itself)

Comment 10

[Benjamin Smedberg]

Comment on attachment 597277 [details] [diff] [review]
Refactor test case to pass on non en-US systems

This seems pretty fragile, especially because you aren't implementing all of the locale service and you're relying on it not being instantiated before.

One possible way around this is to have the chrome registry ask for a testing-only locale service first, and if that is not present use the regular one. But if you're fine with possibly becoming the maintainer of a potentially fragile test, I think I'm ok with this as it is.

Comment 11

[Axel Hecht [:Pike]]

Comment on attachment 597277 [details] [diff] [review]
Refactor test case to pass on non en-US systems

Review of attachment 597277 [details] [diff] [review]:
-----------------------------------------------------------------

I guess I'm with Benjamin on the feelings about how reliable this is.

I think there are more configuration pitfalls, and future proof questions, see below.

I also don't think this test gets worse by what you propose, so f+ from me.

::: chrome/test/unit/test_bug519468.js
@@ +102,1 @@
>  ];

I'd prefer non-standards locale codes to be prefixed with x-. If the chrome reg becomes more strict, we'd see fake errors here.

@@ +110,2 @@
>    prefService.setBoolPref("intl.locale.matchOS", aTest.matchOS);
>    prefService.setCharPref("general.useragent.locale", aTest.selected || "en-US");

As we're at it, this code may not do what we're expecting in non-en-US builds, i.e., if you're building with --enable-ui-locale.

Comment 12

[:Irving Reid (No longer working on Firefox)]

Attachment: Refactored test case, incorporating feedback

Incorporated comments from :bsmedberg and :pike. In my understanding, the xpcshell harness gets re-created for each .js file so I'm not concerned with this test case causing headaches for anything outside itself; based on that, I'm OK with fixing it in future if additional parts of the mock service become necessary.

The new version has been tested both with a default build and with --enable-ui-locale=fr

Comment 13

[Axel Hecht [:Pike]]

Comment on attachment 601267 [details] [diff] [review]
Refactored test case, incorporating feedback

Review of attachment 601267 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry for the lag.

I think the changes look good, but I'm too far away from coding practices these days to do a real review.

Benjamin?

Comment 14

[Benjamin Smedberg]

Comment on attachment 601267 [details] [diff] [review]
Refactored test case, incorporating feedback

Why is the unregisterFactory call necessary? I'm pretty sure it's sufficient to register the new factory (which replaces the contractid mapping) and leave the old one in place. r=me with that change, or please explain and re-request review if that won't work for some reason

Comment 15

[:Irving Reid (No longer working on Firefox)]

Attachment: Make test reliable on non en_US systems, incorporating bsmedberg's feedback

The unregisterFactory code was copied from many other examples of test cases that register mock factories; see http://mxr.mozilla.org/comm-central/search?string=unregisterFactory&find=test&findi=&filter=^[^\0]*%24&hitlimit=&tree=comm-central

That said, the test works without the unregisterFactory calls so I've attached an updated version.

Carrying forward the r=bsmedberg from comment 14.

Comment 16

[:Irving Reid (No longer working on Firefox)]

Pushed to mozilla-inbound:

https://hg.mozilla.org/integration/mozilla-inbound/rev/9819f1e610ef

Comment 17

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/9819f1e610ef

Comment 18

[Phil Ringnalda (:philor)]

Pushed to beta in https://hg.mozilla.org/releases/mozilla-beta/rev/aa77ad5b8f49 since for no visible reason some of the new 10.8 slaves fail this test claiming that their locale is "en".