628865 - Add tests for dropping referrer, sessionId persisting, and failed insertion with duplicate guid

Status: RESOLVED FIXED

(Toolkit :: Places, defect)

Description

[Mehdi Mulani [:mmm] (I don't check this)]

These tests are for the Async History API.

From sdwilsh's comment:
- Test for dropping referrer when it isn't "recent enough"
- Test for inserting a place with a duplicate guid, place id, and existing uri
fails
- Test for inserting a place with a duplicate place id and existing uri fails
- Test for inserting a place with duplicate guid and existing uri fails

I was not able to correctly create a test for the last case, inserting a place with duplicate guid. It seems that an NS_ERROR_STORAGE_CONSTRAINT is raised instead of an NS_ERROR_INVALID_ARG as expected.

Comment 1

[Mehdi Mulani [:mmm] (I don't check this)]

Attachment: Tests v1.

Comment 2

[Shawn Wilsher :sdwilsh]

(In reply to comment #0)
> I was not able to correctly create a test for the last case, inserting a place
> with duplicate guid. It seems that an NS_ERROR_STORAGE_CONSTRAINT is raised
> instead of an NS_ERROR_INVALID_ARG as expected.
That is a fine error for it to raise (in fact, it's what I'd expect!).  Please include this test as well.

Comment 3

[Mehdi Mulani [:mmm] (I don't check this)]

Attachment: Tests v2

Fixed up bit about error code and re-enabled test.

Comment 4

[Shawn Wilsher :sdwilsh]

Comment on attachment 507269 [details] [diff] [review]
Tests v2

globally: third argument to the do_check_* functions is a stack, not a message.  Usually you don't want to provide it.

>+function test_old_referrer_drops()
Instead of _drops, let's go with _ignored.

Please add comments to what you are doing here!
>+  let oldVisitInfo = new VisitInfo(undefined, (Date.now() - 10) * 1000);
>+  let recentVisitInfo = new VisitInfo();
So, the code that does this is here:
https://hg.mozilla.org/mozilla-central/annotate/e98b94aa64fa/toolkit/components/places/src/History.cpp#l620
That constant is defined to be 15 minutes here:
https://hg.mozilla.org/mozilla-central/annotate/e98b94aa64fa/toolkit/components/places/src/nsNavHistory.h#l93
I'd like us to test just past that threshold.

Also, pass a transition type; passing undefined looks silly, even if it is correct :)

>+  let place = {
>+    uri: NetUtil.newURI(TEST_DOMAIN + "test_old_referrer_drops_page"),
>+    visits: [oldVisitInfo],
nit: follow the formatting of other tests please

You'll want to ensure that both things are not visited before calling updatePlaces.

>+  gHistory.updatePlaces(place, function(aResultCode, aPlaceInfo) {
>+    do_check_eq(aPlaceInfo.visits[0].referrerURI.spec,
>+                oldVisitInfo.referrerURI.spec,
>+                "should have the old referrer");
You'll also want to check that it has a visit in the database, and that aResultCode is a success code.

>+    gHistory.updatePlaces(place, function(aResultCode, aPlaceInfo) {
>+      do_check_eq(aPlaceInfo.visits[0].referrerURI, null,
>+                  "referrer should be gone with recent visit");
ditto to both above statements.

>+function test_duplicate_guid_or_id_fails()
can we split this out into to two methods please?
test_duplicate_guid_errors
test_duplicate_id_fails

>+  gHistory.updatePlaces(places[0], function(aResultCode, aPlaceInfo) {
>+    do_check_eq(aResultCode, Cr.NS_OK);
See how the other tests do this; you'll want to do the same thing.

>+    gHistory.updatePlaces(places[1], function(aResultCode, aPlaceInfo) {
>+      do_check_eq(aResultCode, Cr.NS_OK);
ditto

>+
>+      let duplicateGuid = {
>+        uri: NetUtil.newURI(specToDupe),
you could just use the same URI object, instead of creating a new one all the time.

>+      try {
>+        gHistory.updatePlaces(duplicateId);
>+        do_throw("Should have thrown!");
>+      }
>+      catch (e) {
>+        do_check_eq(e.result, Cr.NS_ERROR_INVALID_ARG);
>+      }
>+
>+      try {
>+        gHistory.updatePlaces(duplicateIdAndGuid);
>+        do_throw("Should have thrown!");
>+      }
>+      catch(e) {
>+        do_check_eq(e.result, Cr.NS_ERROR_INVALID_ARG);
>+      };
these shouldn't actually throw, but they should raise an error.  Do they throw now?

>+      gHistory.updatePlaces(duplicateGuid, function(aResultCode, aPlaceInfo) {
>+        do_check_eq(aResultCode, Cr.NS_ERROR_STORAGE_CONSTRAINT);
Huh...this apparently got defined in bug 599799.  Yay!

>   test_properties_saved,
>   test_guid_saved,
>   test_referrer_saved,
>   test_sessionId_saved,
>   test_guid_change_saved,
>   test_title_change_saved,
>   test_no_title_does_not_clear_title,
>   test_title_change_notifies,
>+  test_old_referrer_drops,
>+  test_duplicate_guid_or_id_fails,
you should move these to be with other _errors and _ignores tests.

Comment 5

[Shawn Wilsher :sdwilsh]

(In reply to comment #0)
> - Test for inserting a place with a duplicate guid, place id, and existing uri
> fails
> - Test for inserting a place with a duplicate place id and existing uri fails
> - Test for inserting a place with duplicate guid and existing uri fails
Actually, it looks like you wrote a test for the first thing here, but not the last two.  Clean up your current test, and then add the two I suggested as a split.

Comment 6

[Mehdi Mulani [:mmm] (I don't check this)]

(In reply to comment #4)
> >+      try {
> >+        gHistory.updatePlaces(duplicateId);
> >+        do_throw("Should have thrown!");
> >+      }
> >+      catch (e) {
> >+        do_check_eq(e.result, Cr.NS_ERROR_INVALID_ARG);
> >+      }
> >+
> >+      try {
> >+        gHistory.updatePlaces(duplicateIdAndGuid);
> >+        do_throw("Should have thrown!");
> >+      }
> >+      catch(e) {
> >+        do_check_eq(e.result, Cr.NS_ERROR_INVALID_ARG);
> >+      };
> these shouldn't actually throw, but they should raise an error.  Do they throw
> now?

Yup.

(In reply to comment #5)
> (In reply to comment #0)
> > - Test for inserting a place with a duplicate guid, place id, and existing uri
> > fails
> > - Test for inserting a place with a duplicate place id and existing uri fails
> > - Test for inserting a place with duplicate guid and existing uri fails
> Actually, it looks like you wrote a test for the first thing here, but not the
> last two.  Clean up your current test, and then add the two I suggested as a
> split.

The test_duplicate_guid_or_id_fails encompassed all three cases but used the same URI. I'll split as suggested with different URIs to be safer :)

Comment 7

[Shawn Wilsher :sdwilsh]

(In reply to comment #6)
> Yup.
Fun.  Can you figure out why?  If you run check-interactive, you can see the debug spew and what lines failed.  I suspect you are passing in something bad, and that's why they actually throw.

> The test_duplicate_guid_or_id_fails encompassed all three cases but used the
> same URI. I'll split as suggested with different URIs to be safer :)
word

Comment 8

[Mehdi Mulani [:mmm] (I don't check this)]

Attachment: Tests v3.

Splits up tests and completely changes how most of the tests check results.
The placeId duplication tests are moved out to bug 629814 because of problems with the API.

I also temporarily tests with checking the referrer in the visits passed to the callback as this was also buggy.

Comment 9

[Shawn Wilsher :sdwilsh]

Comment on attachment 508025 [details] [diff] [review]
Tests v3.

>+++ b/toolkit/components/places/tests/unit/test_async_history_api.js
>+  // Date.now() returns the time in milliseconds while the mozIVisitInfo
>+  // stores time in microseconds.
I wouldn't bother with this; it's considered common knowledge in the places world.

>+  let oldTime = (Date.now() - (16 * 60 * 1000)) * 1000;
I'd like 15 * 60 * 1000000 constified at the top (with other constants) as RECENT_EVENT_THRESHOLD.

>+  let visitInfo = new VisitInfo();
>+  visitInfo.referrerURI = referrerPlace.uri;
>+  let place = {
>+    uri: NetUtil.newURI(TEST_DOMAIN + "test_old_referrer_ignored_page"),
>+    visits: [
>+      visitInfo,
>+    ],
>+  };
Move to just before first use.

>+  // First we must add our referrer to the history so that it is not ignored
>+  // as being invalid.
>+  do_check_false(gGlobalHistory.isVisited(referrerPlace.uri));
>+  gHistory.updatePlaces(referrerPlace, function(aResultCode, aPlaceInfo) {
>+    do_check_true(Components.isSuccessCode(aResultCode));
>+    do_check_true(gGlobalHistory.isVisited(referrerPlace.uri));
>+
>+    do_check_false(gGlobalHistory.isVisited(place.uri));
>+    gHistory.updatePlaces(place, function(aResultCode, aPlaceInfo) {
You'll want to continue your commenting here saying what you are doing next.

>+      // Currently, the visit still holds a reference to the referrer
>+      // though it may not be present in the database. To work around this,
>+      // we manually check the database.
>+      // do_check_eq(aPlaceInfo.visits[0].referrerURI, null);
You only have to add one line to make this test pass, so I'm going to have you do that:
https://hg.mozilla.org/mozilla-central/annotate/1c2d53a2dcfb/toolkit/components/places/src/History.cpp#l921
(just add aPlace.referrerSpec.Truncate(); and update the comments)

>+  gHistory.updatePlaces(place, function(aResultCode, aPlaceInfo) {
>+    do_check_true(Components.isSuccessCode(aResultCode));
>+    do_check_true(gGlobalHistory.isVisited(place.uri));
>+
>+    do_check_false(gGlobalHistory.isVisited(visitInfo.referrerURI));
nit: ditch the newline between these

>+    // We run into the same issue as in |test_old_referrer_ignored| and must
>+    // query the database to see if the referrer is set.
>+    // do_check_eq(aPlaceInfo.visits[0].referrerURI, null);
fix this too (previous fix should do the trick)

>+    let stmt = DBConn().createStatement(
>+      "SELECT COUNT(1) AS count " +
>+      "FROM moz_historyvisits " +
>+      "WHERE place_id = (SELECT id FROM moz_places WHERE url = :page_url) " +
>+      "AND from_visit = ( " +
>+        "SELECT id " +
>+        "FROM moz_historyvisits " +
>+        "WHERE place_id = (SELECT id FROM moz_places WHERE url = :referrer) " +
>+      ") "
>+    );
this isn't right.  What you really want is to test that from_visit is 0, so your AND is wrong.

>+function test_referrer_sessionId_persists()
>+{
>+  // This test ensures that when updating the referrer on a visit, the
>+  // sessionId of the visit remains the same.
I think we've had some miscommunication on what this test needs to look at.  What we need to test is this:
1) Add a visit (it's going to be our referrer).
2) Note the session id it was given.
3) Add a visit to another place (different URI) and have its referrer be the same place we added in (1).
4) Ensure that the visit added in (3) has the same sessionId that we got in (2)
I think you already have this, but your test does a lot of other stuff that we don't actually care about too.

>+    let guid = aPlaceInfo.guid;
>+    do_check_neq(guid, null);
nit: don't need to check this; other tests do

>+    let badPlace = {
>+      uri: NetUtil.newURI(TEST_DOMAIN + "test_duplicate_guid_fails_second"),
>+      visits: [
>+        new VisitInfo(),
>+      ],
>+      guid: guid,
and then just use aPlaceInfo.guid here

Almost there!

Comment 10

[Shawn Wilsher :sdwilsh]

And actually, margaret has an implementation of test_nonexistent_referrer_ignored in bug 628805...

Comment 11

[Mehdi Mulani [:mmm] (I don't check this)]

Attachment: Tests v4.

Fixes up tests from previous patch as per review comments and incorporates tests from bug 629814.

I was unsure of where to put the _ignored tests in gTests and after a _does_not_clear_title test seemed like a good spot.

Comment 12

[Shawn Wilsher :sdwilsh]

(In reply to comment #11)
> I was unsure of where to put the _ignored tests in gTests and after a
> _does_not_clear_title test seemed like a good spot.
Actually, if you base your patch off of bug 628805, margaret already has the right spot :)

Comment 13

[Shawn Wilsher :sdwilsh]

Comment on attachment 509821 [details] [diff] [review]
Tests v4.

You are just adding all your tests at the end.  Please keep them in the same order as the gTests array listing.

I'll review this once comment 12 (the bug mentioned has already landed) and this is addressed.

Comment 14

[Mehdi Mulani [:mmm] (I don't check this)]

Attachment: Tests v5.

Addressed comments and changed test_invalid_referrerURI_ignored to reflect that the visits returned will not contain the referrer info if the referrer is not added.

Comment 15

[Shawn Wilsher :sdwilsh]

Comment on attachment 511252 [details] [diff] [review]
Tests v5.

>+++ b/toolkit/components/places/tests/unit/test_async_history_api.js
>+function test_duplicate_guid_and_id_errors()
This test isn't actually useful given that we ignore placeIds now.  Just nuke it.

>@@ -408,17 +478,17 @@ function test_invalid_referrerURI_ignore
> 
>   gHistory.updatePlaces(place, function(aResultCode, aPlaceInfo) {
>     do_check_true(Components.isSuccessCode(aResultCode));
>     let uri = aPlaceInfo.uri;
>     do_check_true(gGlobalHistory.isVisited(uri));
> 
>     // Check to make sure we do not visit the invalid referrer.
>     let visit = aPlaceInfo.visits[0];
>-    do_check_false(gGlobalHistory.isVisited(visit.referrerURI));
>+    do_check_false(gGlobalHistory.isVisited(place.visits[0].referrerURI));
I don't think you meant to do this.

>+
>+function test_old_referrer_ignored()
You have some random extra whitespace here

>+  // This tests that a referrer for a visit which is not recent (specifically,
>+  // older than 15 minutes as per RECENT_EVENT_THRESHOLD) is not saved by
>+  // updatePlaces.
>+  let oldTime = (Date.now() - (RECENT_EVENT_THRESHOLD + 1));
This isn't right.  Date.now() is in milliseconds, and RECENT_EVENT_THRESHOLD is in microseconds.  You want:
let oldTime = Date.now() * 1000 - (RECENT_EVENT_THRESHOLD + 1);

>+      // Though the visit will not contain the referrer, we must examine the
>+      // database to be sure.
>+      do_check_eq(aPlaceInfo.visits[0].referrerURI, null);
>+      let stmt = DBConn().createStatement(
>+        "SELECT COUNT(1) AS count " +
>+        "FROM moz_historyvisits " +
>+        "WHERE place_id = (SELECT id FROM moz_places WHERE url = :page_url) " +
>+        "AND from_visit = 0 "
We actually want "from_visit IS NULL" here I believe.

>+function test_insert_id_ignored()
insert_id in this whole method is weird.  Please call it place_id like it really is :)

>+function test_referrer_sessionId_persists()
>+{
>+  // This test ensures that when updating the referrer on a visit, the
>+  // sessionId of the visit remains the same.
What you mean to say here is this:
This test makes sure that a visit that has a valid referrer gets the sessionId of the referrer.

>+  let sessionId;
declare at first use; don't need it out here.

r=sdwilsh

Comment 16

[Mehdi Mulani [:mmm] (I don't check this)]

(In reply to comment #15)
> >@@ -408,17 +478,17 @@ function test_invalid_referrerURI_ignore
> > 
> >   gHistory.updatePlaces(place, function(aResultCode, aPlaceInfo) {
> >     do_check_true(Components.isSuccessCode(aResultCode));
> >     let uri = aPlaceInfo.uri;
> >     do_check_true(gGlobalHistory.isVisited(uri));
> > 
> >     // Check to make sure we do not visit the invalid referrer.
> >     let visit = aPlaceInfo.visits[0];
> >-    do_check_false(gGlobalHistory.isVisited(visit.referrerURI));
> >+    do_check_false(gGlobalHistory.isVisited(place.visits[0].referrerURI));
> I don't think you meant to do this.

Had to change that test as this patch would make |visit.referrerURI| null.

Comment 17

[Shawn Wilsher :sdwilsh]

(In reply to comment #16)
> Had to change that test as this patch would make |visit.referrerURI| null.
oh right

Comment 18

[Mehdi Mulani [:mmm] (I don't check this)]

(In reply to comment #15)
> >+      // Though the visit will not contain the referrer, we must examine the
> >+      // database to be sure.
> >+      do_check_eq(aPlaceInfo.visits[0].referrerURI, null);
> >+      let stmt = DBConn().createStatement(
> >+        "SELECT COUNT(1) AS count " +
> >+        "FROM moz_historyvisits " +
> >+        "WHERE place_id = (SELECT id FROM moz_places WHERE url = :page_url) " +
> >+        "AND from_visit = 0 "
> We actually want "from_visit IS NULL" here I believe.

This causes the query to return 0 rows and looking at the table, from_visit is an integer.

Comment 19

[Shawn Wilsher :sdwilsh]

(In reply to comment #18)
> This causes the query to return 0 rows and looking at the table, from_visit is
> an integer.
Our schema is so broken :(

Yeah, you need to leave it as-is.

Comment 20

[Mehdi Mulani [:mmm] (I don't check this)]

Attachment: Patch to be checked in.

Comment 21

[Mehdi Mulani [:mmm] (I don't check this)]

http://hg.mozilla.org/mozilla-central/rev/d77d5f196286