Closed Bug 993372 Opened 11 years ago Closed 11 years ago

Port autocomplete and inline tests to unified autocomplete

Categories

(Toolkit :: Places, defect)

defect
Not set
normal
Points:
8

Tracking

()

RESOLVED FIXED
mozilla33
Iteration:
33.3

People

(Reporter: mak, Assigned: mak)

References

Details

(Whiteboard: [search])

Attachments

(2 files, 1 obsolete file)

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+
Blocks: 995092
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-]
Assignee: mak77 → nobody
Status: ASSIGNED → NEW
Whiteboard: p=8 s=it-31c-30a-29b.3 [qa-] → p=8 [qa-]
Assignee: nobody → mak77
Status: NEW → ASSIGNED
Whiteboard: p=8 [qa-] → p=8 s=33.1 [qa-]
Attached patch part 1 (obsolete) — Splinter Review
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)
Attached patch inline tests v2Splinter Review
Attachment #8442861 - Attachment is obsolete: true
Attachment #8442861 - Flags: review?(mano)
Attachment #8444037 - Flags: review?(mano)
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)
Iteration: --- → 33.2
Points: --- → 8
QA Whiteboard: [qa-]
Whiteboard: p=8 s=33.1 [qa-]
Whiteboard: [search]
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+
(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.
(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...
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.
Depends on: 1041262
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: