Closed
Bug 628805
Opened 10 years ago
Closed 10 years ago
Add tests for invalid referrerURI and invalid sessionId to test_async_history_api.js
Categories
(Toolkit :: Places, defect)
Toolkit
Places
Tracking
()
RESOLVED
FIXED
mozilla2.0b12
Tracking | Status | |
---|---|---|
blocking2.0 | --- | final+ |
People
(Reporter: Margaret, Assigned: Margaret)
References
Details
(Whiteboard: [softblocker])
Attachments
(2 files, 3 obsolete files)
5.05 KB,
patch
|
Details | Diff | Splinter Review | |
4.85 KB,
patch
|
sdwilsh
:
review+
|
Details | Diff | Splinter Review |
See bug 606966 comment 199.
Attachment #506906 -
Flags: review?(sdwilsh)
Comment 1•10 years ago
|
||
To be clear, these tests pass as-is with no code changes needed?
blocking2.0: --- → final+
Updated•10 years ago
|
Whiteboard: [softblocker]
Assignee | ||
Comment 2•10 years ago
|
||
Yes, these tests pass right now.
Comment 3•10 years ago
|
||
Comment on attachment 506906 [details] [diff] [review] patch >+function test_add_visit_invalid_referrerURI() Let's go with test_add_visit_invalid_referrerURI_ignored >+ place.visits[0].referrerURI = NetUtil.newURI(TEST_DOMAIN + >+ "test_add_visit_invalid_referrerURI_unvisistedURI"); You could save some sanity on this line length here by just doing NetUtil.newURI(place.uri.spec + "_unvisitedURI") Also, you should add a check to make sure that place.visits[0].referrerURI is not visited. >+ gHistory.updatePlaces(place, function(aResultCode, aPlaceInfo) { >+ do_check_true(Components.isSuccessCode(aResultCode)); >+ let visit = aPlaceInfo.visits[0]; >+ // Check to make sure we do not insert an unvisited URI into the database. >+ do_check_false(gGlobalHistory.isVisited(visit.referrerURI)); You should also check that the visit we just added in moz_historyvisits has a null from_visit. This will require you to roll your own query. >+ run_next_test(); You want this in the updatePlaces callback, otherwise more than one test will be running at a time. >+function test_add_visit_invalid_sessionId() Let's go with test_add_visit_invalid_sessionId_ignored >+ let place = { >+ uri: NetUtil.newURI(TEST_DOMAIN + >+ "test_add_visit_invalidSessionId"), nit: uri doesn't match test name. We could actually just use gRunningTest.name in most of our tests... >+ place.visits[0].sessionId = -1; You should also check for the case where a sessionId is not in the database. You'll have to query the database to get the highest value, then add something like ten to it. >+ gHistory.updatePlaces(place, function(aResultCode, aPlaceInfo) { >+ do_check_true(Components.isSuccessCode(aResultCode)); >+ let visit = aPlaceInfo.visits[0]; We should make sure that the visit was added in the database though! >+ // Check to make sure we do not persist bogus sessionId. >+ do_check_neq(visit.sessionId, -1); >+ }); >+ >+ run_next_test(); ditto about where this runs > test_add_visit_no_date_throws, > test_add_visit_no_transitionType_throws, > test_add_visit_invalid_transitionType_throws, >+ test_add_visit_invalid_referrerURI, >+ test_add_visit_invalid_sessionId, > test_non_addable_uri_errors, Both of the tests you add don't throw, nor do they raise error events, so lets move it after the *_errors tests. This is r+ with comments addressed, but I want to see the changes in case I missed something.
Attachment #506906 -
Flags: review?(sdwilsh) → review-
Comment 4•10 years ago
|
||
We should also add a test about having a non-nsIURI object on the referrer, and it should be ignored as well. We may have talked about that on irc, but I can't recall offhand.
Assignee | ||
Comment 5•10 years ago
|
||
Addressed review comments and added a test to address comment 4. These tests all pass.
Attachment #506906 -
Attachment is obsolete: true
Attachment #507696 -
Flags: review?(sdwilsh)
Updated•10 years ago
|
Assignee | ||
Comment 6•10 years ago
|
||
This test does not currently pass because we are storing the bogus sessionId in the database.
Attachment #507699 -
Flags: review?(sdwilsh)
Comment 7•10 years ago
|
||
Comment on attachment 507696 [details] [diff] [review] invalid_referrerURI, nonnsIURI_referrerURI, invalid_sessionId >+++ b/toolkit/components/places/tests/unit/test_async_history_api.js >+function test_add_visit_invalid_referrerURI_ignored() I know I suggested it, but mmmulani came up with a better name over in bug 628865. Just drop the add_visit_ part and we'll be golden :) >+ do_check_false(gGlobalHistory.isVisited(place.visits[0].referrerURI)); Also check that place.uri is not visited. >+ // Check to make sure we do not insert an unvisited URI into the database. I think what you mean to say is something about making sure we don't visit the referrer. >+ // Check to make sure visit has a null from_visit in database. We are actually checking that it is zero ;) >+function test_add_visit_nonnsIURI_referrerURI_ignored() ditto about dropping add_viist_, although you could combine this test into the one above it. >+ let place = { >+ uri: NetUtil.newURI(TEST_DOMAIN + >+ "test_add_visit_nonnsIURI_referrerURI_ignored"), >+ visits: [ >+ new VisitInfo(), >+ ], >+ }; >+ place.visits[0].referrerURI = "NON_NSIURI"; let's make it an actually uri looking thing though please Also, check that place.uri is not visited >+ // Check to make sure visit has a null from_visit in database. checking for zero, not null >+function test_add_visit_invalid_sessionId_ignored() >+{ >+ let place = { >+ uri: NetUtil.newURI(TEST_DOMAIN + >+ "test_add_visit_invalid_sessionId_ignored"), >+ visits: [ >+ new VisitInfo(), >+ ], >+ }; >+ place.visits[0].sessionId = -1; check that place.uri is not visited >+ 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 persist bogus sessionId with the visit. >+ let visit = aPlaceInfo.visits[0]; >+ do_check_neq(visit.sessionId, -1); Compare against place.visits[0].sessionId please Also, check the database to make sure it's not -1 please > test_non_addable_uri_errors, >+ test_add_visit_invalid_referrerURI_ignored, >+ test_add_visit_nonnsIURI_referrerURI_ignored, >+ test_add_visit_invalid_sessionId_ignored, > test_observer_topic_dispatched_when_complete, Where you added these here is not the same place as where you added them with the actual test methods. Keep the order the same please :) r=sdwilsh
Attachment #507696 -
Flags: review?(sdwilsh) → review+
Comment 8•10 years ago
|
||
Comment on attachment 507699 [details] [diff] [review] unstored_sessionId test I'm going to have you patch this. The code you need to change is here: https://hg.mozilla.org/mozilla-central/annotate/1c2d53a2dcfb/toolkit/components/places/src/History.cpp#l746 In the if, add an or statement that does something like this: (i && mPlaces[i].sessionId < mPlaces[0].sessionId) That should make your test pass (and you may want to update the comment too). I'll review this once you make that change.
Attachment #507699 -
Flags: review?(sdwilsh)
Assignee | ||
Comment 9•10 years ago
|
||
Attachment #507696 -
Attachment is obsolete: true
Assignee | ||
Comment 11•10 years ago
|
||
Attachment #507699 -
Attachment is obsolete: true
Attachment #509873 -
Flags: review?(sdwilsh)
Assignee | ||
Updated•10 years ago
|
Whiteboard: [softblocker] → [softblocker][needs review sdwilsh]
Comment 12•10 years ago
|
||
Comment on attachment 509873 [details] [diff] [review] updated unstored_sessionId test w/ fix to make it pass >+++ b/toolkit/components/places/src/History.cpp >+ // If we are inserting a place into an empty mPlaces array, we need to >+ // check to make sure we do not store a bogus session id that is higher >+ // than the current maximum session id. >+ if (i == 0) { >+ PRInt64 newSessionId = navHistory->GetNewSessionID(); >+ if (mPlaces[0].sessionId > newSessionId) >+ mPlaces[0].sessionId = newSessionId; >+ } nit: brace one line ifs > // Speculatively get a new session id for our visit if the current session > // id is non-valid. While it is true that we will use the session id from > // the referrer if the visit was "recent" enough, we cannot call this > // method off of the main thread, so we have to consume an id now. this comment needs to be updated too >- if (mPlaces[i].sessionId <= 0) { >+ if (mPlaces[i].sessionId <= 0 || >+ (i > 0 && mPlaces[i].sessionId > mPlaces[0].sessionId)) { I think we actually want >= here (equal to the new value should be ignored, and we should test for it instead of +10) r=sdwilsh
Attachment #509873 -
Flags: review?(sdwilsh) → review+
Assignee | ||
Comment 13•10 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/175623f0113a http://hg.mozilla.org/mozilla-central/rev/ef4d491f9ba0
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Assignee | ||
Updated•10 years ago
|
Whiteboard: [softblocker][needs review sdwilsh] → [softblocker]
Updated•10 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•