1403486 - Convert some xpcshell-tests in toolkit/components/places/tests/bookmarks to Bookmarks.jsm API

Status: RESOLVED FIXED

(Toolkit :: Places, enhancement, P1)

Description

[Mark Banner (:standard8)]

As part of the ongoing conversion to the newer async APIs, I would like to land some of the changes required for bug 1094910. There's still more to do in the bookmarks/ sub-directory, but they need separate bugs / waiting on other things.

Comment 1

[Mark Banner (:standard8)]

Attachment: Bug 1403486 - Convert some xpcshell-tests in toolkit/components/places/tests/bookmarks to Bookmarks.jsm API.

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

Comment 2

[Marco Bonardo [:mak]]

Comment on attachment 8913063 [details]
Bug 1403486 - Convert some xpcshell-tests in toolkit/components/places/tests/bookmarks to Bookmarks.jsm API.

https://reviewboard.mozilla.org/r/184470/#review189718

r=me with the test_bookmarks.js changes reverted per the below comment.

::: toolkit/components/places/tests/bookmarks/test_bmindex.js:19
(Diff revision 1)
> -function check_contiguous_indexes(aBookmarks) {
>    var indexes = [];
> -  aBookmarks.forEach(function(aBookmarkId) {
> -    let bmIndex = bs.getItemIndex(aBookmarkId);
> -    dump("Index: " + bmIndex + "\n");
> +  for (let bm of bookmarks) {
> +    let bmIndex = (await PlacesUtils.bookmarks.fetch(bm.guid)).index;
> +    dump(`Index: ${bmIndex}\n`);
>      dump("Checking duplicates\n");

in general in xpcshell-tests we use do_print instead of plain dumps

::: toolkit/components/places/tests/bookmarks/test_bookmarks.js:109
(Diff revision 1)
>    do_check_eq(bs.getFolderIdForItem(bs.toolbarFolder), bs.placesRoot);
>    do_check_eq(bs.getFolderIdForItem(bs.unfiledBookmarksFolder), bs.placesRoot);
>  
>    // create a folder to hold all the tests
>    // this makes the tests more tolerant of changes to default_places.html
> -  let testRoot = bs.createFolder(root, "places bookmarks xpcshell tests",
> +  let testRoot = await bs.insert({

I'm not sure I understand the scope of this change, looks like we decided to consider this a legacy API test (and it should likely be moved to the legacy/ folder) and indeed you are not converting most of the calls here... but this one.

Though, the tests checking history result views are  something we miss for the new API... so probably this test should be split into 2, one part just checking the API should move to legacy/, the other part involving queries should move to queries/ and be converted to the new API.

For now you could even just move this to legacy unchanged, and file a separate bug for splitting out the queries checks.

Comment 3

[Mark Banner (:standard8)]

Comment on attachment 8913063 [details]
Bug 1403486 - Convert some xpcshell-tests in toolkit/components/places/tests/bookmarks to Bookmarks.jsm API.

https://reviewboard.mozilla.org/r/184470/#review189718

> I'm not sure I understand the scope of this change, looks like we decided to consider this a legacy API test (and it should likely be moved to the legacy/ folder) and indeed you are not converting most of the calls here... but this one.
> 
> Though, the tests checking history result views are  something we miss for the new API... so probably this test should be split into 2, one part just checking the API should move to legacy/, the other part involving queries should move to queries/ and be converted to the new API.
> 
> For now you could even just move this to legacy unchanged, and file a separate bug for splitting out the queries checks.

Ah yes, I forgot to revert these. I've already got bug 1403487 filed, so I think I'll just revert and do the work there.

Comment 4

[Mark Banner (:standard8)]

Comment on attachment 8913063 [details]
Bug 1403486 - Convert some xpcshell-tests in toolkit/components/places/tests/bookmarks to Bookmarks.jsm API.

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

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/90960f0aa023
Convert some xpcshell-tests in toolkit/components/places/tests/bookmarks to Bookmarks.jsm API. r=mak

Comment 6

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

https://hg.mozilla.org/mozilla-central/rev/90960f0aa023