384226 - comprehensive tests for complex places queries

Status: RESOLVED FIXED

(Firefox :: Bookmarks & History, defect, P4)

Description

[Asaf Romano (gone)]

Spun off from bug 383803.

We have almost no tests for complex queries (i.e. queryresultnode).

Comment 1

[Mike Connor [:mconnor]]

Not going to continue to block on this.  If you disagree with this decision, please renominate with reasons why we can't ship with this in final

Comment 2

[cmtalbert]

Asaf, Dietrich:
Can you be more specific about what is needed for this?  Large query sets? Complicated queries? Character encoding issues (in page titles)? 

 

Comment 3

[Dietrich Ayala (:dietrich)]

Sure, all of the above. Also:

- exercise all api options and combinations for queries that are not simple bookmarks folder queries.

- in each query, test that live-update works as expected (eg: addition, modification and removal of items that are in the query results)

Comment 4

[cmtalbert]

Attachment: The infrastructure and the first places Query API test

This is the first places query API and the infrastructure to support future tests.  Dietrich, if you could look at this and give me some feedback that would be great.  Since I got behind due to minotaur, I intend to do some more of these over the weekend.

= Implementation Notes =
I don't intend this test to check the order, but others will definitely have to.  I decided to make the "add visit" and the historySvc part of the head file, since those will be used by all tests, even though that appears to be against convention given the tests in unit/  This test verifies that the query parameters work as expected in limiting the data returned from a fairly complex query.  It also does a very simple live update check.

Comment 5

[Marco Bonardo [:mak]]

transition download could become like transition_embed, so not counted in visit_count since it's not really a visit. So the bahaviour of that could change (probably visits will be NOT IN(0(invalid),4(embed),7(download)). see Bug 412217

imho maxVisits could become invalid since it rely on visit_count (i've not checked minVisits but i suppose that's the same), and visit_count is not realiable at the moment (see Bug 416313) we could replace maxVisits with the real count, but that would be slow. i think that maxVisits will become invalid after a clear private data, and after one of the removePages function (that do removal in batch mode).

Comment 6

[cmtalbert]

