1271201 - Convert xpcshell-tests in toolkit/components/places/tests/bookmarks/test_384228.js to Bookmarks.jsm API

Status: RESOLVED FIXED

(Toolkit :: Places, defect)

Description

[[e-mail]]

Convert test cases to use new bookmark API based on bug 1094903 comment 8

Comment 1

[[e-mail]]

change to new API test by test

Comment 2

[[e-mail]]

Attachment: MozReview Request: Bug 1271201 - Convert xpcshell-tests in toolkit/..bookmarks/test_384228.js to Bookmarks.jsm API; r?mak

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

Comment 3

[[e-mail]]

Comment on attachment 8750238 [details]
MozReview Request: Bug 1271201 - Convert xpcshell-tests in toolkit/..bookmarks/test_384228.js to Bookmarks.jsm API; r?mak

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

Comment 4

[[e-mail]]

The patch did

1. convert nsIBookmark & nsIHistory to PlaceUtils.Bookmarks and PlaceUtils.history api
2. convert old Bookmarks API to PlaceUtils.Bookmarks.insert
3. use title of bug 384228, rename test_384228.js to test_search_bookmark_in_folder.js

Comment 5

[[e-mail]]

https://reviewboard.mozilla.org/r/51327/#review48023

::: toolkit/components/places/tests/bookmarks/test_search_bookmark_in_folder.js:8
(Diff revision 2)
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +/**
> + * test querying for bookmarks in multiple folders.

The original test file is at https://dxr.mozilla.org/mozilla-central/source/toolkit/components/places/tests/bookmarks/test_384228.js

Comment 6

[Marco Bonardo [:mak]]

Comment on attachment 8750238 [details]
MozReview Request: Bug 1271201 - Convert xpcshell-tests in toolkit/..bookmarks/test_384228.js to Bookmarks.jsm API; r?mak

https://reviewboard.mozilla.org/r/51327/#review48355

it mostly looks good, but when you rename a file, you should use "hg mv" and not do the rename manually through the filesystem. mv will preserve blame, as far as possible

::: toolkit/components/places/tests/bookmarks/test_search_bookmark_in_folder.js:13
(Diff revision 2)
> + * test querying for bookmarks in multiple folders.
> + */
> +add_task(function* search_bookmark_in_folder() {
> +  var testFolder1 = yield PlacesUtils.bookmarks.insert({
> +    parentGuid: PlacesUtils.bookmarks.unfiledGuid,
> +    index: PlacesUtils.bookmarks.DEFAULT_INDEX,

IIRC you can avoid passing index, when appending

::: toolkit/components/places/tests/bookmarks/test_search_bookmark_in_folder.js:90
(Diff revision 2)
> +  // query folder 1, folder 2 and get 4 bookmarks
> +  let folderIds = [];
> +  folderIds.push(yield PlacesUtils.promiseItemId(testFolder1.guid));
> +  folderIds.push(yield PlacesUtils.promiseItemId(testFolder2.guid));
> +
> +  var hs = PlacesUtils.history;

unless needed, please do not mixup let and var. using let everywhere but on the global scope is fine.

::: toolkit/components/places/tests/bookmarks/test_search_bookmark_in_folder.js:103
(Diff revision 2)
> +
> +  // should not match item from folder 3
> +  Assert.equal(rootNode.childCount, 4);
> +
> +  Assert.equal(rootNode.getChild(0).itemId,
> +               yield PlacesUtils.promiseItemId(b1.guid));

I think each node has also a .bookmarkGuid property that may be nice to test.

Comment 7

[[e-mail]]

https://reviewboard.mozilla.org/r/51327/#review48355

I use git cinnabar to do the mozreview push, will try if `git mv` works as well, or I'll keep the origin file name instead to ease the review.

> I think each node has also a .bookmarkGuid property that may be nice to test.

I dump the bookmark1 object, which does not contain `.bookmarkGuid` property

{"parentGuid":"Q3zmoezTCAzz","url":{},"title":"title b1 (folder 1)","type":1,"index":0,"dateAdded":"2016-05-11T02:54:47.602Z","lastModified":"2016-05-11T02:54:47.602Z","guid":"AhmTJ2V7Qd-n"}"

This property seems only contained in PlacesUIUtils created object?

Comment 8

[[e-mail]]

Comment on attachment 8750238 [details]
MozReview Request: Bug 1271201 - Convert xpcshell-tests in toolkit/..bookmarks/test_384228.js to Bookmarks.jsm API; r?mak

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

Comment 9

[Marco Bonardo [:mak]]

https://reviewboard.mozilla.org/r/51327/#review48355

> I dump the bookmark1 object, which does not contain `.bookmarkGuid` property
> 
> {"parentGuid":"Q3zmoezTCAzz","url":{},"title":"title b1 (folder 1)","type":1,"index":0,"dateAdded":"2016-05-11T02:54:47.602Z","lastModified":"2016-05-11T02:54:47.602Z","guid":"AhmTJ2V7Qd-n"}"
> 
> This property seems only contained in PlacesUIUtils created object?

sorry, I meant that rootNode.getChild(0) should have a bookmarkGuid you can compare with b1.guid

Comment 10

[[e-mail]]

Comment on attachment 8750238 [details]
MozReview Request: Bug 1271201 - Convert xpcshell-tests in toolkit/..bookmarks/test_384228.js to Bookmarks.jsm API; r?mak

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

Comment 11

[[e-mail]]

Ooh I got it, patch updated and test passed. Please take a look again. Thanks!

Comment 12

[Marco Bonardo [:mak]]

Comment on attachment 8750238 [details]
MozReview Request: Bug 1271201 - Convert xpcshell-tests in toolkit/..bookmarks/test_384228.js to Bookmarks.jsm API; r?mak

https://reviewboard.mozilla.org/r/51327/#review49101

aswesome, looks good!

::: toolkit/components/places/tests/bookmarks/test_384228.js:34
(Diff revision 4)
> -  do_check_eq(bmsvc.getItemIndex(testFolder1), 0);
> -  var testFolder2 = bmsvc.createFolder(root, "bug 384228 test folder 2",
> -                                       bmsvc.DEFAULT_INDEX);
> -  do_check_eq(bmsvc.getItemIndex(testFolder2), 1);
> -  var testFolder3 = bmsvc.createFolder(root, "bug 384228 test folder 3",
> -                                       bmsvc.DEFAULT_INDEX);
> +  });
> +  Assert.equal(testFolder3.index, 2);
> +
> +  let b1 = yield PlacesUtils.bookmarks.insert({
> +    parentGuid: testFolder1.guid,
> +    url: uri("http://foo.tld/"),

the new API also accepts strings, so you can avoid uri()
also in the code below

Comment 13

[[e-mail]]

Comment on attachment 8750238 [details]
MozReview Request: Bug 1271201 - Convert xpcshell-tests in toolkit/..bookmarks/test_384228.js to Bookmarks.jsm API; r?mak

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51327/diff/4-5/

Comment 14

[[e-mail]]

Thanks!

I'd plan to fix another one. If it works as well, I'll create good-first bugs file by file and maybe help to mentor those bugs.

Comment 15

[Marco Bonardo [:mak]]

that sounds great!

Comment 16

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/558349f82dd9

Comment 17

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/558349f82dd9