1376234 - Convert some xpcshell-tests in toolkit/components/places/tests/unit to async Bookmarks.jsm API

Status: RESOLVED FIXED

(Toolkit :: Places, enhancement, P2)

Description

[Mark Banner (:standard8)]

I've transitioned around 20 tests or so mostly to the new async Bookmarks.jsm API.

The only API I haven't transitioned is runInBatchMode - we can do that in a follow-up bug.

Comment 1

[Mark Banner (:standard8)]

Attachment: Bug 1376234 - Convert some xpcshell-tests in toolkit/components/places/tests/unit to async Bookmarks.jsm API.

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

Comment 2

[Mark Banner (:standard8)]

Attachment: Bug 1376234 - Convert more xpcshell-tests in toolkit/components/places/tests/unit to async Bookmarks.jsm API.

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

Comment 3

[Marco Bonardo [:mak]]

Comment on attachment 8881187 [details]
Bug 1376234 - Convert some xpcshell-tests in toolkit/components/places/tests/unit to async Bookmarks.jsm API.

https://reviewboard.mozilla.org/r/152460/#review157766

::: toolkit/components/places/tests/unit/test_PlacesUtils_lazyobservers.js:5
(Diff revision 1)
>  /* Any copyright is dedicated to the Public Domain.
>     http://creativecommons.org/publicdomain/zero/1.0/ */
>  
> -function run_test() {
> -  do_test_pending();
> +add_task(async function test_lazyBookmarksObservers() {
> +  const TEST_URI = NetUtil.newURI("http://moz.org/");

Services.io

::: toolkit/components/places/tests/unit/test_PlacesUtils_lazyobservers.js:7
(Diff revision 1)
>     http://creativecommons.org/publicdomain/zero/1.0/ */
>  
> -function run_test() {
> -  do_test_pending();
> +add_task(async function test_lazyBookmarksObservers() {
> +  const TEST_URI = NetUtil.newURI("http://moz.org/");
> +
> +  let promise = Promise.defer();

promiseUtils.defer() or new Promise()

::: toolkit/components/places/tests/unit/test_frecency_observers.js:64
(Diff revision 1)
>  });
>  
>  function onFrecencyChanged(expectedURI) {
>    return new Promise(resolve => {
>      let obs = new NavHistoryObserver();
> +    do_print("Observing");

leftover or actually wanted?

::: toolkit/components/places/tests/unit/test_history_autocomplete_tags.js:146
(Diff revision 1)
>   * @param aURI
>   *        The nsIURI to tag.
>   * @param aTags
>   *        The tags to add.
>   */
> -function tagURI(aURI, aTags) {
> +async function tagURI(aURI, aTags) {

nit: may s/aURI/url/ and aTags/tags/ and just pass url to insert.

::: toolkit/components/places/tests/unit/test_hosts_triggers.js:137
(Diff revision 1)
> +    unfiledBookmarksRoot.getChild(unfiledBookmarksRoot.childCount - 1).bookmarkGuid;
> +
> +  await PlacesUtils.bookmarks.remove(itemGuid);

Services.io

::: toolkit/components/places/tests/unit/test_isvisited.js:67
(Diff revision 1)
>          do_check_true(error.message.includes("No items were added to history"));
>        });
>        do_check_false(await promiseIsURIVisited(cantAddUri));
>      }
>    }
>  });

Is there a newline at EOF here per coding style? mozreview is not able to tell me that and I don't recall if we have eslint for that.

::: toolkit/components/places/tests/unit/test_markpageas.js
(Diff revision 1)
>  
>    await completionPromise;
>  
>    PlacesUtils.history.removeObserver(observer);
>  });
> -

ditto for the closing newline

::: toolkit/components/places/tests/unit/test_nsINavHistoryViewer.js:192
(Diff revision 1)
> -    PlacesUtils.bookmarks.insertBookmark(PlacesUtils.bookmarks.bookmarksMenuFolder,
> -                                         uri("http://google.com"),
> -                                         PlacesUtils.bookmarks.DEFAULT_INDEX,
> -                                         "foo");
> -  PlacesUtils.bookmarks.moveItem(testBookmark2, PlacesUtils.bookmarks.bookmarksMenuFolder, 0);
> -  do_check_eq(resultObserver.movedNode.itemId, testBookmark2);
> +    parentGuid: PlacesUtils.bookmarks.menuGuid,
> +    url: "http://google.com",
> +    title: "foo"
> +  });
> +
> +  let bm2Id = await PlacesUtils.promiseItemId(testBookmark2.guid);

can you rather use movedNode.bookmarkGuid to compare?

::: toolkit/components/places/tests/unit/test_nsINavHistoryViewer.js:203
(Diff revision 1)
> +  });
> +  do_check_eq(resultObserver.movedNode.itemId, bm2Id);
>  
>    // nsINavHistoryResultObserver.nodeRemoved
> -  PlacesUtils.bookmarks.removeItem(testBookmark2);
> -  do_check_eq(testBookmark2, resultObserver.removedNode.itemId);
> +  await PlacesUtils.bookmarks.remove(testBookmark2.guid);
> +  do_check_eq(bm2Id, resultObserver.removedNode.itemId);

