1041534 - Refactor search tests to remove some code duplication

Status: RESOLVED FIXED

(Firefox :: Search, defect)

Description

[:Paolo Amadini]

Attachment: The patch

This patch removes some code duplication in the search service tests, in preparation for the addition of new tests in bug 1040721.

It also fixes a failure resulting from two of the tests not having been converted to use a dynamic HTTP port.

Comment 1

[Matthew N. [:MattN]]

Comment on attachment 8459589 [details] [diff] [review]
The patch

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

Thanks for doing this. It's something that has bothered me in the past. I would also like to use it for bug 1007979 which would mean uplifting to 33.

r=me assuming this passes Try.

::: toolkit/components/search/tests/xpcshell/head_search.js
@@ +167,5 @@
> +  let httpServer = new HttpServer();
> +  httpServer.start(-1);
> +  httpServer.registerDirectory("/", do_get_cwd());
> +  gDataUrl = "http://localhost:" + httpServer.identity.primaryPort + "/data/";
> +  do_register_cleanup(() => httpServer.stop(() => {}));

Nit: Could you return the server from the function so that consumers can register other things e.g. registerContentType

@@ +185,5 @@
> + *                   except for the engine name.  Alternative to xmlFileName.
> + *        }
> + */
> +function addTestEngines(aItems) {
> +  return new Promise((resolve, reject) => {

Ideally this function would also register a cleanup function to remove the engines after. We currently delete leftovers at the beginning of every test but we shouldn't rely on that.

Comment 2

[Matthew N. [:MattN]]

Comment on attachment 8459589 [details] [diff] [review]
The patch

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

::: toolkit/components/search/tests/xpcshell/head_search.js
@@ +224,5 @@
> +        Services.search.addEngine(gDataUrl + item.srcFileName,
> +                                  Ci.nsISearchEngine.DATA_TEXT,
> +                                  gDataUrl + item.iconFileName, false);
> +      } else {
> +        Services.search.addEngineWithDetails(item.name, ...item.details)

Nit: missing semicolon.

Comment 3

[Matthew N. [:MattN]]

Comment on attachment 8459589 [details] [diff] [review]
The patch

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

::: toolkit/components/search/tests/xpcshell/head_search.js
@@ +217,5 @@
> +
> +    for (let item of aItems) {
> +      do_print("Adding engine: " + item.name);
> +      if (item.xmlFileName) {
> +        Services.search.addEngine(gDataUrl + item.xmlFileName,

This method (addTestEngines) relies on gDataURL from useHttpServer which may be non-obvious. I think this method should throw if it tries to use gDataURL and it's undefined.

Comment 4

[:Paolo Amadini]

(In reply to Matthew N. [:MattN] from comment #1)
> Nit: Could you return the server from the function so that consumers can
> register other things e.g. registerContentType

Good idea, will do, though if you only need to do registerContentType("sjs", "sjs") I recommend doing that in the head function itself, so that all tests run in the same environment.

I think in the future useHttpServer should be implicit for all tests, and the same for environment initialization and cleanup, but I haven't worked on it in this bug to avoid scope creep. Only a bunch of tests don't add new engines.

> @@ +185,5 @@
> > +function addTestEngines(aItems) {
> > +  return new Promise((resolve, reject) => {
> 
> Ideally this function would also register a cleanup function to remove the
> engines after. We currently delete leftovers at the beginning of every test
> but we shouldn't rely on that.

Filed bug 1043379.

Comment 5

[:Paolo Amadini]

https://tbpl.mozilla.org/?tree=Try&rev=497e8dde6702

Comment 6

[:Paolo Amadini]

Attachment: Updated patch

Comment 7

[:Paolo Amadini]

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

Comment 8

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/3065e143a627

Comment 9

[Drew Willcoxon :adw]

Comment on attachment 8461549 [details] [diff] [review]
Updated patch

This is applies cleanly to Aurora/33.

Bug 1054516 is the uplift bug for this feature.

mozilla-aurora try push with this and patches for other bugs mentioned in bug 1054516: https://tbpl.mozilla.org/?tree=Try&rev=fba51f37ff40

Approval Request Comment
[Feature/regressing bug #]:
this bug is needed by bug 1007979, which is necessary to uplift search suggestions in about:home/newtab (bug 612453, bug 1028985)

[User impact if declined]:
no search suggestions in about:home/newtab

[Describe test coverage new/current, TBPL]:
this patch itself is tests, which are running on 34 already; see also try link above

[Risks and why]:
low risk, tests only; we want about:home/newtab search suggestions on 33 as stated in bug 1054516

[String/UUID change made/needed]:
none

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/83a0ea7613e5