Closed Bug 1271201 Opened 4 years ago Closed 4 years ago

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

Categories

(Toolkit :: Places, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
firefox49 --- fixed

People

(Reporter: gasolin, Assigned: gasolin)

References

Details

Attachments

(1 file)

Convert test cases to use new bookmark API based on bug 1094903 comment 8
change to new API test by test
Assignee: nobody → gasolin
Blocks: 1094910
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/
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
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 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.
Attachment #8750238 - Flags: review?(mak77)
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 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/
Attachment #8750238 - Flags: review?(mak77)
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 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/
Ooh I got it, patch updated and test passed. Please take a look again. Thanks!
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
Attachment #8750238 - Flags: review?(mak77) → review+
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/
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.
Keywords: checkin-needed
that sounds great!
https://hg.mozilla.org/mozilla-central/rev/558349f82dd9
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
See Also: → 1275145
You need to log in before you can comment on or make changes to this bug.