967000 - Add test for the functionality of Panel Menu non-default buttons in Australis

Status: RESOLVED FIXED

(Firefox :: Toolbars and Customization, defect)

Description

[Mihaela Velimiroviciu (:mihaelav)]

Create automated tests for the following:

Steps:
1. Start Firefox
2. Open the Panel Menu
3. Enter customize mode
4. Add a button prom the palette to the Panel Menu
5. Exit customize mode
6. Open the Panel menu
7. Click on the button added at step 4.

Expected result:
The appropriate action related to the button is performed.

Comment 1

[Justin Dolske [:Dolske]]

Pretty sure we already have test coverage for this.

Comment 2

[Mike Conley (:mconley) (:⚙️)]

We have coverage for some of these things. We do, for example, test that we can enter and exit customize mode, and that dragging things to the menu panel works. I do not believe, however, we have a test that ensures that the buttons command or onclick is handled after customizing into the menu panel and exiting customize mode.

So while we have some coverage of the behaviours described in comment 0, we don't have the entire scenario covered, to the best of my knowledge.

Comment 3

[Mihaela Velimiroviciu (:mihaelav)]

Attachment: tests

This patch contains tests for the Character Encoding, Subscribe, Sync and Tab Groups buttons.
Open File and Email Link buttons open non-browser windows and I'm not sure how I can handle those.

Comment 4

[:Gijs (he/him)]

Comment on attachment 8373900 [details] [diff] [review]
tests

Review of attachment 8373900 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/customizableui/test/browser_967000_button_charEncoding.js
@@ +9,5 @@
> +
> +  let initialLocation = gBrowser.currentURI.spec;
> +
> +  // add the Character Encoding button to the panel
> +  yield startCustomizing();

I'd prefer if these tests didn't do this. Instead, you can just use:

CustomizableUI.addWidgetToArea("characterencoding-button", CustomizableUI.AREA_PANEL);

and similar code for the other buttons, which saves a lot of code and time.

Comment 5

[Mihaela Velimiroviciu (:mihaelav)]

Attachment: patch v2

Thanks for the quick review. I updated the tests based on your suggestion

Comment 6

[:Gijs (he/him)]

Comment on attachment 8374024 [details] [diff] [review]
patch v2

Review of attachment 8374024 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/customizableui/test/browser_967000_button_charEncoding.js
@@ +19,5 @@
> +  ok(charEncodingButton, "The Character Encoding button was added to the Panel Menu");
> +  ok(charEncodingButton.hasAttribute("disabled"), "The Character encoding button is initially disabled");
> +  yield PanelUI.hide();
> +
> +  gBrowser.selectedTab = gBrowser.addTab("http://www.mozilla.org/en-US/");

You shouldn't open pages off the real network in a mochitest. Instead, use something served by mochitest's http server, and add it as a support-file for the test.

@@ +43,5 @@
> +  yield resetCustomization();
> +  ok(CustomizableUI.inDefaultState, "Panel is in default state again.");
> +
> +  // close the added tab
> +  if(gBrowser.tabs.length > 1) {

I'm confused. Under what circumstances doesn't the addTab call create a tab? And why can't you just save that tab in a local variable and always remove that here? Seems much simpler to me.

::: browser/components/customizableui/test/browser_967000_button_rss.js
@@ +14,5 @@
> +                                  CustomizableUI.AREA_PANEL);
> +
> +  // check the button's functionality
> +  yield PanelUI.show();
> +  let rssButton = document.getElementById("feed-button");

nit: feedButton, please.

@@ +19,5 @@
> +  ok(rssButton, "The Subscribe button was added to the Panel Menu");
> +  ok(rssButton.hasAttribute("disabled"), "The Subscribe button is initially disabled");
> +  yield PanelUI.hide();
> +
> +  gBrowser.selectedTab = gBrowser.addTab("https://blog.mozilla.org/");

Again, can't do this.

