Closed Bug 987640 Opened 6 years ago Closed 6 years ago

Add automated test for the functionality of the Character encoding panel in Australis


(Firefox :: Toolbars and Customization, defect)

Not set



Firefox 31


(Reporter: mihaelav, Assigned: mihaelav)



(Whiteboard: [Australis:P-])


(1 file, 2 obsolete files)

This is a follow-up bug for the Character encoding button functionality in Australis - bug #967000 comment #19:
"check that clicking items in the view reloads the page with a different charset, and after that has finished, the correct charset should be checked."
Blocks: 978386
Closed: 6 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 967000
Eh? This isn't a dupe.
Resolution: DUPLICATE → ---
Whiteboard: [Australis:P-]
Assignee: nobody → mihaela.velimiroviciu
Attached patch test (obsolete) — Splinter Review
Gijs, does this cover what you wanted?
Attachment #8408923 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8408923 [details] [diff] [review]

Review of attachment 8408923 [details] [diff] [review]:

This is the right idea! f+ because of the initialLocation thing... I'm pretty sure the code doesn't work as-is. Try looking at what other tests that load supporting files into tabs do... perhaps it makes sense to just add a tab for the page you're loading, and then removing that tab again?

::: browser/components/customizableui/test/browser_987640_charEncoding.js
@@ +29,5 @@
> +  //change the encoding
> +  let encodings = characterEncodingView.querySelectorAll("toolbarbutton");
> +  let newEncoding = encodings[0].hasAttribute("checked") ? encodings[1] : encodings[0];
> +;
> +  yield promiseTabLoadEvent(gBrowser.selectedTab, TEST_PAGE);

To avoid a race condition here (what if the load event has fired before the promise is set up?), set up the promise before, and then yield it after, so:

let tabLoadPromise = promiseTabLoadEvent(gBrowser.selectedTab, TEST_PAGE);;
yield tabLoadPromise;

And the same for the similar code lower down, please. :-)

@@ +55,5 @@
> +  yield resetCustomization();
> +  ok(CustomizableUI.inDefaultState, "The UI is in default state again.");
> +
> +  // restore the initial location
> +  gBrowser.selectedTab.value = initialLocation;

... does this work? There is no other code in MXR that uses this, as far as I can tell...
Attachment #8408923 - Flags: review?(gijskruitbosch+bugs) → feedback+
Attached patch updated patch (obsolete) — Splinter Review
Updated based on review
Attachment #8408923 - Attachment is obsolete: true
Attachment #8410321 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8410321 [details] [diff] [review]
updated patch

Review of attachment 8410321 [details] [diff] [review]:

This looks good to me. Can you push this to try to verify it works everywhere and we haven't missed anything? :-)

::: browser/components/customizableui/test/browser_987640_charEncoding.js
@@ +6,5 @@
> +
> +const TEST_PAGE = "http://mochi.test:8888/browser/browser/components/customizableui/test/support/test_967000_charEncoding_page.html";
> +let newTab = null;
> +
> +let initialLocation = gBrowser.currentURI.spec;

This variable is now dead, please remove it. :-)
Attachment #8410321 - Flags: review?(gijskruitbosch+bugs) → review+
Attached patch final patchSplinter Review
Removed the initialLocation variable.

The try tree is now closed, but I made a push yesterday with the previous version of the patch:

Carrying over the r+ from Gijs.
Attachment #8410321 - Attachment is obsolete: true
Attachment #8410862 - Flags: review+
Keywords: checkin-needed
Keywords: checkin-needed
Whiteboard: [Australis:P-] → [Australis:P-][fixed-in-fx-team]
Closed: 6 years ago6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [Australis:P-][fixed-in-fx-team] → [Australis:P-]
Target Milestone: --- → Firefox 31
You need to log in before you can comment on or make changes to this bug.