Closed
Bug 993372
Opened 11 years ago
Closed 11 years ago
Port autocomplete and inline tests to unified autocomplete
Categories
(Toolkit :: Places, defect)
Toolkit
Places
Tracking
()
People
(Reporter: mak, Assigned: mak)
References
Details
(Whiteboard: [search])
Attachments
(2 files, 1 obsolete file)
77.71 KB,
patch
|
asaf
:
review+
|
Details | Diff | Splinter Review |
103.16 KB,
patch
|
asaf
:
review+
|
Details | Diff | Splinter Review |
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?
Updated•11 years ago
|
Flags: firefox-backlog? → firefox-backlog+
Assignee | ||
Updated•11 years ago
|
Blocks: UnifiedComplete
Updated•11 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•11 years ago
|
Assignee: mak77 → nobody
Status: ASSIGNED → NEW
Whiteboard: p=8 s=it-31c-30a-29b.3 [qa-] → p=8 [qa-]
Updated•11 years ago
|
Assignee: nobody → mak77
Status: NEW → ASSIGNED
Whiteboard: p=8 [qa-] → p=8 s=33.1 [qa-]
Assignee | ||
Comment 2•11 years ago
|
||
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•11 years ago
|
||
Attachment #8442861 -
Attachment is obsolete: true
Attachment #8442861 -
Flags: review?(mano)
Attachment #8444037 -
Flags: review?(mano)
Assignee | ||
Comment 4•11 years ago
|
||
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•11 years ago
|
Iteration: --- → 33.2
Points: --- → 8
QA Whiteboard: [qa-]
Whiteboard: p=8 s=33.1 [qa-]
Updated•11 years ago
|
Whiteboard: [search]
Updated•11 years ago
|
Iteration: 33.2 → 33.3
Comment 5•11 years ago
|
||
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 6•11 years ago
|
||
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•11 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•11 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•11 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•11 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
Comment 11•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/93d6fa42ff9f
https://hg.mozilla.org/mozilla-central/rev/b16a100a246f
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•