Closed Bug 1275145 Opened 8 years ago Closed 7 years ago

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

Categories

(Toolkit :: Places, defect, P3)

defect

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: gasolin, Assigned: leni1, Mentored)

References

Details

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

Attachments

(1 file, 13 obsolete files)

4.50 KB, patch
mak
: review+
Details | Diff | Splinter Review
Convert test cases to use new bookmark API based on bug 1094903 comment 8.

The required API change could reference to bug 1271201 & bug 1273023
Whiteboard: [good first bug][mentor-lang=zh][lang=js]
I'd like to work on this bug could I be assigned? thanks
Flags: needinfo?(gasolin)
uh was this not completed already in bug 1094903 ?
Sure, thanks for interest in fix this issue!

It's a good start when you found there's something reusable from Bug 1094903 

The PR there provide the basic skeleton of new test_388695.js . You can copy that and do 
`./mach test /path/to/test_388695.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.


Feel free to check `Need more information from` box if you need any help. 
I'll be in IRC with `gasolin` as well.
Assignee: nobody → mitchdevel
Flags: needinfo?(gasolin)
Just an update: I had mistakenly taken on another bug after thinking this was complete, I was going to try to complete that and then come back to this bug but it is taking me a lot longer than I thought. I'm still willing to work on this bug but if somebody else wants to complete this that is fine with me. 

Appolgies from a newbie
Thanks for update! That's fine just follow your pace :)
Attached patch newapi.diff (obsolete) — Splinter Review
I updated all of the old calls to the new api :)
Attachment #8759988 - Flags: review?(gasolin)
Comment on attachment 8759988 [details] [diff] [review]
newapi.diff

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

Thanks for the patch, overall looks good!  Some nits need to be addressed.

> var gTestRoot;
> var gURI;
> var gItemId1;
> var gItemId2;

Since there's only one test case in this file now, we can move the variable declaration into the function, and use `let` keyword instead of `var`.

> gTestRoot = (yield bm.insert({type: bm.TYPE_FOLDER,
>                               title: "test folder",
>                               parentGuid: bm.rootGuid})).guid;

I'd prefer call `.guid` later when it in use, so we can remove the brackets and make it more readable.

> var b = yield bm.search({url: gURI});

for consistency, we can replace `var` keyword to `let` inside of the test case.

> \ No newline at end of file

please add a new line at end of the file (to pass the style check)


Once issues are addressed, you can set review again to :mak (mak77@bonardo.net), who is review most of recent bookmark tests https://github.com/mozilla/gecko-dev/commits/master/toolkit/components/places/tests/bookmarks
Attachment #8759988 - Flags: review?(gasolin) → feedback+
Attached patch stylechanges.diff (obsolete) — Splinter Review
Fixed the small issues thanks for the feedback!

although for 
> var b = yield bm.search({url: gURI});
I did not change it to use let as this would cause problems later when its used in the assert statements
> Assert.equal(b[0].guid, gItemId1.guid);
Attachment #8759988 - Attachment is obsolete: true
Attachment #8760760 - Flags: review?(mak77)
Comment on attachment 8760760 [details] [diff] [review]
stylechanges.diff

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

::: toolkit/components/places/tests/bookmarks/test_388695.js
@@ +26,5 @@
> +                              url: gURI,
> +                              parentGuid: gTestRoot})).guid;
> +  gItemId2 = (yield bm.insert({type: bm.TYPE_BOOKMARK,
> +                              url: gURI,
> +                              parentGuid: gTestRoot})).guid;

Is this the complete patch? I don't see where this change comes from, looks like you forgot to squash some commits?

@@ +35,4 @@
>  
> +  yield bm.update({guid: gItemId1,
> +                   title: "modified"});
> +  var b = yield bm.search({url: gURI});

Please, don't use bookmarks.search. that's a very special API that was created for a specific web extension case.
always use bookmarks.fetch when fetching by url.

I guess we need to fix Bookmarks.jsm to clarify that.
Attachment #8760760 - Flags: review?(mak77)
Mitch, sorry for late response. Would you like to complete the patch? I think its just one step further :)


When we encounter a new API that never met before, we can search dxr and check related API usage, such as

https://dxr.mozilla.org/mozilla-central/search?q=bookmarks.fetch&redirect=falsec


When you make the change, you can run `./mach test toolkit/components/places/tests/bookmarks/test_388695.js` to make sure the patch works fine.

Please don't hesitate to needinfo me if you encounter any problem.
Flags: needinfo?(mitchdevel)
Priority: -- → P3
Attached file Sample test_388695.js (obsolete) —
I have used Mitch's most recent patch as a base and made the changes that Marco suggested. If there are any other changes to be made, let me know. Going to test and submit as a patch for review.
Flags: needinfo?(me)
Attachment #8856105 - Flags: review?(gasolin)
Attached file test_388695.js (obsolete) —
Ran ./mach eslint on the previous attachment and came up with some errors, namely a re-declaration of the variable b and it being assigned a value, yet it's already been assigned previously.
Attachment #8856105 - Attachment is obsolete: true
Attachment #8856105 - Flags: review?(gasolin)
Flags: needinfo?(me)
Attachment #8856133 - Flags: review?(gasolin)
This is the output of `./mach test toolkit/components/places/tests/bookmarks/test_388695.js`. I'm not sure where I'm going wrong as the code seems to be fine. Pointers in the right direction would be helpful.
Flags: needinfo?(gasolin)
Thanks Leni!

The main error is 
```
TypeError: b[0] is undefined at /home/user/mozilla-central/obj-x86_64-pc-linux-gnu/_tests/xpcshell/toolkit/components/places/tests/bookmarks/test_388695.js:29
```

Which means the test can't find b[0] in test. To avoid a re-declaration of the variable b, you can use `let` instead of `var` in each function scope(preferred), or rename the duplicate variables.

If b[0] is still undefined, check if the API is called with wrong params.
Assignee: mitchdevel → lenikmutungi
Flags: needinfo?(mitchdevel)
Flags: needinfo?(gasolin)
Attachment #8856133 - Flags: review?(gasolin)
Convert xpcshell-tests in toolkit/components/places/tests/bookmarks/test_388695.js to Bookmarks.jsm API
Modified variable names and replaced `var` with `let`
Leni Kadali <lenikmutungi@gmail.com>
branch 'default'
changed toolkit/components/places/tests/bookmarks/test_388695.js
Comment on attachment 8857412 [details] [diff] [review]
Convert xpcshell-tests in toolkit/components/places/tests/bookmarks/test_388695.js to Bookmarks.jsm API

Here's the sample test_388695.js file. The other attachment I submitted was blank and that wouldn't be useful for anyone trying to follow up on this.
Attachment #8857412 - Attachment filename: . → test_388695.js
Flags: needinfo?(mak77)
replied on IRC.
Flags: needinfo?(mak77)
I'm writing this up for reference purposes. If there is something that I've left out or I should add, let me know. 

Task: Write a test that checks the proper ordering of each bookmark's lastModified property. 

To do: 
1. Write a cystom lastModified property whose time value should be in the past e.g. 1, 2 hours ago. 
(Should the range be limited to 1 - 2 hours or this should be left open?)

2. Update within the test should change the order of the bookmarks according to which item has the most recent lastModified property e.g. if item2 has a more recent lastModified property than item1, then the returned results should have item2 on top of item1. 
(For this, should a loop be used on the bookmarks' array to evaluate which has the more recent lastModified property? I imagine they would need sorting and a statement doesn't strike me as flexible enough for this.)
Flags: needinfo?(mak77)
(In reply to Leni Kadali from comment #18)
>> 1. Write a cystom lastModified property whose time value should be in the
> past e.g. 1, 2 hours ago. 
> (Should the range be limited to 1 - 2 hours or this should be left open?)

when you create the bookmark you can pass a lastModified property that is a Date object. 2 hours earlier sounds good.
Again, the scope of the test is checking in which order the bookmarks are fetched, and that order depends on lastModified.

> (For this, should a loop be used on the bookmarks' array to evaluate which
> has the more recent lastModified property? I imagine they would need sorting
> and a statement doesn't strike me as flexible enough for this.)

You can just hardcode the checks as they are currently, it's only 2 bookmarks, there's no need to over-engineer the thing.
The fact is if you don't set lastModified first, your checks are pointless since you don't start from a known situation.
Flags: needinfo?(mak77)
Comment on attachment 8860630 [details] [diff] [review]
Convert xpcshell-tests in toolkit/components/places/tests/bookmarks/test_388695.js to Bookmarks.jsm API

This was my attempt at making a custom lastModified property. When I ran the test `./mach test toolkit/components/places/tests/bookmarks/test_388695.js` I got the following error message: 

 0:02.20 LOG: Thread-3 ERROR Unexpected exception Error: Invalid value for property 'lastModified': 1492857972755 at resource://gre/modules/PlacesUtils.jsm:551
validateItemProperties@resource://gre/modules/PlacesUtils.jsm:551:15
validateBookmarkObject@resource://gre/modules/Bookmarks.jsm:1774:10
insert@resource://gre/modules/Bookmarks.jsm:168:22
sort_bookmark_by_relevance@/home/herabus/mozilla-central/obj-x86_64-pc-linux-gnu/_tests/xpcshell/toolkit/components/places/tests/bookmarks/test_388695.js:33:23
_run_next_test@/home/herabus/mozilla-central/testing/xpcshell/head.js:1554:9
run@/home/herabus/mozilla-central/testing/xpcshell/head.js:703:9
_do_main@/home/herabus/mozilla-central/testing/xpcshell/head.js:211:5
_execute_test@/home/herabus/mozilla-central/testing/xpcshell/head.js:546:5
I forgot to add the needinfo and review flags for this. See attachment and comment above.
Flags: needinfo?(mak77)
Comment on attachment 8860630 [details] [diff] [review]
Convert xpcshell-tests in toolkit/components/places/tests/bookmarks/test_388695.js to Bookmarks.jsm API

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

::: toolkit/components/places/tests/bookmarks/test_388695.js
@@ +24,5 @@
>  
> +    let modifiedTime = now.setHours(now.getHours() - 2);
> +
> +
> +//    let modifiedTime = new Date(now.getHours() - 2);

you likely need a combination of these.
The bookmarks API expects a Date object, so you need new Date()
you also need to set the hours back in time.
something like this probably:
let modifiedTime = new Date(now.setHours(now.getHours() - 2));

note that, thanks to ES6 shorthands, if you name this lastModified, later in the object passed to insert you could just do ..., lastModified }

@@ +32,3 @@
>          
> +    gItemId1 = (yield bm.insert({type: bm.TYPE_BOOKMARK, url: gURI, parentGuid: gTestTags, lastModified: modifiedTime})).guid;
> +    gItemId2 = (yield bm.insert({type: bm.TYPE_BOOKMARK, url: gURI, parentGuid: gTestTags, lastModified: modifiedTime})).guid;

you are setting the same lastModified for both bookmarks, how will you be able to then figure out if the order is correct?
One bookmark should use the past lastModified, the other one should not.

The one that should use the past lastModified, is the one that later you will change with update, so that the order in the second test should be inverted.
Flags: needinfo?(mak77)
I tried the modification you suggested. Unfortunately it gives this error: 

0:02.40 LOG: Thread-3 ERROR Unexpected exception Error: Invalid value for property 'lastModified': "2017-04-27T13:19:01.810Z" at resource://gre/modules/PlacesUtils.jsm:551
validateItemProperties@resource://gre/modules/PlacesUtils.jsm:551:15
validateBookmarkObject@resource://gre/modules/Bookmarks.jsm:1774:10
insert@resource://gre/modules/Bookmarks.jsm:168:22
sort_bookmark_by_relevance@/home/user/mozilla-central/obj-x86_64-pc-linux-gnu/_tests/xpcshell/toolkit/components/places/tests/bookmarks/test_388695.js:34:23
_run_next_test@/home/user/mozilla-central/testing/xpcshell/head.js:1554:9
run@/home/user/mozilla-central/testing/xpcshell/head.js:703:9
_do_main@/home/user/mozilla-central/testing/xpcshell/head.js:211:5
_execute_test@/home/user/mozilla-central/testing/xpcshell/head.js:546:5


> @@ +32,3 @@
> >          
> > +    gItemId1 = (yield bm.insert({type: bm.TYPE_BOOKMARK, url: gURI, parentGuid: gTestTags, lastModified: modifiedTime})).guid;
> > +    gItemId2 = (yield bm.insert({type: bm.TYPE_BOOKMARK, url: gURI, parentGuid: gTestTags, lastModified: modifiedTime})).guid;
> 
> you are setting the same lastModified for both bookmarks, how will you be
> able to then figure out if the order is correct?
> One bookmark should use the past lastModified, the other one should not.
> 
> The one that should use the past lastModified, is the one that later you
> will change with update, so that the order in the second test should be
> inverted.

So the difference between gItemId1 and gItemId2 would be something along these lines:
gItemId1 = (yield bm.insert({type: bm.TYPE_BOOKMARK, url: gURI, parentGuid: gTestTags})).guid;
gItemId2 = (yield bm.insert({type: bm.TYPE_BOOKMARK, url: gURI, parentGuid: gTestTags, lastModified: modifiedTime})).guid;

Or would it mean having two variables with different time values for each (e.g. present time and time two hours before) assigned to gItemId1 and gItemId2 respectively?
Flags: needinfo?(mak77)
(In reply to Leni Kadali from comment #24)
> I tried the modification you suggested. Unfortunately it gives this error: 
> 
> 0:02.40 LOG: Thread-3 ERROR Unexpected exception Error: Invalid value for
> property 'lastModified': "2017-04-27T13:19:01.810Z" at

Ah, probably it's because it shouldn't be older than dateAdded.
Please try setting both dateAdded AND lastModified properties to the new value.

> So the difference between gItemId1 and gItemId2 would be something along
> these lines:
> gItemId1 = (yield bm.insert({type: bm.TYPE_BOOKMARK, url: gURI, parentGuid:
> gTestTags})).guid;
> gItemId2 = (yield bm.insert({type: bm.TYPE_BOOKMARK, url: gURI, parentGuid:
> gTestTags, lastModified: modifiedTime})).guid;
> 
> Or would it mean having two variables with different time values for each

the default for lastModified is now, so it's fine to pass nothing in one of the cases.
Flags: needinfo?(mak77)
Added dateAdded and lastModified to gItemId1. Running the test gives the following error message:

0:01.25 TEST_STATUS: Thread-1 sort_bookmark_by_relevance FAIL [sort_bookmark_by_relevance : 40] "n23wtddufRMz" == "fn4SMth2UKeg"
    /home/user/mozilla-central/obj-x86_64-pc-linux-gnu/_tests/xpcshell/toolkit/components/places/tests/bookmarks/test_388695.js:sort_bookmark_by_relevance:40
    _run_next_test@/home/user/mozilla-central/testing/xpcshell/head.js:1554:9
    run@/home/user/mozilla-central/testing/xpcshell/head.js:703:9
    _do_main@/home/user/mozilla-central/testing/xpcshell/head.js:211:5
    _execute_test@/home/user/mozilla-central/testing/xpcshell/head.js:546:5
    @-e:1:1
 0:01.25 LOG: Thread-1 INFO exiting test
 0:01.25 LOG: Thread-1 ERROR Unexpected exception 2147500036
undefined
 0:01.25 LOG: Thread-1 INFO exiting test
 0:01.49 TEST_END: Thread-1 Harness FAIL. Subtests passed 0/1. Unexpected 0
 0:01.50 LOG: Thread-1 INFO toolkit/components/places/tests/bookmarks/test_388695.js failed or timed out, will retry.
 0:01.50 LOG: MainThread INFO Retrying tests that failed when run in parallel.
 0:01.51 TEST_START: Thread-3 toolkit/components/places/tests/bookmarks/test_388695.js
 0:01.51 LOG: Thread-3 INFO toolkit/components/places/tests/bookmarks/test_388695.js | full command: ['/home/user/mozilla-central/obj-x86_64-pc-linux-gnu/dist/bin/xpcshell', '-g', '/home/user/mozilla-central/obj-x86_64-pc-linux-gnu/dist/bin', '-a', '/home/user/mozilla-central/obj-x86_64-pc-linux-gnu/dist/bin', '-r', '/home/user/mozilla-central/obj-x86_64-pc-linux-gnu/dist/bin/components/httpd.manifest', '-m', '-s', '-e', 'const _HEAD_JS_PATH = "/home/user/mozilla-central/testing/xpcshell/head.js";', '-e', 'const _MOZINFO_JS_PATH = "/home/user/mozilla-central/obj-x86_64-pc-linux-gnu/temp/xpc-profile-82IzHg/mozinfo.json";', '-e', 'const _TESTING_MODULES_DIR = "/home/user/mozilla-central/obj-x86_64-pc-linux-gnu/_tests/modules/";', '-f', '/home/user/mozilla-central/testing/xpcshell/head.js', '-p', '/home/user/mozilla-central/obj-x86_64-pc-linux-gnu/temp/xpc-plugins-6aXhY_', '-e', 'const _SERVER_ADDR = "localhost"', '-e', u'const _HEAD_FILES = ["/home/user/mozilla-central/obj-x86_64-pc-linux-gnu/_tests/xpcshell/toolkit/components/places/tests/bookmarks/head_bookmarks.js"];', '-e', 'const _JSDEBUGGER_PORT = 0;', '-e', u'const _TEST_FILE = ["/home/user/mozilla-central/obj-x86_64-pc-linux-gnu/_tests/xpcshell/toolkit/components/places/tests/bookmarks/test_388695.js"];', '-e', u'const _TEST_NAME = "toolkit/components/places/tests/bookmarks/test_388695.js"', '-e', '_execute_test(); quit(0);']
 0:01.51 LOG: Thread-3 INFO toolkit/components/places/tests/bookmarks/test_388695.js | current directory: u'/home/user/mozilla-central/obj-x86_64-pc-linux-gnu/_tests/xpcshell/toolkit/components/places/tests/bookmarks'
 0:01.51 LOG: Thread-3 INFO toolkit/components/places/tests/bookmarks/test_388695.js | environment: ['XPCSHELL_TEST_PROFILE_DIR=/home/user/mozilla-central/obj-x86_64-pc-linux-gnu/temp/xpc-profile-82IzHg', 'XPCOM_DEBUG_BREAK=stack-and-abort', 'MOZ_CRASHREPORTER=1', 'LD_LIBRARY_PATH=/home/user/mozilla-central/obj-x86_64-pc-linux-gnu/dist/bin', 'XPCSHELL_TEST_TEMP_DIR=/home/user/mozilla-central/obj-x86_64-pc-linux-gnu/temp/xpc-other-aI28Qq', 'MOZ_DISABLE_NONLOCAL_CONNECTIONS=1', 'MOZ_CRASHREPORTER_NO_REPORT=1']
 0:02.14 LOG: Thread-3 INFO (xpcshell/head.js) | test MAIN run_test pending (1)
 0:02.14 LOG: Thread-3 INFO (xpcshell/head.js) | test run_next_test 0 pending (2)
 0:02.15 LOG: Thread-3 INFO (xpcshell/head.js) | test MAIN run_test finished (2)
 0:02.15 LOG: Thread-3 INFO running event loop
 0:02.15 LOG: Thread-3 INFO toolkit/components/places/tests/bookmarks/test_388695.js | Starting sort_bookmark_by_relevance
 0:02.15 LOG: Thread-3 INFO (xpcshell/head.js) | test sort_bookmark_by_relevance pending (2)
 0:02.17 LOG: Thread-3 INFO (xpcshell/head.js) | test run_next_test 0 finished (2)
 0:02.18 LOG: Thread-3 INFO "CONSOLE_MESSAGE: (info) No chrome package registered for chrome://branding/locale/brand.properties"
 0:02.18 LOG: Thread-3 INFO "CONSOLE_MESSAGE: (info) No chrome package registered for chrome://branding/locale/brand.properties"
 0:02.53 TEST_STATUS: Thread-3 sort_bookmark_by_relevance FAIL [sort_bookmark_by_relevance : 40] "PNvlAft70fUT" == "3Qqnv7BTbINc"
    /home/user/mozilla-central/obj-x86_64-pc-linux-gnu/_tests/xpcshell/toolkit/components/places/tests/bookmarks/test_388695.js:sort_bookmark_by_relevance:40
    _run_next_test@/home/user/mozilla-central/testing/xpcshell/head.js:1554:9
    run@/home/user/mozilla-central/testing/xpcshell/head.js:703:9
    _do_main@/home/user/mozilla-central/testing/xpcshell/head.js:211:5
    _execute_test@/home/user/mozilla-central/testing/xpcshell/head.js:546:5
    @-e:1:1
 0:02.53 LOG: Thread-3 INFO exiting test
 0:02.54 LOG: Thread-3 ERROR Unexpected exception 2147500036
undefined
 0:02.54 LOG: Thread-3 INFO exiting test
 0:02.79 TEST_END: Thread-3 Harness FAIL, expected PASS. Subtests passed 0/1. Unexpected 1
xpcshell return code: 0
 0:02.79 LOG: MainThread INFO INFO | Result summary:
 0:02.79 LOG: MainThread INFO INFO | Passed: 0
 0:02.79 LOG: MainThread INFO INFO | Failed: 1
 0:02.79 LOG: MainThread INFO INFO | Todo: 0
 0:02.79 LOG: MainThread INFO INFO | Retried: 1
 0:02.79 SUITE_END: MainThread
Attachment #8860630 - Attachment is obsolete: true
Attachment #8863166 - Flags: review?(mak77)
(In reply to Leni Kadali from comment #26)
> 0:01.25 TEST_STATUS: Thread-1 sort_bookmark_by_relevance FAIL
> [sort_bookmark_by_relevance : 40] "n23wtddufRMz" == "fn4SMth2UKeg"

The found guid is not the expected one at line 40, that means the order is not the expected one.
you set bookmark1 to a past date, and bookmark2 to a new date.
Then you fetch the bookmarks.
What you want is that the first returned bookmark is the most recent one. when bookmarks are returned you push them into an array, so you array will look like
[most_recent, older]. and this is what the test should check. maybe the current check is inverted?

Also, please merge all the commits you have into one, and when attaching the whole changeset mark old ones as obsolete, otherwise I can only see one small piece of the thing.
Attachment #8863166 - Flags: review?(mak77)
Changed variable name from gTestRoot to gTestUnfiled
Changed parentGuid value from rootGuid to unfiledGuid.
Created variable modifiedTime that holds the time values for dateAdded and lastModified
Created variables bms, bms1, bm1, and bm2 for the test.
Attachment #8863166 - Attachment is obsolete: true
Comment on attachment 8864451 [details] [diff] [review]
Convert xpcshell-tests in toolkit/components/places/tests/bookmarks/test_388695.js to Bookmarks.jsm API

Running `./mach test toolkit/components/places/tests/bookmarks/test_388695.js` is a success, no error messages.
Attachment #8864451 - Flags: review?(mak77)
Attachment #8760760 - Attachment is obsolete: true
Attachment #8856133 - Attachment is obsolete: true
Attachment #8856134 - Attachment is obsolete: true
Attachment #8857412 - Attachment is obsolete: true
Comment on attachment 8864451 [details] [diff] [review]
Convert xpcshell-tests in toolkit/components/places/tests/bookmarks/test_388695.js to Bookmarks.jsm API

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

The patch looks incomplete and thus not landable, please merge all the patches you have into a single one, that goes from current checked-in test_388695.js to the latest version you have.
For example, this diff is missing the "bm" variable definition. it's likely you have that in a previous changeset.
You can merge mercurial queues changesets using hg qfold, or normal changesets using hg histedit (you have to enable a mercurial extension for this).

::: toolkit/components/places/tests/bookmarks/test_388695.js
@@ +18,4 @@
>      let gURI;
>      let gItemId1;
>      let gItemId2;
> +    

there are various trailing spaces left around, please remove all of them.
You can check code using `./mach eslint toolkit/components/places/tests`
it should return no errors.

@@ +24,3 @@
>  
>      gURI = uri("http://foo.tld.com/");
> +    gTestUnfiled = (yield bm.insert({type: bm.TYPE_FOLDER, title: "test folder", parentGuid: bm.unfiledGuid})).guid;

For coding style we have a soft limit of 80 chars before wrapping, that means in general you should reindent code to try staying below 80 chars.
In this case for example you could indent parentGuid and title below type. And so on.

@@ +37,2 @@
>      
> +    yield bm.update({guid: gItemId1, title: "modified"});

itemId1 is already the most recent bookmark, so this doesn't make much sense, you are updating the already most recent bookmark, that won't change the order of bookmarks returned by fetch.
You should instead update the LEAST recent bookmark, so that the order of bookmarks returned by fetch will be inverted from this point on.
Attachment #8864451 - Flags: review?(mak77)
(In reply to Marco Bonardo [::mak] from comment #30)
> Comment on attachment 8864451 [details] [diff] [review]
> Convert xpcshell-tests in
> toolkit/components/places/tests/bookmarks/test_388695.js to Bookmarks.jsm API
> 
> Review of attachment 8864451 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> The patch looks incomplete and thus not landable, please merge all the
> patches you have into a single one, that goes from current checked-in
> test_388695.js to the latest version you have.
> For example, this diff is missing the "bm" variable definition. it's likely
> you have that in a previous changeset.
> You can merge mercurial queues changesets using hg qfold, or normal
> changesets using hg histedit (you have to enable a mercurial extension for
> this).
> 
> ::: toolkit/components/places/tests/bookmarks/test_388695.js
> @@ +18,4 @@
> >      let gURI;
> >      let gItemId1;
> >      let gItemId2;
> > +    
> 
> there are various trailing spaces left around, please remove all of them.
> You can check code using `./mach eslint toolkit/components/places/tests`
> it should return no errors.
> 
> @@ +24,3 @@
> >  
> >      gURI = uri("http://foo.tld.com/");
> > +    gTestUnfiled = (yield bm.insert({type: bm.TYPE_FOLDER, title: "test folder", parentGuid: bm.unfiledGuid})).guid;
> 
> For coding style we have a soft limit of 80 chars before wrapping, that
> means in general you should reindent code to try staying below 80 chars.
> In this case for example you could indent parentGuid and title below type.
> And so on.
Removed the trailing spaces. However when I attempted to indent the code by pressing the Enter key and tabbing up to the point where title and parentGuid were underneath type, running `./mach eslint toolkit/components/places/tests/bookmarks/test_388695.js` would give me the 'Trailing spaces' error. I guess I'm doing it wrong perhaps?

> @@ +37,2 @@
> >      
> > +    yield bm.update({guid: gItemId1, title: "modified"});
> 
> itemId1 is already the most recent bookmark, so this doesn't make much
> sense, you are updating the already most recent bookmark, that won't change
> the order of bookmarks returned by fetch.
> You should instead update the LEAST recent bookmark, so that the order of
> bookmarks returned by fetch will be inverted from this point on.

Tried updating the LEAST recent bookmark. Got an error:
 user@trojan:~/mozilla-central$ ./mach test toolkit/components/places/tests/bookmarks/test_388695.js
 0:00.35 LOG: MainThread WARNING MOZ_NODE_PATH environment variable not set. Tests requiring http/2 will fail.
 0:00.35 LOG: MainThread INFO Using at most 6 threads.
 0:00.35 SUITE_START: MainThread 1
 0:00.36 TEST_START: Thread-1 toolkit/components/places/tests/bookmarks/test_388695.js
 0:00.36 LOG: Thread-1 INFO toolkit/components/places/tests/bookmarks/test_388695.js | full command: ['/home/user/mozilla-central/obj-x86_64-pc-linux-gnu/dist/bin/xpcshell', '-g', '/home/user/mozilla-central/obj-x86_64-pc-linux-gnu/dist/bin', '-a', '/home/user/mozilla-central/obj-x86_64-pc-linux-gnu/dist/bin', '-r', '/home/user/mozilla-central/obj-x86_64-pc-linux-gnu/dist/bin/components/httpd.manifest', '-m', '-s', '-e', 'const _HEAD_JS_PATH = "/home/user/mozilla-central/testing/xpcshell/head.js";', '-e', 'const _MOZINFO_JS_PATH = "/home/user/mozilla-central/obj-x86_64-pc-linux-gnu/temp/xpc-profile-ZzZwdH/mozinfo.json";', '-e', 'const _TESTING_MODULES_DIR = "/home/user/mozilla-central/obj-x86_64-pc-linux-gnu/_tests/modules/";', '-f', '/home/user/mozilla-central/testing/xpcshell/head.js', '-p', '/home/user/mozilla-central/obj-x86_64-pc-linux-gnu/temp/xpc-plugins-LwuLNX', '-e', 'const _SERVER_ADDR = "localhost"', '-e', u'const _HEAD_FILES = ["/home/user/mozilla-central/obj-x86_64-pc-linux-gnu/_tests/xpcshell/toolkit/components/places/tests/bookmarks/head_bookmarks.js"];', '-e', 'const _JSDEBUGGER_PORT = 0;', '-e', u'const _TEST_FILE = ["/home/user/mozilla-central/obj-x86_64-pc-linux-gnu/_tests/xpcshell/toolkit/components/places/tests/bookmarks/test_388695.js"];', '-e', u'const _TEST_NAME = "toolkit/components/places/tests/bookmarks/test_388695.js"', '-e', '_execute_test(); quit(0);']
 0:00.36 LOG: Thread-1 INFO toolkit/components/places/tests/bookmarks/test_388695.js | current directory: u'/home/user/mozilla-central/obj-x86_64-pc-linux-gnu/_tests/xpcshell/toolkit/components/places/tests/bookmarks'
 0:00.36 LOG: Thread-1 INFO toolkit/components/places/tests/bookmarks/test_388695.js | environment: ['XPCOM_DEBUG_BREAK=stack-and-abort', 'MOZ_CRASHREPORTER=1', 'LD_LIBRARY_PATH=/home/user/mozilla-central/obj-x86_64-pc-linux-gnu/dist/bin', 'MOZ_DISABLE_NONLOCAL_CONNECTIONS=1', 'XPCSHELL_TEST_TEMP_DIR=/home/user/mozilla-central/obj-x86_64-pc-linux-gnu/temp/xpc-other-lw2u8b', 'MOZ_CRASHREPORTER_NO_REPORT=1', 'XPCSHELL_TEST_PROFILE_DIR=/home/user/mozilla-central/obj-x86_64-pc-linux-gnu/temp/xpc-profile-ZzZwdH']
 0:00.95 LOG: Thread-1 INFO (xpcshell/head.js) | test MAIN run_test pending (1)
 0:00.95 LOG: Thread-1 INFO (xpcshell/head.js) | test run_next_test 0 pending (2)
 0:00.95 LOG: Thread-1 INFO (xpcshell/head.js) | test MAIN run_test finished (2)
 0:00.95 LOG: Thread-1 INFO running event loop
 0:00.95 LOG: Thread-1 INFO toolkit/components/places/tests/bookmarks/test_388695.js | Starting sort_bookmark_by_relevance
 0:00.95 LOG: Thread-1 INFO (xpcshell/head.js) | test sort_bookmark_by_relevance pending (2)
 0:00.98 LOG: Thread-1 INFO (xpcshell/head.js) | test run_next_test 0 finished (2)
 0:00.99 LOG: Thread-1 INFO "CONSOLE_MESSAGE: (info) No chrome package registered for chrome://branding/locale/brand.properties"
 0:00.99 LOG: Thread-1 INFO "CONSOLE_MESSAGE: (info) No chrome package registered for chrome://branding/locale/brand.properties"
 0:01.30 TEST_STATUS: Thread-1 sort_bookmark_by_relevance PASS [sort_bookmark_by_relevance : 31] "2jAG99i-JpqO" == "2jAG99i-JpqO"
 0:01.30 TEST_STATUS: Thread-1 sort_bookmark_by_relevance PASS [sort_bookmark_by_relevance : 32] "m3UqUF0RsOLL" == "m3UqUF0RsOLL"
 0:01.32 TEST_STATUS: Thread-1 sort_bookmark_by_relevance FAIL [sort_bookmark_by_relevance : 37] "m3UqUF0RsOLL" == "2jAG99i-JpqO"
    /home/user/mozilla-central/obj-x86_64-pc-linux-gnu/_tests/xpcshell/toolkit/components/places/tests/bookmarks/test_388695.js:sort_bookmark_by_relevance:37
    _run_next_test@/home/user/mozilla-central/testing/xpcshell/head.js:1554:9
    run@/home/user/mozilla-central/testing/xpcshell/head.js:703:9
    _do_main@/home/user/mozilla-central/testing/xpcshell/head.js:211:5
    _execute_test@/home/user/mozilla-central/testing/xpcshell/head.js:546:5
    @-e:1:1
 0:01.32 LOG: Thread-1 INFO exiting test
 0:01.32 LOG: Thread-1 ERROR Unexpected exception 2147500036
undefined
 0:01.32 LOG: Thread-1 INFO exiting test
 0:01.56 TEST_END: Thread-1 Harness FAIL. Subtests passed 2/3. Unexpected 0
 0:01.57 LOG: Thread-1 INFO toolkit/components/places/tests/bookmarks/test_388695.js failed or timed out, will retry.
 0:01.61 LOG: MainThread INFO Retrying tests that failed when run in parallel.
 0:01.62 TEST_START: Thread-3 toolkit/components/places/tests/bookmarks/test_388695.js
 0:01.62 LOG: Thread-3 INFO toolkit/components/places/tests/bookmarks/test_388695.js | full command: ['/home/user/mozilla-central/obj-x86_64-pc-linux-gnu/dist/bin/xpcshell', '-g', '/home/user/mozilla-central/obj-x86_64-pc-linux-gnu/dist/bin', '-a', '/home/user/mozilla-central/obj-x86_64-pc-linux-gnu/dist/bin', '-r', '/home/user/mozilla-central/obj-x86_64-pc-linux-gnu/dist/bin/components/httpd.manifest', '-m', '-s', '-e', 'const _HEAD_JS_PATH = "/home/user/mozilla-central/testing/xpcshell/head.js";', '-e', 'const _MOZINFO_JS_PATH = "/home/user/mozilla-central/obj-x86_64-pc-linux-gnu/temp/xpc-profile-_jlA4k/mozinfo.json";', '-e', 'const _TESTING_MODULES_DIR = "/home/user/mozilla-central/obj-x86_64-pc-linux-gnu/_tests/modules/";', '-f', '/home/user/mozilla-central/testing/xpcshell/head.js', '-p', '/home/user/mozilla-central/obj-x86_64-pc-linux-gnu/temp/xpc-plugins-eKdyyE', '-e', 'const _SERVER_ADDR = "localhost"', '-e', u'const _HEAD_FILES = ["/home/user/mozilla-central/obj-x86_64-pc-linux-gnu/_tests/xpcshell/toolkit/components/places/tests/bookmarks/head_bookmarks.js"];', '-e', 'const _JSDEBUGGER_PORT = 0;', '-e', u'const _TEST_FILE = ["/home/user/mozilla-central/obj-x86_64-pc-linux-gnu/_tests/xpcshell/toolkit/components/places/tests/bookmarks/test_388695.js"];', '-e', u'const _TEST_NAME = "toolkit/components/places/tests/bookmarks/test_388695.js"', '-e', '_execute_test(); quit(0);']
 0:01.62 LOG: Thread-3 INFO toolkit/components/places/tests/bookmarks/test_388695.js | current directory: u'/home/user/mozilla-central/obj-x86_64-pc-linux-gnu/_tests/xpcshell/toolkit/components/places/tests/bookmarks'
 0:01.62 LOG: Thread-3 INFO toolkit/components/places/tests/bookmarks/test_388695.js | environment: ['XPCSHELL_TEST_TEMP_DIR=/home/user/mozilla-central/obj-x86_64-pc-linux-gnu/temp/xpc-other-l3rbjr', 'MOZ_CRASHREPORTER=1', 'LD_LIBRARY_PATH=/home/user/mozilla-central/obj-x86_64-pc-linux-gnu/dist/bin', 'XPCSHELL_TEST_PROFILE_DIR=/home/user/mozilla-central/obj-x86_64-pc-linux-gnu/temp/xpc-profile-_jlA4k', 'MOZ_DISABLE_NONLOCAL_CONNECTIONS=1', 'XPCOM_DEBUG_BREAK=stack-and-abort', 'MOZ_CRASHREPORTER_NO_REPORT=1']
 0:02.22 LOG: Thread-3 INFO (xpcshell/head.js) | test MAIN run_test pending (1)
 0:02.22 LOG: Thread-3 INFO (xpcshell/head.js) | test run_next_test 0 pending (2)
 0:02.22 LOG: Thread-3 INFO (xpcshell/head.js) | test MAIN run_test finished (2)
 0:02.23 LOG: Thread-3 INFO running event loop
 0:02.23 LOG: Thread-3 INFO toolkit/components/places/tests/bookmarks/test_388695.js | Starting sort_bookmark_by_relevance
 0:02.23 LOG: Thread-3 INFO (xpcshell/head.js) | test sort_bookmark_by_relevance pending (2)
 0:02.25 LOG: Thread-3 INFO (xpcshell/head.js) | test run_next_test 0 finished (2)
 0:02.26 LOG: Thread-3 INFO "CONSOLE_MESSAGE: (info) No chrome package registered for chrome://branding/locale/brand.properties"
 0:02.26 LOG: Thread-3 INFO "CONSOLE_MESSAGE: (info) No chrome package registered for chrome://branding/locale/brand.properties"
 0:02.56 TEST_STATUS: Thread-3 sort_bookmark_by_relevance PASS [sort_bookmark_by_relevance : 31] "bf4FW42_HnMl" == "bf4FW42_HnMl"
 0:02.56 TEST_STATUS: Thread-3 sort_bookmark_by_relevance PASS [sort_bookmark_by_relevance : 32] "xEZ4U-rprs5T" == "xEZ4U-rprs5T"
 0:02.59 TEST_STATUS: Thread-3 sort_bookmark_by_relevance FAIL [sort_bookmark_by_relevance : 37] "xEZ4U-rprs5T" == "bf4FW42_HnMl"
    /home/user/mozilla-central/obj-x86_64-pc-linux-gnu/_tests/xpcshell/toolkit/components/places/tests/bookmarks/test_388695.js:sort_bookmark_by_relevance:37
    _run_next_test@/home/user/mozilla-central/testing/xpcshell/head.js:1554:9
    run@/home/user/mozilla-central/testing/xpcshell/head.js:703:9
    _do_main@/home/user/mozilla-central/testing/xpcshell/head.js:211:5
    _execute_test@/home/user/mozilla-central/testing/xpcshell/head.js:546:5
    @-e:1:1
 0:02.59 LOG: Thread-3 INFO exiting test
 0:02.59 LOG: Thread-3 ERROR Unexpected exception 2147500036
undefined
 0:02.59 LOG: Thread-3 INFO exiting test
 0:02.83 TEST_END: Thread-3 Harness FAIL, expected PASS. Subtests passed 2/3. Unexpected 1
xpcshell return code: 0
 0:02.84 LOG: MainThread INFO INFO | Result summary:
 0:02.84 LOG: MainThread INFO INFO | Passed: 0
 0:02.84 LOG: MainThread INFO INFO | Failed: 1
 0:02.84 LOG: MainThread INFO INFO | Todo: 0
 0:02.84 LOG: MainThread INFO INFO | Retried: 1
 0:02.84 SUITE_END: MainThread 
Summary
=======

Ran 8 tests (2 parents, 6 subtests)
Expected results: 5
Unexpected results: 3 (FAIL: 3)

Unexpected Results
==================

toolkit/components/places/tests/bookmarks/test_388695.js
--------------------------------------------------------
FAIL [Parent]
Tests were run in parallel. Try running with --sequential to make sure the failures were not caused by this.
Flags: needinfo?(mak77)
(In reply to Leni Kadali from comment #31)
> (In reply to Marco Bonardo [::mak] from comment #30)
> > Comment on attachment 8864451 [details] [diff] [review]
> > Convert xpcshell-tests in
> > toolkit/components/places/tests/bookmarks/test_388695.js to Bookmarks.jsm API
> > 
> > Review of attachment 8864451 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > The patch looks incomplete and thus not landable, please merge all the
> > patches you have into a single one, that goes from current checked-in
> > test_388695.js to the latest version you have.
> > For example, this diff is missing the "bm" variable definition. it's likely
> > you have that in a previous changeset.
> > You can merge mercurial queues changesets using hg qfold, or normal
> > changesets using hg histedit (you have to enable a mercurial extension for
> > this).
> > 
> > ::: toolkit/components/places/tests/bookmarks/test_388695.js
> > @@ +18,4 @@
> > >      let gURI;
> > >      let gItemId1;
> > >      let gItemId2;
> > > +    
> > 
> > there are various trailing spaces left around, please remove all of them.
> > You can check code using `./mach eslint toolkit/components/places/tests`
> > it should return no errors.
> > 
> > @@ +24,3 @@
> > >  
> > >      gURI = uri("http://foo.tld.com/");
> > > +    gTestUnfiled = (yield bm.insert({type: bm.TYPE_FOLDER, title: "test folder", parentGuid: bm.unfiledGuid})).guid;
> > 
> > For coding style we have a soft limit of 80 chars before wrapping, that
> > means in general you should reindent code to try staying below 80 chars.
> > In this case for example you could indent parentGuid and title below type.
> > And so on.

> Removed the trailing spaces. However when I attempted to indent the code by
> pressing the Enter key and tabbing up to the point where title and
> parentGuid were underneath type, running `./mach eslint
> toolkit/components/places/tests/bookmarks/test_388695.js` would give me the
> 'Trailing spaces' error. I guess I'm doing it wrong perhaps?

> 
> > @@ +37,2 @@
> > >      
> > > +    yield bm.update({guid: gItemId1, title: "modified"});
> > 
> > itemId1 is already the most recent bookmark, so this doesn't make much
> > sense, you are updating the already most recent bookmark, that won't change
> > the order of bookmarks returned by fetch.
> > You should instead update the LEAST recent bookmark, so that the order of
> > bookmarks returned by fetch will be inverted from this point on.
> 
> Tried updating the LEAST recent bookmark. Got an error:
Solved on IRC. Now working on merging patches. Advice needed regards the trailing spaces issue. See comment above.
Flags: needinfo?(mak77)
(In reply to Leni Kadali from comment #31)
> Removed the trailing spaces. However when I attempted to indent the code by
> pressing the Enter key and tabbing up to the point where title and
> parentGuid were underneath type, running `./mach eslint
> toolkit/components/places/tests/bookmarks/test_388695.js` would give me the
> 'Trailing spaces' error. I guess I'm doing it wrong perhaps?

trailing spaces are things like:
      property: value,___
or
  callThatFunction();_

Basically it's spaces at the end of a line that are not necessary at all.
Eslint should tell you on which line the trailing spaces are, and most editors have utils to hilight the trailing spaces for you to notice them more easily (Visual Studio Code for example has an hilight trailing spaces extension, it also has an eslint extension).
Used bookmarks.fetch to get the bookmark
Created variable modifiedTime that holds the time values for dateAdded and lastModified.
whose time value is in the past i.e. 2 hours ago.
The update within the test changes the order of the bookmarks according to which item has the most recent lastModified property e.g.
If item2 has a more recent lastModified property than item1, then the returned results should have item2 on top of item1.
Changed variable name from gTestRoot to gTestUnfiled
Changed parentGuid value from rootGuid to unfiledGuid.
Created variable modifiedTime that holds the time values for dateAdded and lastModified
Created variables bm, bms, bms1, bm1, and bm2 for the test.
File passes eslint checks without errors or warnings
Attachment #8864451 - Attachment is obsolete: true
Comment on attachment 8873683 [details] [diff] [review]
Convert xpcshell-tests in toolkit/components/places/tests/bookmarks/test_388695.js to Bookmarks.jsm API

I hope this is landable.
Flags: needinfo?(mak77)
Attachment #8873683 - Flags: review?(mak77)
Comment on attachment 8873683 [details] [diff] [review]
Convert xpcshell-tests in toolkit/components/places/tests/bookmarks/test_388695.js to Bookmarks.jsm API

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

Thanks, the patch functionality looks ok, there's just some coding style and var names issues to fix.

::: toolkit/components/places/tests/bookmarks/test_388695.js
@@ +11,5 @@
> +// test getBookmarkIdsForURI
> +// getBookmarkIdsForURI sorts by the most recently added/modified (descending)
> +//
> +// we cannot rely on dateAdded growing when doing so in a simple iteration,
> +// see PR_Now() documentation

This whole comment is referring to getBookmarkIdsForURI, but now we are instead testing Bookmarks.fetch.

I think it should just be replaced with:

// Test that Bookmarks fetch properly orders its results based on
// the last modified value. Note we cannot rely on dateAdded due to
// the low PR_Now() resolution.

@@ +16,5 @@
> +add_task(function* sort_bookmark_by_relevance() {
> +    let gTestUnfiled;
> +    let gURI;
> +    let gItemId1;
> +    let gItemId2;

These variables are not in the global scope, thus the "g" prefix doesn't make sense anymore... just rename them removing "g" and lowercasing the first letter (for example gTestUnfiled => testUnfiled).

It would also be better to just declare them on first use, rather than all at the top.

@@ +23,2 @@
>  
> +    gURI = uri("http://foo.tld.com/");

There's no need to build an nsIURI anymore (through the uri call) and renaming gURI to uri would be confusing (it would shadow the existing uri). Thus I'd suggest replacing this with:
let url = "http://foo.tld.com/";
and then just pass url to the various API calls below

@@ +23,4 @@
>  
> +    gURI = uri("http://foo.tld.com/");
> +    gTestUnfiled = (yield bm.insert({type: bm.TYPE_FOLDER,
> +                                    title: "test folder", parentGuid: bm.unfiledGuid})).guid;

let's name this variable "let parentGuid" instead, and later you can just pass parentGuid to the API calls.

@@ +24,5 @@
> +    gURI = uri("http://foo.tld.com/");
> +    gTestUnfiled = (yield bm.insert({type: bm.TYPE_FOLDER,
> +                                    title: "test folder", parentGuid: bm.unfiledGuid})).guid;
> +    gItemId1 = (yield bm.insert({type: bm.TYPE_BOOKMARK, url: gURI,
> +                                parentGuid: gTestUnfiled})).guid;

type: bm.TYPE_BOOKMARK is not necessary, since you are passing a url the API already knows it's a bookmarked url.

@@ +26,5 @@
> +                                    title: "test folder", parentGuid: bm.unfiledGuid})).guid;
> +    gItemId1 = (yield bm.insert({type: bm.TYPE_BOOKMARK, url: gURI,
> +                                parentGuid: gTestUnfiled})).guid;
> +    gItemId2 = (yield bm.insert({type: bm.TYPE_BOOKMARK, url: gURI,
> +                                parentGuid: gTestUnfiled, dateAdded: modifiedTime, lastModified: modifiedTime})).guid;

and these 2 should be named item1Guid and item2Guid.

also please indent so that properties are aligned:
...insert({ url: ...,
            parentGuid: ...,
          })...
Attachment #8873683 - Flags: review?(mak77)
Flags: needinfo?(mak77)
Updated comment referring to using Bookmarks fetch instead of getBookmarkIdsForURI
Declared variables on first use instead of all of them at the beginnning of the
function's code.
Renamed the variables, removing the 'g' prefix and lowercasing the first letter.
Replaced variable `gURI` with `url`
Replaced variable `gTestUnfiled` with `parentGuid`
Removed `type: bm.TYPE_BOOKMARK` since it is not necessary, as we are passing a url the API already knows
is a bookmarked url.
Attachment #8873683 - Attachment is obsolete: true
Updated comment referring to using Bookmarks fetch instead of getBookmarkIdsForURI
Declared variables on first use instead of all of them at the beginnning of the
function's code.
Renamed the variables, removing the 'g' prefix and lowercasing the first letter.
Replaced variable `gURI` with `url`
Replaced variable `gTestUnfiled` with `parentGuid`
Replaced variables `gItemId1` and `gItemId2` with `item1Guid` and `item2Guid` respectively.
Removed `type: bm.TYPE_BOOKMARK` since it is not necessary, as we are passing a url the API already knows
is a bookmarked url.
Attachment #8874799 - Attachment is obsolete: true
please, merge the patches.
Attachment #8874800 - Flags: review?(mak77)
(In reply to Marco Bonardo [::mak] from comment #39)
> please, merge the patches.

Sorry, I forgot to substitute one of the gItemId* variables with the new name.
Comment on attachment 8874800 [details] [diff] [review]
Convert xpcshell-tests in toolkit/components/places/tests/bookmarks/test_388695.js to Bookmarks.jsm API

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

this is still not merged, for reviewing I need a single patch that goes from the current mozilla-central to the final version. This is just a patch on top of a previous one

::: toolkit/components/places/tests/bookmarks/test_388695.js
@@ +14,1 @@
>  add_task(function* sort_bookmark_by_relevance() {

Should be "async function" instead of "function *"

@@ +15,5 @@
>      let now = new Date();
>      let modifiedTime = new Date(now.setHours(now.getHours() - 2));
>  
> +    let url = "http://foo.tld.com/";
> +    let parentGuid = (yield bm.insert({type: bm.TYPE_FOLDER,

all the "yield" should be changed to "await", everywhere
Attachment #8874800 - Flags: review?(mak77) → review-
Updated all of the old calls to the new API
Replaced the keyword `var` with `let`
Used bookmarks.fetch to get the bookmark instead of bookmarks.search
Created variable modifiedTime that holds the time values for dateAdded and lastModified.
whose time value is in the past i.e. 2 hours ago.
The update within the test changes the order of the bookmarks according to which item has the most recent lastModified property e.g.
If item2 has a more recent lastModified property than item1, then the returned results should have item2 on top of item1.
Changed variable name from gTestRoot to gTestUnfiled
Changed parentGuid value from rootGuid to unfiledGuid.
Created variable modifiedTime that holds the time values for dateAdded and lastModified
Created variables bm, bms, bms1, bm1, and bm2 for the test.

Added a comment referring to using Bookmarks fetch instead of getBookmarksIdForURI
Declared variables on first use instead of all of them at the beginnning of the
function's code.
Renamed the variables, removing the 'g' prefix and lowercasing the first letter.
Replaced variable `gURI` with `url`
Replaced variable `gTestUnfiled` with `parentGuid`
Replaced variables `gItemId1` and `gItemId2` with `item1Guid` and `item2Guid` respectively.
Removed `type: bm.TYPE_BOOKMARK` since it is not necessary, as we are passing a url the API already knows
is a bookmarked url.
Indented properties so that they are aligned
Replaced `function *` with `async function`
Replaced `yield` with `await`
Attachment #8874800 - Attachment is obsolete: true
Comment on attachment 8874812 [details] [diff] [review]
Convert xpcshell-tests in toolkit/components/places/tests/bookmarks/test_388695.js to Bookmarks.jsm API

I hope this is landable and that the comments aren't overkill, since I'm trying to provide a detailed history for the commit.
Attachment #8874812 - Flags: review?(mak77)
Comment on attachment 8874812 [details] [diff] [review]
Convert xpcshell-tests in toolkit/components/places/tests/bookmarks/test_388695.js to Bookmarks.jsm API

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

The multi-line commit message is exagerated, it should be useful to understand why things were done a certain way. in this case it's not useful since we are just updating a test. Please reduce it to just the commented out headers plus:
Bug 1275145 - Convert xpcshell-tests in toolkit/components/places/tests/bookmarks/test_388695.js to Bookmarks.jsm API. r=mak

The other stuff has been fixed properly, so we should be fine at the next iteration, thanks!

::: toolkit/components/places/tests/bookmarks/test_388695.js
@@ +14,3 @@
>  
> +add_task(async function sort_bookmark_by_relevance() {
> +    

this empty newline with trailing spaces should be removed

@@ +19,4 @@
>  
> +    let url = "http://foo.tld.com/";
> +    let parentGuid = (await bm.insert({type: bm.TYPE_FOLDER,
> +                                       title: "test folder", parentGuid: bm.unfiledGuid})).guid;

parentGuid should also be indented to be aligned with type and title

@@ +22,5 @@
> +                                       title: "test folder", parentGuid: bm.unfiledGuid})).guid;
> +    let item1Guid = (await bm.insert({ url,
> +                                   parentGuid})).guid;
> +    let item2Guid = (await bm.insert({ url,
> +                                   parentGuid, dateAdded: modifiedTime, lastModified: modifiedTime})).guid;

please indent properties in the object so that they are aligned, to improve readability.

@@ +23,5 @@
> +    let item1Guid = (await bm.insert({ url,
> +                                   parentGuid})).guid;
> +    let item2Guid = (await bm.insert({ url,
> +                                   parentGuid, dateAdded: modifiedTime, lastModified: modifiedTime})).guid;
> +                                

lots of trailing spaces to be removed here
Attachment #8874812 - Flags: review?(mak77) → feedback+
Used bookmarks.fetch to get the bookmark and added corresponding comment
Created variable modifiedTime that holds the time values for dateAdded and lastModified.
whose time value is in the past i.e. 2 hours ago.
The update within the test changes the order of the bookmarks according to which item has the most recent lastModified property e.g.
If item2 has a more recent lastModified property than item1, then the returned results should have item2 on top of item1.

Added a comment referring to using Bookmarks fetch instead of getBookmarksIdForURI
Declared variables on first use instead of all of them at the beginnning of the
function's code.
Replaced variable `gURI` with `url`
Replaced variable `gTestUnfiled` with `parentGuid`
Replaced variables `gItemId1` and `gItemId2` with `item1Guid` and `item2Guid` respectively.
Removed `type: bm.TYPE_BOOKMARK` since it is not necessary, as we are passing a url the API already knows
is a bookmarked url.
Indented properties so that they are aligned
Replaced `function *` with `async function`
Replaced `yield` with `await`
Removed trailing spaces. File passes eslint checks.
Attachment #8874812 - Attachment is obsolete: true
Comment on attachment 8875549 [details] [diff] [review]
Convert xpcshell-tests in toolkit/components/places/tests/bookmarks/test_388695.js to Bookmarks.jsm API

I hope the commit --amend worked. As far as I could tell, there were no trailing spaces at the item1Guid line, or if there are, eslint doesn't detect them.
Flags: needinfo?(mak77)
Attachment #8875549 - Flags: review?(mak77)
Comment on attachment 8875549 [details] [diff] [review]
Convert xpcshell-tests in toolkit/components/places/tests/bookmarks/test_388695.js to Bookmarks.jsm API

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

there is one indentation problem yet, but I'll fix it on push.
Attachment #8875549 - Flags: review?(mak77) → review+
Pushed by mak77@bonardo.net:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d4a231038b54
Convert xpcshell-tests in toolkit/components/places/tests/bookmarks/test_388695.js to Bookmarks.jsm API r=mak
https://hg.mozilla.org/mozilla-central/rev/d4a231038b54
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Flags: needinfo?(mak77)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: