1094864 - Convert mochitest-browser tests in browser/components/places to Bookmarks.jsm API

Status: RESOLVED FIXED

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

Description

[Marco Bonardo [:mak]]

convert tests to the new async API.

This may be a bit more time consuming than other tests.

Comment 1

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0001-Bug-1094864-Convert-mochitest-browser-tests-in-brows.patch (WIP)

Comment 2

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0001-Bug-1094864-Convert-mochitest-browser-tests-in-brows.patch

Partial patch to land the work I've put into it so far. I'll then wait for bug 1119282 to be done before continuing.

Comment 3

[Tim Taubert [:ttaubert] (inactive)]

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

Comment 4

[Asaf Romano (gone)]

Comment on attachment 8562067 [details] [diff] [review]
0001-Bug-1094864-Convert-mochitest-browser-tests-in-brows.patch

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

::: browser/components/places/tests/browser/browser_425884.js
@@ +58,5 @@
>    // confirm serialization
>    ok(rawNode.type, "confirm json node");
>    folderANode.containerOpen = false;
>  
> +  let testRootId = yield PlacesUtils.promiseItemId(testRoot.guid);

An alternative approach for this test would be to use something like ensureEqualBookmarksTrees (see test_async_transactions), rather than comparing result nodes. It would pave the way for removing itemId usage too.

But you don't have to do it here, and I could also do that when I convert these tests to use PT.

::: browser/components/places/tests/browser/browser_forgetthissite_single.js
@@ +44,5 @@
> +  selection.clearSelection();
> +  selection.rangedSelect(0, selectionCount - 1, true);
> +  is(selection.count, selectionCount, "The selected range is as big as expected");
> +
> +  // Open the context menu

nit: .

@@ +48,5 @@
> +  // Open the context menu
> +  let contextmenu = doc.getElementById("placesContext");
> +  let popupShown = promisePopupShown(contextmenu);
> +
> +  // Get cell coordinates

ditto (and elsewhere)

@@ +58,5 @@
> +  let forgetThisSite = doc.getElementById("placesContext_deleteHost");
> +  let hideForgetThisSite = (selectionCount != 1);
> +  is(forgetThisSite.hidden, hideForgetThisSite,
> +    "The Forget this site menu item should " + (hideForgetThisSite ? "" : "not ") +
> +    "be hidden with " + selectionCount + " items selected");

Please use template strings for things like this in new code.

Comment 5

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Mano (::mano, needinfo? for any questions; not reading general bugmail) from comment #4)
> > +  let testRootId = yield PlacesUtils.promiseItemId(testRoot.guid);
> 
> An alternative approach for this test would be to use something like
> ensureEqualBookmarksTrees (see test_async_transactions), rather than
> comparing result nodes. It would pave the way for removing itemId usage too.
> 
> But you don't have to do it here, and I could also do that when I convert
> these tests to use PT.

Ok, left that for you :)

https://hg.mozilla.org/integration/fx-team/rev/3215184d1171

Comment 6

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

https://hg.mozilla.org/mozilla-central/rev/3215184d1171

Comment 7

[Tim Taubert [:ttaubert] (inactive)]

That bug is blocked on bug 1119282. Will pick it up again when that's fixed. Shouldn't even have been carried over to the current iteration, sorry :/

Comment 8

[Mark Banner (:standard8)]

Attachment: Bug 1094864 - Remove unnecessary checks from browser/components/places.

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

Comment 9

[Mark Banner (:standard8)]

Attachment: Bug 1094864 - Rewrite most of browser/components/places/tests/browser to use the newer async Places APIs.

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

Comment 10

[Mark Banner (:standard8)]

Comment on attachment 8911731 [details]
Bug 1094864 - Remove unnecessary checks from browser/components/places.

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

Comment 11

[Mark Banner (:standard8)]

Comment on attachment 8911732 [details]
Bug 1094864 - Rewrite most of browser/components/places/tests/browser to use the newer async Places APIs.

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

Comment 12

