1439832 - Add automated test for "Bookmarks can be opened from the Bookmarks Menu"

Status: RESOLVED FIXED

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

Description

[George Echim]

TestRail link: https://testrail.stage.mozaws.net/index.php?/cases/view/4095

Steps:
1. Launch Firefox.
2. From the toolbar, click the 'View history, saved bookmarks and more' button and select `Bookmarks'.
3. From the Bookmarks Menu, open one of the websites displayed in that list.

Expected results:
1. Firefox is successfully launched.
2. Clicking the button brings up the Bookmarks Menu.
3. The bookmark is successfully opened and displayed in the current tab.

Comment 1

[George Echim]

Attachment: browser_bookmarks_recently_added

Comment 2

[George Echim]

Attachment: browser_bookmarks_recently_added

Comment 3

[George Echim]

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

Comment 4

[Mark Banner (:standard8)]

Comment on attachment 8952658 [details] [diff] [review]
browser_bookmarks_recently_added

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

Thanks for the test, the general idea is fine, but I think we can make it simpler and clearer to read.

::: browser/components/places/tests/browser/browser.ini
@@ +18,4 @@
>  [browser_bookmark_change_location.js]
>  skip-if = (os == 'win' && ccov) # Bug 1423667
>  [browser_bookmark_folder_moveability.js]
> +[browser_bookmark_recently_added.js]

I think we should call this something like: browser_toolbar_library_open_recent.js

That will distinguish it from the other tests and from the library window tests.

@@ +22,1 @@
>  skip-if = (os == 'win' && ccov) # Bug 1423667

You need to duplicate the skip-if as well, otherwise browser_bookmark_folder_moveability.js is going to be run on ccov when we don't want it to be (we don't want the new test to be run on it either at the moment).

::: browser/components/places/tests/browser/browser_bookmark_recently_added.js
@@ +1,3 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */

We generally prefer to use the public domain license for tests these days:

https://www.mozilla.org/en-US/MPL/headers/#pd-c

@@ +8,5 @@
> + * Tests that recently added bookmarks can be opened.
> + */
> +
> +const BASE_URL = "http://example.org/browser/browser/components/places/tests/browser/";
> +const MOCK_HISTORY = [

These aren't history, they're going to be bookmarks.

@@ +24,5 @@
> +  await ViewShownPromise;
> +  info("Library panel shown.");
> +}
> +
> +async function openBookmarksPanel() {

I think openLibraryPanel and openBookmarksPanel could be combined into one function. I would propose something like openBookmarksPanelInLibraryToolbarButton as that saves confusion with the library window and other panels that we have.

@@ +25,5 @@
> +  info("Library panel shown.");
> +}
> +
> +async function openBookmarksPanel() {
> +  const bookmarksButton = document.getElementById("appMenu-library-bookmarks-button");

Generally we prefer to use `let`s within functions, we only use constants for global related items.

@@ +26,5 @@
> +}
> +
> +async function openBookmarksPanel() {
> +  const bookmarksButton = document.getElementById("appMenu-library-bookmarks-button");
> +  ok(bookmarksButton, "Bookmarks button exists");

Please use Assert.ok and other Assert functions - they provide slightly better debug output and are generally being moved towards.

@@ +38,5 @@
> +  return BookmarksView;
> +}
> +
> +async function openBookmarkedItemInNewTab(itemFromMenu, shouldHaveTitle) {
> +  return new Promise(async (resolve, reject) => {

No need for the promise wrapper here. await will wait until the item is resolved before moving onto the next command in the function (or returning from the function).

@@ +42,5 @@
> +  return new Promise(async (resolve, reject) => {
> +    const mouseMovePromise = BrowserTestUtils.waitForEvent(itemFromMenu, "mousemove");
> +    EventUtils.synthesizeMouseAtCenter(itemFromMenu, { type: "mousemove" });
> +    await mouseMovePromise;
> +    info("Moved bookmark item");

There shouldn't be a need to move the mouse, synthesizing the mouse click will do it at the right location.

@@ +46,5 @@
> +    info("Moved bookmark item");
> +
> +    const placesContext = document.getElementById("placesContext");
> +    const openContextMenuPromise = BrowserTestUtils.waitForEvent(placesContext, "popupshown");
> +    await EventUtils.synthesizeMouseAtCenter(itemFromMenu, {

No need for await, `synthesizeMouseAtCenter` doesn't return a promise, and we're not interested in the return value.

@@ +61,5 @@
> +    });
> +    info("Click open in new tab");
> +
> +    const lastOpenedTab = await tabCreatedPromise;
> +    Assert.equal(lastOpenedTab.getAttribute("label"), shouldHaveTitle, "Check title");

I think it would be better to check the url here.

@@ +70,5 @@
> +
> +async function closeTabs() {
> +  const tabCount = gBrowser.tabs.length;
> +  for (var i = 1; i < tabCount; i++) {
> +    await gBrowser.removeTab(gBrowser.selectedTab);

I think it would be better to record the tabs that were opened, and then iterate through the list and use `await BrowserTestUtils.removeTab(...)` to close them. That way, you guarantee you close the right tabs.

@@ +90,5 @@
> +  return items;
> +}
> +
> +// Add new bookmarks before running main test.
> +add_task(async function before_test() {

No need for the comment, and please rename before_test to setup to be consistent with our other tests.

@@ +93,5 @@
> +// Add new bookmarks before running main test.
> +add_task(async function before_test() {
> +  const libraryButton = CustomizableUI.getPlacementOfWidget("library-button");
> +  if (!libraryButton) {
> +    CustomizableUI.addWidgetToArea("library-button", CustomizableUI.AREA_NAVBAR);

Looking at other tests, I don't think we need to do this, but if you want to keep it, we should add within this setup function something like:

registerTestCleanup(async () => {
  CustomizableUI.reset();
});

@@ +95,5 @@
> +  const libraryButton = CustomizableUI.getPlacementOfWidget("library-button");
> +  if (!libraryButton) {
> +    CustomizableUI.addWidgetToArea("library-button", CustomizableUI.AREA_NAVBAR);
> +  } else {
> +    ok(libraryButton, "Library button present by default");

No need to check this here, the if statement has done enough for us.

@@ +98,5 @@
> +  } else {
> +    ok(libraryButton, "Library button present by default");
> +  }
> +  // Fake bookmarks.
> +  await Promise.all(MOCK_HISTORY.map(async (newUrlToBokmark, index) => {

Please use PlacesUtils.bookmarks.insertTree() - there's various examples in the places tests already.

I would also suggest that you make the source array an array of objects that suites the children passed to insertTree. Then when it comes to checking the results, you can use the source data directly, and avoid hard-coding titles in multiple places.

@@ +121,5 @@
> +  }
> +
> +  registerCleanupFunction(async () => {
> +    await closeTabs();
> +    await PlacesUtils.bookmarks.eraseEverything();

I'd prefer the eraseEverything part of this be moved to the setup section of the test - then people know it is erased afterwards, regardless of if setup fails.

Comment 5

[George Echim]

Attachment: browser_toolbar_library_open_recent

Thank you for the review !
I have attached the new updated patch.

Comment 6

[Mark Banner (:standard8)]

Comment on attachment 8953447 [details] [diff] [review]
browser_toolbar_library_open_recent

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

Thank you, this is looking a lot better. I think there's a few things to tidy up still so that we can make it flow and read a bit better.

::: browser/components/places/tests/browser/browser_toolbar_library_open_recent.js
@@ +20,5 @@
> +  let bookmarksButton;
> +  await BrowserTestUtils.waitForCondition(() => {
> +    bookmarksButton = document.getElementById("appMenu-library-bookmarks-button");
> +    return bookmarksButton;
> +  }, "Was unable verify status");

Nit: Generally I like the descriptions to be a bit more "should...." and more descriptive. So for this one I'd probably say something like "Should have the library bookmarks button"

@@ +28,5 @@
> +  bookmarksButton.click();
> +  await viewRecentPromise;
> +}
> +
> +async function openBookmarkedItemInNewTab(itemFromMenu, shouldHaveUri) {

Generally we use "expected" rather than "should" for variable names.

However, I think we can make this a bit nicer, if in getRecentlyBookmarkedItems you add a check that:

Assert.equal(items[0]._placesNode.uri, bookmarkItems[1].url, "Should match the expected url");

Then here, we know that the uri of the item is right, so you can drop the current `shouldHaveUri` parameter and use `itemFromMenu._placesNode.uri` in its place when chekcing the spec.

@@ +46,5 @@
> +  });
> +  info("Click open in new tab");
> +
> +  let lastOpenedTab = await tabCreatedPromise;
> +  Assert.equal(lastOpenedTab.linkedBrowser.currentURI.spec, shouldHaveUri, "Check URI");

"Check URI" -> "Should have opened the correct URI"

@@ +54,5 @@
> +async function closeTabs() {
> +  for (var i = 0; i < openedTabs.length; i++) {
> +    await gBrowser.removeTab(openedTabs[i]);
> +  }
> +  Assert.equal(gBrowser.tabs.length, 1, "Should close all new opened tabs");

nit: drop the 'new'

@@ +65,5 @@
> +
> +  await BrowserTestUtils.waitForCondition(() => items[0].attributes !== "undefined", "Custom bookmark exists");
> +
> +  if (items) {
> +    Assert.equal(items[0].getAttribute("label"), "Custom Title 2", "Check bookmark title");

nit: "Check bookmark title" -> "Should have the expected title"

@@ +80,5 @@
> +  await PlacesUtils.bookmarks.insertTree({
> +    guid: PlacesUtils.bookmarks.menuGuid,
> +    children: [{
> +      url: `${BASE_URL}bookmark_dummy_1.html`,
> +      title: "Custom Title 1",

I still think you should create a const at the top of the file for this. Something like:

```
const bookmarkItems = [{
  url: `${BASE_URL}bookmark_dummy_1.html`,
  title: "Custom Title 1"
}, [
  url: `${BASE_URL}bookmark_dummy_2.html`,
  title: "Custom Title 2",
}];
```

You can just do `children: bookmarkItems,`.

Then in getRecentlyBookmarkedItems, you can do something like:

Assert.equal(items[0].getAttribute("label"), bookmarkItems[1].title, "Should be the expected title");

@@ +94,5 @@
> +  });
> +});
> +
> +add_task(async function test_recently_added() {
> +  await promisePlacesInitComplete();

This shouldn't be necessary for this test.

@@ +101,5 @@
> +  let historyItems = await getRecentlyBookmarkedItems();
> +
> +  if (historyItems) {
> +    await openBookmarkedItemInNewTab(historyItems[0], `${BASE_URL}bookmark_dummy_2.html`);
> +    await openBookmarkedItemInNewTab(historyItems[1], `${BASE_URL}bookmark_dummy_1.html`);

This would be better using a for loop, e.g. `for (let item of historyItems) { await ... };`

Comment 7

[George Echim]

Attachment: browser_toolbar_library_open_recent_2

Hi Mark,

Thanks for your suggestions.
I have modified the patch accordingly.

Comment 8

[Mark Banner (:standard8)]

Comment on attachment 8954797 [details] [diff] [review]
browser_toolbar_library_open_recent_2

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

Thanks for the update.

Comment 9

[George Echim]

Hi Mark,

After putting my test to try : https://treeherder.mozilla.org/#/jobs?repo=try&revision=49863131674084fd6bd06074efb4e463d6fb19e2
I found that my test fails with: at browser_toolbar_library_open_recent.js:46 - ReferenceError: info is not defined

Should I add a skip with a bug number or can I ask a sheriff to merge it ?

Thank you !

Comment 10

[Mark Banner (:standard8)]

There's no point in skipping it, as the test just won't run, and it won't be worth landing it.

For the issue, you need to look further up in the test log - the error you've pointed to there happens after the test has timed out, and is a resultant fallout.

Earlier in the test we see:

12:45:19     INFO -  567 INFO TEST-PASS | browser/components/places/tests/browser/browser_toolbar_library_open_recent.js | Should match the expected url - "http://example.org/browser/browser/components/places/tests/browser/bookmark_dummy_1.html" == "http://example.org/browser/browser/components/places/tests/browser/bookmark_dummy_1.html" -
12:45:19     INFO -  568 INFO TEST-PASS | browser/components/places/tests/browser/browser_toolbar_library_open_recent.js | Should be the expected title - "Custom Title 1" == "Custom Title 1" -
12:45:19     INFO -  Buffered messages logged at 12:42:38
12:45:19     INFO -  569 INFO Console message: [JavaScript Error: "Clamped larged numeric value."]
12:45:19     INFO -  Buffered messages logged at 12:42:44
12:45:19     INFO -  570 INFO Console message: [JavaScript Error: "Clamped larged numeric value."]
12:45:19     INFO -  Buffered messages logged at 12:43:18
12:45:19     INFO -  571 INFO Console message: [JavaScript Error: "Clamped larged numeric value."]
12:45:19     INFO -  Buffered messages logged at 12:43:20
12:45:19     INFO -  572 INFO Console message: [JavaScript Error: "Clamped larged numeric value."]
12:45:19     INFO -  Buffered messages logged at 12:43:24
12:45:19     INFO -  573 INFO Console message: [JavaScript Error: "Clamped larged numeric value."]
12:45:19     INFO -  Buffered messages logged at 12:44:28
12:45:19     INFO -  574 INFO Console message: [JavaScript Error: "Clamped larged numeric value."]
12:45:19     INFO -  Buffered messages finished
12:45:19    ERROR -  575 INFO TEST-UNEXPECTED-FAIL | browser/components/places/tests/browser/browser_toolbar_library_open_recent.js | Test timed out -

I'd ignore the large numeric value stuff, since I don't think they are relevant to us.

The important bit here is that it did the checks in getRecentlyBookmarkedItems(), but then it hung.

If you look at the screenshots linked from the logs, you'll see that the library button is open, but nothing else has happened. This suggests that attempting to open the context menu is not reliable.

You might want to add some more debug to confirm this is the case - i.e. just before the await on the promise.

You could also take a look and see if you find existing tests where we test the library menu and see what they do.

Comment 11

[George Echim]

Attachment: mochitest_browser_toolbar_library_open_recent_3

Hi Mark,

I have looked at other tests and noticed "button:2" parameter
I have updated the patch and put it to try with no errors this time.

+    EventUtils.synthesizeMouseAtCenter(itemFromMenu, {
+        button: 2,
+        type: "contextmenu"
+    });

Try link: https://treeherder.mozilla.org/#/jobs?repo=try&revision=17d412d1d0a70d1b2db4bf16980a7048a2061dfa

Comment 12

[Mark Banner (:standard8)]

Comment on attachment 8956734 [details] [diff] [review]
mochitest_browser_toolbar_library_open_recent_3

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

Unfortunately this patch has also reverted to 4-space indentation. Something wrong with your editor?

Comment 13

[George Echim]

Attachment: mochitest_browser_toolbar_library_open_recent_4

Hi Mark,
Sorry about that, I uploaded the new patch.

Comment 14

[Mark Banner (:standard8)]

Comment on attachment 8957092 [details] [diff] [review]
mochitest_browser_toolbar_library_open_recent_4

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

Thanks for the update. One minor change to make that I only just spotted. No need to re-request review as long as that skip-if line addition is dropped & no other changes.

::: browser/components/places/tests/browser/browser.ini
@@ +111,5 @@
>  [browser_stayopenmenu.js]
>  skip-if = os == "mac" && debug # bug 1400323
>  [browser_toolbar_drop_text.js]
> +skip-if = (os == 'win' && ccov) # Bug 1423667
> +[browser_toolbar_library_open_recent.js]

Sorry, just noticed this, the skip-if line here is being added for browser_toolbar_drop_text.js - I don't think that's intentional.

Lets try not adding the skip-if for landing this. If we get issues, we can always add it in again later.

Comment 15

[Mark Banner (:standard8)]

Attachment: Bug 1439832 - Add a test for checking that bookmarks can be opened from Bookmarks in the Library Menu.

Review commit: https://reviewboard.mozilla.org/r/243034/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/243034/

Comment 16

[Mark Banner (:standard8)]

Comment on attachment 8974644 [details]
Bug 1439832 - Add a test for checking that bookmarks can be opened from Bookmarks in the Library Menu.

https://reviewboard.mozilla.org/r/243034/#review248832

Comment 17

[Mark Banner (:standard8)]

Comment on attachment 8957092 [details] [diff] [review]
mochitest_browser_toolbar_library_open_recent_4

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

The patch I just attached via mozreview, is the same, except for removing the skip-if line I mentioned.

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6bc652b4e61a
Add a test for checking that bookmarks can be opened from Bookmarks Menu - Recently Added Bookmarks section. r=standard8

Comment 19

[Raul Gurzau (:RaulG)]

Backed out changeset 6bc652b4e61a (bug 1439832) for failing on /tests/browser/browser_toolbar_overflow.js on a CLOSED TREE

Backout link: https://hg.mozilla.org/integration/autoland/rev/be56777fb305802ed1f80cecace5376003bc3fb5

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=6bc652b4e61a3abf09dc2a150236e8eaaf8ec0c4

Log link: https://treeherder.mozilla.org/logviewer.html#?job_id=177873090&repo=autoland&lineNumber=9635

Log snippet: 

[task 2018-05-10T15:21:50.632Z] 15:21:50     INFO - TEST-START | browser/components/places/tests/browser/browser_toolbar_overflow.js
[task 2018-05-10T15:21:56.561Z] 15:21:56     INFO - GECKO(3027) | --DOCSHELL 0xe2571800 == 0 [pid = 3178] [id = {6198dbd4-7162-49cd-85b2-69a2d66e82af}]
[task 2018-05-10T15:21:58.657Z] 15:21:58     INFO - GECKO(3027) | --DOCSHELL 0xe2048800 == 2 [pid = 3149] [id = {03b50e5b-baba-494b-a7a3-8428a092b796}]
[task 2018-05-10T15:21:58.659Z] 15:21:58     INFO - GECKO(3027) | --DOCSHELL 0xe2047000 == 1 [pid = 3149] [id = {5862e5c0-e592-42c9-b993-05c417b1665a}]
[task 2018-05-10T15:21:59.408Z] 15:21:59     INFO - GECKO(3027) | --DOMWINDOW == 6 (0xe27112e0) [pid = 3149] [serial = 104] [outer = (nil)] [url = about:blank]
[task 2018-05-10T15:21:59.410Z] 15:21:59     INFO - GECKO(3027) | --DOMWINDOW == 5 (0xe2711410) [pid = 3149] [serial = 106] [outer = (nil)] [url = http://example.org/browser/browser/components/places/tests/browser/bookmark_dummy_2.html]
[task 2018-05-10T15:22:01.060Z] 15:22:01     INFO - GECKO(3027) | --DOMWINDOW == 2 (0xe7a97a00) [pid = 3178] [serial = 45] [outer = (nil)] [url = about:blank]
[task 2018-05-10T15:22:01.461Z] 15:22:01     INFO - GECKO(3027) | --DOCSHELL 0xe43abc00 == 0 [pid = 3122] [id = {e301b232-a88a-4c49-a50b-5cdb3cafb607}]
[task 2018-05-10T15:22:02.003Z] 15:22:02     INFO - GECKO(3027) | --DOMWINDOW == 1 (0xf714e120) [pid = 3122] [serial = 91] [outer = (nil)] [url = about:blank]
[task 2018-05-10T15:22:03.137Z] 15:22:03     INFO - GECKO(3027) | --DOMWINDOW == 4 (0xe4225400) [pid = 3149] [serial = 108] [outer = (nil)] [url = http://example.org/browser/browser/components/places/tests/browser/bookmark_dummy_2.html]
[task 2018-05-10T15:22:03.137Z] 15:22:03     INFO - GECKO(3027) | --DOMWINDOW == 3 (0xe4222000) [pid = 3149] [serial = 107] [outer = (nil)] [url = about:blank]
[task 2018-05-10T15:22:03.139Z] 15:22:03     INFO - GECKO(3027) | --DOMWINDOW == 2 (0xe4222600) [pid = 3149] [serial = 105] [outer = (nil)] [url = about:blank]
[task 2018-05-10T15:22:05.115Z] 15:22:05     INFO - GECKO(3027) | --DOMWINDOW == 1 (0xe25351b0) [pid = 3178] [serial = 44] [outer = (nil)] [url = http://example.org/browser/browser/components/places/tests/browser/bookmark_dummy_1.html]
[task 2018-05-10T15:22:05.907Z] 15:22:05     INFO - GECKO(3027) | --DOMWINDOW == 0 (0xe435de00) [pid = 3122] [serial = 92] [outer = (nil)] [url = about:blank]
[task 2018-05-10T15:22:09.013Z] 15:22:09     INFO - GECKO(3027) | --DOMWINDOW == 0 (0xe7a97200) [pid = 3178] [serial = 46] [outer = (nil)] [url = http://example.org/browser/browser/components/places/tests/browser/bookmark_dummy_1.html]
[task 2018-05-10T15:22:42.781Z] 15:22:42     INFO - GECKO(3027) | --DOMWINDOW == 12 (0xc2715e00) [pid = 3027] [serial = 316] [outer = (nil)] [url = about:blank]
[task 2018-05-10T15:23:31.821Z] 15:23:31     INFO - GECKO(3027) | ++DOCSHELL 0x1420f800 == 7 [pid = 3027] [id = {a38d2778-26c4-488e-929d-dc519da043f2}]
[task 2018-05-10T15:23:31.821Z] 15:23:31     INFO - GECKO(3027) | ++DOMWINDOW == 13 (0xc24b4410) [pid = 3027] [serial = 317] [outer = (nil)]
[task 2018-05-10T15:23:31.821Z] 15:23:31     INFO - GECKO(3027) | ++DOMWINDOW == 14 (0x3cdc3600) [pid = 3027] [serial = 318] [outer = 0xc24b4410]
[task 2018-05-10T15:23:32.343Z] 15:23:32     INFO - GECKO(3027) | ++DOCSHELL 0xcc0ce400 == 8 [pid = 3027] [id = {6df04b55-ff25-47d8-897e-7bd7ffa517bc}]
[task 2018-05-10T15:23:32.344Z] 15:23:32     INFO - GECKO(3027) | ++DOMWINDOW == 15 (0xc24b4540) [pid = 3027] [serial = 319] [outer = (nil)]
[task 2018-05-10T15:23:32.344Z] 15:23:32     INFO - GECKO(3027) | ++DOCSHELL 0xcc0ce800 == 9 [pid = 3027] [id = {778d6adc-a4cd-4d5f-bd6b-33a014aaf6fc}]
[task 2018-05-10T15:23:32.344Z] 15:23:32     INFO - GECKO(3027) | ++DOMWINDOW == 16 (0xc24b4670) [pid = 3027] [serial = 320] [outer = (nil)]
[task 2018-05-10T15:23:32.501Z] 15:23:32     INFO - GECKO(3027) | ++DOMWINDOW == 17 (0xc26b2000) [pid = 3027] [serial = 321] [outer = 0xc24b4670]
[task 2018-05-10T15:23:32.850Z] 15:23:32     INFO - GECKO(3027) | ++DOCSHELL 0xe43abc00 == 1 [pid = 3122] [id = {e32b5ab2-fcd4-4fda-9028-3772edae743c}]
[task 2018-05-10T15:23:32.851Z] 15:23:32     INFO - GECKO(3027) | ++DOMWINDOW == 1 (0xf714e120) [pid = 3122] [serial = 93] [outer = (nil)]
[task 2018-05-10T15:23:32.997Z] 15:23:32     INFO - GECKO(3027) | ++DOMWINDOW == 2 (0xe435de00) [pid = 3122] [serial = 94] [outer = 0xf714e120]
[task 2018-05-10T15:23:33.107Z] 15:23:33     INFO - GECKO(3027) | ++DOMWINDOW == 18 (0xc26b3000) [pid = 3027] [serial = 322] [outer = 0xc24b4540]
[task 2018-05-10T15:23:33.123Z] 15:23:33     INFO - GECKO(3027) | ++DOMWINDOW == 3 (0xe435ea00) [pid = 3122] [serial = 95] [outer = 0xf714e120]
[task 2018-05-10T15:23:33.322Z] 15:23:33     INFO - GECKO(3027) | [Parent 3027, Main Thread] WARNING: ENSURE_TRUE(weakFrame.IsAlive()) failed: file /builds/worker/workspace/build/src/layout/xul/nsXULPopupManager.cpp, line 1165
[task 2018-05-10T15:23:34.499Z] 15:23:34     INFO - GECKO(3027) | --DOCSHELL 0xcc0ce400 == 8 [pid = 3027] [id = {6df04b55-ff25-47d8-897e-7bd7ffa517bc}]
[task 2018-05-10T15:23:34.639Z] 15:23:34     INFO - GECKO(3027) | [Parent 3027, Main Thread] WARNING: Suboptimal indexes for the SQL statement 0xdc806820 (http://mzl.la/1FuID0j).: file /builds/worker/workspace/build/src/storage/mozStoragePrivateHelpers.cpp, line 114
[task 2018-05-10T15:23:34.640Z] 15:23:34     INFO - GECKO(3027) | [Parent 3027, Main Thread] WARNING: Suboptimal indexes for the SQL statement 0xd04c8740 (http://mzl.la/1FuID0j).: file /builds/worker/workspace/build/src/storage/mozStoragePrivateHelpers.cpp, line 114
[task 2018-05-10T15:23:34.706Z] 15:23:34     INFO - TEST-INFO | started process screentopng
[task 2018-05-10T15:23:35.477Z] 15:23:35     INFO - TEST-INFO | screentopng: exit 0
[task 2018-05-10T15:23:35.478Z] 15:23:35     INFO - Buffered messages logged at 15:21:50
[task 2018-05-10T15:23:35.478Z] 15:23:35     INFO - Entering test bound setup
[task 2018-05-10T15:23:35.478Z] 15:23:35     INFO - Buffered messages logged at 15:22:41
[task 2018-05-10T15:23:35.478Z] 15:23:35     INFO - TEST-PASS | browser/components/places/tests/browser/browser_toolbar_overflow.js | The toolbar is collapsed by default - true == true - 
[task 2018-05-10T15:23:35.479Z] 15:23:35     INFO - Buffered messages logged at 15:23:18
[task 2018-05-10T15:23:35.479Z] 15:23:35     INFO - Leaving test bound setup
[task 2018-05-10T15:23:35.479Z] 15:23:35     INFO - Entering test bound 
[task 2018-05-10T15:23:35.479Z] 15:23:35     INFO - TEST-PASS | browser/components/places/tests/browser/browser_toolbar_overflow.js | The overflow chevron should be visible - true == true - 
[task 2018-05-10T15:23:35.482Z] 15:23:35     INFO - TEST-PASS | browser/components/places/tests/browser/browser_toolbar_overflow.js | Not all the nodes should be built by default - true == true - 
[task 2018-05-10T15:23:35.484Z] 15:23:35     INFO - TEST-PASS | browser/components/places/tests/browser/browser_toolbar_overflow.js | The number of visible nodes should be smaller than the number of built nodes - true == true - 
[task 2018-05-10T15:23:35.485Z] 15:23:35     INFO - Node at the last visible index
[task 2018-05-10T15:23:35.486Z] 15:23:35     INFO - Buffered messages logged at 15:23:22
[task 2018-05-10T15:23:35.487Z] 15:23:35     INFO - TEST-PASS | browser/components/places/tests/browser/browser_toolbar_overflow.js | Sanity check the bookmark index - 43 == 43 - 
[task 2018-05-10T15:23:35.489Z] 15:23:35     INFO - TEST-PASS | browser/components/places/tests/browser/browser_toolbar_overflow.js | Found the added bookmark at the expected position - "6wkysgVUEY2P" == "6wkysgVUEY2P" - 
[task 2018-05-10T15:23:35.490Z] 15:23:35     INFO - TEST-PASS | browser/components/places/tests/browser/browser_toolbar_overflow.js | The bookmark node should be visible - "visible" == "visible" - 
[task 2018-05-10T15:23:35.491Z] 15:23:35     INFO - TEST-PASS | browser/components/places/tests/browser/browser_toolbar_overflow.js | Number of built nodes should stay the same - 126 == 126 - 
[task 2018-05-10T15:23:35.492Z] 15:23:35     INFO - Remove the node
[task 2018-05-10T15:23:35.493Z] 15:23:35     INFO - Buffered messages logged at 15:23:24
[task 2018-05-10T15:23:35.495Z] 15:23:35     INFO - TEST-PASS | browser/components/places/tests/browser/browser_toolbar_overflow.js | Found the previous bookmark at the expected position - "http://test.places.43/" == "http://test.places.43/" - 
[task 2018-05-10T15:23:35.498Z] 15:23:35     INFO - TEST-PASS | browser/components/places/tests/browser/browser_toolbar_overflow.js | The bookmark node should be visible - "visible" == "visible" - 
[task 2018-05-10T15:23:35.499Z] 15:23:35     INFO - TEST-PASS | browser/components/places/tests/browser/browser_toolbar_overflow.js | Number of built nodes should stay the same - 126 == 126 - 
[task 2018-05-10T15:23:35.499Z] 15:23:35     INFO - Node at the first invisible index
[task 2018-05-10T15:23:35.500Z] 15:23:35     INFO - TEST-PASS | browser/components/places/tests/browser/browser_toolbar_overflow.js | Sanity check the bookmark index - 44 == 44 - 
[task 2018-05-10T15:23:35.501Z] 15:23:35     INFO - TEST-PASS | browser/components/places/tests/browser/browser_toolbar_overflow.js | Found the added bookmark at the expected position - "CV2umLhMs-7a" == "CV2umLhMs-7a" - 
[task 2018-05-10T15:23:35.502Z] 15:23:35     INFO - TEST-PASS | browser/components/places/tests/browser/browser_toolbar_overflow.js | The bookmark node should be hidden - "hidden" == "hidden" - 
[task 2018-05-10T15:23:35.503Z] 15:23:35     INFO - TEST-PASS | browser/components/places/tests/browser/browser_toolbar_overflow.js | Number of built nodes should stay the same - 126 == 126 - 
[task 2018-05-10T15:23:35.504Z] 15:23:35     INFO - Remove the node
[task 2018-05-10T15:23:35.506Z] 15:23:35     INFO - Buffered messages logged at 15:23:25
[task 2018-05-10T15:23:35.507Z] 15:23:35     INFO - TEST-PASS | browser/components/places/tests/browser/browser_toolbar_overflow.js | Found the previous bookmark at the expected position - "http://test.places.44/" == "http://test.places.44/" - 
[task 2018-05-10T15:23:35.509Z] 15:23:35     INFO - TEST-PASS | browser/components/places/tests/browser/browser_toolbar_overflow.js | The bookmark node should be hidden - "hidden" == "hidden" - 
[task 2018-05-10T15:23:35.510Z] 15:23:35     INFO - TEST-PASS | browser/components/places/tests/browser/browser_toolbar_overflow.js | Number of built nodes should stay the same - 126 == 126 - 
[task 2018-05-10T15:23:35.510Z] 15:23:35     INFO - First non-built node
[task 2018-05-10T15:23:35.511Z] 15:23:35     INFO - TEST-PASS | browser/components/places/tests/browser/browser_toolbar_overflow.js | Sanity check the bookmark index - 126 == 126 - 
[task 2018-05-10T15:23:35.512Z] 15:23:35     INFO - TEST-PASS | browser/components/places/tests/browser/browser_toolbar_overflow.js | The new node should not have been added - true == true - 
[task 2018-05-10T15:23:35.515Z] 15:23:35     INFO - TEST-PASS | browser/components/places/tests/browser/browser_toolbar_overflow.js | Number of built nodes should stay the same - 126 == 126 - 
[task 2018-05-10T15:23:35.516Z] 15:23:35     INFO - Remove the node
[task 2018-05-10T15:23:35.516Z] 15:23:35     INFO - TEST-PASS | browser/components/places/tests/browser/browser_toolbar_overflow.js | Number of built nodes should stay the same - 126 == 126 - 
[task 2018-05-10T15:23:35.517Z] 15:23:35     INFO - Later non-built node
[task 2018-05-10T15:23:35.520Z] 15:23:35     INFO - Buffered messages logged at 15:23:26
[task 2018-05-10T15:23:35.520Z] 15:23:35     INFO - TEST-PASS | browser/components/places/tests/browser/browser_toolbar_overflow.js | Sanity check the bookmark index - 127 == 127 - 
[task 2018-05-10T15:23:35.523Z] 15:23:35     INFO - TEST-PASS | browser/components/places/tests/browser/browser_toolbar_overflow.js | The new node should not have been added - true == true - 
[task 2018-05-10T15:23:35.524Z] 15:23:35     INFO - TEST-PASS | browser/components/places/tests/browser/browser_toolbar_overflow.js | Number of built nodes should stay the same - 126 == 126 - 
[task 2018-05-10T15:23:35.524Z] 15:23:35     INFO - Remove the node
[task 2018-05-10T15:23:35.525Z] 15:23:35     INFO - TEST-PASS | browser/components/places/tests/browser/browser_toolbar_overflow.js | Number of built nodes should stay the same - 126 == 126 - 
[task 2018-05-10T15:23:35.526Z] 15:23:35     INFO - Move node from last visible to first hidden
[task 2018-05-10T15:23:35.528Z] 15:23:35     INFO - TEST-PASS | browser/components/places/tests/browser/browser_toolbar_overflow.js | The bookmark node should be visible - "visible" == "visible" - 
[task 2018-05-10T15:23:35.529Z] 15:23:35     INFO - TEST-PASS | browser/components/places/tests/browser/browser_toolbar_overflow.js | Found the moved bookmark at the expected position - "6IrSIBmz46qh" == "6IrSIBmz46qh" - 
[task 2018-05-10T15:23:35.531Z] 15:23:35     INFO - TEST-PASS | browser/components/places/tests/browser/browser_toolbar_overflow.js | The destination bookmark node should be hidden - "hidden" == "hidden" - 
[task 2018-05-10T15:23:35.533Z] 15:23:35     INFO - TEST-PASS | browser/components/places/tests/browser/browser_toolbar_overflow.js | The origin bookmark node should be visible - "visible" == "visible" - 
[task 2018-05-10T15:23:35.534Z] 15:23:35     INFO - TEST-PASS | browser/components/places/tests/browser/browser_toolbar_overflow.js | Found the pushed away bookmark at the expected position - "3VqRLj1XkRZJ" == "3VqRLj1XkRZJ" - 
[task 2018-05-10T15:23:35.535Z] 15:23:35     INFO - TEST-PASS | browser/components/places/tests/browser/browser_toolbar_overflow.js | Number of built nodes should stay the same - 126 == 126 - 
[task 2018-05-10T15:23:35.535Z] 15:23:35     INFO - Moving back the node
[task 2018-05-10T15:23:35.537Z] 15:23:35     INFO - TEST-PASS | browser/components/places/tests/browser/browser_toolbar_overflow.js | Found the moved bookmark at the expected position - "6IrSIBmz46qh" == "6IrSIBmz46qh" - 
[task 2018-05-10T15:23:35.538Z] 15:23:35     INFO - TEST-PASS | browser/components/places/tests/browser/browser_toolbar_overflow.js | The bookmark node should be hidden - "hidden" == "hidden" - 
[task 2018-05-10T15:23:35.540Z] 15:23:35     INFO - TEST-PASS | browser/components/places/tests/browser/browser_toolbar_overflow.js | The bookmark node should be visible - "visible" == "visible" - 
[task 2018-05-10T15:23:35.541Z] 15:23:35     INFO - TEST-PASS | browser/components/places/tests/browser/browser_toolbar_overflow.js | Found the pushed away bookmark at the expected position - "3VqRLj1XkRZJ" == "3VqRLj1XkRZJ" - 
[task 2018-05-10T15:23:35.544Z] 15:23:35     INFO - TEST-PASS | browser/components/places/tests/browser/browser_toolbar_overflow.js | Number of built nodes should stay the same - 126 == 126 - 
[task 2018-05-10T15:23:35.545Z] 15:23:35     INFO - Move node from fist visible to last built
[task 2018-05-10T15:23:35.546Z] 15:23:35     INFO - TEST-PASS | browser/components/places/tests/browser/browser_toolbar_overflow.js | The bookmark node should be visible - "visible" == "visible" - 
[task 2018-05-10T15:23:35.548Z] 15:23:35     INFO - Buffered messages logged at 15:23:27
[task 2018-05-10T15:23:35.550Z] 15:23:35     INFO - TEST-PASS | browser/components/places/tests/browser/browser_toolbar_overflow.js | Found the moved bookmark at the expected position - "seZLVG1BvPxs" == "seZLVG1BvPxs" - 
[task 2018-05-10T15:23:35.551Z] 15:23:35     INFO - TEST-PASS | browser/components/places/tests/browser/browser_toolbar_overflow.js | The destination bookmark node should be hidden - "hidden" == "hidden" - 
[task 2018-05-10T15:23:35.552Z] 15:23:35     INFO - TEST-PASS | browser/components/places/tests/browser/browser_toolbar_overflow.js | The origin bookmark node should be visible - "visible" == "visible" - 
[task 2018-05-10T15:23:35.554Z] 15:23:35     INFO - TEST-PASS | browser/components/places/tests/browser/browser_toolbar_overflow.js | Found the pushed away bookmark at the expected position - "qsaMUwRqKiI0" == "qsaMUwRqKiI0" - 
[task 2018-05-10T15:23:35.555Z] 15:23:35     INFO - TEST-PASS | browser/components/places/tests/browser/browser_toolbar_overflow.js | Number of built nodes should stay the same - 126 == 126 - 
[task 2018-05-10T15:23:35.556Z] 15:23:35     INFO - Moving back the node
[task 2018-05-10T15:23:35.559Z] 15:23:35     INFO - TEST-PASS | browser/components/places/tests/browser/browser_toolbar_overflow.js | Found the moved bookmark at the expected position - "seZLVG1BvPxs" == "seZLVG1BvPxs" - 
[task 2018-05-10T15:23:35.560Z] 15:23:35     INFO - TEST-PASS | browser/components/places/tests/browser/browser_toolbar_overflow.js | The bookmark node should be hidden - "hidden" == "hidden" - 
[task 2018-05-10T15:23:35.563Z] 15:23:35     INFO - TEST-PASS | browser/components/places/tests/browser/browser_toolbar_overflow.js | The bookmark node should be visible - "visible" == "visible" - 
[task 2018-05-10T15:23:35.564Z] 15:23:35     INFO - TEST-PASS | browser/components/places/tests/browser/browser_toolbar_overflow.js | Found the pushed away bookmark at the expected position - "qsaMUwRqKiI0" == "qsaMUwRqKiI0" - 
[task 2018-05-10T15:23:35.565Z] 15:23:35     INFO - TEST-PASS | browser/components/places/tests/browser/browser_toolbar_overflow.js | Number of built nodes should stay the same - 126 == 126 - 
[task 2018-05-10T15:23:35.566Z] 15:23:35     INFO - Move node from fist visible to first non built
[task 2018-05-10T15:23:35.567Z] 15:23:35     INFO - TEST-PASS | browser/components/places/tests/browser/browser_toolbar_overflow.js | The bookmark node should be visible - "visible" == "visible" - 
[task 2018-05-10T15:23:35.568Z] 15:23:35     INFO - TEST-PASS | browser/components/places/tests/browser/browser_toolbar_overflow.js | Node in the new position is not expected - true == true - 
[task 2018-05-10T15:23:35.569Z] 15:23:35     INFO - TEST-PASS | browser/components/places/tests/browser/browser_toolbar_overflow.js | We should keep number of built nodes consistent - [object XULElement] == true - 
[task 2018-05-10T15:23:35.573Z] 15:23:35     INFO - TEST-PASS | browser/components/places/tests/browser/browser_toolbar_overflow.js | The origin bookmark node should be visible - "visible" == "visible" - 
[task 2018-05-10T15:23:35.574Z] 15:23:35     INFO - TEST-PASS | browser/components/places/tests/browser/browser_toolbar_overflow.js | Number of built nodes should stay the same - 126 == 126 - 
[task 2018-05-10T15:23:35.574Z] 15:23:35     INFO - Moving back the node
[task 2018-05-10T15:23:35.575Z] 15:23:35     INFO - Buffered messages logged at 15:23:28
[task 2018-05-10T15:23:35.576Z] 15:23:35     INFO - TEST-PASS | browser/components/places/tests/browser/browser_toolbar_overflow.js | Found the moved bookmark at the expected position - "seZLVG1BvPxs" == "seZLVG1BvPxs" - 
[task 2018-05-10T15:23:35.577Z] 15:23:35     INFO - TEST-PASS | browser/components/places/tests/browser/browser_toolbar_overflow.js | The bookmark node should be visible - "visible" == "visible" - 
[task 2018-05-10T15:23:35.577Z] 15:23:35     INFO - TEST-PASS | browser/components/places/tests/browser/browser_toolbar_overflow.js | Number of built nodes should stay the same - 126 == 126 - 
[task 2018-05-10T15:23:35.580Z] 15:23:35     INFO - Leaving test bound 
[task 2018-05-10T15:23:35.583Z] 15:23:35     INFO - Entering test bound test_newWindow_noOverflow
[task 2018-05-10T15:23:35.583Z] 15:23:35     INFO - Check toolbar in a new widow when it was already visible and not overflowed
[task 2018-05-10T15:23:35.584Z] 15:23:35     INFO - Buffered messages logged at 15:23:33
[task 2018-05-10T15:23:35.585Z] 15:23:35     INFO - TEST-PASS | browser/components/places/tests/browser/browser_toolbar_overflow.js | The toolbar is not collapsed - true == true - 
[task 2018-05-10T15:23:35.585Z] 15:23:35     INFO - TEST-PASS | browser/components/places/tests/browser/browser_toolbar_overflow.js | The chevron should be collapsed - true == true - 
[task 2018-05-10T15:23:35.587Z] 15:23:35     INFO - Buffered messages logged at 15:23:34
[task 2018-05-10T15:23:35.589Z] 15:23:35     INFO - Leaving test bound test_newWindow_noOverflow
[task 2018-05-10T15:23:35.590Z] 15:23:35     INFO - Buffered messages finished
[task 2018-05-10T15:23:35.593Z] 15:23:35     INFO - TEST-UNEXPECTED-FAIL | browser/components/places/tests/browser/browser_toolbar_overflow.js | This test exceeded the timeout threshold. It should be rewritten or split up. If that's not possible, use requestLongerTimeout(N), but only as a last resort. -

Comment 20

[Mark Banner (:standard8)]

Think I eventually figured this out.

The test was using the "Open in new tab" option from the library menu, which happens to leave the menu open. That was interfering in other tests (and with itself, when running with --verify).

Now running the updated version through try:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0b905ce95087ea36c71dadda25046f7ee6a4e9fa

Comment 21

[Mark Banner (:standard8)]

Comment on attachment 8974644 [details]
Bug 1439832 - Add a test for checking that bookmarks can be opened from Bookmarks in the Library Menu.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/243034/diff/1-2/

Comment 22

[Mark Banner (:standard8)]

The change worked on try, so as it was just a small test-only change, I'll push this out again.

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2ef5c0e6dd2c
Add a test for checking that bookmarks can be opened from Bookmarks in the Library Menu. r=standard8

Comment 24

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/2ef5c0e6dd2c