Closed Bug 684241 Opened 11 years ago Closed 11 years ago

Add tests for locale picker

Categories

(Firefox for Android Graveyard :: General, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 10

People

(Reporter: wesj, Assigned: wesj)

References

Details

Attachments

(1 file, 2 obsolete files)

Attached file WIP Patch (obsolete) —
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.
Blocks: 689706
Attached patch Patch (obsolete) — Splinter Review
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.
Assignee: nobody → wjohnston
Attachment #557838 - Attachment is obsolete: true
Attachment #563320 - Flags: review?(mark.finkle)
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...
Attachment #563320 - Flags: review?(mark.finkle)
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...
Attached patch Patch v2Splinter Review
Yay! Try looks green (at least on b-c tests).
Attachment #563320 - Attachment is obsolete: true
Attachment #563845 - Flags: review?(mark.finkle)
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
Attachment #563845 - Flags: review?(mark.finkle) → review+
https://hg.mozilla.org/mozilla-central/rev/752009d34465
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Whiteboard: [inbound]
Target Milestone: --- → Firefox 10
You need to log in before you can comment on or make changes to this bug.