[Marco Bonardo [:mak]]

Comment on attachment 8911731 [details]
Bug 1094864 - Remove unnecessary checks from browser/components/places.

https://reviewboard.mozilla.org/r/183142/#review188420

Comment 13

[Marco Bonardo [:mak]]

Comment on attachment 8911732 [details]
Bug 1094864 - Rewrite most of browser/components/places/tests/browser to use the newer async Places APIs.

https://reviewboard.mozilla.org/r/183144/#review188522

::: browser/components/places/tests/browser/browser_library_search.js:171
(Diff revision 2)
> +
> +  // Cleanup.
> +  PlacesUtils.tagging.untagURI(PlacesUtils._uri(TEST_URL), ["dummyTag"]);
> +
> +  await PlacesUtils.bookmarks.eraseEverything();
> +  await PlacesTestUtils.clearHistory();

PU.history.clear()

::: browser/components/places/tests/browser/browser_sidebarpanels_click.js:26
(Diff revision 2)
>      ok(false, "Unexpected sidebar found - a previous test failed to cleanup correctly");
>      SidebarUI.hide();
>    }
>  
> -  let sidebar = document.getElementById("sidebar");
> +  // Ensure history is clean before starting the test.
> +  await PlacesTestUtils.clearHistory();

PU.history.clear()

::: browser/components/places/tests/browser/browser_sidebarpanels_click.js:42
(Diff revision 2)
> -        PlacesUtils.unfiledBookmarksFolderId, PlacesUtils._uri(TEST_URL),
> -        PlacesUtils.bookmarks.DEFAULT_INDEX, "test"
> -      );
> -      aCallback();
> +        parentGuid: PlacesUtils.bookmarks.unfiledGuid,
> +        title: "test",
> +        url: TEST_URL,
> +      });
> +
> +      this._bookmarkId = await PlacesUtils.promiseItemId(this._bookmark.guid);

Looks like you could avoid storing _bookmarkId and just do this in selectNode?

::: browser/components/places/tests/browser/browser_sidebarpanels_click.js:82
(Diff revision 2)
>      sidebarName: HISTORY_SIDEBAR_ID,
>      treeName: HISTORY_SIDEBAR_TREE_ID,
>      desc: "History sidebar test"
>    });
>  
> -  function testPlacesPanel(preFunc, postFunc) {
> +  for (let i = 0; i < tests.length; i++) {

for...of

::: browser/components/places/tests/browser/browser_views_liveupdate.js:106
(Diff revision 2)
> -  bs.setItemTitle(id, "bubf1_edited");
> -  addedBookmarks.push(id);
> -  bs.moveItem(id, bs.unfiledBookmarksFolder, 0);
>  
> -  // Remove all added bookmarks.
> +    // Remove all added bookmarks.
> -  addedBookmarks.forEach(function(aItem) {
> +    for (let i = 0; i < addedBookmarks.length; i++) {

for...of

Comment 14

[Mark Banner (:standard8)]

Comment on attachment 8911731 [details]
Bug 1094864 - Remove unnecessary checks from browser/components/places.

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

Comment 15

[Mark Banner (:standard8)]

Comment on attachment 8911732 [details]
Bug 1094864 - Rewrite most of browser/components/places/tests/browser to use the newer async Places APIs.

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

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b8d59e74c0bb
Remove unnecessary checks from browser/components/places. r=mak
https://hg.mozilla.org/integration/autoland/rev/c2131082228a
Rewrite most of browser/components/places/tests/browser to use the newer async Places APIs. r=mak

Comment 17

[Mark Banner (:standard8)]

I probably should have mentioned before... the patches here get most of the remaining instances. There's a couple of tests I'm addressing in other bugs. There may also be some rarer old APIs in use that I missed, but we'll get those in the cleanup - I think the important part here is to convert most of the cases.

Comment 18

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

https://hg.mozilla.org/mozilla-central/rev/b8d59e74c0bb
https://hg.mozilla.org/mozilla-central/rev/c2131082228a