1065191 - Add test coverage for the style property of places autocomplete results

Status: RESOLVED FIXED

(Toolkit :: Places, defect)

Description

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Test coverage for the style property of places autocomplete items is woefully lacking - given UnifiedComplete, and an upcoming autocomplete controller rewrite, we really need to expand coverage in such areas.

Comment 1

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Marco: Yet another followup I'll pickup immediately.

Comment 2

[Marco Mucci [:MarcoM]]

Added to IT 35.1

(In reply to Blair McBride [:Unfocused] from comment #1)
> Marco: Yet another followup I'll pickup immediately.

Comment 3

[Mike Hoye [:mhoye]]

Unfocused: Would you be willing to mentor a contributor through this, and flesh out the description a bit?

Comment 4

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Yes indeed!

So what we want is a new test added in toolkit/components/places/test/unifiedcomplete/, which tries a series of queries that will result in different types of results:
* autofill
* history
* bookmark
* tagged bookmark
* bookmark with keyword
* switch-to-tab
* search engine
* normal (but never visited/bookmarked) URL
* matching a previous search via a search engine

And ensures that the .style property on each result is correct.

Looking through _adjustAcItem in autocomplete.xml will likely help with understanding how the .style property (set as "type" in that binding) relates to the different types of results.

Comment 5

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Want this done ASAP, so taking this for myself.

Comment 6

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Attachment: MozReview Request: bz://1065191/Unfocused

Comment 7

[Blair McBride [:Unfocused] (UNAVAILABLE)]

/r/359 - Bug 1065191 - Add test coverage for the style property of places autocomplete results.

Pull down this commit:

hg pull review -r e830269b3b1efc655c0317a240551dfd3ef96265

Comment 8

[Blair McBride [:Unfocused] (UNAVAILABLE)]

/r/359 - Bug 1065191 - Add test coverage for the style property of places autocomplete results.

Pull down this commit:

hg pull review -r e830269b3b1efc655c0317a240551dfd3ef96265

Comment 9

[:Paolo Amadini]

https://reviewboard.mozilla.org/r/357/#review173

Looks good!

::: toolkit/components/places/tests/unifiedcomplete/head_autocomplete.js
(Diff revision 1)
> +          let actualStyle = normalizeStyle(controller.getStyleAt(i));

I'd not use a helper function here (and the name is slightly misleading, because it takes a string and returns an array rather than a normlaized string).

::: toolkit/components/places/tests/unifiedcomplete/head_autocomplete.js
(Diff revision 1)
> +            Assert.equal(actualStyle.toString(), style.toString(), "Match should have expected style");

nit: I think xpcshell tests report line numbers of failures, so the description in the third parameter is unnecessary. Same below.

Comment 10

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to :Paolo Amadini from comment #9)
> nit: I think xpcshell tests report line numbers of failures, so the
> description in the third parameter is unnecessary. Same below.

The message is used. eg:

 0:01.37 TEST_STATUS: Thread-1  FAIL Match should have expected style - "action,visiturl" == "action,visiturldfdff"
    d:/mozilla/central/obj-i686-pc-mingw32/_tests/xpcshell/toolkit/components/places/tests/unifiedcomplete/head_autocomplete.js:check_autocomplete:177

I lot of xpcshell tests don't provide a message, and it makes it much harder to deal with.

Comment 11

[Blair McBride [:Unfocused] (UNAVAILABLE)]

https://hg.mozilla.org/integration/fx-team/rev/fb67e4e32ef2

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/fb67e4e32ef2