Closed
Bug 1275145
Opened 9 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)
Toolkit
Places
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)
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
Reporter | ||
Updated•9 years ago
|
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 ?
Reporter | ||
Comment 3•9 years ago
|
||
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
Reporter | ||
Comment 5•9 years ago
|
||
Thanks for update! That's fine just follow your pace :)
I updated all of the old calls to the new api :)
Attachment #8759988 -
Flags: review?(gasolin)
Reporter | ||
Comment 7•8 years ago
|
||
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+
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 9•8 years ago
|
||
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)
Reporter | ||
Comment 10•8 years ago
|
||
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)
Updated•8 years ago
|
Priority: -- → P3
Assignee | ||
Comment 11•8 years ago
|
||
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)
Assignee | ||
Comment 12•8 years ago
|
||
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)
Assignee | ||
Comment 13•8 years ago
|
||
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)
Reporter | ||
Comment 14•8 years ago
|
||
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)
Reporter | ||
Updated•8 years ago
|
Attachment #8856133 -
Flags: review?(gasolin)
Assignee | ||
Comment 15•8 years ago
|
||
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
Assignee | ||
Comment 16•8 years ago
|
||
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)
Assignee | ||
Comment 18•8 years ago
|
||
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)
Comment 19•8 years ago
|
||
(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)
Assignee | ||
Comment 20•8 years ago
|
||
Sample custom lastModified property
Assignee | ||
Comment 21•8 years ago
|
||
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
Assignee | ||
Comment 22•8 years ago
|
||
I forgot to add the needinfo and review flags for this. See attachment and comment above.
Flags: needinfo?(mak77)
Comment 23•8 years ago
|
||
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.
Updated•8 years ago
|
Flags: needinfo?(mak77)
Assignee | ||
Comment 24•8 years ago
|
||
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)
Comment 25•8 years ago
|
||
(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)
Assignee | ||
Comment 26•8 years ago
|
||
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
Assignee | ||
Updated•8 years ago
|
Attachment #8860630 -
Attachment is obsolete: true
Assignee | ||
Updated•8 years ago
|
Attachment #8863166 -
Flags: review?(mak77)
Comment 27•8 years ago
|
||
(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.
Updated•8 years ago
|
Attachment #8863166 -
Flags: review?(mak77)
Assignee | ||
Comment 28•8 years ago
|
||
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.
Assignee | ||
Updated•8 years ago
|
Attachment #8863166 -
Attachment is obsolete: true
Assignee | ||
Comment 29•8 years ago
|
||
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)
Updated•8 years ago
|
Attachment #8760760 -
Attachment is obsolete: true
Updated•8 years ago
|
Attachment #8856133 -
Attachment is obsolete: true
Updated•8 years ago
|
Attachment #8856134 -
Attachment is obsolete: true
Updated•8 years ago
|
Attachment #8857412 -
Attachment is obsolete: true
Comment 30•8 years ago
|
||
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)
Assignee | ||
Comment 31•8 years ago
|
||
(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)
Assignee | ||
Comment 32•8 years ago
|
||
(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)
Comment 33•8 years ago
|
||
(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).
Assignee | ||
Comment 34•7 years ago
|
||
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
Assignee | ||
Updated•7 years ago
|
Attachment #8864451 -
Attachment is obsolete: true
Assignee | ||
Comment 35•7 years ago
|
||
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 36•7 years ago
|
||
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)
Updated•7 years ago
|
Flags: needinfo?(mak77)
Assignee | ||
Comment 37•7 years ago
|
||
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.
Assignee | ||
Updated•7 years ago
|
Attachment #8873683 -
Attachment is obsolete: true
Assignee | ||
Comment 38•7 years ago
|
||
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.
Assignee | ||
Updated•7 years ago
|
Attachment #8874799 -
Attachment is obsolete: true
Comment 39•7 years ago
|
||
please, merge the patches.
Assignee | ||
Updated•7 years ago
|
Attachment #8874800 -
Flags: review?(mak77)
Assignee | ||
Comment 40•7 years ago
|
||
(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 41•7 years ago
|
||
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-
Assignee | ||
Comment 42•7 years ago
|
||
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`
Assignee | ||
Updated•7 years ago
|
Attachment #8874800 -
Attachment is obsolete: true
Assignee | ||
Comment 43•7 years ago
|
||
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 44•7 years ago
|
||
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+
Assignee | ||
Comment 45•7 years ago
|
||
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.
Assignee | ||
Updated•7 years ago
|
Attachment #8874812 -
Attachment is obsolete: true
Assignee | ||
Comment 46•7 years ago
|
||
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 47•7 years ago
|
||
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+
Comment 48•7 years ago
|
||
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
Comment 49•7 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox55:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Updated•7 years ago
|
Flags: needinfo?(mak77)
You need to log in
before you can comment on or make changes to this bug.
Description
•