Closed Bug 1276128 Opened 8 years ago Closed 7 years ago

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

Categories

(Toolkit :: Places, defect, P3)

Product:
Toolkit
Component:
Places
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla56
Tracking Flags:
Tracking Status
firefox56 --- fixed

People

(Reporter: gasolin, Assigned: hemantsingh1612, Mentored)

References

Details

(Whiteboard: [good first bug][mentor-lang=zh][lang=js])

Attachments

(1 file, 1 obsolete file)

Bug 1276128 - Convert xpcshell-tests in toolkit/components/places/tests/bookmarks/test_393498.js to Bookmarks.jsm API.
59 bytes, text/x-review-board-request
standard8
: review+
Details 
Convert test cases to use new bookmark API based on bug 1094903 comment 8.

There's something reusable from Bug 1094903. The draft patch there provide the basic skeleton. You can copy that and do 
`./mach test /path/to/test_393498.js` to make sure existing test works fine.

Then we need to update bookmark API to async one. It means replacing bm.createFolder, bm.insertBookmark ... to `PlacesUtils.bookmarks.insert`, and update related assertion accordingly. You can take the updated tests such as test_384228.js or test_385829.js for reference.

The required API change could reference to bug 1271201 & bug 1273023
Hi,
I'm kind of new to the open source projects and I like to contribute here.I'm planning to work on this bug.Could someone help me to start things? where should I start from the code ? What kind of things I should know ?I'm very glad if someone can help me to sort out things.I have already built Firefox source code in windows.
Flags: needinfo?(gasolin)
Is anyone working already on this bug? i am asking because i want to do this. i have read the comments and have a little idea of what to do, should i go ahead? 
P.S. : I am new to open source please guide me.
Hey Fred [:gasolin],

I think i have solved the bug,
I have attached the `hg diff` file along,
if you find anything else to be done, please tell me.

File : http://pastebin.com/yHFNS384

Thank You,
Noveen Sachdeva.
Hi, there are 2 main paths for you to submit a patch for review:
1. The new path is to go through mozreview, you can read more about it here: http://mozilla-version-control-tools.readthedocs.io/en/latest/mozreview-user.html . Thie is the newest and preferred way, the initial difficulty is a bit higher but once setup it becomes easier.
2. The old path is to generate a diff after having setup mercurial appropriately:https://developer.mozilla.org/en-US/docs/Mercurial/Using_Mercurial#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F . Then you just attach your diff file to this bug report, and set the review? flag to an appropriate reviewer (usually the bug mentor)

This is a good introduction guide too:
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Introduction
Attached patch patch_1276128_v1.diff (obsolete) — — Splinter Review 
I have updated the APIs to the latest async Bookmarks.jsm APIs.

Every deprecated API listed in https://bugzilla.mozilla.org/show_bug.cgi?id=1094903#c8 has been updated to the newer Bookmarks.jsm APIs.
Attachment #8826421 - Flags: review?(mak77)
Comment on attachment 8826421 [details] [diff] [review]
patch_1276128_v1.diff

Review of attachment 8826421 [details] [diff] [review]:
-----------------------------------------------------------------

There are various things to fixup, but you're on the right path. Please try to recompile and execute the test locally, that should help.

::: toolkit/components/places/tests/bookmarks/test_393498.js
@@ +32,1 @@
>    const PAST_PRTIME = (Date.now() - 86400000) * 1000;

