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

RESOLVED FIXED in Firefox 60

Status

()

P2
normal
RESOLVED FIXED
a year ago
a year ago

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

a year ago
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

a year ago
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

a year ago
(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

a year ago
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

a year ago
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

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/2339a45e7261
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox60: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 60
Depends on: 1444226
(Assignee)

Comment 10

a year ago
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.