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

RESOLVED FIXED in Firefox 60

Status

()

enhancement
P2
normal
RESOLVED FIXED
Last year
Last year

People

(Reporter: pauly, Assigned: pauly)

Tracking

(Blocks 1 bug)

unspecified
Firefox 60
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox60 fixed)

Details

(Whiteboard: [fxsearch])

Attachments

(1 attachment, 1 obsolete attachment)

Assignee

Description

Last year
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.
Priority: -- → P2
Whiteboard: [fxsearch]
Assignee

Comment 1

Last year
Assignee: nobody → paul.silaghi
Attachment #8955123 - Flags: review?(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
Attachment #8955123 - Flags: review?(standard8)
Assignee

Comment 3

Last year
(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;
}
Flags: needinfo?(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.
Flags: needinfo?(standard8)
Assignee

Comment 5

Last year
Attachment #8955123 - Attachment is obsolete: true
Attachment #8956756 - Flags: review?(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.
Attachment #8956756 - Flags: review?(standard8) → review+

Comment 8

Last year
Pushed by ryanvm@gmail.com:
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
Keywords: checkin-needed

Comment 9

Last year
bugherder
https://hg.mozilla.org/mozilla-central/rev/2339a45e7261
Status: NEW → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → Firefox 60
Depends on: 1444226
Assignee

Comment 10

Last year
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?
Flags: needinfo?(standard8)
Yes, I think we should do that (but please attach the patch to that bug).
Flags: needinfo?(standard8)
You need to log in before you can comment on or make changes to this bug.