Closed Bug 1094864 Opened 5 years ago Closed 2 years ago

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

Categories

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

defect
Points:
8

Tracking

()

RESOLVED FIXED
Firefox 58
Tracking Status
firefox58 --- fixed

People

(Reporter: mak, Assigned: standard8)

References

Details

(Whiteboard: [fxsearch])

Attachments

(3 files, 1 obsolete file)

convert tests to the new async API.

This may be a bit more time consuming than other tests.
Points: 5 → 8
Flags: firefox-backlog+
Flags: qe-verify-
Assignee: nobody → ttaubert
Status: NEW → ASSIGNED
Iteration: --- → 38.2 - 9 Feb
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.
Attachment #8562012 - Attachment is obsolete: true
Attachment #8562067 - Flags: review?(mano)
Iteration: 38.2 - 9 Feb → 38.3 - 23 Feb
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.
Attachment #8562067 - Flags: review?(mano) → review+
(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
Iteration: 38.3 - 23 Feb → 39.1 - 9 Mar
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 :/
Assignee: ttaubert → nobody
Status: ASSIGNED → NEW
Iteration: 39.1 - 9 Mar → ---
Priority: -- → P2
Depends on: 1376925
Depends on: 1376929
Assignee: nobody → standard8
Whiteboard: [fxsearch]
Depends on: 1402707
Attachment #8911731 - Flags: review?(mak77)
Comment on attachment 8911731 [details]
Bug 1094864 - Remove unnecessary checks from browser/components/places.

https://reviewboard.mozilla.org/r/183142/#review188420
Attachment #8911731 - Flags: review?(mak77) → review+
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
Attachment #8911732 - Flags: review?(mak77) → review+
Pushed by mbanner@mozilla.com:
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
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.
Keywords: leave-open
https://hg.mozilla.org/mozilla-central/rev/b8d59e74c0bb
https://hg.mozilla.org/mozilla-central/rev/c2131082228a
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 58
You need to log in before you can comment on or make changes to this bug.