1094903 - Convert xpcshell-tests in toolkit/components/places/tests/unit to Bookmarks.jsm API

Status: RESOLVED FIXED

(Toolkit :: Places, defect, P2)

Description

[Marco Bonardo [:mak]]

This is A LOT of tests. We should also avoding to convert tests that are duped by the new API tests.

Comment 1

[[e-mail]]

search dxr with `nsINavBookmarksService`, the effected files are

toolkit/components/places/tests/bookmarks/test_384228.js
toolkit/components/places/tests/bookmarks/test_385829.js
toolkit/components/places/tests/bookmarks/test_388695.js 
toolkit/components/places/tests/bookmarks/test_395101.js
toolkit/components/places/tests/bookmarks/test_395593.js
toolkit/components/places/tests/bookmarks/test_bmindex.js
toolkit/components/places/tests/bookmarks/test_changeBookmarkURI.js

Comment 2

[[e-mail]]

Attachment: MozReview Request: Bug 1094903 - Convert xpcshell-tests in toolkit/components/places/tests/unit to Bookmarks.jsm API;

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

Comment 3

[[e-mail]]

Do the WIP of quick bookmarks and history api replacement, will check if there's a dup by the new API tests or new syntax.

Comment 4

[[e-mail]]

Comment on attachment 8748473 [details]
MozReview Request: Bug 1094903 - Convert xpcshell-tests in toolkit/components/places/tests/unit to Bookmarks.jsm API;

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

Comment 5

[[e-mail]]

Comment on attachment 8748473 [details]
MozReview Request: Bug 1094903 - Convert xpcshell-tests in toolkit/components/places/tests/unit to Bookmarks.jsm API;

The patch did:

1. convert nsiBookmark & nsiHistory to PlaceUtils

2. wrap tests in add_task

3. replace do_assert_eq with Assert.equal and similar syntax


Would like to do:

1. rename test_xxxxx to more meaningful name (ex test_384228.js to test_search_bookmark_in_folder.js, which is used in new add_task function name)


Can do in separate patch:

2. convert `bm.insertBookmark` to `yield bm.insert` syntax

The problem I found is I can't get guid from bookmarks.createFolder. Should I change all createFolder to use `yield bm.insert` syntax as well?

https://dxr.mozilla.org/mozilla-central/source/browser/components/places/tests/browser/browser_0_library_left_pane_migration.js#40

Comment 6

[Marco Bonardo [:mak]]

(In reply to Fred Lin [:gasolin] from comment #5)
> The problem I found is I can't get guid from bookmarks.createFolder. Should
> I change all createFolder to use `yield bm.insert` syntax as well?

absolutely, createFolder is part of the old API. Anything in nsINavBookmarksService should be converted.

Comment 7

[Marco Bonardo [:mak]]

Note that I think you are making confusion between this bug and bug 1094910.
The patch you posted here looks for bug 1094910, not this one.

Comment 8

[Marco Bonardo [:mak]]

https://reviewboard.mozilla.org/r/50311/#review47725

::: toolkit/components/places/tests/bookmarks/test_384228.js:36
(Diff revision 2)
> -  var b3 = bmsvc.insertBookmark(testFolder2, uri("http://foo.tld/"),
> -                                bmsvc.DEFAULT_INDEX, "title b3 (folder 2)");
> -  do_check_eq(bmsvc.getItemIndex(b3), 0);
> -  var b4 = bmsvc.insertBookmark(testFolder3, uri("http://foo.tld/"),
> -                                bmsvc.DEFAULT_INDEX, "title b4 (folder 3)");
> -  do_check_eq(bmsvc.getItemIndex(b4), 0);
> +  var b3 = bm.insertBookmark(testFolder2, uri("http://foo.tld/"),
> +                                bm.DEFAULT_INDEX, "title b3 (folder 2)");
> +  Assert.equal(bm.getItemIndex(b3), 0);
> +  var b4 = bm.insertBookmark(testFolder3, uri("http://foo.tld/"),
> +                                bm.DEFAULT_INDEX, "title b4 (folder 3)");
> +  Assert.equal(bm.getItemIndex(b4), 0);

OK I think there's a bit of confusion of what should be done in these bugs.

It is not enough to move from nsINavBookmarksService to PlacesUtils.bookmarks, we are completely changing the bookmarking API.

The following methods/properties must be replaced with their async counter-part (that is in Bookmarks.jsm and accessibile from PlacesUtils.bookmarks). Everywhere one of these appers, the test should be changed to instead use the Bookmarks.jsm async API.

placesRoot
bookmarksMenuFolder;
tagsFolder
unfiledBookmarksFolder;
toolbarFolder;
insertBookmark
removeItem
createFolder
removeFolderChildren
moveItem
insertSeparator
getIdForItemAt
setItemTitle
getItemTitle
setItemDateAdded
getItemDateAdded
setItemLastModified
getItemLastModified
getBookmarkURI
getItemIndex
setItemIndex
getItemType
isBookmarked
getBookmarkedURIFor
changeBookmarkURI
getBookmarkIdsForURI
runInBatchMode;

Comment 9

[[e-mail]]

thanks for feedback, will try bug 1094910 first

Comment 10

[Mark Banner (:standard8)]

I'm going to take a stab at this over the weekend.

Comment 11

[Mark Banner (:standard8)]

Attachment: Bug 1094903 - Convert xpcshell-tests in toolkit/components/places/tests/unit to Bookmarks.jsm (async) API.

This converts the majority of tests to the newer Async APIs. Some tests that are designed to only test the old APIs are moved to /legacy/,
these either have duplicate tests already existing, or no comparable test required.

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

Comment 12

[Marco Bonardo [:mak]]

Comment on attachment 8912388 [details]
Bug 1094903 - Convert xpcshell-tests in toolkit/components/places/tests/unit to Bookmarks.jsm (async) API.

https://reviewboard.mozilla.org/r/183712/#review189104

::: toolkit/components/places/tests/legacy/xpcshell.ini:7
(Diff revision 1)
> +# until we remove the APIs themselves.
> +
> +[DEFAULT]
> +head = head_legacy.js
> +firefox-appdir = browser
> +skip-if = toolkit == 'android'

we should not really need this, since android should not define MOZ_PLACES and thus it won't even build Places.

::: toolkit/components/places/tests/unit/test_autocomplete_stopSearch_no_throw.js:21
(Diff revision 1)
>    try {
>      ac.stopSearch();
>    } catch (e) {
>      do_throw("we should not have caught anything!");
>    }
> -}
> +});

Looks like this test should be moved to the unifiedcomplete/ folder

Comment 13

[Mark Banner (:standard8)]

Comment on attachment 8912388 [details]
Bug 1094903 - Convert xpcshell-tests in toolkit/components/places/tests/unit to Bookmarks.jsm (async) API.

https://reviewboard.mozilla.org/r/183712/#review189104

> Looks like this test should be moved to the unifiedcomplete/ folder

I split this out to bug 1403502.

Comment 14

[Mark Banner (:standard8)]

Comment on attachment 8912388 [details]
Bug 1094903 - Convert xpcshell-tests in toolkit/components/places/tests/unit to Bookmarks.jsm (async) API.

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

Comment 15

[Pulsebot]

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

Comment 16

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/e81bc5a17a4f