1092357 - Update search tests to check search telemetry

Status: RESOLVED WORKSFORME

(Firefox :: Search, defect)

Description

[Georg Fritzsche [:gfritzsche]]

We have test-coverage of the "org.mozilla.searches" FHR provider across some files:
http://dxr.mozilla.org/mozilla-central/search?q=org.mozilla.searches+path%3Atest&case=true

We need to update this to cover the "SEARCH_COUNTS" & "SEARCH_DEFAULT_ENGINE" probes as well.

Comment 1

[Georg Fritzsche [:gfritzsche]]

Attachment: Update search tests to cover the telemetry probes

Comment 2

[Georg Fritzsche [:gfritzsche]]

Note that i did a bigger rewrite of browser_search_datacollection.js to have a more readable linear test instead of callbacks and promise.then() nesting.

Comment 3

[Georg Fritzsche [:gfritzsche]]

... i mean browser_datacollection.js.

Comment 4

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 8519770 [details] [diff] [review]
Update search tests to cover the telemetry probes

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

Mostly good.  There's one thing that should really be fixed, and a couple of other style notes, but I trust you can fix them.  :)

r=me!

::: browser/base/content/test/general/browser_aboutHome.js
@@ +143,5 @@
>          searchEventDeferred.resolve();
>        });
> +
> +      let num = getNumberOfSearchesFromTelemetry(engineName);
> +      is(num, numSearchesBeforeTelemetry + 1, "One more search recorded in Telemetry.");

Since this is synchronous, and the FHR code above is asynchronous, I feel that the function would more closely match reality if we moved this check up above the FHR check.

::: browser/base/content/test/general/browser_contextSearchTabPosition.js
@@ +3,5 @@
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  function test() {
> +  // Will need to be changed if Google isn't the default search engine.
> +  const SEARCH_ID = "google.contextmenu";

Would it make sense (or is it even possible) to generate this SEARCH_ID from the default search engine programmatically?

(I realize this is code you've copied from below, but if we can fix it now…  ;)

::: browser/base/content/test/general/browser_urlbar_search_datacollection.js
@@ +13,5 @@
> +
> +  // We should record the default search engine in FHR if Telemetry is enabled.
> +  let oldTelemetry = Services.prefs.getBoolPref("toolkit.telemetry.enabled");
> +  Services.prefs.setBoolPref("toolkit.telemetry.enabled", true);
> +  registerCleanupFunction(() => Services.prefs.setBoolPref("toolkit.telemetry.enabled", true));

Shouldn't we be resetting this to oldTelemetry, not "true"?

@@ +46,2 @@
>  
> +  let oldCountFHR = 0;

To be consistent with the variable above, how about naming this "oldSearchCountFHR"?  (Or the other variable "oldCountTelemetry"…)

::: browser/components/search/test/browser_datacollection.js
@@ +123,3 @@
>  
> +  EventUtils.synthesizeKey("VK_RETURN", {});
> +  yield new Promise((resolve) => executeSoon(() => executeSoon(resolve)));

Just out of curiousity, do you know why we're calling executeSoon twice?

Comment 5

[Georg Fritzsche [:gfritzsche]]

Ah, thanks for the catches, looking into them tomorrow.

(In reply to Blake Winton (:bwinton) from comment #4)
> > +  EventUtils.synthesizeKey("VK_RETURN", {});
> > +  yield new Promise((resolve) => executeSoon(() => executeSoon(resolve)));
> 
> Just out of curiousity, do you know why we're calling executeSoon twice?

Sadly no and i didn't get to digging there. Seems like a line that is based on internal search service or browser components behavior (?) and could really have used an explanatory comment.

Comment 6

[Georg Fritzsche [:gfritzsche]]

I don't remember why this bug was (sadly) orphaned.
Are the additions here even still useful or is this already covered?

Comment 7

[Alessio Placitelli [:Dexter]]

As far as I can see, the changes to browser_aboutHome are not useful anymore, as we now have the test coverage here:

https://dxr.mozilla.org/mozilla-central/rev/ade8d4a63e57560410de106450f37b50ed71cca5/browser/base/content/test/general/browser_aboutHome.js#93,120

Same for browser_contextSearchTabPosition.js :

https://dxr.mozilla.org/mozilla-central/rev/ade8d4a63e57560410de106450f37b50ed71cca5/browser/components/search/test/browser_contextSearchTabPosition.js#12,51

And  browser_urlbar_search_datacollection.js:

https://dxr.mozilla.org/mozilla-central/rev/ade8d4a63e57560410de106450f37b50ed71cca5/browser/base/content/test/urlbar/browser_urlbarSearchTelemetry.js#132,163

And the last bits seem to be here:

https://dxr.mozilla.org/mozilla-central/source/browser/components/search/test/browser_healthreport.js#18

It looks like we got that stuff covered?

Comment 8

[Georg Fritzsche [:gfritzsche]]

Ok, thanks.