684241 - Add tests for locale picker

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Wesley Johnston (:wesj)]

Attachment: WIP Patch

We need some tests for this! I started writing some and found a few bugs along the way as well. I'm attaching a WIP, but still needs some cleanup to be ready for review. Just wanted to save my place for now.

Comment 1

[Wesley Johnston (:wesj)]

Attachment: Patch

This looks kinda complex and will likely give you a headache to read. Sorry :(

There's a little bit of cleanup in here. A spelling fix, and one small "fix" so that if you cancel installing an downloadable locale (likely because the download failed?), we don't immediately revert the locale picker language back to English.

The rest is mostly a single "test". By feeding in various options, it can test over various permutations of:

1.) opening the locale picker with and without an "opener" window,
2.) canceling out (or "Continue In English" for the first-run state), or continuing through and
3.) attempting to install valid and invalid (404) locales.

Lots of checking that labels are updated at the correct times and that we end up with restart notifications shown or not shown and with prefs in the right order.

I also removed a check in set locale() because (like I noticed in bug 688824) it can cause problems with downloadable locales. Downloadable locales currently don't change the value of LocalesUI._locale, but they could. In fact, with some tests here now, I'd feel better about doing that and leaving this check in if you want.

These all pass fine on desktop for me. I'm going to push all of these to try in the morning and see how they fare. Maybe get them running on my device for testing as well.

Comment 2

[Wesley Johnston (:wesj)]

Comment on attachment 563320 [details] [diff] [review]
Patch

This had some problems on try: https://tbpl.mozilla.org/php/getParsedLog.php?id=6610731&tree=Try&full=1#error0

removing review while I fix...

Comment 3

[Wesley Johnston (:wesj)]

Just to update. Fix try push was missing some files. Second try run a problem with these tests not cleaning up very well. Third one is on try now...

Comment 4

[Wesley Johnston (:wesj)]

Attachment: Patch v2

Yay! Try looks green (at least on b-c tests).

Comment 5

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 563845 [details] [diff] [review]
Patch v2


>-    if (needsRestart)
>+    if (needsRestart) {
>+      dump("xxx: show restart?\n");
>       ExtensionsView.showRestart(mode);
>+    }

revert this

>diff --git a/mobile/chrome/tests/browser_localepicker.js b/mobile/chrome/tests/browser_localepicker.js
>+function LocaleTest(aName, aOptions) {

>+    run: function lt_run() {
>+      this.win = Services.ww.openWindow(aOptions.opener, "chrome://browser/content/localePicker.xul", "_browser", "chrome,dialog=no,all", null);
>+      this.win.addEventListener("load", this.loadedWindow.bind(this), false);
>+    },
>+  
>+    loadedWindow: function lt_loadedWindow() {
>+      this.win.removeEventListener("load", this.loadedWindow, false);

do you need to use the "make the bound function a variable" trick that Matt used so you can really remove the event listener?

r+, but test the removeEventListener change and revert the dump

Comment 6

[Wesley Johnston (:wesj)]

http://hg.mozilla.org/integration/mozilla-inbound/rev/752009d34465

Comment 7

[Matt Brubeck (:mbrubeck)]

https://hg.mozilla.org/mozilla-central/rev/752009d34465