test_bug519468.js fails on non-English Linux systems

RESOLVED FIXED in Firefox 16

Status

()

Toolkit
Startup and Profile System
RESOLVED FIXED
7 years ago
5 years ago

People

(Reporter: BenB, Assigned: Irving)

Tracking

(Blocks: 2 bugs)

Trunk
mozilla17
All
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox16 fixed)

Details

(Whiteboard: [qa-])

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

7 years ago
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?
(Reporter)

Comment 1

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

Updated

7 years ago
Blocks: 514067
Blocks: 542999
(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)
I am seeing the same issue with en-CA on OS X.
(Reporter)

Updated

6 years ago
Severity: normal → major
Anyone want to work up a patch for this?
Severity: major → minor
(Reporter)

Comment 5

6 years ago
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.
Severity: minor → normal
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?
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?
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.
Created attachment 597277 [details] [diff] [review]
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)
Attachment #597277 - Flags: feedback?(l10n)
Attachment #597277 - Flags: feedback?(benjamin)
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.
Attachment #597277 - Flags: feedback?(benjamin) → feedback+
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.
Attachment #597277 - Flags: feedback?(l10n) → feedback+
Assignee: 21 → irving
Created attachment 601267 [details] [diff] [review]
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
Attachment #597277 - Attachment is obsolete: true
Attachment #601267 - Flags: review?(l10n)
Status: NEW → ASSIGNED
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?
Attachment #601267 - Flags: review?(l10n)
Attachment #601267 - Flags: review?(benjamin)
Attachment #601267 - Flags: feedback+
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
Attachment #601267 - Flags: review?(benjamin) → review+
Created attachment 652194 [details] [diff] [review]
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.
Attachment #601267 - Attachment is obsolete: true
Attachment #652194 - Flags: review+
Pushed to mozilla-inbound:

https://hg.mozilla.org/integration/mozilla-inbound/rev/9819f1e610ef
https://hg.mozilla.org/mozilla-central/rev/9819f1e610ef
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
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".
Blocks: 792864
status-firefox16: --- → fixed
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.