Closed Bug 1273023 Opened 3 years ago Closed 3 years ago

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

Categories

(Toolkit :: Places, defect)

defect
Not set

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.

The required API change could reference to bug 1271201
Assignee: nobody → gasolin
Blocks: 1094910
Summary: Bug 1271201 - Convert xpcshell-tests in toolkit/components/places/tests/bookmarks/test_385829.js to Bookmarks.jsm API → Convert xpcshell-tests in toolkit/components/places/tests/bookmarks/test_385829.js to Bookmarks.jsm API
Comment on attachment 8752653 [details]
MozReview Request: Bug 1273023 - Convert xpcshell-tests in toolkit/components/places/tests/bookmarks/test_385829.js to Bookmarks.jsm API; r?mak

https://reviewboard.mozilla.org/r/52688/#review50900

::: toolkit/components/places/tests/bookmarks/test_385829.js:18
(Diff revision 1)
> -  var b4 = bmsvc.insertBookmark(folder, uri("http://a4.com/"),
> -                                bmsvc.DEFAULT_INDEX, "4 title");
>  
>    // ensure some unique values for date added and last modified
>    // for date added:    b1 < b2 < b3 < b4
>    // for last modified: b1 > b2 > b3 > b4

You missed setting the dateAdded and lastModified, while those may look nonsense, they are important to guarantee the test won't randomly fail, since timers resolution is low on some systems (Windows and virtual machines) and our timing methods are not monotone, 2 bookmarks may end up with the same value, and then sometimes ordering could fail.

::: toolkit/components/places/tests/bookmarks/test_385829.js:46
(Diff revision 1)
> +  Assert.equal(b2.title, "2 title");
> +  Assert.equal(b3.title, "3 title");
> +
> +  yield PlacesUtils.bookmarks.update({
> +    guid: b3.guid,
> +    title: "title 3"

why are you doing this? the original test doesn't seem to...
Attachment #8752653 - Flags: review?(mak77)
https://reviewboard.mozilla.org/r/52688/#review50900

> why are you doing this? the original test doesn't seem to...

I haven't find a way to update dateAdded/lastModified directly after create bookmark with bm.insert, 
therefore I use bm.update to update modifyDate for last modified: b1 > b2 > b3 > b4 order.
Comment on attachment 8752653 [details]
MozReview Request: Bug 1273023 - Convert xpcshell-tests in toolkit/components/places/tests/bookmarks/test_385829.js to Bookmarks.jsm API; r?mak

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52688/diff/1-2/
Attachment #8752653 - Flags: review?(mak77)
Thanks for review. After more investigation, I found insert/update API validate dateAdded/modifiedDate time format and  modifiedDate should always larger then dateAdded time. 

Change dateAdded/modifiedDate with valid time format in insert/update method can do the update/modify as previous test does \o/
Attachment #8752653 - Flags: review?(mak77) → review+
Comment on attachment 8752653 [details]
MozReview Request: Bug 1273023 - Convert xpcshell-tests in toolkit/components/places/tests/bookmarks/test_385829.js to Bookmarks.jsm API; r?mak

https://reviewboard.mozilla.org/r/52688/#review51198

LGTM! thank you!
thanks!
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/c470151f0f84
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
You need to log in before you can comment on or make changes to this bug.