1151994 - test_telemetryPing.js in asyncSetup if datareporting.healthreport.service.enabled is undefined or false

Status: RESOLVED FIXED

(Toolkit :: Telemetry, defect)

Description

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Attachment: patch, v1

Since bug 1134279 landed, the https://dxr.mozilla.org/mozilla-central/source/toolkit/components/telemetry/tests/unit/test_TelemetryPing.js (test_TelemetrySendOldPings.js also fails, but I haven't investigated yet).

This happens because datareporting.healthreport.service.enabled doesn't exist (likely yet). The constant for this pref has been defined in the file, but it doesn't get turned on (because it's enabled on all branches using healthreport). So this patch set the preferences explicitly to true.

Comment 1

[Georg Fritzsche [:gfritzsche]]

What are the failures?
I can review this too, but i'd like to understand what happens.

Comment 2

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

The tests fails for Thunderbird builds which ships without data reporting atm, see https://treeherder.mozilla.org/#/jobs?repo=comm-central&filter-searchStr=Ubuntu VM 12.04 comm-central opt test xpcshell

 TEST-UNEXPECTED-FAIL | toolkit/components/telemetry/tests/unit/test_TelemetryPing.js | xpcshell return code: 0

TEST-UNEXPECTED-FAIL | toolkit/components/telemetry/tests/unit/test_TelemetryPing.js | asyncSetup - [asyncSetup : 185] null == "e008dcfc-2f94-400b-8d18-fc9995e4d1e4"

What happens:

TelemetryPing.clientId is null: http://hg.mozilla.org/mozilla-central/file/078128c2600a/toolkit/components/telemetry/tests/unit/test_TelemetryPing.js#l185

This is derived from Impl._clientID. This would get set in http://hg.mozilla.org/mozilla-central/file/078128c2600a/toolkit/components/telemetry/TelemetryPing.jsm#l786 or even later in _delayedInitTask, but there is a check |!this.enableTelemetryRecording(testing)| before at http://hg.mozilla.org/mozilla-central/file/078128c2600a/toolkit/components/telemetry/TelemetryPing.jsm#l777

enableTelemetryRecording returns always false for environments without datareporting.healthreport.service.enabled http://hg.mozilla.org/mozilla-central/file/078128c2600a/toolkit/components/telemetry/TelemetryPing.jsm#l706 (but Firefox has it always set to true).

Comment 3

[Vladan Djeric (:vladan)]

Comment on attachment 8589244 [details] [diff] [review]
patch, v1

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

::: toolkit/components/telemetry/tests/unit/test_TelemetryPing.js
@@ +175,5 @@
>    loadAddonManager("[e-mail]", "XPCShell", "1", "1.9.2");
>  
>    Services.prefs.setBoolPref(PREF_ENABLED, true);
>    Services.prefs.setBoolPref(PREF_FHR_UPLOAD_ENABLED, true);
> +  Services.prefs.setBoolPref(PREF_FHR_SERVICE_ENABLED, true);

So Thunderbird doesn't have the FHR service but it has the SessionRecorder?

Comment 4

[Georg Fritzsche [:gfritzsche]]

(In reply to Archaeopteryx [:aryx] from comment #2)
> enableTelemetryRecording returns always false for environments without
> datareporting.healthreport.service.enabled
> http://hg.mozilla.org/mozilla-central/file/078128c2600a/toolkit/components/
> telemetry/TelemetryPing.jsm#l706 (but Firefox has it always set to true).

Alessio, is this getting fixed in an active bug?
We don't want Telemetry to be off when we are disabling FHR.

Comment 5

[Alessio Placitelli [:Dexter]]

No, this isn't. I claim we should probably introduce a preference that enables/disables Telemetry base recording (opt-out) and stop relying on the FHR prefs. This would also fix bug 1148500.

Comment 6

[Georg Fritzsche [:gfritzsche]]

Ok, i filed bug 1153252 on this.

(In reply to Vladan Djeric (:vladan) -- please needinfo! from comment #3)
> ::: toolkit/components/telemetry/tests/unit/test_TelemetryPing.js
> @@ +175,5 @@
> >    loadAddonManager("[e-mail]", "XPCShell", "1", "1.9.2");
> >  
> >    Services.prefs.setBoolPref(PREF_ENABLED, true);
> >    Services.prefs.setBoolPref(PREF_FHR_UPLOAD_ENABLED, true);
> > +  Services.prefs.setBoolPref(PREF_FHR_SERVICE_ENABLED, true);
> 
> So Thunderbird doesn't have the FHR service but it has the SessionRecorder?

Yes, we moved the SessionRecorder (bug 1147522) and client id implementation (bug 1137353) so that we still can use them when we turn FHR off.

Given the above i think we should flip the pref on for now (with a TODO comment pointing to bug 1153252). Sound ok vladan?

Comment 7

[Vladan Djeric (:vladan)]

Comment on attachment 8589244 [details] [diff] [review]
patch, v1

Sounds good

Comment 8

[Magnus Melin [:mkmelin]]

I landed this for you on inbound - https://hg.mozilla.org/integration/mozilla-inbound/rev/8e5fd409bfeb

Comment 9

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/8e5fd409bfeb

Comment 10

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8589244 [details] [diff] [review]
patch, v1

Approved for Aurora. For approval request see bug 1139460 comment 42. For approval comments see bug 1139460 comment 43.

Comment 11

[Georg Fritzsche [:gfritzsche]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/7dee328009b7