bugzilla.mozilla.org has resumed normal operation. Attachments prior to 2014 will be unavailable for a few days. This is tracked in Bug 1475801.
Please report any other irregularities here.

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

RESOLVED FIXED in mozilla2.0b12

Status

()

Toolkit
Places
RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: mmm, Assigned: mmm)

Tracking

unspecified
mozilla2.0b12
x86_64
Linux
Points:
---

Firefox Tracking Flags

(blocking2.0 final+)

Details

(Whiteboard: [softblocker][has review])

Attachments

(1 attachment, 5 obsolete attachments)

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.
Created attachment 506988 [details] [diff] [review]
Tests v1.
Assignee: nobody → mars.martian+bugmail
Status: NEW → ASSIGNED
Blocks: 606966
(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.
blocking2.0: --- → final+
Whiteboard: [softblocker]
Created attachment 507269 [details] [diff] [review]
Tests v2

Fixed up bit about error code and re-enabled test.
Attachment #506988 - Attachment is obsolete: true
Attachment #507269 - Flags: review?(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.
Attachment #507269 - Flags: review?(sdwilsh) → review-
(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.
(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 :)

Updated

8 years ago
No longer blocks: 606966
Depends on: 606966
(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
Created attachment 508025 [details] [diff] [review]
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.
Attachment #507269 - Attachment is obsolete: true
Attachment #508025 - Flags: review?(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!
Attachment #508025 - Flags: review?(sdwilsh) → review-

Updated

8 years ago
Summary: Add tests for dropping referrer and failed insertion with duplicate id/guid → Add tests for dropping referrer, sessionId persisting, and failed insertion with duplicate guid
And actually, margaret has an implementation of test_nonexistent_referrer_ignored in bug 628805...
Created attachment 509821 [details] [diff] [review]
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.
Attachment #508025 - Attachment is obsolete: true
Attachment #509821 - Flags: review?(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 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.
Attachment #509821 - Flags: review?(sdwilsh)
Created attachment 511252 [details] [diff] [review]
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.
Attachment #509821 - Attachment is obsolete: true
Attachment #511252 - Flags: review?(sdwilsh)
Whiteboard: [softblocker] → [softblocker][has patch][needs review 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
Attachment #511252 - Flags: review?(sdwilsh) → review+

Updated

8 years ago
Whiteboard: [softblocker][has patch][needs review sdwilsh] → [softblocker][needs new patch][has review]
(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.
(In reply to comment #16)
> Had to change that test as this patch would make |visit.referrerURI| null.
oh right
(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.
(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.
Created attachment 512899 [details] [diff] [review]
Patch to be checked in.
Attachment #511252 - Attachment is obsolete: true
http://hg.mozilla.org/mozilla-central/rev/d77d5f196286
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b12
Whiteboard: [softblocker][needs new patch][has review] → [softblocker][has review]
You need to log in before you can comment on or make changes to this bug.