(In reply to comment #5)
Thanks for the comments.
> transition download could become like transition_embed, so not counted in
> visit_count since it's not really a visit. So the bahaviour of that could
> change (probably visits will be NOT IN(0(invalid),4(embed),7(download)). see
> Bug 412217
> 
I'll update the test to not use download transitions in when it generates content in the database.
> imho maxVisits could become invalid since it rely on visit_count (i've not
> checked minVisits but i suppose that's the same), and visit_count is not
> realiable at the moment (see Bug 416313) we could replace maxVisits with the
> real count, but that would be slow. i think that maxVisits will become invalid
> after a clear private data, and after one of the removePages function (that do
> removal in batch mode).
This is a valid concern, but the test is not interactive and so there is no way to do a clear private data or a batch delete on the history file.  Due to the clearDB calls in head_queries.js and in tail_queries.js, the only items in the database are the items we put into the database using the buildTestDatabase function.  So, while your concern is valid, I don't think it applies to this test (unless I am misunderstanding). 

For large database testing of databases that I can't completely control the content of, this will certainly have to be a consideration.

Comment 7

[Marco Bonardo [:mak]]

(In reply to comment #6)
> This is a valid concern, but the test is not interactive and so there is no way
> to do a clear private data or a batch delete on the history file.  Due to the
> clearDB calls in head_queries.js and in tail_queries.js, the only items in the
> database are the items we put into the database using the buildTestDatabase
> function.  So, while your concern is valid, I don't think it applies to this
> test (unless I am misunderstanding). 

no it does not apply ACTUALLY, but in a real world we should have a test that do count queries after a cpd and a batch delete, and that test should pass, you can't rely on a test that does not take in count all api removal calls... this is not blocking actually, but after fixing visit_count we should test also that cases (maybe with a specific test for visit_count, not necessarily here)

Comment 8

[cmtalbert]

(In reply to comment #7)
> no it does not apply ACTUALLY, but in a real world we should have a test that
> do count queries after a cpd and a batch delete, and that test should pass, you
> can't rely on a test that does not take in count all api removal calls... this
> is not blocking actually, but after fixing visit_count we should test also that
> cases (maybe with a specific test for visit_count, not necessarily here)
> 
Yes.  I definitely agree with you that we need tests like that.  I'll see if I can find a way to add tests like this to my list of things to create.  

Comment 9

[cmtalbert]

Attachment: Full first set of tests to cover major query functionality for history

This is a new set of tests that test combinations of queries using relative time, searchterms, minvists, maxvisits, uri, domain, and annotation query parameters for history queries.  Note that the domain and uri tests will fail on run due to bug 419749.

Comment 10

[Dietrich Ayala (:dietrich)]

Comment on attachment 305881 [details] [diff] [review]
Full first set of tests to cover major query functionality for history

the tests themselves look mostly ok. however, i'm a little worried about the overall manageability of the test suite: lots of duplication of data and code. most of my suggestions below are about this.

also, please separate the failing test that you noted, and attach it to that bug, so that this bug can move forward independently.

general nit: please keep line length at 80 chars, really helps readability.

>+ * Software distributed under the License is distributed on an "AS IS" basis,
>+ * WITHOUT WARRANTY OF ANY KIND, either express or implied. See the License
>+ * for the specific language governing rights and limitations under the
>+ * License.
>+ *
>+ * The Original Code is mozilla.org code.

should probably be something like Places Test Code

ditto throughout

>+const DAY_MSEC = 86400000;
>+var today = Date.now();
>+var yesterday = today - DAY_MSEC;
>+var lastweek = today - (DAY_MSEC * 7);
>+var tomorrow = today + DAY_MSEC;
>+var old = today - (DAY_MSEC * 3);
>+var futureDay = today + (DAY_MSEC * 3);

afaict, you're only interacting w/ microseconds below. it would be less error-prone to convert everything to microseconds here, instead of adding "*1000" at the point of each api call.

also these are used in enough of the tests that they should be in the header file.

>+
>+/**
>+ * Builds a test database by hand using various search strings, domains
>+ */
>+function buildTestDatabase() {

you repeat this in most of these test files. however, as is being discussed in bug 419749, setPageDetails might go away, which would require much search and replace work here.

also, you're setting up the same or similar datasets in some of the tests.

so it might make this more manageable to do 2 things:

1. make a function that takes an array of data and iterates over it, calling these apis, eg:

function populateDB(aMyArray) {
  aMyArray.forEach(function(data) {
    add_visit(...);
    setPageDetails(...);
  });
}

myTestData = [
  {uri: "", title: "", numVisits: 23}
];
populateDB(myTestData);

and put it in the test header file. this way, if we make api changes, you can update this code in one location, instead of in 273 different places.

2. move the dataset arrays into the test header file so they can be re-used across the tests. (maybe this isn't useful, up to you)

also, you could also encapsulate the verification stage:

function verifyQueryResults(aListOfURLs) {
  do_check_eq(result.root.childCount, aListofURLs.length);
  // ... iterate over the url list, checking each
}

cost->benefit might not pan out in this example though, since you do things like counting of URI occurrences in result sets sometimes.

>+
>+// These should not be in the queries

why so many invalid results? if you can use less code/data to achieve a valid testcase, please do. it increases readability and decreases maintenance burden. in this particular test, you check the size and contents of the result set, which should be enough.

>+
>+  // 6 results should be returned:
>+  do_check_eq(cc, 6);
>+  dump("----> cc is: " + cc + "\n");

you've defined LOG() in the header, which'll add line endings for you, as well as prefix your output to make it easier to search for in logs. please use that throughout instead of dump().

not done yet, but there's some comments to get started on.

Comment 11

[Marco Bonardo [:mak]]

you should be able to replace SetPageDetails with setPageTitle and markPageAsTyped

Comment 12

[Ondrej Brablc]

(In reply to comment #9)
> Created an attachment (id=305881) [details]
> Full first set of tests to cover major query functionality for history
> 
> This is a new set of tests that test combinations of queries using relative
> time, searchterms, minvists, maxvisits, uri, domain, and annotation query
> parameters for history queries.  Note that the domain and uri tests will fail
> on run due to bug 419749.
> 

You are using 3 times:

options.resultType = options.RESULTS_AS_DOMAIN

But I'm afraid we have never head this result type.

Comment 13

[cmtalbert]

(In reply to comment #12)
> (In reply to comment #9)
> > Created an attachment (id=305881) [details] [details]
> > Full first set of tests to cover major query functionality for history
> > 
> > This is a new set of tests that test combinations of queries using relative
> > time, searchterms, minvists, maxvisits, uri, domain, and annotation query
> > parameters for history queries.  Note that the domain and uri tests will fail
> > on run due to bug 419749.
> > 
> 
> You are using 3 times:
> 
> options.resultType = options.RESULTS_AS_DOMAIN
> 
> But I'm afraid we have never head this result type.
> 
Yes, I noticed this too just yesterday (I think I meant RESULTS_AS_URI), and meant to ask if this was something that disappeared from the IDL or if it was merely a typo on my part? That doesn't really matter, but it was odd to find it, just the same.  

However, the larger question is: shouldn't using an invalid RESULTS_AS_* cause an error?  Or does it merely get interpreted as 0?

If it should cause an error, I'll be happy to file a bug on it.

Comment 14

[Ondrej Brablc]

(In reply to comment #13)
> Yes, I noticed this too just yesterday (I think I meant RESULTS_AS_URI), and
> meant to ask if this was something that disappeared from the IDL or if it was
> merely a typo on my part? That doesn't really matter, but it was odd to find
> it, just the same.  

We simply never had it.

> However, the larger question is: shouldn't using an invalid RESULTS_AS_* cause
> an error?  Or does it merely get interpreted as 0?
> 
> If it should cause an error, I'll be happy to file a bug on it.

It should cause an error, but we cannot fix this in places. The thing is that query is a C++ component, and when the setter for integer member is called the undefined value (of non existent property) gets changed to 0. So you can file a generic JS bug, but I'm afraid this could break a lot of code if they change the default behavior.

BTW, we do check whether the value is not higher than the biggest known value, but we have no way of checking for undefined value (or at least I do not know how).

Comment 15

[cmtalbert]

Attachment: History Queries Take 3

I took all the comments into consideration, and created a framework that insulates future query tests from changes to the interfaces while at the same time making it fairly easy to build specific databases for new query tests.

I did not use a canned database for each query test (i.e. putting the database in head_queries.js), because I think varying the database per query allows us to test specific features per query and makes for better testing.  However, I did consider each database element in each test and wrote a comment as to why it was being included in the test (for both the database elements that are in the query and those that are not).

I apologize in this taking so long to do.  I tried a couple of designs for the organization of the code until I settled on this one.  Future query tests should be much faster with this framework in place.

Comment 16

[cmtalbert]

Attachment: History Queries Take 3a

Found a typo in the earlier patch, all the same comments apply. Apologize for the bugmail.

Comment 17

[cmtalbert]

Attachment: History Queries Take 3b

Damn!  Found another problem.  Fixed now.  Apologize for all the spam.

Comment 18

[cmtalbert]

Attachment: Patch with only passing tests

This patch contains only passing tests.  The abstime* tests have one section where they test the same issue that is addressed in bug 424050, but those are commented out until that bug is resolved.

I'll attach a patch to 424050 so that removes these comments and contains an explicit test for this issue.

Comment 19

[Dietrich Ayala (:dietrich)]

Comment on attachment 311897 [details] [diff] [review]
Patch with only passing tests



>Index: toolkit/components/places/tests/queries/head_queries.js
>===================================================================

>+
>+function LOG(aMsg) {
>+ aMsg = ("*** PLACES TESTS: " + aMsg);
>+ Cc["@mozilla.org/consoleservice;1"].getService(Ci.nsIConsoleService).
>+                                     logStringMessage(aMsg);

not necessary, this is always run headless

>+ print(aMsg);
>+}
>+
>+// If there's no location registered for the profile direcotry, register one now.

spelling nit: directory

>+
>+queryData.prototype = {
>+  isVist: false,

typo: isVisit

though, this does showcase that there's not much point in these prototype defaults, since you set these defaults in the constructor anyway. maybe just remove these, excepting those that aren't set in the constructor.

>+/**
>+ * Helper function to compare an array of query objects with a result set
>+ * NOTE: It assumes the array of query objects contains the SAME SORT as the
>+ *       result set.

should note that it only checks uri and title properties

>+ */
>+function compareArrayToResult(aArray, aRoot) {
>+  LOG("Comparing Array to Results");
>+  var validResults = 0;
>+  if (!aRoot.containerOpen)
>+    aRoot.containerOpen = true;
>+  
>+  if (!aRoot.childCount) {
>+    do_throw("Empty Result Set!");
>+  }

better would be: do_check_eq(aArray.length, childCount)

it does the validResults validation up front, and also covers zero results, since having zero results could be a valid test scenario.

>+
>+/**
>+ * Helper function to check to see if one object either is or is not in the
>+ * result set.  It can accept either a queryData object or an array of queryData
>+ * objects.  If it gets an array, it only compares the first object in the array
>+ * to see if it is in the result set.

why bother supporting passing an array then?

>Index: toolkit/components/places/tests/queries/test_abstime-annotation-domain.js
>===================================================================

>+
>+  // Ensue the result set is correct
>+  compareArrayToResult(testData, root);

spelling nit: Ensure

here and in the other tests as well

r=me with comments addressed

Comment 20

[Dietrich Ayala (:dietrich)]

two final items i'd like to see included in the patch:

1. a README that either contains or links to:

 - a list of what's tested
 - what tests need to be written
 - best-practices for new tests (eg: make sure to test live update, batch update, etc)

2. a stub test, to make it super easy for others to create new ones from

Comment 21

[cmtalbert]

(In reply to comment #19)
> (From update of attachment 311897 [details] [diff] [review])
> 
> 
> >Index: toolkit/components/places/tests/queries/head_queries.js
> >===================================================================
> 
> >+
> >+function LOG(aMsg) {
> >+ aMsg = ("*** PLACES TESTS: " + aMsg);
> >+ Cc["@mozilla.org/consoleservice;1"].getService(Ci.nsIConsoleService).
> >+                                     logStringMessage(aMsg);
> 
> not necessary, this is always run headless
Removed

> >+// If there's no location registered for the profile direcotry, register one now.
> 
> spelling nit: directory
Thanks, fixed
> 
> >+
> >+queryData.prototype = {
> >+  isVist: false,
> 
> typo: isVisit
> 
> though, this does showcase that there's not much point in these prototype
> defaults, since you set these defaults in the constructor anyway. maybe just
> remove these, excepting those that aren't set in the constructor.
> 
True, removed all of these.

> >+/**
> >+ * Helper function to compare an array of query objects with a result set
> >+ * NOTE: It assumes the array of query objects contains the SAME SORT as the
> >+ *       result set.
> 
> should note that it only checks uri and title properties
Done

> 
> >+ */
> >+function compareArrayToResult(aArray, aRoot) {
> >+  LOG("Comparing Array to Results");
> >+  var validResults = 0;
> >+  if (!aRoot.containerOpen)
> >+    aRoot.containerOpen = true;
> >+  
> >+  if (!aRoot.childCount) {
> >+    do_throw("Empty Result Set!");
> >+  }
> 
> better would be: do_check_eq(aArray.length, childCount)
> 
> it does the validResults validation up front, and also covers zero results,
> since having zero results could be a valid test scenario.
> 
But the test data array also includes items that are not in the result set,so aArray.length will always be > than childCount.  Since an empty result set may be valid, I just removed this check entirely.

> >+
> >+/**
> >+ * Helper function to check to see if one object either is or is not in the
> >+ * result set.  It can accept either a queryData object or an array of queryData
> >+ * objects.  If it gets an array, it only compares the first object in the array
> >+ * to see if it is in the result set.
> 
> why bother supporting passing an array then?
We decided on IRC to not worry about this: 
[11:04am] dietrich: howdy sir, what's up
[11:04am] ctalbert: I'm going to get the patch for 384226 in this morning.  I thought I'd answer your question about why to take an array at all for the "isInResult" api in head_queries.js.
[11:04am] ctalbert: The reason was to make the API easier to use
[11:05am] ctalbert: Since one of the compare methods takes and array and the other didn't (originally) I kept messing them up
[11:05am] • dietrich looks at the comments...
[11:05am] ctalbert: And I figured that if *i* was messing them up, other people might too.
[11:05am] dietrich: heh, ok
[11:05am] ctalbert: But, that's the only reason.
[11:05am] dietrich: thanks for finishing that up
[11:05am] ctalbert: Do you want it refactored?
[11:06am] dietrich: will be great to get that landed
[11:06am] dietrich: no, that bit is fine as-is

> 
> >Index: toolkit/components/places/tests/queries/test_abstime-annotation-domain.js
> >===================================================================
> 
> >+
> >+  // Ensue the result set is correct
> >+  compareArrayToResult(testData, root);
> 
> spelling nit: Ensure
> 
> here and in the other tests as well
Thanks, fixed.

I also included the stub test and the readme file, and started a formal Query test coverage tracker on the wiki: http://wiki.mozilla.org/QA/TDAI/Projects/Places_Tests. I'll add the tests I have in progress to that tracker next.

New patch forthcoming, and will carry review forward.

Comment 22

[cmtalbert]

Attachment: New patch addressing comments

Comment 23

[Justin Dolske [:Dolske]]

This appears to have landed, but the change to toolkit/components/places/tests/Makefile.in references a test that doesn't exist. Burnage resulted, so I backed it out. (Just the Makefile (so far:)).

gmake[6]: *** No rule to make target `mochitest/test_bug_405924.html', needed by `libs'.  Stop.

Comment 24

[cmtalbert]

Ok, re-landed.  Moving to Fixed.  Apologies for bustage.

Comment 26

[Marco Bonardo [:mak]]

(In reply to comment #24)
> Ok, re-landed.  Moving to Fixed.  Apologies for bustage.

should this be marked as fixed then?

Comment 27

[cmtalbert]

(In reply to comment #26)
> (In reply to comment #24)
> > Ok, re-landed.  Moving to Fixed.  Apologies for bustage.
> 
> should this be marked as fixed then?
> 

Um, yeah, it should.  I wonder how I missed that.
--> FIXED

Comment 28

[Gervase Markham [:gerv]]

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