Port autocomplete and inline tests to unified autocomplete

RESOLVED FIXED in mozilla33

Status

()

Toolkit
Places
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: mak, Assigned: mak)

Tracking

(Blocks: 1 bug)

Trunk
mozilla33
Points:
8
Dependency tree / graph
Bug Flags:
firefox-backlog +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [search])

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

3 years ago
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.
Flags: firefox-backlog?
Flags: firefox-backlog? → firefox-backlog+
(Assignee)

Updated

3 years ago
Blocks: 995091
(Assignee)

Updated

3 years ago
Blocks: 995092

Updated

3 years ago
Assignee: nobody → mak77
Status: NEW → ASSIGNED
Whiteboard: p=8 s=it-31c-30a-29b.3 [qa?]
No QA verification needed here. Please needinfo me if you think that's a mistake.
Whiteboard: p=8 s=it-31c-30a-29b.3 [qa?] → p=8 s=it-31c-30a-29b.3 [qa-]

Updated

3 years ago
Assignee: mak77 → nobody
Status: ASSIGNED → NEW
Whiteboard: p=8 s=it-31c-30a-29b.3 [qa-] → p=8 [qa-]

Updated

3 years ago
Assignee: nobody → mak77
Status: NEW → ASSIGNED
Whiteboard: p=8 [qa-] → p=8 s=33.1 [qa-]
(Assignee)

Comment 2

3 years ago
Created attachment 8442861 [details] [diff] [review]
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".
Attachment #8442861 - Flags: review?(mano)
(Assignee)

Comment 3

3 years ago
Created attachment 8444037 [details] [diff] [review]
inline tests v2
Attachment #8442861 - Attachment is obsolete: true
Attachment #8442861 - Flags: review?(mano)
Attachment #8444037 - Flags: review?(mano)
(Assignee)

Comment 4

3 years ago
Created attachment 8444038 [details] [diff] [review]
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.
Attachment #8444038 - Flags: review?(mano)

Updated

3 years ago
Iteration: --- → 33.2
Points: --- → 8
QA Whiteboard: [qa-]
Whiteboard: p=8 s=33.1 [qa-]

Updated

3 years ago
Whiteboard: [search]

Updated

3 years ago
Iteration: 33.2 → 33.3
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.
Attachment #8444037 - Flags: review?(mano) → review+
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.
Attachment #8444038 - Flags: review?(mano) → review+
(Assignee)

Comment 7

3 years ago
(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.
(Assignee)

Comment 8

3 years ago
(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...
(Assignee)

Comment 9

3 years ago
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.
(Assignee)

Comment 10

3 years ago
https://hg.mozilla.org/integration/fx-team/rev/93d6fa42ff9f
https://hg.mozilla.org/integration/fx-team/rev/b16a100a246f
Flags: in-testsuite+
Target Milestone: --- → mozilla33
https://hg.mozilla.org/mozilla-central/rev/93d6fa42ff9f
https://hg.mozilla.org/mozilla-central/rev/b16a100a246f
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Depends on: 1041262
You need to log in before you can comment on or make changes to this bug.