Closed Bug 479089 Opened 12 years ago Closed 11 years ago

Need to test that removing a page from autocomplete actually removes a page

Categories

(Toolkit :: Places, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla1.9.2a1

People

(Reporter: sdwilsh, Assigned: ddahl)

References

Details

Attachments

(1 file, 2 obsolete files)

We don't test the code path of onValueRemoved for the autocomplete implementation
Notes for me:

http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/src/nsNavHistoryAutoComplete.cpp#1193

[15:34:33] <@sdwilsh> the autocomplete results calls the function that I linked to
[15:34:52] <@sdwilsh> so you just need to test that function
[15:36:10] <@sdwilsh> ddahl: basically, add a uri, get at the autocomplete provider, but createInstance on Ci.nsIAutoCompleteSimpleResultListener
[15:36:34] <@sdwilsh> then call function with empty result, that uri, and with false remove from db
[15:36:37] <@sdwilsh> check to make sure it's in the db
[15:36:44] <@sdwilsh> then call function with empty result, that uri, and with true remove from db
[15:36:48] <@sdwilsh> check to make sure it's not in the db
[15:36:49] <@sdwilsh> end of test
Attached patch testcase (obsolete) — Splinter Review
Finally - a test that seems too simple to be useful. I had to go and read all of the xpconnect docs. delightful.
Attachment #363430 - Flags: review-
Comment on attachment 363430 [details] [diff] [review]
testcase

>+var gResultListener = {
>+  _lastResult: null,
>+  _lastValue: "",
>+  _lastRemoveFromDb: false,
>+
>+  onValueRemoved: function(aResult, aValue, aRemoveFromDb) {
>+    this._lastResult = aResult;
>+    this._lastValue = aValue;
>+    this._lastRemoveFromDb = aRemoveFromDb;
>+    onValueRemovedCalled = true;
>+  }
>+};
So, this is testing your own listener, which isn't what we want.

>+    result.setListener(gResultListener);
What you really want to do here is set this to our listener.  Do so by:
var result = Cc["@mozilla.org/autocomplete/search;1?name=history"].
             getService(Ci.nsIAutoCompleteSimpleResult);
(In reply to comment #3)
> >+    result.setListener(gResultListener);
> What you really want to do here is set this to our listener.  Do so by:
> var result = Cc["@mozilla.org/autocomplete/search;1?name=history"].
>              getService(Ci.nsIAutoCompleteSimpleResult);

That doesn't work, I hope? mozilla.org/autocomplete/search;1?name=history is nsNavHistory, which only implements nsIAutocompleteSearch, right?
Yeah, what we actually want there is to test nsIAutoCompleteSimpleResultListener.  And we should use a contractid that gives us history.  Not sure what I was thinking with that last comment...
I changed the result to:

var result = Cc["@mozilla.org/autocomplete/search;1?name=history"].
  getService(Ci.nsIAutoCompleteSimpleResultListener);

Now, when I call result.appendMatch("http://mozilla.com", "");

I get this error:
TEST-UNEXPECTED-FAIL | (xpcshell/head.js) | TypeError: result.appendMatch is not a function

What is the deal, according to the IDL this is a legit call:

http://mxr.mozilla.org/mozilla-central/source/toolkit/components/autocomplete/public/nsIAutoCompleteSimpleResult.idl
right - you want to set that to gResultListener (and rename it), and keep your old result.
You gotta love that "Query API"
Attachment #363430 - Attachment is obsolete: true
Attachment #387948 - Flags: review?(sdwilsh)
Attachment #387948 - Flags: review?(sdwilsh) → review+
Comment on attachment 387948 [details] [diff] [review]
updated patch to actually test onValueRemoved

http://reviews.visophyte.org/r/387948/
on file: toolkit/components/places/tests/autocomplete/test_479089.js line 16
>  * The Original Code is Bug 479089 unit test code.

just "Places unit test code" please.  bug numbers are vague


on file: toolkit/components/places/tests/autocomplete/test_479089.js line 46
> try {
>   var ios = Cc["@mozilla.org/network/io-service;1"].
>               getService(Components.interfaces.nsIIOService);
> } catch(ex) {
>   do_throw("Could not get the io service");
> }
> 
> try {
>   var histsvc = Cc["@mozilla.org/browser/nav-history-service;1"].
>                 getService(Ci.nsINavHistoryService);
> } 
> catch(ex) {
>   do_throw("Could not get history service\n");
> }

no need to wrap these in a try-catch block


on file: toolkit/components/places/tests/autocomplete/test_479089.js line 61
> function run_test(){

nit: brace on newline


on file: toolkit/components/places/tests/autocomplete/test_479089.js line 64
>   histsvc.QueryInterface(Components.interfaces.nsIAutoCompleteSimpleResultListener);

store this in a new variable please (and use it)

Also, can we have a more descriptive file name please?

r=sdwilsh with these changes
okee dokee test passes and is cleaned up
Attachment #387948 - Attachment is obsolete: true
Attachment #388830 - Flags: review+
Keywords: checkin-needed
http://hg.mozilla.org/mozilla-central/rev/2ff218904256
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Flags: in-testsuite+
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.2a1
You need to log in before you can comment on or make changes to this bug.