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

Status: RESOLVED FIXED

(Toolkit :: Places, defect)

Description

[Shawn Wilsher :sdwilsh]

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

Comment 1

[David Dahl :ddahl]

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

Comment 2

[David Dahl :ddahl]

Attachment: testcase

Finally - a test that seems too simple to be useful. I had to go and read all of the xpconnect docs. delightful.

Comment 3

[Shawn Wilsher :sdwilsh]

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);

Comment 4

[:Gavin Sharp [email: [e-mail]]]

(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?

Comment 5

[Shawn Wilsher :sdwilsh]

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...

Comment 6

[David Dahl :ddahl]

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

Comment 7

[Shawn Wilsher :sdwilsh]

right - you want to set that to gResultListener (and rename it), and keep your old result.

Comment 8

[David Dahl :ddahl]

Attachment: updated patch to actually test onValueRemoved

You gotta love that "Query API"

Comment 9

[Shawn Wilsher :sdwilsh]

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

Comment 10

[David Dahl :ddahl]

Attachment: 1.0 Autocomplete onValueRemoved test

okee dokee test passes and is cleaned up

Comment 11

[Marco Bonardo [:mak]]

http://hg.mozilla.org/mozilla-central/rev/2ff218904256