1440703 - Add automated test to check that the Bookmarks Toolbar and Sidebar can be enabled from the Bookmarks Menu

Status: RESOLVED FIXED

(Firefox :: Bookmarks & History, enhancement, P2)

Description

[Paul Silaghi, QA [:pauly]]

TestRail links:

Test 1:
https://testrail.stage.mozaws.net/index.php?/cases/view/4089

Steps:
1. Launch Firefox with a new/clean profile.
2. From the toolbar, click the 'View history, saved bookmarks and more' button and select `Bookmarks'.
3. Select Bookmarking Tools → click View Bookmarks Toolbar.

Expected results:
1. Firefox is successfully launched.
2. Clicking the button brings up the Bookmarks Menu.
3. The Bookmarks Toolbar is enabled and displayed below the Location Bar.


Test 2:
https://testrail.stage.mozaws.net/index.php?/cases/view/4091

Steps:
1. Launch Firefox.
2. From the toolbar, click the 'View history, saved bookmarks and more' button and select `Bookmarks'.
3. From the menu, select 'Bookmarking tools' and click on View Bookmarks Sidebar button.

Expected results:
1. Firefox is successfully launched.
2. Clicking the button brings up the Bookmarks Menu.
3. The Bookmarks Sidebar is enabled and displayed at the left side.

Comment 1

[Paul Silaghi, QA [:pauly]]

Attachment: browser_enable_toolbar_sidebar

Comment 2

[Mark Banner (:standard8)]

Comment on attachment 8955123 [details] [diff] [review]
browser_enable_toolbar_sidebar

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

::: browser/components/places/tests/browser/browser_enable_toolbar_sidebar.js
@@ +19,5 @@
> +    bookmarksBtn = document.getElementById("appMenu-library-bookmarks-button");
> +    return bookmarksBtn;
> +  }, "Should have the library 'Bookmarks' button.");
> +  let bookmarksView = document.getElementById("PanelUI-bookmarks");
> +  let bookmarksViewPromise = BrowserTestUtils.waitForEvent(bookmarksView, "ViewShown");

It might be nicer to factor these view - waitForEvent - click into a function, e.g. await selectAppMenuView("PanelUI-bookmarks", bookmarksBtn);

It hasn't been done elsewhere afaict, but might be worth seeing if it makes the code a bit more readable.

@@ +34,5 @@
> +  bookmarkingToolsBtn.click();
> +  await bookmarkingToolsViewPromise;
> +}
> +
> +add_task(async function test_enable_toolbar_sidebar() {

Please split this into two - one add_task for the toolbar, one for the sidebar.

@@ +35,5 @@
> +  await bookmarkingToolsViewPromise;
> +}
> +
> +add_task(async function test_enable_toolbar_sidebar() {
> +  await openBookmarkingToolsPanelInLibraryToolbarButton;

nit: missing brackets `()`

@@ +48,5 @@
> +  await BrowserTestUtils.waitForCondition(() => {
> +    toolbar = document.getElementById("PersonalToolbar");
> +    return !toolbar.collapsed;
> +  }, "Should have the Bookmarks Toolbar enabled.");
> +  Assert.ok(!toolbar.collapsed, "Bookmarks Toolbar is enabled");

We should also reset the state of the toolbar & sidebar after these tests so as to not break other tests.

@@ +50,5 @@
> +    return !toolbar.collapsed;
> +  }, "Should have the Bookmarks Toolbar enabled.");
> +  Assert.ok(!toolbar.collapsed, "Bookmarks Toolbar is enabled");
> +
> +  await openBookmarkingToolsPanelInLibraryToolbarButton;

nit: missing function call brackets

Comment 3

[Paul Silaghi, QA [:pauly]]

(In reply to Mark Banner (:standard8) from comment #2)
> Comment on attachment 8955123 [details] [diff] [review]
> browser_enable_toolbar_sidebar
> 
> Review of attachment 8955123 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/components/places/tests/browser/browser_enable_toolbar_sidebar.js
> @@ +19,5 @@
> > +    bookmarksBtn = document.getElementById("appMenu-library-bookmarks-button");
> > +    return bookmarksBtn;
> > +  }, "Should have the library 'Bookmarks' button.");
> > +  let bookmarksView = document.getElementById("PanelUI-bookmarks");
> > +  let bookmarksViewPromise = BrowserTestUtils.waitForEvent(bookmarksView, "ViewShown");
> 
> It might be nicer to factor these view - waitForEvent - click into a
> function, e.g. await selectAppMenuView("PanelUI-bookmarks", bookmarksBtn);
> 
> It hasn't been done elsewhere afaict, but might be worth seeing if it makes
> the code a bit more readable.

Thanks for the review.
How about to make this a little more general and include the buttonId also in the function?
e.g. async function selectAppMenuView(buttonId, viewId) {
  let btn;
  await BrowserTestUtils.waitForCondition(() => {
    btn = document.getElementById(buttonId);
    return btn;
  }, "Should have the " + buttonId + "button");
  let view = document.getElementById(viewId);
  let viewPromise = BrowserTestUtils.waitForEvent(view, "ViewShown");
  btn.click();
  await viewPromise;
}

Comment 4

[Mark Banner (:standard8)]

I wasn't sure we really need to waitForCondition for each button, but I guess it doesn't hurt that much, so I think that'd be fine.

Comment 5

[Paul Silaghi, QA [:pauly]]

Attachment: browser_enable_toolbar_sidebar

Comment 6

[Mark Banner (:standard8)]

Comment on attachment 8956756 [details] [diff] [review]
browser_enable_toolbar_sidebar

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

Great, thanks for the update. Much nicer with that extra function.

Comment 7

[Paul Silaghi, QA [:pauly]]

Try results:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=38da038756633103d4453767608ae1041f96961d

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2339a45e7261
Add automated test to check that the Bookmarks Toolbar and Sidebar can be enabled from the Bookmarks Menu. r=Standard8

Comment 9

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/2339a45e7261

Comment 10

[Paul Silaghi, QA [:pauly]]

Hi Mark,
Per https://bugzilla.mozilla.org/show_bug.cgi?id=1444226#c3, do you think adding "skip-if = (os == 'win' && ccov) # Bug 1423667" will solve this?

Comment 11

[Mark Banner (:standard8)]

Yes, I think we should do that (but please attach the patch to that bug).