You should change run_test to
add_task(function* () {

Because run_test is a synchronous function, with the new API instead we yield, that means we wait for async promises. So we need a generator function* and in our test harness we usually use add_task to wrap the function in a task. the task allows to keep a synchronous flow and use yield, similarly to how async functions and await work in ecmascript 6.

@@ +32,5 @@
>    const PAST_PRTIME = (Date.now() - 86400000) * 1000;
>  
>    // Insert a new bookmark.
> +  let testFolder = yield PlacesUtils.bookmarks.insert({ type: PlacesUtils.bookmarks.TYPE_FOLDER,
> +                                                 title: "test folder" });

please where you go over 80 chars, try to reindent and align arguments. something like:
let testFolder = yield PlacesUtils.bookmarks.insert({
  type: PlacesUtils.bookmarks.TYPE_FOLDER,
  title: "test folder"
});

Here you must also specify a parentGuid.
I think you didn't try to run the test, or this should have failed?
You can recompile the test using
./mach build faster
and run it using 
./mach test toolkit/components/places/tests/bookmarks/test_393498.js

@@ +34,5 @@
>    // Insert a new bookmark.
> +  let testFolder = yield PlacesUtils.bookmarks.insert({ type: PlacesUtils.bookmarks.TYPE_FOLDER,
> +                                                 title: "test folder" });
> +
> +  let bookmarkId = yield PlacesUtils.bookmarks.insert({ parentGuid: testFolder,

shouldn't it be testFolder.parentGuid?
Also the return value is not a bookmarkId, it's a bookmark object

@@ +35,5 @@
> +  let testFolder = yield PlacesUtils.bookmarks.insert({ type: PlacesUtils.bookmarks.TYPE_FOLDER,
> +                                                 title: "test folder" });
> +
> +  let bookmarkId = yield PlacesUtils.bookmarks.insert({ parentGuid: testFolder,
> +                                                 type: PlacesUtils.bookmarks.TYPE_BOOKMARK,

This can be omitted (since you specify a url, it's for sure a type_bookmark)

@@ +43,5 @@
>    // Sanity check.
>    do_check_true(observer.itemChangedProperty === undefined);
>  
>    // Set dateAdded in the past and verify the bookmarks cache.
> +  PlacesUtils.bookmarks.update({guid: bookmarkId.guid, dateAdded: PAST_PRTIME});

should yield on this, and I think you could specify a dateAdded directly on insert?

@@ +48,3 @@
>    do_check_eq(observer._itemChangedProperty, "dateAdded");
>    do_check_eq(observer._itemChangedValue, PAST_PRTIME);
> +  let dateAdded = yield PlacesUtils.bookmarks.fetch({guid: bookmarkId.guid}).dateAdded;

may want a parenthesis to wait for the yield before extracting .dateAdded

@@ +51,4 @@
>    do_check_eq(dateAdded, PAST_PRTIME);
>  
>    // After just inserting, modified should be the same as dateAdded.
> +  do_check_eq(PlacesUtils.bookmarks.fetch({guid: bookmarkId.guid}).lastModified, dateAdded);

this cannot work without a yield

@@ +55,3 @@
>  
>    // Set lastModified in the past and verify the bookmarks cache.
> +  PlacesUtils.bookmarks.update({guid: bookmarkId.guid, lastModified: PAST_PRTIME});

needs a yield

also, the new bookmarks API uses Date objects, not prtime. You can maybe remove the usage of PAST_PRTIME and rather make it a date, or you can use PlacesUtils.toDate or PlacesUtils.toPRTime to convert.

@@ +58,3 @@
>    do_check_eq(observer._itemChangedProperty, "lastModified");
>    do_check_eq(observer._itemChangedValue, PAST_PRTIME);
> +  do_check_eq(PlacesUtils.bookmarks.fetch({guid: bookmarkId.guid}).lastModified, PAST_PRTIME);

same here and below. I'm not repating further, there are other calls to fix down here.
Everywhere you have an API returning a promise, you must wait for it to resolve, either through yield or callbacks.

@@ +77,4 @@
>    let childNode = root.getChild(0);
>  
>    // confirm current dates match node properties
> +  do_check_eq(PlacesUtils.bookmarks.fetch({guid: bookmarkId.guid}).itemDateAdded,

I don't know what .itemDateAdded is, it doesn't exist.

@@ +81,3 @@
>                childNode.dateAdded);
> +  do_check_eq(PlacesUtils.bookmarks.fetch({guid: bookmarkId.guid}).lastModified,
> +              childNode.lastModified);              

you have some trailing spaces here and there, please remove them.
Attachment #8826421 - Flags: review?(mak77) → review-
Priority: -- → P3
Flags: needinfo?(gasolin)
A couple of notes for the latest code-base:

(In reply to Marco Bonardo [::mak] from comment #6)
> ::: toolkit/components/places/tests/bookmarks/test_393498.js
> @@ +32,1 @@
> >    const PAST_PRTIME = (Date.now() - 86400000) * 1000;
> 
> You should change run_test to
> add_task(function* () {

We should now use async/await, i.e.

add_task(async function() {

> @@ +34,5 @@
> >    // Insert a new bookmark.
> > +  let testFolder = yield PlacesUtils.bookmarks.insert({ type: PlacesUtils.bookmarks.TYPE_FOLDER,

`yield` should now be `await`.
Hi I want to  work on this.
After trying |./mach build faster| and then running test |./mach test toolkit/components/places/tests/bookmarks/test_393498.js|,
I am getting error, hopefully I will fix it.
Assignee: nobody → hemantsingh1612
Mentor: standard8
Comment on attachment 8876780 [details]
Bug 1276128 - Convert xpcshell-tests in toolkit/components/places/tests/bookmarks/test_393498.js to Bookmarks.jsm API.

https://reviewboard.mozilla.org/r/148102/#review152896

Thank you for taking this on. Unfortunately there's still quite a bit to clean up. As I said below, I'd highly recommend getting ESLint to pass on the file - if you're unsure of what the test failure output is telling you, sometimes the eslint description is clearer.

I think there's a reasonable amount of hints for you here, but feel free to ask for more. Either on irc or here. Hopefully once we get the basics sorted, then it'll be easier.

Also, if you're unsure of bits, remember to revisit what the original code was doing and check what has been changed - I find that helps a lot sometimes.

::: toolkit/components/places/tests/bookmarks/test_393498.js:28
(Diff revision 2)
>  
>  do_register_cleanup(function() {
>    PlacesUtils.bookmarks.removeObserver(observer);
>  });
>  
> -function run_test() {
> +function add_task(async function() {

Almost there but not quite, drop the first `function` on this line and it'll be happier.

Also, I'd suggest running ESLint against the file if you're struggling to work out why the test doesn't run. It will give you lots of pointers:

./mach eslint toolkit/components/places/tests/bookmarks/test_393498.js

(there's editor integrations available as well, depending on what you're using).

::: toolkit/components/places/tests/bookmarks/test_393498.js:32
(Diff revision 2)
>  
> -function run_test() {
> +function add_task(async function() {
>    // We set times in the past to workaround a timing bug due to virtual
>    // machines and the skew between PR_Now() and Date.now(), see bug 427142 and
>    // bug 858377 for details.
>    const PAST_PRTIME = (Date.now() - 86400000) * 1000;

Due to the new APIs, this will need to become something like:

const PAST_DATE = new Date(Date.now() - 86400000);

and change all the references of PAST_PRTIME to PAST_DATE.

Where dates are compared, you'll also need to change the do_check_eq to be something like:

do_check_eq(updatedBookmark.dateAdded.getTime(), PAST_DATE.getTime());

::: toolkit/components/places/tests/bookmarks/test_393498.js:38
(Diff revision 2)
>  
>    // Insert a new bookmark.
> -  let testFolder = PlacesUtils.bookmarks.createFolder(
> -    PlacesUtils.placesRootId, "test Folder",
> -    PlacesUtils.bookmarks.DEFAULT_INDEX);
> -  let bookmarkId = PlacesUtils.bookmarks.insertBookmark(
> +  let testFolder = await PlacesUtils.bookmarks.insert(
> +    type: PlacesUtils.bookmarks.TYPE_FOLDER,
> +    title: "test folder",
> +    parentGuid: testFolder.parentGuid,

This parentGuid should be `PlacesUtils.bookmarks.menuGuid` (or one of those), currently it is trying to reference itself.

::: toolkit/components/places/tests/bookmarks/test_393498.js:40
(Diff revision 2)
> -  let testFolder = PlacesUtils.bookmarks.createFolder(
> -    PlacesUtils.placesRootId, "test Folder",
> -    PlacesUtils.bookmarks.DEFAULT_INDEX);
> -  let bookmarkId = PlacesUtils.bookmarks.insertBookmark(
> -    testFolder, uri("http://google.com/"),
> -    PlacesUtils.bookmarks.DEFAULT_INDEX, "");
> +  let testFolder = await PlacesUtils.bookmarks.insert(
> +    type: PlacesUtils.bookmarks.TYPE_FOLDER,
> +    title: "test folder",
> +    parentGuid: testFolder.parentGuid,
> +  });
> +  let bookmarkId = await PlacesUtils.bookmarks.insert(

Lets change `bookmarkId` to be `bookmark`, that'll be clearer for reading through.

Also the insert lines both need a `{` on the end to pass ESLint & make javascript happy.

::: toolkit/components/places/tests/bookmarks/test_393498.js:51
(Diff revision 2)
> +  });
>    // Sanity check.
>    do_check_true(observer.itemChangedProperty === undefined);
>  
>    // Set dateAdded in the past and verify the bookmarks cache.
> -  PlacesUtils.bookmarks.setItemDateAdded(bookmarkId, PAST_PRTIME);
> +  await PlacesUtils.bookmarks.update({guid: bookmarkId.parentGuid, dateAdded: bookmarkId.dateAdded});

The guid here should be `bookmark.guid`, as we want to update the newly added bookmark, not its parent.

::: toolkit/components/places/tests/bookmarks/test_393498.js:54
(Diff revision 2)
>  
>    // Set dateAdded in the past and verify the bookmarks cache.
> -  PlacesUtils.bookmarks.setItemDateAdded(bookmarkId, PAST_PRTIME);
> +  await PlacesUtils.bookmarks.update({guid: bookmarkId.parentGuid, dateAdded: bookmarkId.dateAdded});
>    do_check_eq(observer._itemChangedProperty, "dateAdded");
>    do_check_eq(observer._itemChangedValue, PAST_PRTIME);
> -  let dateAdded = PlacesUtils.bookmarks.getItemDateAdded(bookmarkId);
> +  let dateAdded = await PlacesUtils.bookmarks.fetch({guid: bookmarkId.parentGuid});

It is probably better to change this to a `let updatedBookmark = await...` then change the check on the next time to use `updatedBookmark.dateAdded`

::: toolkit/components/places/tests/bookmarks/test_393498.js:61
(Diff revision 2)
>  
>    // After just inserting, modified should be the same as dateAdded.
> -  do_check_eq(PlacesUtils.bookmarks.getItemLastModified(bookmarkId), dateAdded);
> +   do_check_eq(PlacesUtils.bookmarks.fetch({guid: bookmarkId.parentGuid}).lastModified, dateAdded);
>  
>    // Set lastModified in the past and verify the bookmarks cache.
> -  PlacesUtils.bookmarks.setItemLastModified(bookmarkId, PAST_PRTIME);
> +  await PlacesUtils.bookmarks.update({guid: , lastModified: PAST_PRTIME});

You're missing specifying the guid value here.

::: toolkit/components/places/tests/bookmarks/test_393498.js:64
(Diff revision 2)
>  
>    // Set lastModified in the past and verify the bookmarks cache.
> -  PlacesUtils.bookmarks.setItemLastModified(bookmarkId, PAST_PRTIME);
> +  await PlacesUtils.bookmarks.update({guid: , lastModified: PAST_PRTIME});
>    do_check_eq(observer._itemChangedProperty, "lastModified");
>    do_check_eq(observer._itemChangedValue, PAST_PRTIME);
> -  do_check_eq(PlacesUtils.bookmarks.getItemLastModified(bookmarkId),
> +  do_check_eq(PlacesUtils.bookmarks.fetch({guid: bookmarkId.parentGuid}).lastModified,

The new APIs are async, so you'll need to await for them. Generally lines like this should be split into two, e.g.

updatedBookmark = PlacesUtils.bookmarks.fetch(...
do_check_eq(updatedBookmark.lastModifed.getTime(), ...

::: toolkit/components/places/tests/bookmarks/test_393498.js:107
(Diff revision 2)
>    do_check_eq(childNode.dateAdded, PAST_PRTIME);
> -  PlacesUtils.bookmarks.setItemLastModified(bookmarkId, PAST_PRTIME);
> +  await PlacesUtils.bookmarks.update({guid: bookmarkId.parentGuid, lastModified: PAST_PRTIME});
>    do_check_eq(childNode.lastModified, PAST_PRTIME);
>  
>    root.containerOpen = false;
>  }

As the original function is now wrapped, you need to change this last line from `}` to `});`
Attachment #8876780 - Flags: review?(standard8)
Comment on attachment 8876780 [details]
Bug 1276128 - Convert xpcshell-tests in toolkit/components/places/tests/bookmarks/test_393498.js to Bookmarks.jsm API.

https://reviewboard.mozilla.org/r/148102/#review154390

Great, thank you for the update. We're definitely making progress. I've added a bunch more comments that should help you to start getting most of the first part of the test running. See if you can get the rest of it going as well.

Hint: In some places you'll want to read through the test again, and check that you're using and updating the correct variables at the right times.

::: toolkit/components/places/tests/bookmarks/test_393498.js:46
(Diff revision 4)
> +  let bookmark = await PlacesUtils.bookmarks.insert({
> +    parentGuid: testFolder.parentGuid,
> +    type: PlacesUtils.bookmarks.TYPE_BOOKMARK,
> +    url: "http://google.com/",
> +    title: "a bookmark",
> +    dateAdded: PAST_DATE

We don't want to specify dateAdded here - we'll let it default (to now), and then the first update will set it to the past date.

::: toolkit/components/places/tests/bookmarks/test_393498.js:50
(Diff revision 4)
> +    title: "a bookmark",
> +    dateAdded: PAST_DATE
> +  });
>  
>    // Sanity check.
>    do_check_true(observer.itemChangedProperty === undefined);

nit: missing '_', should be `observer.itemChangedProperty`

::: toolkit/components/places/tests/bookmarks/test_393498.js:52
(Diff revision 4)
> +  });
>  
>    // Sanity check.
>    do_check_true(observer.itemChangedProperty === undefined);
>  
> -  // Set dateAdded in the past and verify the bookmarks cache.
> +  // Set d9ateAdded in the past and verify the bookmarks cache.

nit: it looks like you accidentally added a 9 in the dateAdded here.

::: toolkit/components/places/tests/bookmarks/test_393498.js:53
(Diff revision 4)
>  
>    // Sanity check.
>    do_check_true(observer.itemChangedProperty === undefined);
>  
> -  // Set dateAdded in the past and verify the bookmarks cache.
> -  PlacesUtils.bookmarks.setItemDateAdded(bookmarkId, PAST_PRTIME);
> +  // Set d9ateAdded in the past and verify the bookmarks cache.
> +  PlacesUtils.bookmarks.update({

This needs an await as well - update is an async api.

::: toolkit/components/places/tests/bookmarks/test_393498.js:55
(Diff revision 4)
>    do_check_true(observer.itemChangedProperty === undefined);
>  
> -  // Set dateAdded in the past and verify the bookmarks cache.
> -  PlacesUtils.bookmarks.setItemDateAdded(bookmarkId, PAST_PRTIME);
> +  // Set d9ateAdded in the past and verify the bookmarks cache.
> +  PlacesUtils.bookmarks.update({
> +    guid: bookmark.guid,
> +    dateAdded: bookmark.dateAdded

This should be `dateAdded: PAST_DATE` - otherwise you're trying to set the date to the same as what it is already (and hence we wouldn't get the item changed notification).

::: toolkit/components/places/tests/bookmarks/test_393498.js:58
(Diff revision 4)
> -  PlacesUtils.bookmarks.setItemDateAdded(bookmarkId, PAST_PRTIME);
> +  PlacesUtils.bookmarks.update({
> +    guid: bookmark.guid,
> +    dateAdded: bookmark.dateAdded
> +  });
> +
>    do_check_eq(observer._itemChangedProperty, "dateAdded");

Once the other items are fixed, the test currently fails here.

The reason is that the new update API isn't notifying about dateAdded being changed.

This should be quite easy to fix - if you look at the update() function in Bookmarks.jsm, near the end of it there's a lot of notifying onItemChanged listeners.

If you duplicate the lastModified if statement section of that (and change lastModified to dateAdded), then the test should start passing here.

::: toolkit/components/places/tests/bookmarks/test_393498.js:59
(Diff revision 4)
> +    guid: bookmark.guid,
> +    dateAdded: bookmark.dateAdded
> +  });
> +
>    do_check_eq(observer._itemChangedProperty, "dateAdded");
> -  do_check_eq(observer._itemChangedValue, PAST_PRTIME);
> +  do_check_eq(observer._itemChangedValue, PAST_DATE);

The observers are dealing with the slightly older PRTime, so you'll need to change the `PAST_DATE` parameter to `PlacesUtils.toPRTime(PAST_DATE)`.

Please do this for the other _itemChangedValue comparisons as well.

::: toolkit/components/places/tests/bookmarks/test_393498.js:62
(Diff revision 4)
> +
>    do_check_eq(observer._itemChangedProperty, "dateAdded");
> -  do_check_eq(observer._itemChangedValue, PAST_PRTIME);
> -  let dateAdded = PlacesUtils.bookmarks.getItemDateAdded(bookmarkId);
> -  do_check_eq(dateAdded, PAST_PRTIME);
> +  do_check_eq(observer._itemChangedValue, PAST_DATE);
> +
> +  let updatedBookmark = await PlacesUtils.bookmarks.fetch({
> +    guid: bookmark.parentGuid

I think all the references to `bookmark.parentGuid` below this line (and including it) should be changed to `bookmark.guid`, as that's the one we are dealing with.

::: toolkit/components/places/tests/bookmarks/test_393498.js:65
(Diff revision 4)
> -  let dateAdded = PlacesUtils.bookmarks.getItemDateAdded(bookmarkId);
> -  do_check_eq(dateAdded, PAST_PRTIME);
> +
> +  let updatedBookmark = await PlacesUtils.bookmarks.fetch({
> +    guid: bookmark.parentGuid
> +  });
> +
> +  do_check_eq(updatedBookmark.dateAdded, PAST_DATE);

I was going to say: you'll need to convert both parameters to a number by adding `.getTime()`.

However, how about creating a utility function do_check_date_eq - that works in the same way to call do_check_eq, but just adds .getTime() onto the parameters?

Then use that utility function whenever we need to compare date objects.

::: toolkit/components/places/tests/bookmarks/test_393498.js:68
(Diff revision 4)
> +  });
> +
> +  do_check_eq(updatedBookmark.dateAdded, PAST_DATE);
>  
>    // After just inserting, modified should be the same as dateAdded.
> -  do_check_eq(PlacesUtils.bookmarks.getItemLastModified(bookmarkId), dateAdded);
> +  do_check_eq(updatedBookmark.lastModifed.getTime(), updatedBookmark);

It looks like the order has got a bit switched here based on the comment. This line needs to be just after the observer.itemChangedValue check around line 59.

It also needs changing to:

do_check_eq(bookmark.lastModified.getTime(), bookmark.dateAdded.getTime());

So that it is actually testing what it says.

::: toolkit/components/places/tests/bookmarks/test_393498.js:71
(Diff revision 4)
>  
>    // After just inserting, modified should be the same as dateAdded.
> -  do_check_eq(PlacesUtils.bookmarks.getItemLastModified(bookmarkId), dateAdded);
> +  do_check_eq(updatedBookmark.lastModifed.getTime(), updatedBookmark);
>  
>    // Set lastModified in the past and verify the bookmarks cache.
> -  PlacesUtils.bookmarks.setItemLastModified(bookmarkId, PAST_PRTIME);
> +  await PlacesUtils.bookmarks.update({

You'll need to change this:

updatedBookmark = await PlacesUtils...

::: toolkit/components/places/tests/bookmarks/test_393498.js:78
(Diff revision 4)
> +    lastModified: PAST_DATE
> +  });
> +
>    do_check_eq(observer._itemChangedProperty, "lastModified");
> -  do_check_eq(observer._itemChangedValue, PAST_PRTIME);
> -  do_check_eq(PlacesUtils.bookmarks.getItemLastModified(bookmarkId),
> +  do_check_eq(observer._itemChangedValue, PAST_DATE);
> +  do_check_eq(updatedBookmark.lastModifed.getTime(), PAST_DATE);

nit: spelling of `lastModified` (missing the second i)

::: toolkit/components/places/tests/bookmarks/test_393498.js:82
(Diff revision 4)
> -  do_check_eq(observer._itemChangedValue, PAST_PRTIME);
> -  do_check_eq(PlacesUtils.bookmarks.getItemLastModified(bookmarkId),
> -              PAST_PRTIME);
> +  do_check_eq(observer._itemChangedValue, PAST_DATE);
> +  do_check_eq(updatedBookmark.lastModifed.getTime(), PAST_DATE);
> +
>  
>    // Set bookmark title
> -  PlacesUtils.bookmarks.setItemTitle(bookmarkId, "Google");
> +  await PlacesUtils.bookmarks.setItemTitle(bookmark, "Google");

This should be using the update() API - similar to the update calls you've already got elsewhere. Just pass title instead of lastModified in the object.
Attachment #8876780 - Flags: review?(standard8)
Comment on attachment 8876780 [details]
Bug 1276128 - Convert xpcshell-tests in toolkit/components/places/tests/bookmarks/test_393498.js to Bookmarks.jsm API.

https://reviewboard.mozilla.org/r/148102/#review154390

> I was going to say: you'll need to convert both parameters to a number by adding `.getTime()`.
> 
> However, how about creating a utility function do_check_date_eq - that works in the same way to call do_check_eq, but just adds .getTime() onto the parameters?
> 
> Then use that utility function whenever we need to compare date objects.

I think something like this will work:

function do_check_date_eq(updatedBookmark.dateAdded, PAST_DATE){ 
return do_check_eq(updatedBookmark.dateAdded.getTime() , PAST_DATE.getTime()) ;
}
(In reply to Hemant Singh Patwal [:hhhh1612] from comment #15)
> I think something like this will work:
> 
> function do_check_date_eq(updatedBookmark.dateAdded, PAST_DATE){ 
> return do_check_eq(updatedBookmark.dateAdded.getTime() ,
> PAST_DATE.getTime()) ;
> }

Yes, perfect. Just make the spaces consistent with the rest of the file please.
Attachment #8826421 - Attachment is obsolete: true
Comment on attachment 8876780 [details]
Bug 1276128 - Convert xpcshell-tests in toolkit/components/places/tests/bookmarks/test_393498.js to Bookmarks.jsm API.

https://reviewboard.mozilla.org/r/148102/#review154804

Definitely heading in the right direction. I've given you some help and a few hints. This should get at least the first few parts of the test passing.

I think if you then run the test & look at the results, you should be able to work out the issues with most of the rest of the test - there's various hints as to where the issues are, like `test_393498.js:null:69` says an issue is on line 69.

Sometimes there will be a stack and you'll need to go up a few to find the location at issue - generally though you do want to be looking in the test file.

Also make sure you check spelling, and check what is being compared - does it make sense if an object is compared to part of itself. The failure output should help you.

If you have questions, please feel free to ask.

::: toolkit/components/places/Bookmarks.jsm:593
(Diff revision 5)
>                                                 updatedItem.guid,
>                                                 updatedItem.parentGuid,
>                                                 item.url.href,
>                                                 updatedItem.source ]);
>          }
> +        if (updateInfo.hasOwnProperty("url")) {

You missed updating the url here, although really this should be something like:

if (info.hasOwnProperty("dateAdded") &&
    updateInfo.hasOwnProperty("dateAdded") &&
    item.dateAdded != updatedItem.dateAdded) {

So that we don't notify unnecessarily.

::: toolkit/components/places/Bookmarks.jsm:594
(Diff revision 5)
>                                                 updatedItem.parentGuid,
>                                                 item.url.href,
>                                                 updatedItem.source ]);
>          }
> +        if (updateInfo.hasOwnProperty("url")) {
> +          notify(observers, "onItemChanged", [ updatedItem._id, "uri",

Change "uri" to "dateAdded"

::: toolkit/components/places/Bookmarks.jsm:595
(Diff revision 5)
>                                                 item.url.href,
>                                                 updatedItem.source ]);
>          }
> +        if (updateInfo.hasOwnProperty("url")) {
> +          notify(observers, "onItemChanged", [ updatedItem._id, "uri",
> +                                               false, updatedItem.url.href,

Change "updatedItem.url.href" to "`${PlacesUtils.toPRTime(updatedItem.dateAdded)}`" (include the backticks, but not the double quotes).

With these changes, you should be able to run the test and see it pass at least the 5 initial sub-tests.

::: toolkit/components/places/Bookmarks.jsm:601
(Diff revision 5)
> +                                               PlacesUtils.toPRTime(updatedItem.dateAdded),
> +                                               updatedItem.type,
> +                                               updatedItem._parentId,
> +                                               updatedItem.guid,
> +                                               updatedItem.parentGuid,
> +                                               item.url.href,

item.url.href should be changed to just an empty string, i.e. ""

::: toolkit/components/places/tests/bookmarks/test_393498.js:58
(Diff revision 5)
> +    guid: bookmark.guid,
> +    dateAdded: PAST_DATE
> +  });
> +
> +// Returns do_check_eq with .getTime() added onto parameters
> +  function do_check_date_eq( t1, t2) {

Please define this just before and outside of the add_task.

::: toolkit/components/places/tests/bookmarks/test_393498.js:73
(Diff revision 5)
> +  });
> +
> +  do_check_date_eq(updatedBookmark.dateAdded, PAST_DATE);
>  
>    // After just inserting, modified should be the same as dateAdded.
> -  do_check_eq(PlacesUtils.bookmarks.getItemLastModified(bookmarkId), dateAdded);
> +  do_check_eq(updatedBookmark.lastModifed.getTime(), updatedBookmark);

It looks like you moved this line up to 64, but forgot to drop it from here. Please can you do that (but move the comment up to be with the new test as well).

::: toolkit/components/places/tests/bookmarks/test_393498.js:76
(Diff revision 5)
>  
>    // After just inserting, modified should be the same as dateAdded.
> -  do_check_eq(PlacesUtils.bookmarks.getItemLastModified(bookmarkId), dateAdded);
> +  do_check_eq(updatedBookmark.lastModifed.getTime(), updatedBookmark);
>  
>    // Set lastModified in the past and verify the bookmarks cache.
> -  PlacesUtils.bookmarks.setItemLastModified(bookmarkId, PAST_PRTIME);
> +  await PlacesUtils.bookmarks.update({

The result here needs assigning to updatedBookmark.

::: toolkit/components/places/tests/bookmarks/test_393498.js:90
(Diff revision 5)
>  
>    // Set bookmark title
> -  PlacesUtils.bookmarks.setItemTitle(bookmarkId, "Google");
> +  updatedBookmark = await PlacesUtils.bookmarks.update({title: "Google"});
>  
>    // Test notifications.
> -  do_check_eq(observer._itemChangedId, bookmarkId);
> +  do_check_eq(observer._itemChangedId, bookmark);

The `bookmark` here needs to change to `await PlacesUtils.promiseItemId(bookmark.guid)`, so that you're comparing an id rather than a bookmark object.
Attachment #8876780 - Flags: review?(standard8)
Comment on attachment 8876780 [details]
Bug 1276128 - Convert xpcshell-tests in toolkit/components/places/tests/bookmarks/test_393498.js to Bookmarks.jsm API.

https://reviewboard.mozilla.org/r/148102/#review155912

Thanks for the update, it is good to see some of this start passing.

I've added comments below which should get everything going apart from the very last check. I'm not yet sure what is up with that, I'll take a look again tomorrow.

::: toolkit/components/places/Bookmarks.jsm:594
(Diff revision 6)
>                                                 updatedItem.parentGuid,
>                                                 item.url.href,
>                                                 updatedItem.source ]);
>          }
> +        if (info.hasOwnProperty("dateAdded") && updateInfo.hasOwnProperty("dateAdded")
> +        && item.dateAdded != updatedItem.dateAdded) {

nit: Our convention is to always have the operator (&&) on the end of the line.

::: toolkit/components/places/Bookmarks.jsm:596
(Diff revision 6)
>                                                 updatedItem.source ]);
>          }
> +        if (info.hasOwnProperty("dateAdded") && updateInfo.hasOwnProperty("dateAdded")
> +        && item.dateAdded != updatedItem.dateAdded) {
> +          notify(observers, "onItemChanged", [ updatedItem._id, "dateAdded",
> +                                               false, updatedItem.url.href,

This line should be:

false, `${PlacesUtils.toPRTime(updatedItem.dateAdded)}`,

::: toolkit/components/places/Bookmarks.jsm:597
(Diff revision 6)
>          }
> +        if (info.hasOwnProperty("dateAdded") && updateInfo.hasOwnProperty("dateAdded")
> +        && item.dateAdded != updatedItem.dateAdded) {
> +          notify(observers, "onItemChanged", [ updatedItem._id, "dateAdded",
> +                                               false, updatedItem.url.href,
> +                                               `${PlacesUtils.toPRTime(updatedItem.dateAdded)}`,

This line should be:

PlacesUtils.toPRTime(updatedItem.dateAdded)

::: toolkit/components/places/tests/bookmarks/test_393498.js:28
(Diff revision 6)
>  
>  do_register_cleanup(function() {
>    PlacesUtils.bookmarks.removeObserver(observer);
>  });
>  
> -function run_test() {
> +  // Returns do_check_eq with .getTime() added onto parameters

nit: no space at start of line and no blank line after.

::: toolkit/components/places/tests/bookmarks/test_393498.js:64
(Diff revision 6)
> +    guid: bookmark.guid,
> +    dateAdded: PAST_DATE
> +  });
> +
>    do_check_eq(observer._itemChangedProperty, "dateAdded");
> -  do_check_eq(observer._itemChangedValue, PAST_PRTIME);
> +  do_check_eq(observer._itemChangedValue, bookmark.url);

This is wrong, we should still be checking against PAST_DATE here (well, probably `PlacesUtils.toPRTime(PAST_DATE)`). Did you miss my previous comments about correcting the code you added in Bookmarks.jsm?

::: toolkit/components/places/tests/bookmarks/test_393498.js:83
(Diff revision 6)
> +    lastModified: PAST_DATE
> +  });
> +
>    do_check_eq(observer._itemChangedProperty, "lastModified");
> -  do_check_eq(observer._itemChangedValue, PAST_PRTIME);
> -  do_check_eq(PlacesUtils.bookmarks.getItemLastModified(bookmarkId),
> +  do_check_eq(observer._itemChangedValue, PlacesUtils.toPRTime(PAST_DATE));
> +  do_check_date_eq(updatedBookmark.lastModifed, PAST_DATE);

The current error shows:

ERROR Unexpected exception TypeError: t1 is undefined at /Users/mark/dev/gecko/objdir-ff/_tests/xpcshell/toolkit/components/places/tests/bookmarks/test_393498.js:31
do_check_date_eq@/Users/mark/dev/gecko/objdir-ff/_tests/xpcshell/toolkit/components/places/tests/bookmarks/test_393498.js:31:3
@/Users/mark/dev/gecko/objdir-ff/_tests/xpcshell/toolkit/components/places/tests/bookmarks/test_393498.js:83:3

So you need to think about why t1 is undefined - if you come back up the stack, you hit this line - think about why that first param might be undefined (hint: spelling).

::: toolkit/components/places/tests/bookmarks/test_393498.js:87
(Diff revision 6)
> -  do_check_eq(observer._itemChangedValue, PAST_PRTIME);
> -  do_check_eq(PlacesUtils.bookmarks.getItemLastModified(bookmarkId),
> -              PAST_PRTIME);
> +  do_check_eq(observer._itemChangedValue, PlacesUtils.toPRTime(PAST_DATE));
> +  do_check_date_eq(updatedBookmark.lastModifed, PAST_DATE);
> +
>  
>    // Set bookmark title
> -  PlacesUtils.bookmarks.setItemTitle(bookmarkId, "Google");
> +  updatedBookmark = await PlacesUtils.bookmarks.update({title: "Google"});

This part should be (guid missing):

updatedBookmark = await PlacesUtils.bookmarks.update({
  guid: bookmark.guid,
  title: "Google"
});

::: toolkit/components/places/tests/bookmarks/test_393498.js:105
(Diff revision 6)
>    do_check_eq(root.childCount, 1);
>    let childNode = root.getChild(0);
>  
>    // confirm current dates match node properties
> -  do_check_eq(PlacesUtils.bookmarks.getItemDateAdded(bookmarkId),
> +  do_check_eq(updatedBookmark,
>                childNode.dateAdded);

do_check_eq(PlacesUtils.toPRTime(updatedBookmark.dateAdded),
   childNode.dateAdded);

::: toolkit/components/places/tests/bookmarks/test_393498.js:106
(Diff revision 6)
>    let childNode = root.getChild(0);
>  
>    // confirm current dates match node properties
> -  do_check_eq(PlacesUtils.bookmarks.getItemDateAdded(bookmarkId),
> +  do_check_eq(updatedBookmark,
>                childNode.dateAdded);
> -  do_check_eq(PlacesUtils.bookmarks.getItemLastModified(bookmarkId),
> +  do_check_eq(updatedBookmark,

do_check_eq(PlacesUtils.toPRTime(updatedBookmark.lastModified),
  childNode.lastModified);

::: toolkit/components/places/tests/bookmarks/test_393498.js:111
(Diff revision 6)
> -  do_check_eq(PlacesUtils.bookmarks.getItemLastModified(bookmarkId),
> +  do_check_eq(updatedBookmark,
>                childNode.lastModified);
>  
>    // Test live update of lastModified when setting title.
> -  PlacesUtils.bookmarks.setItemLastModified(bookmarkId, PAST_PRTIME);
> -  PlacesUtils.bookmarks.setItemTitle(bookmarkId, "Google");
> +  await PlacesUtils.bookmarks.update({guid: bookmark.guid, lastModified: PAST_DATE});
> +  await PlacesUtils.bookmarks.update({guid: bookmark.guid, title: "Google"});

I think these can be merged into one call - i.e. specify guid, lastModified and title all within the one object that is passed to update().

::: toolkit/components/places/tests/bookmarks/test_393498.js:118
(Diff revision 6)
>  
>    // Check lastModified has been updated.
> -  is_time_ordered(PAST_PRTIME, childNode.lastModified);
> +  is_time_ordered(PAST_DATE, childNode.lastModified);
>    // Test that node value matches db value.
> -  do_check_eq(PlacesUtils.bookmarks.getItemLastModified(bookmarkId),
> +  do_check_date_eq(updatedBookmark.lastModified,
>                childNode.lastModified);

do_check_eq(PlacesUtils.toPRTime(updatedBookmark.lastModified),
  childNode.lastModified);

::: toolkit/components/places/tests/bookmarks/test_393498.js:123
(Diff revision 6)
>                childNode.lastModified);
>  
> +
>    // Test live update of the exposed date apis.
> -  PlacesUtils.bookmarks.setItemDateAdded(bookmarkId, PAST_PRTIME);
> -  do_check_eq(childNode.dateAdded, PAST_PRTIME);
> +  await PlacesUtils.bookmarks.update({guid: bookmark.guid, dateAdded: PAST_DATE});
> +  do_check_date_eq(childNode.dateAdded, PAST_DATE);

do_check_eq(childNode.dateAdded, PlacesUtils.toPRTime(PAST_DATE));

::: toolkit/components/places/tests/bookmarks/test_393498.js:125
(Diff revision 6)
> +
>    // Test live update of the exposed date apis.
> -  PlacesUtils.bookmarks.setItemDateAdded(bookmarkId, PAST_PRTIME);
> -  do_check_eq(childNode.dateAdded, PAST_PRTIME);
> -  PlacesUtils.bookmarks.setItemLastModified(bookmarkId, PAST_PRTIME);
> -  do_check_eq(childNode.lastModified, PAST_PRTIME);
> +  await PlacesUtils.bookmarks.update({guid: bookmark.guid, dateAdded: PAST_DATE});
> +  do_check_date_eq(childNode.dateAdded, PAST_DATE);
> +  await PlacesUtils.bookmarks.update({guid: bookmark.guid, lastModified: PAST_DATE})
> +  do_check_date_eq(childNode.lastModified, PAST_DATE);

do_check_eq(childNode.lastModified, PlacesUtils.toPRTime(PAST_DATE));

::: toolkit/components/places/tests/bookmarks/test_393498.js:126
(Diff revision 6)
>    // Test live update of the exposed date apis.
> -  PlacesUtils.bookmarks.setItemDateAdded(bookmarkId, PAST_PRTIME);
> -  do_check_eq(childNode.dateAdded, PAST_PRTIME);
> -  PlacesUtils.bookmarks.setItemLastModified(bookmarkId, PAST_PRTIME);
> -  do_check_eq(childNode.lastModified, PAST_PRTIME);
> +  await PlacesUtils.bookmarks.update({guid: bookmark.guid, dateAdded: PAST_DATE});
> +  do_check_date_eq(childNode.dateAdded, PAST_DATE);
> +  await PlacesUtils.bookmarks.update({guid: bookmark.guid, lastModified: PAST_DATE})
> +  do_check_date_eq(childNode.lastModified, PAST_DATE);
>  

For some reason I haven't discovered yet, I can get the test to pass apart from this last test. I'm not currently sure if it is a real failure, or something behaving differently. I'll try and take another look tomorrow.
Attachment #8876780 - Flags: review?(standard8)
Comment on attachment 8876780 [details]
Bug 1276128 - Convert xpcshell-tests in toolkit/components/places/tests/bookmarks/test_393498.js to Bookmarks.jsm API.

https://reviewboard.mozilla.org/r/148102/#review155912

> This is wrong, we should still be checking against PAST_DATE here (well, probably `PlacesUtils.toPRTime(PAST_DATE)`). Did you miss my previous comments about correcting the code you added in Bookmarks.jsm?

No. Just because test was comaparing  "http://google.com/" to Number so I changed.And it seemed it worked. :)
Comment on attachment 8876780 [details]
Bug 1276128 - Convert xpcshell-tests in toolkit/components/places/tests/bookmarks/test_393498.js to Bookmarks.jsm API.

https://reviewboard.mozilla.org/r/148102/#review155912

> I think these can be merged into one call - i.e. specify guid, lastModified and title all within the one object that is passed to update().

Also, you need to assign to `updatedBookmark = await  ... `
Comment on attachment 8876780 [details]
Bug 1276128 - Convert xpcshell-tests in toolkit/components/places/tests/bookmarks/test_393498.js to Bookmarks.jsm API.

https://reviewboard.mozilla.org/r/148102/#review157130

Ok, I found the last few issues. What I'll do is send you a diff against the current version - there's a few other changes that need tidying as well, and rather than go through more reviews, this way we'll be a bit quicker.

::: toolkit/components/places/tests/bookmarks/test_393498.js:47
(Diff revision 7)
> -    PlacesUtils.bookmarks.DEFAULT_INDEX);
> -  let bookmarkId = PlacesUtils.bookmarks.insertBookmark(
> -    testFolder, uri("http://google.com/"),
> -    PlacesUtils.bookmarks.DEFAULT_INDEX, "");
> +    title: "test Folder",
> +    parentGuid: PlacesUtils.bookmarks.menuGuid
> +  });
> +
> +  let bookmark = await PlacesUtils.bookmarks.insert({
> +    parentGuid: testFolder.parentGuid,

This should be testFolder.guid, otherwise we're inserting into the testFolder's parent, rather than the test folder.

::: toolkit/components/places/tests/bookmarks/test_393498.js:98
(Diff revision 7)
>    // Check lastModified has been updated.
> -  is_time_ordered(PAST_PRTIME,
> -                  PlacesUtils.bookmarks.getItemLastModified(bookmarkId));
> +  is_time_ordered(PAST_DATE,
> +                  updatedBookmark.lastModified.getTime());
>  
>    // Check that node properties are updated.
>    let root = PlacesUtils.getFolderContents(testFolder).root;

Here we need to await and get the item Id of the folder. Due to the earlier changes, testFolder is now an object rather than an id, so getFolderContents is getting confused, which upsets the rest of the test.
Attachment #8876780 - Flags: review?(standard8)
Comment on attachment 8876780 [details]
Bug 1276128 - Convert xpcshell-tests in toolkit/components/places/tests/bookmarks/test_393498.js to Bookmarks.jsm API.

https://reviewboard.mozilla.org/r/148102/#review157162

Thank you for sticking with this and all the updates. It turned out more complicated than I expected. Anyway, looks good now. r=Standard8
Attachment #8876780 - Flags: review?(standard8) → review+
Pushed by mbanner@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/8f8f1555bb34
Convert xpcshell-tests in toolkit/components/places/tests/bookmarks/test_393498.js to Bookmarks.jsm API. r=standard8
https://hg.mozilla.org/mozilla-central/rev/8f8f1555bb34
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: