Closed Bug 1173751 Opened 5 years ago Closed 5 years ago

Add unit tests for search suggestions in awesomebar

Categories

(Firefox :: Address Bar, defect, P1)

defect
Points:
3

Tracking

()

RESOLVED FIXED
Firefox 41
Tracking Status
firefox41 --- fixed

People

(Reporter: mak, Assigned: adw)

References

Details

(Whiteboard: [suggestions][fxsearch])

Attachments

(1 file)

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.
Flags: qe-verify-
Flags: firefox-backlog+
Assignee: nobody → adw
Status: NEW → ASSIGNED
Rank: 7
Not sure there's much more than this to test at the moment.
Attachment #8621888 - Flags: review?(mak77)
unifiedcomplete/ tests pass locally, but here's a try run anyway:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=fafef85e5505
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)
Attachment #8621888 - Flags: review?(mak77) → review+
(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.
https://hg.mozilla.org/mozilla-central/rev/31fbd3b1027c
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 41
Depends on: 1176215
(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.
You need to log in before you can comment on or make changes to this bug.