@@ +33,5 @@
> +  try {
> +    yield waitForCondition(function() !rssButton.hasAttribute("disabled"));
> +  }
> +  catch (e){
> +    ok(false, "The Subscribe button doesn't get enabled");

You're asserting this straight after, no need to do it here.

@@ +56,5 @@
> +  ok(CustomizableUI.inDefaultState, "Should be in default state again.");
> +
> +  // restore the tabs
> +  if(gBrowser.tabs.length > 1) {
> +    gBrowser.removeTab(gBrowser.selectedTab);

Again, you are always addTab'ing so you should always be able to removeTab that tab.

::: browser/components/customizableui/test/browser_967000_button_tabView.js
@@ +20,5 @@
> +  try {
> +    yield waitForCondition(() => window.TabView.getContentWindow());
> +  }
> +  catch(e) {
> +    ok(false, "Tab Groups view doesn't load");

Again, you're asserting this straight after.

@@ +26,5 @@
> +
> +  ok(window.TabView.getContentWindow(), "Tab Groups view is loaded");
> +
> +  // exit tab view
> +  yield window.TabView.toggle();

You shouldn't do this if the tab groups view didn't load, so before the previous assertion, store the result of getContentWindow in a local variable, check it in the assertion, and only do this if it is true.

Also, window.TabView.toggle doesn't return anything, nevermind a promise, so you should waitForCondition() that we've gone out of tabview again.

Comment 7

[Justin Dolske [:Dolske]]

Tests are good, but there's no user impact here so untracking.

Comment 8

[Mihaela Velimiroviciu (:mihaelav)]

window.TabView.toggle() doesn't seem to work in this test (doesn't exist tab view mode) and neither does window.TabView.hide(). I'm not sure why...

Comment 9

[Mihaela Velimiroviciu (:mihaelav)]

Attachment: v3

Updated patch based on review from comment #6

Comment 10

[:Gijs (he/him)]

Comment on attachment 8384550 [details] [diff] [review]
v3

Review of attachment 8384550 [details] [diff] [review]:
-----------------------------------------------------------------

This patch is missing a lot of files...

Comment 11

[Mihaela Velimiroviciu (:mihaelav)]

Attachment: v3

(In reply to :Gijs Kruitbosch from comment #10)
> Comment on attachment 8384550 [details] [diff] [review]
> v3
> 
> Review of attachment 8384550 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This patch is missing a lot of files...

Oups, sorry! It looks like I  forgot to hg add them...

Comment 12

[Mihaela Velimiroviciu (:mihaelav)]

remote:   https://tbpl.mozilla.org/?tree=Try&rev=a9d4458236d1

Comment 13

[:Gijs (he/him)]

Comment on attachment 8385390 [details] [diff] [review]
v3

Review of attachment 8385390 [details] [diff] [review]:
-----------------------------------------------------------------

Please split these up into individual patches, to make it simpler to iterate and land them piecemeal.

::: browser/components/customizableui/test/browser_967000_button_charEncoding.js
@@ +4,5 @@
> +
> +"use strict";
> +
> +const TEST_PAGE = "http://mochi.test:8888/browser/browser/components/customizableui/test/support/test_page.html";
> +var newTab;

Use 'let' instead of 'var'

@@ +20,5 @@
> +  yield PanelUI.show();
> +  let charEncodingButton = document.getElementById("characterencoding-button");
> +  ok(charEncodingButton, "The Character Encoding button was added to the Panel Menu");
> +  ok(charEncodingButton.hasAttribute("disabled"), "The Character encoding button is initially disabled");
> +  yield PanelUI.hide();

This doesn't return a promise and so you should be using the utility function inside head.js to have a promise that resolves when the panel is hidden. Please fix this throughout all the tests.

@@ +26,5 @@
> +  newTab = gBrowser.selectedTab = gBrowser.addTab(TEST_PAGE);
> +
> +  try {
> +    yield waitForCondition(function() gBrowser.currentURI &&
> +                                      gBrowser.currentURI.spec == TEST_PAGE);

This seems off. Instead, get newTab.linkedBrowser and add a load() event listener there, and wait for that to fire. You can wrap this in a utility function that takes a URL and returns a promise, and add it to head.js. Please give it an optional timeout parameter that defaults to 10000ms that rejects the promise.

Once implemented, use this function throughout the tests.

@@ +38,5 @@
> +  ok(!charEncodingButton.hasAttribute("disabled"), "The Character encoding button gets enabled");
> +  charEncodingButton.click();
> +
> +  let characterEncodingView = document.getElementById("PanelUI-characterEncodingView");
> +  ok(characterEncodingView.hasAttribute("current"), "The Character encoding panel is displayed");

You should check that one item gets checked, and, ideally, that it corresponds to the charset defined by the test page you're using. Even better, you should try to then load a new page with a different encoding and check that that is checked, and finally remove the tab and check that the widget is disabled again.

@@ +43,5 @@
> +  yield PanelUI.hide();
> +
> +    // reset the panel to the default state
> +  yield resetCustomization();
> +  ok(CustomizableUI.inDefaultState, "Panel is in default state again.");

Nit: UI is in default state again

@@ +46,5 @@
> +  yield resetCustomization();
> +  ok(CustomizableUI.inDefaultState, "Panel is in default state again.");
> +
> +  // close the opened tab
> +  gBrowser.removeTab(newTab);

This should be in a cleanup function.

::: browser/components/customizableui/test/browser_967000_button_feeds.js
@@ +36,5 @@
> +  ok(!feedButton.hasAttribute("disabled"), "The Subscribe button gets enabled");
> +
> +  feedButton.click();
> +  yield waitForCondition(function() gBrowser.currentURI &&
> +                                    gBrowser.currentURI.spec == TEST_FEED);

You should check that the panel has closed, too, and if not, add a failure message and hide it here.

::: browser/components/customizableui/test/browser_967000_button_sync.js
@@ +16,5 @@
> +  // check the button's functionality
> +  yield PanelUI.show();
> +  let syncButton = document.getElementById("sync-button");
> +  ok(syncButton, "The Sync button was added to the Panel Menu");
> +  syncButton.click();

Again, doublecheck the panel closes.

@@ +38,5 @@
> +    gBrowser.removeTab(gBrowser.selectedTab);
> +  }
> +  else {
> +    var tabToRemove = gBrowser.selectedTab;
> +    gBrowser.addTab(initialLocation);

Reuse the proposed tab opening function that returns a promise here, too, so that we can be sure the window is in the correct state again at the end of the test.

::: browser/components/customizableui/test/browser_967000_button_tabView.js
@@ +28,5 @@
> +  let tabViewButton = document.getElementById("tabview-button");
> +  ok(tabViewButton, "Tab Groups button was added to the Panel Menu");
> +  tabViewButton.click();
> +
> +  yield waitForCondition(() => !TabView.isVisible());

This seems hacky. Please create a promise above, resolve it from the tabviewhidden listener, add a timeout to reject the promise if it's not resolved within 5 seconds, and yield that promise here?

Comment 14

[Mihaela Velimiroviciu (:mihaelav)]

Attachment: CharEncoding test

(In reply to :Gijs Kruitbosch from comment #13)
> :::
> browser/components/customizableui/test/browser_967000_button_charEncoding.js
> @@ +4,5 @@
> > +
> > +"use strict";
> > +
> > +const TEST_PAGE = "http://mochi.test:8888/browser/browser/components/customizableui/test/support/test_page.html";
> > +var newTab;
> 
> Use 'let' instead of 'var'
Done
 
> @@ +20,5 @@
> > +  yield PanelUI.show();
> > +  let charEncodingButton = document.getElementById("characterencoding-button");
> > +  ok(charEncodingButton, "The Character Encoding button was added to the Panel Menu");
> > +  ok(charEncodingButton.hasAttribute("disabled"), "The Character encoding button is initially disabled");
> > +  yield PanelUI.hide();
Done 

> This doesn't return a promise and so you should be using the utility
> function inside head.js to have a promise that resolves when the panel is
> hidden. Please fix this throughout all the tests.
> 
> @@ +26,5 @@
> > +  newTab = gBrowser.selectedTab = gBrowser.addTab(TEST_PAGE);
> > +
> > +  try {
> > +    yield waitForCondition(function() gBrowser.currentURI &&
> > +                                      gBrowser.currentURI.spec == TEST_PAGE);
> 
> This seems off. Instead, get newTab.linkedBrowser and add a load() event
> listener there, and wait for that to fire. You can wrap this in a utility
> function that takes a URL and returns a promise, and add it to head.js.
> Please give it an optional timeout parameter that defaults to 10000ms that
> rejects the promise.
> 
> Once implemented, use this function throughout the tests.
There was already a similar function in the head.js: promiseTabLoadEvent(aTab, aURL, aEventType="load"), so I used that.
 
> @@ +38,5 @@
> > +  ok(!charEncodingButton.hasAttribute("disabled"), "The Character encoding button gets enabled");
> > +  charEncodingButton.click();
> > +
> > +  let characterEncodingView = document.getElementById("PanelUI-characterEncodingView");
> > +  ok(characterEncodingView.hasAttribute("current"), "The Character encoding panel is displayed");
> 
> You should check that one item gets checked, and, ideally, that it
> corresponds to the charset defined by the test page you're using. Even
> better, you should try to then load a new page with a different encoding and
> check that that is checked, and finally remove the tab and check that the
> widget is disabled again.
I haven't made these checks since it's not in the scope of this test to verify that the Char Encoding "feature" is functional, but to check that the Char Encoding button does what it's supposed to do (reveal the character encoding panel).

> @@ +43,5 @@
> > +  yield PanelUI.hide();
> > +
> > +    // reset the panel to the default state
> > +  yield resetCustomization();
> > +  ok(CustomizableUI.inDefaultState, "Panel is in default state again.");
> 
> Nit: UI is in default state again
Done

> @@ +46,5 @@
> > +  yield resetCustomization();
> > +  ok(CustomizableUI.inDefaultState, "Panel is in default state again.");
> > +
> > +  // close the opened tab
> > +  gBrowser.removeTab(newTab);
> 
> This should be in a cleanup function.
Done

Comment 15

[Mihaela Velimiroviciu (:mihaelav)]

Attachment: Feeds button test

(In reply to :Gijs Kruitbosch from comment #13)
> Comment on attachment 8385390 [details] [diff] [review]
> v3
> 
> Review of attachment 8385390 [details] [diff] [review]:
> 
> ::: browser/components/customizableui/test/browser_967000_button_feeds.js
> @@ +36,5 @@
> > +  ok(!feedButton.hasAttribute("disabled"), "The Subscribe button gets enabled");
> > +
> > +  feedButton.click();
> > +  yield waitForCondition(function() gBrowser.currentURI &&
> > +                                    gBrowser.currentURI.spec == TEST_FEED);
> 
> You should check that the panel has closed, too, and if not, add a failure
> message and hide it here.
> 

Done.
Also, used promisePanelShown and promisePanelHidden when opening/hiding the panel menu, promiseTabLoadEvent when waiting for the page to load and added a cleanup function to restore the UI and the tabs.

Comment 16

[Mihaela Velimiroviciu (:mihaelav)]

Attachment: Sync button test

(In reply to :Gijs Kruitbosch from comment #13)
> Comment on attachment 8385390 [details] [diff] [review]
> v3
> 
> Review of attachment 8385390 [details] [diff] [review]:
> 
> ::: browser/components/customizableui/test/browser_967000_button_sync.js
> @@ +16,5 @@
> > +  // check the button's functionality
> > +  yield PanelUI.show();
> > +  let syncButton = document.getElementById("sync-button");
> > +  ok(syncButton, "The Sync button was added to the Panel Menu");
> > +  syncButton.click();
> 
> Again, doublecheck the panel closes.
> 
Done

> @@ +38,5 @@
> > +    gBrowser.removeTab(gBrowser.selectedTab);
> > +  }
> > +  else {
> > +    var tabToRemove = gBrowser.selectedTab;
> > +    gBrowser.addTab(initialLocation);
> 
> Reuse the proposed tab opening function that returns a promise here, too, so
> that we can be sure the window is in the correct state again at the end of
> the test.
>  
That function (promiseTabLoadEvent) doesn't work if initialLocation is "about:blank" (which normally should be).

Comment 17

[Mihaela Velimiroviciu (:mihaelav)]

Attachment: Tabsview button test

(In reply to :Gijs Kruitbosch from comment #13)
> Comment on attachment 8385390 [details] [diff] [review]
> v3
> 
> Review of attachment 8385390 [details] [diff] [review]:
>
> ::: browser/components/customizableui/test/browser_967000_button_tabView.js
> @@ +28,5 @@
> > +  let tabViewButton = document.getElementById("tabview-button");
> > +  ok(tabViewButton, "Tab Groups button was added to the Panel Menu");
> > +  tabViewButton.click();
> > +
> > +  yield waitForCondition(() => !TabView.isVisible());
> 
> This seems hacky. Please create a promise above, resolve it from the
> tabviewhidden listener, add a timeout to reject the promise if it's not
> resolved within 5 seconds, and yield that promise here?

Done.
Also added a cleanup function to reset the customization

Comment 18

[:Gijs (he/him)]

(I'm sorry for the delay in reviewing here. I'm having to focus on more pressing issues with Australis before we hit beta this week. I hope to be able to get to this next week.)

Comment 19

[:Gijs (he/him)]

Comment on attachment 8389148 [details] [diff] [review]
CharEncoding test

Review of attachment 8389148 [details] [diff] [review]:
-----------------------------------------------------------------

I'm really sorry about the long delays here, especially because some recent bustage in some of these items could have been caught if we'd had good automated tests for them...

::: browser/components/customizableui/test/browser.ini
@@ +1,4 @@
>  [DEFAULT]
>  support-files =
>    head.js
> +  support/test_page.html

Please give this a more descriptive name, like test_967000_charEncoding_page.html

::: browser/components/customizableui/test/browser_967000_button_charEncoding.js
@@ +12,5 @@
> +add_task(function() {
> +  info("Check Character Encoding button functionality");
> +
> +  let panelShownPromise = promisePanelShown(window);
> +  let panelHidePromise = promisePanelHidden(window);

you should do this as close to the actual hiding/showing as possible, so it won't pick up the wrong thing, and so the timeouts don't fire prematurely (both will reject their promises after 20 seconds of not having their handlers called).

@@ +38,5 @@
> +  ok(!charEncodingButton.hasAttribute("disabled"), "The Character encoding button gets enabled");
> +  charEncodingButton.click();
> +
> +  let characterEncodingView = document.getElementById("PanelUI-characterEncodingView");
> +  ok(characterEncodingView.hasAttribute("current"), "The Character encoding panel is displayed");

This test should actually check that the view is populated and that at least one item is checked. Ideally, it should also 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 20

[:Gijs (he/him)]

Comment on attachment 8389154 [details] [diff] [review]
Feeds button test

Review of attachment 8389154 [details] [diff] [review]:
-----------------------------------------------------------------

This looks alright with the below points fixed, but I'd like to see the final test just to be sure. Can you also file a followup bug to add a test that checks showing a subview when multiple feeds are present in the page (and that the right number of items appear in the subview, and clicking them opens the right page)? Thank you!

::: browser/components/customizableui/test/browser_967000_button_feeds.js
@@ +13,5 @@
> +add_task(function() {
> +  info("Check Subscribe button functionality");
> +
> +  let panelShownPromise = promisePanelShown(window);
> +  let panelHidePromise = promisePanelHidden(window);

Here, too, please place these as close to the actual hiding/showing as possible.

@@ +29,5 @@
> +  PanelUI.hide();
> +  yield panelHidePromise;
> +
> +  newTab = gBrowser.selectedTab;
> +  yield promiseTabLoadEvent(newTab, TEST_PAGE)

nit: missing semicolon. Please pay attention to this throughout. :-)

Comment 21

[:Gijs (he/him)]

Comment on attachment 8389164 [details] [diff] [review]
Sync button test

Review of attachment 8389164 [details] [diff] [review]:
-----------------------------------------------------------------

For this, too, can you file a followup bug to test that it starts a sync process if you're signed in? That's significantly more complicated, so I'm OK with this for now, but we should really have tests for that, too.

::: browser/components/customizableui/test/browser_967000_button_sync.js
@@ +10,5 @@
> +add_task(function() {
> +  info("Check Sync button functionality");
> +
> +  let panelShownPromise = promisePanelShown(window);
> +  let panelHidePromise = promisePanelHidden(window);

You guessed it! These, too, need to go as close as possible to the actual opening/closing of the panel. You don't even need the hide promise unless you need to manually close the panel...

Comment 22

[:Gijs (he/him)]

Comment on attachment 8389177 [details] [diff] [review]
Tabsview button test

Review of attachment 8389177 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/customizableui/test/browser_967000_button_tabView.js
@@ +26,5 @@
> +    // close tabs view
> +    window.TabView.hide();
> +    timeout = setTimeout(() => {
> +      window.removeEventListener("tabviewhidden", tabViewHidden, false);
> +      deferred.reject("TabSelect did not happen within 5000ms");

TabSelect ? I think you mean "Tab view wasn't hidden within 5000ms"?

@@ +27,5 @@
> +    window.TabView.hide();
> +    timeout = setTimeout(() => {
> +      window.removeEventListener("tabviewhidden", tabViewHidden, false);
> +      deferred.reject("TabSelect did not happen within 5000ms");
> +    }, 5000);

This should probably be 20s because our debug tests are slow. Please remember to update the comment appropriately.

@@ +34,5 @@
> +  // check the button's functionality
> +  yield PanelUI.show();
> +  let tabViewButton = document.getElementById("tabview-button");
> +  ok(tabViewButton, "Tab Groups button was added to the Panel Menu");
> +  tabViewButton.click();

You should check that this hides the panel, like in the other tests. :-)

Comment 23

[Mihaela Velimiroviciu (:mihaelav)]

Attachment: Char Encoding test

(In reply to :Gijs Kruitbosch from comment #19)
> ::: browser/components/customizableui/test/browser.ini
> @@ +1,4 @@
> >  [DEFAULT]
> >  support-files =
> >    head.js
> > +  support/test_page.html
> 
> Please give this a more descriptive name, like
> test_967000_charEncoding_page.html
Done

> :::
> browser/components/customizableui/test/browser_967000_button_charEncoding.js
> @@ +12,5 @@
> > +add_task(function() {
> > +  info("Check Character Encoding button functionality");
> > +
> > +  let panelShownPromise = promisePanelShown(window);
> > +  let panelHidePromise = promisePanelHidden(window);
> 
> you should do this as close to the actual hiding/showing as possible, so it
> won't pick up the wrong thing, and so the timeouts don't fire prematurely
> (both will reject their promises after 20 seconds of not having their
> handlers called).
Done: added the calls right before opening/closing the panel
>
> @@ +38,5 @@
> > +  ok(!charEncodingButton.hasAttribute("disabled"), "The Character encoding button gets enabled");
> > +  charEncodingButton.click();
> > +
> > +  let characterEncodingView = document.getElementById("PanelUI-characterEncodingView");
> > +  ok(characterEncodingView.hasAttribute("current"), "The Character encoding panel is displayed");
> 
> This test should actually check that the view is populated and that at least
> one item is checked. Ideally, it should also 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.
Done the one item checked check. I'll log a follow up bug for the rest as it involves more detailed feature testing.

Comment 24

[Mihaela Velimiroviciu (:mihaelav)]

Attachment: Feeds button test

(In reply to :Gijs Kruitbosch from comment #20)
> Comment on attachment 8389154 [details] [diff] [review]
> Feeds button test
> 
> Review of attachment 8389154 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This looks alright with the below points fixed, but I'd like to see the
> final test just to be sure. Can you also file a followup bug to add a test
> that checks showing a subview when multiple feeds are present in the page
> (and that the right number of items appear in the subview, and clicking them
> opens the right page)? Thank you!
Logged follow up bug 987159

 
> ::: browser/components/customizableui/test/browser_967000_button_feeds.js
> @@ +13,5 @@
> > +add_task(function() {
> > +  info("Check Subscribe button functionality");
> > +
> > +  let panelShownPromise = promisePanelShown(window);
> > +  let panelHidePromise = promisePanelHidden(window);
> 
> Here, too, please place these as close to the actual hiding/showing as
> possible.
Done - added the calls just before the panel hide/show.
 
> @@ +29,5 @@
> > +  PanelUI.hide();
> > +  yield panelHidePromise;
> > +
> > +  newTab = gBrowser.selectedTab;
> > +  yield promiseTabLoadEvent(newTab, TEST_PAGE)
> 
> nit: missing semicolon. Please pay attention to this throughout. :-)
Done

Comment 25

[:Gijs (he/him)]

Comment on attachment 8395666 [details] [diff] [review]
Char Encoding test

Review of attachment 8395666 [details] [diff] [review]:
-----------------------------------------------------------------

Getting there!

::: browser/components/customizableui/test/browser_967000_button_charEncoding.js
@@ +18,5 @@
> +
> +  // check the button's functionality
> +  let panelShownPromise = promisePanelShown(window);
> +  PanelUI.show();
> +  yield panelShownPromise;

Nit: yield PanelUI.show();

@@ +22,5 @@
> +  yield panelShownPromise;
> +
> +  let charEncodingButton = document.getElementById("characterencoding-button");
> +  ok(charEncodingButton, "The Character Encoding button was added to the Panel Menu");
> +  ok(charEncodingButton.hasAttribute("disabled"), "The Character encoding button is initially disabled");

Nit: is(charEncodingButton.getAttribute("disabled"), "true", "...")

@@ +33,5 @@
> +  yield promiseTabLoadEvent(newTab, TEST_PAGE)
> +
> +  panelShownPromise = promisePanelShown(window);
> +  PanelUI.show();
> +  yield panelShownPromise;

Nit: yield PanelUI.show();

@@ +52,5 @@
> +    if(pinnedEncodings.childNodes[i].getAttribute("checked")){
> +      charsetSelected = true;
> +      selectedCharset = pinnedEncodings.childNodes[i];
> +    }
> +  }

Hrm. Instead of doing several loops here, why not:

let checkedButtons = characterEncodingView.querySelectorAll("toolbarbutton[checked='true']");
is(2, checkedButtons.length, "There should be 2 checked items (1 charset, 1 detector).");
is(checkedButtons[0].getAttribute("label"), "Unicode", "The unicode encoding is correctly selected");
is(1, characterEncodingView.querySelectorAll("#PanelUI-characterEncodingView-autodetect toolbarbutton[checked='true']").length, "There should be 1 checked detector.");

This is a lot simpler and shorter. :-)

::: browser/components/customizableui/test/support/test_967000_charEncoding_page.html
@@ +1,2 @@
> +<html>
> +  <meta http-equiv="Content-Type" content="text/html;charset=UTF-8">

Nit: please add <!DOCTYPE html> at the top, and then just use: <meta charset="utf-8"> instead of the longer Content-Type one.

@@ +4,5 @@
> +    <title>Test page</title>
> +  </head>
> +
> +  <body>
> +    <p id="p1">This is a test page</p>

Nit: no need to add a paragraph here, just leave the text directly into the body. :-)

Comment 26

[:Gijs (he/him)]

Comment on attachment 8395692 [details] [diff] [review]
Feeds button test

Review of attachment 8395692 [details] [diff] [review]:
-----------------------------------------------------------------

Apart from the nits, this looks good to me! :-)

::: browser/components/customizableui/test/browser_967000_button_feeds.js
@@ +19,5 @@
> +
> +  // check the button's functionality
> +  let panelShownPromise = promisePanelShown(window);
> +  PanelUI.show();
> +  yield panelShownPromise;

Nit: yield PanelUI.show();

@@ +23,5 @@
> +  yield panelShownPromise;
> +
> +  let feedButton = document.getElementById("feed-button");
> +  ok(feedButton, "The Subscribe button was added to the Panel Menu");
> +  ok(feedButton.hasAttribute("disabled"), "The Subscribe button is initially disabled");

Nit: is(feedButton.getAttribute("disabled"), "true", "...");

@@ +34,5 @@
> +  yield promiseTabLoadEvent(newTab, TEST_PAGE);
> +
> +  panelShownPromise = promisePanelShown(window);
> +  PanelUI.show();
> +  yield panelShownPromise;

Nit: yield PanelUI.show();

::: browser/components/customizableui/test/support/feeds_test_page.html
@@ +4,5 @@
> +  <link rel="alternate" type="application/rss+xml" href="test-feed.xml" title="Test feed">
> +</head>
> +
> +<body>
> +  <p id="p1">This is a test page for feeds</p>

Nit: no need for the paragraph here.

Comment 27

[Mihaela Velimiroviciu (:mihaelav)]

Attachment: Sync button test

(In reply to :Gijs Kruitbosch from comment #21)
> Comment on attachment 8389164 [details] [diff] [review]
> Sync button test
> 
> Review of attachment 8389164 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> For this, too, can you file a followup bug to test that it starts a sync
> process if you're signed in? That's significantly more complicated, so I'm
> OK with this for now, but we should really have tests for that, too.
Logged bug 987185

> ::: browser/components/customizableui/test/browser_967000_button_sync.js
> @@ +10,5 @@
> > +add_task(function() {
> > +  info("Check Sync button functionality");
> > +
> > +  let panelShownPromise = promisePanelShown(window);
> > +  let panelHidePromise = promisePanelHidden(window);
> 
> You guessed it! These, too, need to go as close as possible to the actual
> opening/closing of the panel. You don't even need the hide promise unless
> you need to manually close the panel...
Done

Comment 28

[:Gijs (he/him)]

Comment on attachment 8395721 [details] [diff] [review]
Sync button test

Review of attachment 8395721 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, but it looks like you're confused about the panel promise stuff.

"yield" inside Task.jsm effectively makes the execution of the rest of the code wait until the promise is resolved.

PanelUI.show() returns a promise, so you can use it directly with "yield".

PanelUI.hide() doesn't return a promise, and so you can't use it directly with "yield", and you need to first get a promise with: let foo = promisePanelShown(window);, then call PanelUI.hide(), then yield the promise you got.

::: browser/components/customizableui/test/browser_967000_button_sync.js
@@ +4,5 @@
> +
> +"use strict";
> +
> +  let initialLocation = gBrowser.currentURI.spec;
> +  let newTab = null;

Nit: please remove the leading indentation/spaces

@@ +11,5 @@
> +  info("Check Sync button functionality");
> +
> +  // add the Sync button to the panel
> +  CustomizableUI.addWidgetToArea("sync-button",
> +                                  CustomizableUI.AREA_PANEL);

Nit: This can go on one line, I think? :-)

@@ +16,5 @@
> +
> +  // check the button's functionality
> +  let panelShownPromise = promisePanelShown(window);
> +  yield PanelUI.show();
> +  yield panelShownPromise;

You don't need the panelShownPromise if you yield for PanelUI.show(). Please also keep this in mind when you update the other tests that I nitted.

@@ +30,5 @@
> +  ok(!isPanelUIOpen(), "The panel closed");
> +
> +  if(isPanelUIOpen()) {
> +    let panelHidePromise = promisePanelHidden(window);
> +    yield PanelUI.hide();

Nit: yielding for PanelUI.hide() itself has no effect, because PanelUI.hide() doesn't return a promise. So remove that 'yield' (but keep the hide promise that's "around" this line)

Comment 29

[Mihaela Velimiroviciu (:mihaelav)]

Attachment: Tabsview button test

(In reply to :Gijs Kruitbosch from comment #22)
> Comment on attachment 8389177 [details] [diff] [review]
> Tabsview button test
> 
> Review of attachment 8389177 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/components/customizableui/test/browser_967000_button_tabView.js
> @@ +26,5 @@
> > +    // close tabs view
> > +    window.TabView.hide();
> > +    timeout = setTimeout(() => {
> > +      window.removeEventListener("tabviewhidden", tabViewHidden, false);
> > +      deferred.reject("TabSelect did not happen within 5000ms");
> 
> TabSelect ? I think you mean "Tab view wasn't hidden within 5000ms"?
Yep, copy-paste is evil. Updated.

> @@ +27,5 @@
> > +    window.TabView.hide();
> > +    timeout = setTimeout(() => {
> > +      window.removeEventListener("tabviewhidden", tabViewHidden, false);
> > +      deferred.reject("TabSelect did not happen within 5000ms");
> > +    }, 5000);
> 
> This should probably be 20s because our debug tests are slow. Please
> remember to update the comment appropriately.
Done
 
> @@ +34,5 @@
> > +  // check the button's functionality
> > +  yield PanelUI.show();
> > +  let tabViewButton = document.getElementById("tabview-button");
> > +  ok(tabViewButton, "Tab Groups button was added to the Panel Menu");
> > +  tabViewButton.click();
> 
> You should check that this hides the panel, like in the other tests. :-)
Done

Comment 30

[:Gijs (he/him)]

Comment on attachment 8395755 [details] [diff] [review]
Tabsview button test

Review of attachment 8395755 [details] [diff] [review]:
-----------------------------------------------------------------

Some nits, but otherwise this looks good! :-)

::: browser/components/customizableui/test/browser_967000_button_tabView.js
@@ +9,5 @@
> +  let deferred = Promise.defer();
> +  let timeout = null;
> +
> +  // add the Tab View button to the panel
> +  CustomizableUI.addWidgetToArea("tabview-button",

Nit: this fits on one line, too.

@@ +39,5 @@
> +  tabViewButton.click();
> +
> +  yield deferred.promise;
> +
> +  if(isPanelUIOpen()) {

Please add an ok(!isPanelUIOpen(), "Panel should be closed"); before the if block - we shouldn't hit this case, but we should check it. :-)

Comment 31

[Mihaela Velimiroviciu (:mihaelav)]

Attachment: Char Encoding test

(In reply to :Gijs Kruitbosch from comment #25)
> Comment on attachment 8395666 [details] [diff] [review]
> Char Encoding test

> browser/components/customizableui/test/browser_967000_button_charEncoding.js
> @@ +18,5 @@
> > +
> > +  // check the button's functionality
> > +  let panelShownPromise = promisePanelShown(window);
> > +  PanelUI.show();
> > +  yield panelShownPromise;
> 
> Nit: yield PanelUI.show();
Done

> @@ +22,5 @@
> > +  yield panelShownPromise;
> > +
> > +  let charEncodingButton = document.getElementById("characterencoding-button");
> > +  ok(charEncodingButton, "The Character Encoding button was added to the Panel Menu");
> > +  ok(charEncodingButton.hasAttribute("disabled"), "The Character encoding button is initially disabled");
> 
> Nit: is(charEncodingButton.getAttribute("disabled"), "true", "...")
Done

> @@ +33,5 @@
> > +  yield promiseTabLoadEvent(newTab, TEST_PAGE)
> > +
> > +  panelShownPromise = promisePanelShown(window);
> > +  PanelUI.show();
> > +  yield panelShownPromise;
> 
> Nit: yield PanelUI.show();
Done
 
> @@ +52,5 @@
> > +    if(pinnedEncodings.childNodes[i].getAttribute("checked")){
> > +      charsetSelected = true;
> > +      selectedCharset = pinnedEncodings.childNodes[i];
> > +    }
> > +  }
> 
> Hrm. Instead of doing several loops here, why not:
> 
> let checkedButtons =
> characterEncodingView.querySelectorAll("toolbarbutton[checked='true']");
> is(2, checkedButtons.length, "There should be 2 checked items (1 charset, 1
> detector).");
> is(checkedButtons[0].getAttribute("label"), "Unicode", "The unicode encoding
> is correctly selected");
> is(1,
> characterEncodingView.querySelectorAll("#PanelUI-characterEncodingView-
> autodetect toolbarbutton[checked='true']").length, "There should be 1
> checked detector.");
> 
> This is a lot simpler and shorter. :-)
Done

> :::
> browser/components/customizableui/test/support/test_967000_charEncoding_page.
> html
> @@ +1,2 @@
> > +<html>
> > +  <meta http-equiv="Content-Type" content="text/html;charset=UTF-8">
> 
> Nit: please add <!DOCTYPE html> at the top, and then just use: <meta
> charset="utf-8"> instead of the longer Content-Type one.
Done

> @@ +4,5 @@
> > +    <title>Test page</title>
> > +  </head>
> > +
> > +  <body>
> > +    <p id="p1">This is a test page</p>
> 
> Nit: no need to add a paragraph here, just leave the text directly into the
> body. :-)
Done

Comment 32

[Mihaela Velimiroviciu (:mihaelav)]

Attachment: Feeds button test

(In reply to :Gijs Kruitbosch from comment #26)
> Comment on attachment 8395692 [details] [diff] [review]
> Feeds button test
> 
> Review of attachment 8395692 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Apart from the nits, this looks good to me! :-)
All nits resolved

Comment 33

[Mihaela Velimiroviciu (:mihaelav)]

Attachment: Sync button test

(In reply to :Gijs Kruitbosch from comment #28)
> Comment on attachment 8395721 [details] [diff] [review]
> Sync button test
> 
> Review of attachment 8395721 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good, but it looks like you're confused about the panel promise stuff.
> 
> "yield" inside Task.jsm effectively makes the execution of the rest of the
> code wait until the promise is resolved.
> 
> PanelUI.show() returns a promise, so you can use it directly with "yield".
> 
> PanelUI.hide() doesn't return a promise, and so you can't use it directly
> with "yield", and you need to first get a promise with: let foo =
> promisePanelShown(window);, then call PanelUI.hide(), then yield the promise
> you got.
Indeed, that wasn't very clear. Thank you very much for explaining it. :)

> ::: browser/components/customizableui/test/browser_967000_button_sync.js
> @@ +4,5 @@
> > +
> > +"use strict";
> > +
> > +  let initialLocation = gBrowser.currentURI.spec;
> > +  let newTab = null;
> 
> Nit: please remove the leading indentation/spaces
Done
 
> @@ +11,5 @@
> > +  info("Check Sync button functionality");
> > +
> > +  // add the Sync button to the panel
> > +  CustomizableUI.addWidgetToArea("sync-button",
> > +                                  CustomizableUI.AREA_PANEL);
> 
> Nit: This can go on one line, I think? :-)
Done

> @@ +16,5 @@
> > +
> > +  // check the button's functionality
> > +  let panelShownPromise = promisePanelShown(window);
> > +  yield PanelUI.show();
> > +  yield panelShownPromise;
> 
> You don't need the panelShownPromise if you yield for PanelUI.show(). Please
> also keep this in mind when you update the other tests that I nitted.
Done

> @@ +30,5 @@
> > +  ok(!isPanelUIOpen(), "The panel closed");
> > +
> > +  if(isPanelUIOpen()) {
> > +    let panelHidePromise = promisePanelHidden(window);
> > +    yield PanelUI.hide();
> 
> Nit: yielding for PanelUI.hide() itself has no effect, because
> PanelUI.hide() doesn't return a promise. So remove that 'yield' (but keep
> the hide promise that's "around" this line)
Done

Comment 34

[Mihaela Velimiroviciu (:mihaelav)]

Attachment: Tabsview button test

(In reply to :Gijs Kruitbosch from comment #30)
> Comment on attachment 8395755 [details] [diff] [review]
> Tabsview button test
> 
> Review of attachment 8395755 [details] [diff] [review]:
> ::: browser/components/customizableui/test/browser_967000_button_tabView.js
> @@ +9,5 @@
> > +  let deferred = Promise.defer();
> > +  let timeout = null;
> > +
> > +  // add the Tab View button to the panel
> > +  CustomizableUI.addWidgetToArea("tabview-button",
> 
> Nit: this fits on one line, too.
Done

> @@ +39,5 @@
> > +  tabViewButton.click();
> > +
> > +  yield deferred.promise;
> > +
> > +  if(isPanelUIOpen()) {
> 
> Please add an ok(!isPanelUIOpen(), "Panel should be closed"); before the if
> block - we shouldn't hit this case, but we should check it. :-)
Done

Comment 35

[:Gijs (he/him)]

Comment on attachment 8396220 [details] [diff] [review]
Char Encoding test

Review of attachment 8396220 [details] [diff] [review]:
-----------------------------------------------------------------

Apart from the nit below, this LGTM.

::: browser/components/customizableui/test/support/test_967000_charEncoding_page.html
@@ +1,3 @@
> +<!DOCTYPE html>
> +<html>
> +  <meta charset="utf-8">

Erm, it's meant to go *inside* the <head>, though. :-)

Comment 36

[:Gijs (he/him)]

remote:   https://hg.mozilla.org/integration/fx-team/rev/100d59736c77
remote:   https://hg.mozilla.org/integration/fx-team/rev/da3d74c47e8e
remote:   https://hg.mozilla.org/integration/fx-team/rev/e3f74e8f72e6
remote:   https://hg.mozilla.org/integration/fx-team/rev/a374f03473f1

Comment 38

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/e3f74e8f72e6
https://hg.mozilla.org/mozilla-central/rev/a374f03473f1

Comment 39

[:Gijs (he/him)]

(In reply to Wes Kocher (:KWierso) from comment #38)
> https://hg.mozilla.org/mozilla-central/rev/e3f74e8f72e6
> https://hg.mozilla.org/mozilla-central/rev/a374f03473f1

And:

https://hg.mozilla.org/mozilla-central/rev/da3d74c47e8e
https://hg.mozilla.org/mozilla-central/rev/100d59736c77

which missed a 0 in the bug number...