993372 - Port autocomplete and inline tests to unified autocomplete

Status: RESOLVED FIXED

(Toolkit :: Places, defect)

Description

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

We should port the existing tests covering Places autocomplete to the new unified search component. Basically a new unifiedcomplete/ folder with tests ported from autocomplete/ and inline/ folders (and eventually others if they are in the wrong test folder). Should also check that we are not breaking existing switch-to-tab tests.

Comment 1

[u279076]

No QA verification needed here. Please needinfo me if you think that's a mistake.

Comment 2

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Attachment: part 1

This is porting of the inline complete tests. It allowed to find and fix various bugs.

Next part is autocomplete tests, I will do that part on top of this one, that means some of the shared testing helpers may change, but I guess it's easier to do by pieces.

I think the review should concentrate on changes to head and UnifiedComplete.js, the test changes are 99% "automatic".

Comment 3

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Attachment: inline tests v2

Comment 4

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Attachment: autocomplete tests v2

the remaining tests.
they are a bit more verbose than before, but honestly I found the old harness very hard to read and follow, so I think it's a win.

Comment 5

[Asaf Romano (gone)]

Comment on attachment 8444037 [details] [diff] [review]
inline tests v2

Review of attachment 8444037 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/places/UnifiedComplete.js
@@ +1117,5 @@
>          yield SwitchToTabStorage.initDatabase(conn);
>  
>          return conn;
> +      }.bind(this)).then(null, ex => { dump("Couldn't get database handle: " + ex + "\n");
> +                                       Cu.reportError(ex); });

Debug left over?

@@ +1176,5 @@
>                                if (search == this._currentSearch) {
>                                  this.finishSearch(true);
>                                }
> +                            }, ex => { dump("Query failed: " + ex + "\n");
> +                                       Cu.reportError(ex); });

Ditto.

Comment 6

[Asaf Romano (gone)]

Comment on attachment 8444038 [details] [diff] [review]
autocomplete tests v2

Review of attachment 8444038 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/places/tests/unifiedcomplete/head_autocomplete.js
@@ +165,5 @@
> +
> +    Assert.equal(controller.matchCount, test.matches.length,
> +                 "Got as many results as expected");
> +
> +    // If we expect results, make sure we got matches

nit: missing dot.

@@ +205,5 @@
>  }
> +
> +function addOpenPages(aUri, aCount=1) {
> +  let ac = Cc["@mozilla.org/autocomplete/search;1?name=unifiedcomplete"]
> +             .getService(Ci.mozIPlacesAutoComplete);           

trailing spaces.

May you add a global getter for the service and use it everywhere?

@@ +240,5 @@
> +  else
> +    branch += "restrict.";
> +
> +  if (Services.prefs.prefHasUserValue(branch + aType))
> +    Services.prefs.clearUserPref(branch + aType);

Isn't clearUeerPref a no-op if there's no user value?

Btw, it seems that in this file/directory the standard is to do wrap single-line blocks.

::: toolkit/components/places/tests/unifiedcomplete/test_word_boundary_search.js
@@ +26,5 @@
> +  let uri6 = NetUtil.newURI("http://tag/2");
> +  let uri7 = NetUtil.newURI("http://crazytitle/");
> +  let uri8 = NetUtil.newURI("http://katakana/");
> +  let uri9 = NetUtil.newURI("http://ideograph/");
> +  let uri10 = NetUtil.newURI("http://camel/pleaseMatchMe/");

I'm tempted to ask you to make promiseAddVisits support uri-specs, because this is too boring :) Your call.

Comment 7

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

(In reply to Mano (needinfo? for any questions; not reading general bugmail) from comment #5)
> ::: toolkit/components/places/UnifiedComplete.js
> @@ +1117,5 @@
> >          yield SwitchToTabStorage.initDatabase(conn);
> >  
> >          return conn;
> > +      }.bind(this)).then(null, ex => { dump("Couldn't get database handle: " + ex + "\n");
> > +                                       Cu.reportError(ex); });
> 
> Debug left over?

nope, I added those cause Cu.reportError is not visible in xpcshell, I lost time trying to figure a failure and it was just that.

Comment 8

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

(In reply to Mano (needinfo? for any questions; not reading general bugmail) from comment #6)
> :::
> toolkit/components/places/tests/unifiedcomplete/test_word_boundary_search.js
> @@ +26,5 @@
> > +  let uri6 = NetUtil.newURI("http://tag/2");
> > +  let uri7 = NetUtil.newURI("http://crazytitle/");
> > +  let uri8 = NetUtil.newURI("http://katakana/");
> > +  let uri9 = NetUtil.newURI("http://ideograph/");
> > +  let uri10 = NetUtil.newURI("http://camel/pleaseMatchMe/");
> 
> I'm tempted to ask you to make promiseAddVisits support uri-specs, because
> this is too boring :) Your call.

It would take a lot of time to s&r everything here and there are many instances of promiseAddVisits in the tree, all of them should be updated. I might file it as a good first bug for contributors.

> @@ +205,5 @@
> >  }
> > +
> > +function addOpenPages(aUri, aCount=1) {
> > +  let ac = Cc["@mozilla.org/autocomplete/search;1?name=unifiedcomplete"]
> > +             .getService(Ci.mozIPlacesAutoComplete);           
> 
> trailing spaces.
> 
> May you add a global getter for the service and use it everywhere?

how much global? I don't think it's worth to add it to PlacesUtils, and here it's already in a head file...

Comment 9

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

I filed bug 1040340 to use specs instead of nsIURIs in these tests, it's a lot of s&r work and I don't feel like is valuable way to spend time for us atm.

Comment 10

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

https://hg.mozilla.org/integration/fx-team/rev/93d6fa42ff9f
https://hg.mozilla.org/integration/fx-team/rev/b16a100a246f

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/93d6fa42ff9f
https://hg.mozilla.org/mozilla-central/rev/b16a100a246f