1173751 - Add unit tests for search suggestions in awesomebar

Status: RESOLVED FIXED

(Firefox :: Address Bar, defect, P1)

Description

[Marco Bonardo [:mak]]

We must write unit tests, some may require bug 1173748.

We need at least to check the pref properly disables these results, and that we fetch them.

Comment 1

[Drew Willcoxon :adw]

Attachment: UnifiedComplete xpcshell test

Not sure there's much more than this to test at the moment.

Comment 2

[Drew Willcoxon :adw]

unifiedcomplete/ tests pass locally, but here's a try run anyway:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=fafef85e5505

Comment 3

[Marco Bonardo [:mak]]

Comment on attachment 8621888 [details] [diff] [review]
UnifiedComplete xpcshell test

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

::: toolkit/components/places/tests/unifiedcomplete/head_autocomplete.js
@@ +428,5 @@
> +      resolve(engine);
> +    }, "browser-search-engine-modified", false);
> +
> +    do_print("Adding engine from URL: " + dataUrl + basename);
> +    Services.search.addEngine(dataUrl + basename,

could you please make us wait for async Services.search.init before using the service?

::: toolkit/components/places/tests/unifiedcomplete/test_searchSuggestions.js
@@ +1,2 @@
> +/* Any copyright is dedicated to the Public Domain.
> + * http://creativecommons.org/publicdomain/zero/1.0/ */

fwiw, it's no more required to put license header in tests code, they default to PD

@@ +19,5 @@
> +    resp.write(JSON.stringify(data));
> +  });
> +
> +  // Install the test engine.
> +  let oldCurrentEngine = Services.search.currentEngine;

also here, let's be good citizen and wait for init()

@@ +62,5 @@
> +      }),
> +      title: ENGINE_NAME,
> +      style: ["action", "searchengine"],
> +      icon: "",
> +    }],

maybe add a multi word test, just in case.
also a test that doesn't match at the beginning and check there's no results (we should only match at the beginning)

Comment 4

[Pulsebot]

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

Comment 5

[Drew Willcoxon :adw]

(In reply to Marco Bonardo [::mak] (Away June 17-22 - then ping me in Whistler) from comment #3)
> could you please make us wait for async Services.search.init before using
> the service?

head_autocomplete.js has one add_task at the bottom where it makes sure a search engine exists, so I added the init() to that.  I had to set browser.search.geoip.url to an empty string like other tests do.

> fwiw, it's no more required to put license header in tests code, they
> default to PD

Removed.

> maybe add a multi word test, just in case.

Added.

> also a test that doesn't match at the beginning and check there's no results
> (we should only match at the beginning)

I'm not sure what you mean.  The only thing I can think of is that you mean a search string like "foo" shouldn't result in suggestions like "bar foo" -- suggestions that don't start with foo?  But right now we don't filter out suggestions that don't match the search string at the beginning, and it's up to the search provider to determine what the suggestions are.

Since I'm not clear on that, I added a test that checks what I just described, that when a search for "hello" produces the suggestions "bar hello" and "quux hello", the suggestions appear in the results.  We can change the test later if necessary.

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/31fbd3b1027c

Comment 7

[Marco Bonardo [:mak]]

(In reply to Drew Willcoxon :adw from comment #5)
> I'm not sure what you mean.  The only thing I can think of is that you mean
> a search string like "foo" shouldn't result in suggestions like "bar foo" --
> suggestions that don't start with foo?  But right now we don't filter out
> suggestions that don't match the search string at the beginning, and it's up
> to the search provider to determine what the suggestions are.

That's interesting, cause in the mockups we are autofilling suggestions in the textfield... but since for now we don't do that, will be fine.