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

Status: RESOLVED FIXED

(Firefox :: Toolbars and Customization, defect)

Description

[Mihaela Velimiroviciu (:mihaelav)]

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."

Comment 2

[:Gijs (he/him)]

Eh? This isn't a dupe.

Comment 3

[Mihaela Velimiroviciu (:mihaelav)]

Attachment: test

Gijs, does this cover what you wanted?

Comment 4

[:Gijs (he/him)]

Comment on attachment 8408923 [details] [diff] [review]
test

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];
> +  newEncoding.click();
> +  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 newEncoding.click(), and then yield it after, so:

let tabLoadPromise = promiseTabLoadEvent(gBrowser.selectedTab, TEST_PAGE);
newEncoding.click();
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...

Comment 5

[Mihaela Velimiroviciu (:mihaelav)]

Attachment: updated patch

Updated based on review

Comment 6

[:Gijs (he/him)]

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. :-)

Comment 7

[Mihaela Velimiroviciu (:mihaelav)]

Attachment: final patch

Removed the initialLocation variable.

The try tree is now closed, but I made a push yesterday with the previous version of the patch:
remote: https://tbpl.mozilla.org/?tree=Try&rev=5e06395a3bc1

Carrying over the r+ from Gijs.

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/fx-team/rev/7aaf686b18a8

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/7aaf686b18a8