ditto

::: toolkit/components/places/tests/unit/test_onItemChanged_tags.js:18
(Diff revision 1)
> -    PlacesUtils.bookmarks.DEFAULT_INDEX, "Bookmark 1"
> -  );
> +    url: uri,
> +    title: "Bookmark 1"
> +  });
>    PlacesUtils.tagging.tagURI(uri, tags);
>  
> +  let id = await PlacesUtils.promiseItemId(bookmark.guid);

we should add the missing arguments to the observer methods and compare guids instead.

::: toolkit/components/places/tests/unit/test_onItemChanged_tags.js:20
(Diff revision 1)
> +  });
>    PlacesUtils.tagging.tagURI(uri, tags);
>  
> +  let id = await PlacesUtils.promiseItemId(bookmark.guid);
> +
> +  let promise = Promise.defer();

PromiseUtils.defer() or new Promise()

::: toolkit/components/places/tests/unit/test_pageGuid_bookmarkGuid.js:35
(Diff revision 1)
> +    }]
> +  });
>  
> -  let root = PlacesUtils.getFolderContents(folder).root;
> +  let folderId = await PlacesUtils.promiseItemId(bookmarks[0].guid);
> +
> +  let root = PlacesUtils.getFolderContents(folderId).root;

We should probably file a bug to make getFolderContents accept a guid and do the promiseItemId internally (for now, then when we will fix bug 824502 the problem will go away).

Comment 4

[Marco Bonardo [:mak]]

Comment on attachment 8881271 [details]
Bug 1376234 - Convert more xpcshell-tests in toolkit/components/places/tests/unit to async Bookmarks.jsm API.

https://reviewboard.mozilla.org/r/152480/#review157786

::: toolkit/components/places/tests/unit/test_utils_getURLsForContainerNode.js:17
(Diff revision 1)
> -var bs = PU.bookmarks;
>  
> -var tests = [
>  
> -function() {
> +add_task(async function test_getURLsForContainerNode_folder() {
>    dump("\n\n*** TEST: folder\n");

please replace all the dump( with do_print( in this test

::: toolkit/components/places/tests/unit/test_utils_getURLsForContainerNode.js:78
(Diff revision 1)
> +      }]
> +    }],
> +  });
>  
>    var query = hs.getNewQuery();
> -  query.setFolders([bs.toolbarFolder], 1);
> +  query.setFolders([PlacesUtils.bookmarks.toolbarFolder], 1);

PlacesUtils.toolbarFolderId (the difference is mostly that this is a lazy getter that needs to cross XPCOM just once).

::: toolkit/components/places/tests/unit/test_utils_getURLsForContainerNode.js:121
(Diff revision 1)
> +      }]
> +    }],
> +  });
>  
>    var query = hs.getNewQuery();
> -  query.setFolders([bs.toolbarFolder], 1);
> +  query.setFolders([PlacesUtils.bookmarks.toolbarFolder], 1);

ditto

::: toolkit/components/places/tests/unit/test_utils_getURLsForContainerNode.js:163
(Diff revision 1)
> +      }]
> +    }],
> +  });
>  
>    var query = hs.getNewQuery();
> -  query.setFolders([bs.toolbarFolder], 1);
> +  query.setFolders([PlacesUtils.bookmarks.toolbarFolder], 1);

ditto

::: toolkit/components/places/tests/unit/test_utils_getURLsForContainerNode.js:206
(Diff revision 1)
> +      }]
> +    }],
> +  });
>  
>    var query = hs.getNewQuery();
> -  query.setFolders([bs.toolbarFolder], 1);
> +  query.setFolders([PlacesUtils.bookmarks.toolbarFolder], 1);

ditto

Comment 5

[Mark Banner (:standard8)]

Comment on attachment 8881187 [details]
Bug 1376234 - Convert some xpcshell-tests in toolkit/components/places/tests/unit to async Bookmarks.jsm API.

https://reviewboard.mozilla.org/r/152460/#review157766

> Is there a newline at EOF here per coding style? mozreview is not able to tell me that and I don't recall if we have eslint for that.

There is a newline, my editor was removing double-newlines. We do have an eslint rule to check there's one.

Comment 6

[Mark Banner (:standard8)]

Comment on attachment 8881187 [details]
Bug 1376234 - Convert some xpcshell-tests in toolkit/components/places/tests/unit to async Bookmarks.jsm API.

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

Comment 7

[Mark Banner (:standard8)]

Comment on attachment 8881271 [details]
Bug 1376234 - Convert more xpcshell-tests in toolkit/components/places/tests/unit to async Bookmarks.jsm API.

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

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4a24d60bca74
Convert some xpcshell-tests in toolkit/components/places/tests/unit to async Bookmarks.jsm API. r=mak
https://hg.mozilla.org/integration/autoland/rev/d1382e1bc996
Convert more xpcshell-tests in toolkit/components/places/tests/unit to async Bookmarks.jsm API. r=mak

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4a24d60bca74
https://hg.mozilla.org/mozilla-central/rev/d1382e1bc996