Closed Bug 334050 Opened 14 years ago Closed 5 years ago

Add a test for queries against multiple folders

Categories

(Firefox :: Bookmarks & History, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 36

People

(Reporter: bugs, Assigned: akshendra521994, Mentored)

References

Details

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

Attachments

(2 files, 4 obsolete files)

I have four livemarks, A, B, C and D.

I construct a query like so:

place:&annotation=livemark/bookmarkFeedURI&folder=A&folder=B&folder=D&sort=1&maxResults=20

I expect to see the contents of A, B & C, sorted by title, capped at 20 results.

What I see instead are the contents of D, sorted by title, capped at 20 results!
Priority: -- → P2
Priority: P2 → P3
Target Milestone: Firefox 2 alpha2 → Firefox 3 alpha2
Assignee: brettw → nobody
Target Milestone: Firefox 3 alpha2 → Firefox 3
need to write a collapsed test to see if this is still valid.
Summary: query produces incorrect results → queries against multiple folders produce incorrect results
Target Milestone: Firefox 3 → Firefox 3.1
Blocks: 437245
writing the test could be a good first bug, if additional issues should come up we could address them after the test.
Whiteboard: [good first bug]
Target Milestone: Firefox 3.1 → Firefox 3.2a1
Bug 451915 - move Firefox/Places bugs to Firefox/Bookmarks and History. Remove all bugspam from this move by filtering for the string "places-to-b-and-h".

In Thunderbird 3.0b, you do that as follows:
Tools | Message Filters
Make sure the correct account is selected. Click "New"
Conditions: Body   contains   places-to-b-and-h
Change the action to "Delete Message".
Select "Manually Run" from the dropdown at the top.
Click OK.

Select the filter in the list, make sure "Inbox" is selected at the bottom, and click "Run Now". This should delete all the bugspam. You can then delete the filter.

Gerv
Component: Places → Bookmarks & History
QA Contact: places → bookmarks
mak, is this a bug that you might sign up to mentor? Or can you suggest another mentor? I notice it's been untouched for years. Maybe making it a mentored bug would encourage someone to try it out.
Flags: needinfo?(mak77)
sure, no problem.
Flags: needinfo?(mak77)
Whiteboard: [good first bug] → [good-first-bug][mentor=mak][lang=js]
i would love to work on this work mak, please assign it to me
Whiteboard: [good-first-bug][mentor=mak][lang=js] → [good first bug][mentor=mak][lang=js]
Flags: needinfo?(nobody)
Flags: needinfo?(nobody)
is this bug still open, i want to work on it as my first bug ,need help :)
hi, the scope here is to write a test that:
- adds 3 folders and a bookmark into each of them
- queries multiple folders, for example: place:folder=A&folder=B&folder=C&sort=1&maxResults=20
- check what is returned (based on that we'll take a direction)

the test should be an xpcshell-test (https://developer.mozilla.org/en-US/docs/Writing_xpcshell-based_unit_tests) added to the /toolkit/components/places/tests/queries folder.
While not extremely up-to-date, this documentation should help: https://developer.mozilla.org/en-US/docs/Querying_Places

There are lots of tests in toolkit/components/places/tests/ you can use for inspiration, even if I suggest looking just at the ones written in the last 6-12 months (to avoid using old code paths)
Assignee: nobody → karthik.188
Status: NEW → ASSIGNED
Priority: P3 → --
Target Milestone: Firefox 3.6a1 → ---
Hi, Karthik - Are you still working on this?
Flags: needinfo?(karthik.188)
hey Mike, 
I'm stuck up with exams, you can work on it
Flags: needinfo?(karthik.188)
Mentor: mak77
Whiteboard: [good first bug][mentor=mak][lang=js] → [good first bug][lang=js]
Can I work on this?
Flags: needinfo?(mak77)
Hi, Akshendra - You certainly can. Because we generally assign bugs to new contributors after we receive their first patch, the thing to do now is get your development environment set up. 

There's some good documentation here: https://developer.mozilla.org/en-US/docs/Introduction

Thanks, and good luck! Please let me know if you've got any more questions.
Flags: needinfo?(mak77)
Comment 8 should have everything needed to start here.
The test should go into toolkit/components/places/tests/queries/
Assignee: karthik.188 → nobody
I am trying to use queryStringtoQueries in
> let query = {};
> let options = {};
> let queryCount = {};
> htsvc.queryStringToQueries("place:folder=" + folderA + "&folder=" + folderB, query, queryCount, 
> options);
> let result = htsvc.executeQuery(query.value[0],options.value);

with folderA , folderB the results createFolder() of bookmarkServices. It works if I use single folder but with multiple folders the test crashes. Any help?

This might be the relevant part of log.
> ASSERTION: The statement 'SELECT h.id, h.url, h.title AS page_title, h.rev_host, h.visit_count, 
> h.last_visit_date, f.url, null, null, null, null, null AS tags , h.frecency, h.hidden, h.guid FROM 
> moz_places h LEFT JOIN moz_favicons f ON h.favicon_id = f.id WHERE 1  AND hidden = 0 AND 
> last_visit_date NOTNULL   AND (( b.parent IN(  6  ,  7  ) ))  ORDER BY h.id ASC ' failed to compile 
> with the error message 'no such column: b.parent'.: 'Error', file ../../../dist/include/mozilla
> /storage/StatementCache.h, line 124
Flags: needinfo?(mak77)
Looks like you hit a bug already!

that query cannot work cause b.parent is not selected (should be instead of those NULLs). Off-hand this seems to be due to taking the wrong branch in this switch:

http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/nsNavHistory.cpp#1479

that basically means mQueryType is nsINavHistoryQueryOptions::QUERY_TYPE_HISTORY... while it should probably be nsINavHistoryQueryOptions::QUERY_TYPE_BOOKMARKS

I think a possible way to fix this, might be to change the query type every time SetFolders is invoked on the query object

http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/nsNavHistoryQuery.cpp#1295

here should basically do mQueryType = QUERY_TYPE_BOOKMARKS;

Let me know if you are not comfortable with cpp code, you can just pick something else from the mentored bugs list.

Once that is done, you should try again and see if we are now taking the right path and executing a meaningful query.

Note that additionally from your method to build a query, you should also test the alternative API method:

let query = PlacesUtils.history.getNewQuery();
let options = PlacesUtils.history.getNewQueryOptions();
query.setFolders([2, 3], 2); // just an example
let root = PlacesUtils.history.executeQuery(query, options).root;
Flags: needinfo?(mak77)
I did the changes in the cpp file, that made it work.
The results for both the cases are same ie
> 0:16.46 TEST_STATUS: Thread-1 test_multiplePlaces PASS [test_multiplePlaces : > 50] http://abookmarkc1.com/ - true == true
> 0:16.46 TEST_STATUS: Thread-1 test_multiplePlaces PASS [test_multiplePlaces : > 50] http://abookmarkc2.com/ - true == true
> 0:16.47 TEST_STATUS: Thread-1 test_multiplePlaces PASS [test_multiplePlaces : > 50] http://abookmarkc3.com/ - true == true
> 0:16.47 TEST_STATUS: Thread-1 test_multiplePlaces PASS [test_multiplePlaces : > 50] http://abookmarkc4.com/ - true == true
> 0:16.48 TEST_STATUS: Thread-1 test_multiplePlaces PASS [test_multiplePlaces : > 50] http://abookmarkc5.com/ - true == true
> 0:16.48 TEST_STATUS: Thread-1 test_multiplePlaces PASS [test_multiplePlaces : > 50] http://abookmarkc6.com/ - true == true
> 0:16.49 TEST_STATUS: Thread-1 test_multiplePlaces PASS [test_multiplePlaces : > 50] http://kbookmarka1.com/ - true == true
> 0:16.49 TEST_STATUS: Thread-1 test_multiplePlaces PASS [test_multiplePlaces : > 50] http://kbookmarka2.com/ - true == true
> 0:16.50 TEST_STATUS: Thread-1 test_multiplePlaces PASS [test_multiplePlaces : > 50] http://kbookmarka3.com/ - true == true
> 0:16.50 TEST_STATUS: Thread-1 test_multiplePlaces PASS [test_multiplePlaces : > 50] http://kbookmarka4.com/ - true == true
> 0:16.51 TEST_STATUS: Thread-1 test_multiplePlaces PASS [test_multiplePlaces : > 50] http://kbookmarka5.com/ - true == true
> 0:16.51 TEST_STATUS: Thread-1 test_multiplePlaces PASS [test_multiplePlaces : > 50] http://kbookmarka6.com/ - true == true
> 0:16.52 TEST_STATUS: Thread-1 test_multiplePlaces PASS [test_multiplePlaces : > 50] http://kbookmarka7.com/ - true == true
> 0:16.52 TEST_STATUS: Thread-1 test_multiplePlaces PASS [test_multiplePlaces : > 50] http://wbookmarkb1.com/ - true == true
> 0:16.52 TEST_STATUS: Thread-1 test_multiplePlaces PASS [test_multiplePlaces : > 50] http://wbookmarkb2.com/ - true == true
> 0:16.52 TEST_STATUS: Thread-1 test_multiplePlaces PASS [test_multiplePlaces : > 50] http://wbookmarkb3.com/ - true == true
> 0:16.53 TEST_STATUS: Thread-1 test_multiplePlaces PASS [test_multiplePlaces : > 50] http://wbookmarkb4.com/ - true == true
> 0:16.53 TEST_STATUS: Thread-1 test_multiplePlaces PASS [test_multiplePlaces : > 50] http://wbookmarkb5.com/ - true == true
> 0:16.54 TEST_STATUS: Thread-1 test_multiplePlaces PASS [test_multiplePlaces : > 50] http://wbookmarkb6.com/ - true == true
> 0:16.54 TEST_STATUS: Thread-1 test_multiplePlaces PASS [test_multiplePlaces : > 50] http://wbookmarkb7.com/ - true == true

Seems like it selects the 20 results first and then sorts them according to the uri?
Attachment #8518074 - Flags: feedback?(mak77)
Comment on attachment 8518074 [details] [diff] [review]
334050_queriesAgainstMultipleFolder.patch

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

::: toolkit/components/places/nsNavHistoryQuery.cpp
@@ +854,2 @@
>      query->SetFolders(folders.Elements(), folders.Length());
> +    aOptions->SetQueryType(nsNavHistoryQueryOptions::QUERY_TYPE_BOOKMARKS);

this fixes parsing a query string into queries, but I think it would not still work if you just setFolders using newQuery and newQueryOptions.
Indeed you had to specify options.queryType = 1; to make that work

I think you can write a more general fix by enforcing the query type in
nsNavHistory::QueryToSelectClause where we check folders.length > 0
http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/nsNavHistory.cpp#3379

then you should not need anymore to specify options.queryType = 1; (FWIW, you should have used Ci.nsINavHistoryQueryOptions.QUERY_TYPE_BOOKMARKS and not a raw value)

::: toolkit/components/places/tests/queries/test_queryMultipleFolder.js
@@ +3,5 @@
> +
> +"use strict";
> +
> +let bmsvc = Components.classes["@mozilla.org/browser/nav-bookmarks-service;1"]
> +                                .getService(Components.interfaces.nsINavBookmarksService);

Please use PlacesUtils.bookmarks.(method) accessor everywhere instead

@@ +10,5 @@
> +  run_next_test();
> +}
> +
> +add_task(function* test_multiplePlaces() {
> +  let menuFolder = bmsvc.bookmarksMenuFolder;

here you should use PlacesUtils.bookmarksMenuFolderId. that's because going through the service you are crossing xpcom boundaries (javascript to cpp) and that has some overhead (minor, but worth to avoid it).

@@ +25,5 @@
> +
> +  // adding bookmarks in the folder B
> +  for(let i = 1; i <= 7; ++i) {
> +    bmsvc.insertBookmark(folderB, uri("http://WBookmarkB" + i + ".com"), bmsvc.DEFAULT_INDEX, "");
> +  }

I think using nested loops would make this code simpler (instead of A,B,C you could just use a numeric index), you can push the ids to an array to keep them around

@@ +38,5 @@
> +    let query = {};
> +    let options = {};
> +    PlacesUtils.history.queryStringToQueries("place:folder=" + folderA +
> +                               "&folder=" + folderB +
> +                               "&folder=" + folderC +

supposing you have ids in an array, you can ids.map(id => "folder=" + id).join("&") to get a string like "folder=1&folder=2&..."

@@ +41,5 @@
> +                               "&folder=" + folderB +
> +                               "&folder=" + folderC +
> +                               "&sort=1&maxResults=20",
> +                               query, {}, options);
> +    options.queryType = 1;

you should not need to set this.

@@ +44,5 @@
> +                               query, {}, options);
> +    options.queryType = 1;
> +    let rootNode = PlacesUtils.history.executeQuery(query.value[0],options.value).root;
> +    rootNode.containerOpen = true;
> +    for (let i=0; i<rootNode.childCount; ++i) {

per coding style please add whitespaces in "i = 0" and "i < rootNode..."

@@ +46,5 @@
> +    let rootNode = PlacesUtils.history.executeQuery(query.value[0],options.value).root;
> +    rootNode.containerOpen = true;
> +    for (let i=0; i<rootNode.childCount; ++i) {
> +      let node = rootNode.getChild(i);
> +      Assert.ok(true, node.uri);

this is ok to print out the uris, but we'll actually need to test the returned order makes sense according to the sorting we defined

sort=1 as you defined is SORT_BY_TITLE_ASCENDING http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/nsINavHistoryService.idl#987
but you haven't defined any title. thus I think sorting fallbacked to URI cause titles are identical..
I think you should use  SORT_BY_URI_ASCENDING directly (sort=5)
the results look correct afaict (should be sorted by uri and then cut at 20)

@@ +49,5 @@
> +      let node = rootNode.getChild(i);
> +      Assert.ok(true, node.uri);
> +    }
> +    rootNode.containerOpen = false;
> +  } catch(ex) {

you don't need to try/catch, if the test throws it will fail automatically.

@@ +54,5 @@
> +    Assert.ok(false, ex.message);
> +  }
> +
> +  // using the query search parameters and the query configuration options
> +  try {

try is not needed... though now strict mode will complain you are redefining vars... so pay attention to that

@@ +57,5 @@
> +  // using the query search parameters and the query configuration options
> +  try {
> +    let query = PlacesUtils.history.getNewQuery();
> +    let options = PlacesUtils.history.getNewQueryOptions();
> +    options.queryType = 1;

should not be needed

@@ +59,5 @@
> +    let query = PlacesUtils.history.getNewQuery();
> +    let options = PlacesUtils.history.getNewQueryOptions();
> +    options.queryType = 1;
> +    query.setFolders([folderA, folderB, folderC], 3);
> +    options.sortingMode = 1;

please use SORT_BY_URI_ASCENDING constant from the interface
Attachment #8518074 - Flags: feedback?(mak77) → feedback+
mak: May you assign that to me please!
> this fixes parsing a query string into queries, but I think it would not
> still work if you just setFolders using newQuery and newQueryOptions.
> Indeed you had to specify options.queryType = 1; to make that work

Isn't this the idea that when one uses newQuery and newQueryOptions, they have to specify all options by hand?
(In reply to Akshendra Pratap Singh from comment #19)
> > this fixes parsing a query string into queries, but I think it would not
> > still work if you just setFolders using newQuery and newQueryOptions.
> > Indeed you had to specify options.queryType = 1; to make that work
> 
> Isn't this the idea that when one uses newQuery and newQueryOptions, they
> have to specify all options by hand?

some options can be optional, in this case since you set Folders it's pretty clear we support only a bookmarks query.
defining it regardless shouldn't hurt, but it should not be mandatory in this case.
Assignee: nobody → akshendra521994
Attachment #8518074 - Attachment is obsolete: true
Attachment #8520568 - Flags: review?(mak77)
Attached file testResult.log (obsolete) —
Comment on attachment 8520568 [details] [diff] [review]
334050_queriesAgainstMultipleFolder.patch

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

::: toolkit/components/places/tests/queries/test_queryMultipleFolder.js
@@ +6,5 @@
> +function run_test() {
> +  run_next_test();
> +}
> +
> +add_task(function* test_queryMultiplePlaces() {

s/test_queryMultiplePlaces/queryMultipleFolders/

@@ +9,5 @@
> +
> +add_task(function* test_queryMultiplePlaces() {
> +  // adding bookmarks in the folders
> +  var folderIds = [];
> +  var bookmarkIds = [];

please use "let" everywhere

@@ +29,5 @@
> +  }).join('&') + "&sort=5&maxResults=20";
> +  PlacesUtils.history.queryStringToQueries(queryString, query, {}, options);
> +  let rootNode = PlacesUtils.history.executeQuery(query.value[0],options.value).root;
> +  rootNode.containerOpen = true;
> +  for (let i = 0; i < rootNode.childCount; ++i) {

please also check childCount equals maxResults (20)

@@ +43,5 @@
> +  options.sortingMode = options.SORT_BY_URI_ASCENDING;
> +  options.maxResults = 20;
> +  rootNode = PlacesUtils.history.executeQuery(query, options).root;
> +  rootNode.containerOpen = true;
> +  for (let i=0; i<rootNode.childCount; ++i) {

ditto

plus, please add whitespaces in i=0 and i<rootNode...
Attachment #8520568 - Flags: review?(mak77) → review+
Attachment #8520568 - Attachment is obsolete: true
Attachment #8521412 - Flags: review?(mak77)
Attached file testResult.log
Attachment #8520571 - Attachment is obsolete: true
Comment on attachment 8521412 [details] [diff] [review]
334050_queriesAgainstMultipleFolder.patch

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

Thanks, do you need help to push to the Try server?
Attachment #8521412 - Flags: review?(mak77) → review+
Yes I do not have access to that.
could you please change the commit message to

Bug 334050 - Add a test for Places queries against multiple folders. r=mak

thank you.
Summary: queries against multiple folders produce incorrect results → Add a test for queries against multiple folders
Attachment #8521412 - Attachment is obsolete: true
Attachment #8521474 - Flags: review?(mak77)
Flags: needinfo?(mak77)
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/3ebc3f27a585
Keywords: checkin-needed
Whiteboard: [good first bug][lang=js] → [good first bug][lang=js][fixed-in-fx-team]
Comment on attachment 8521474 [details] [diff] [review]
334050_queriesAgainstMultipleFolder.patch

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

no need for further reviews.
Attachment #8521474 - Flags: review?(mak77)
https://hg.mozilla.org/mozilla-central/rev/3ebc3f27a585
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Whiteboard: [good first bug][lang=js][fixed-in-fx-team] → [good first bug][lang=js]
Target Milestone: --- → Firefox 36
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.