370197 - Places query serialization needs tests

Status: VERIFIED FIXED

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

Description

[Dietrich Ayala (:dietrich)]

The Places query system supports serialization of queries into URIs, and vice-versa.

We need to write tests that exercise the serialization and de-serialization of all public query options.

Comment 1

[Marco Bonardo [:mak]]

this is test writing using the places query api, so a good first bug to exercise with it.

Comment 2

[Drew Willcoxon :adw]

Attachment: test case

Here's a stab at it.  Instead of simply running through each option individually and making sure it serializes and de-serializes correctly, this test selects all subsets of a certain size from all options and applies the options in those subsets together.  So, every option is tested with every other option, multiple times in multiple combinations.  In fact it starts out by doing each option individually, then doing all subsets of size 2, and then all subsets of size 3.

Once options are applied, it serializes the query, de-serializes it, and makes sure the de-serialized query equals the original.  One thing it is not doing yet is testing serialization of more than one query at once.

Overboard?  The code is documented; I explain in more detail in a big comment at the top.  Comments appreciated.

Comment 3

[Marco Bonardo [:mak]]

i'm going to read through this, hwv a fisrt couple notices...
Personally i would move this test into "queries" folder since it's more specialized than the generic unit folder.
This test is quite "heavy", i mean it takes almost 20s on my system to run, unit boxes are often much slower than it, so i suggest to reduce the amount of testing done here. More comments will come later, this file is quite big.

Comment 4

[Marco Bonardo [:mak]]

also sometimes your code generates empty searches like "place:"

Comment 5

[Marco Bonardo [:mak]]

(In reply to comment #4)
> also sometimes your code generates empty searches like "place:"

well this is a bug in deserialization code instead, i filed Bug 474334

please simply add support for multiple queries (2 are enough) and reduce switches_hi to 2.

Comment 6

[Shawn Wilsher :sdwilsh]

(In reply to comment #3)
> Personally i would move this test into "queries" folder since it's more
> specialized than the generic unit folder.
agreed

> This test is quite "heavy", i mean it takes almost 20s on my system to run,
> unit boxes are often much slower than it, so i suggest to reduce the amount of
> testing done here. More comments will come later, this file is quite big.
I disagree here, however.  I'd rather have good extensive test coverage (and he's done work to minimize it and still have full test coverage based on comment 2).  If unit tests take to long, we should throw more machines at the problem, not reduce test coverage IMO.

Comment 7

[Marco Bonardo [:mak]]

(In reply to comment #6)
> I disagree here, however.  I'd rather have good extensive test coverage (and
> he's done work to minimize it and still have full test coverage based on
> comment 2).  If unit tests take to long, we should throw more machines at the
> problem, not reduce test coverage IMO.

Notice i'm suggesting to move from testing all possible subsets of 3 "options" to all possible subsets of 2 "options", practically having 3 rather than 2 does not bring us so much more testing, since also with 2 all options are tested against each other.

Comment 8

[Drew Willcoxon :adw]

Attachment: test case

Changes:
- moved to tests/queries
- reduced maximum number of subsets chosen from 3 to 2
- now tests serialization of multiple queries together: for each individual query we serialize, we serialize the previous 2 queries together

Re: comment 3
Reducing the maximum number of subsets chosen has likewise reduced running time.  It now takes 4-5s here when it used to take 17-18s.

Re: comment 4
I think that means the test is working! :) It's not doing anything real world code can't do.

Re: comment 7
Anytime there are N independent options it's a good idea to test all 2^N subsets of those options, IMHO, or more realistically as close as we can reasonably get.  Options may interact with each other (or whole groups of others) in erroneous or other ways we don't expect, even if we expect that they won't.  That was my thinking behind this test.

Comment 9

[Marco Bonardo [:mak]]

your thinking is correct but we should also see what we can reasonably do based on how complex/error prone is the code we are testing, how easy will be for a third party to hack into our test in future, how many options we could add in future (double them up?), if testing 3 options will give us much more credit that we won't fail by 4 or by 5 than testing by 2, and so on... i think the current one is a good compromise of all of these, and thanks to customization you added it can be easily incremented/changed without you/anybody having to rewrite it all.

Comment 10

[Marco Bonardo [:mak]]

Comment on attachment 357871 [details] [diff] [review]
test case

>+      // Find the next rightmost pointer that is not adjacent to the current one.
>+      let si = set.length - 2;  // set index
>+      let pi = ptrs.length - 2; // ptrs index
>+      while (pi >= 0 && ptrs[pi] === si)
>+      {
>+        pi--;
>+        si--;
>+      }

this sounds like a for

>+  for (let i = 0; i < aQueryArr.length; i++)
>+  {
>+    let j = 0;
>+    for (; j < queryArr2.length; j++)
>+    {
>+      if (queryObjsEqual(querySwitches, aQueryArr[i], queryArr2[j])) break;
>+    }
>+    if (j < queryArr2.length) queryArr2.splice(j, 1);
>+  }
>+  do_check_eq(queryArr2.length, 0);

you could maybe use .filter to walk the array and check length on the result

I'll not nitpick on code style since this is a test and is quite big to fix it up, hwv please take a look at other js code in other tests, Places, and globally in browser, and try to follow the actual code style even if it's not your usual coding style or you find it less readable. If you think some style is wrong we can discuss about it, but generally you should follow current conventions.
For example parentheses positions (for functions and loops), and going to a newline after an if condition are a couple things you don't follow here. Also usually we try to not mix it up var and let unless is really necessary, to avoid future error prone changes.

btw, looks good, r=me

Comment 11

[Drew Willcoxon :adw]

Attachment: for checkin

(In reply to comment #10)
> this sounds like a for

It would be a for without a body, which is ugly and dangerous IMHO.  I searched mxr for that style and couldn't find any examples.  A while loop seems more natural to me here.

> you could maybe use .filter to walk the array and check length on the result

I think that would be a more convoluted way to do it in this case.  It would be O(2mn) when it's O(mn) as it is now.  We'd have to filter one set, and inside that filter we'd loop over the other set checking if queryObjsEqual.  Then we'd have to do the filter on the other set, again with an inner loop, and finally compare the two filter'ed array lengths.

I did fix the style nits, though. :) Thanks for the comments.

Comment 12

[Marco Bonardo [:mak]]

http://hg.mozilla.org/mozilla-central/rev/1180750fc0e1

Comment 13

[Marco Bonardo [:mak]]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/075573137f3f

Comment 14

[Tracy Walker [:tracy]]

verified per code checkin

Comment 15

[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