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

RESOLVED FIXED in Firefox 39

Status

()

Toolkit
Telemetry
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: aryx, Assigned: aryx)

Tracking

Trunk
mozilla40
x86
Windows 8.1
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox38 unaffected, firefox39 fixed, firefox40 fixed)

Details

Attachments

(1 attachment)

Created attachment 8589244 [details] [diff] [review]
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.
Attachment #8589244 - Flags: review?(vdjeric)
What are the failures?
I can review this too, but i'd like to understand what happens.
Flags: needinfo?(archaeopteryx)
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).
Flags: needinfo?(archaeopteryx)
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("xpcshell@tests.mozilla.org", "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?
Flags: needinfo?(archaeopteryx)
(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.
Flags: needinfo?(alessio.placitelli)
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.
Flags: needinfo?(alessio.placitelli)
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("xpcshell@tests.mozilla.org", "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?
Flags: needinfo?(archaeopteryx)
Comment on attachment 8589244 [details] [diff] [review]
patch, v1

Sounds good
Attachment #8589244 - Flags: review?(vdjeric) → review+
Blocks: 1154737

Comment 8

3 years ago
I landed this for you on inbound - https://hg.mozilla.org/integration/mozilla-inbound/rev/8e5fd409bfeb
https://hg.mozilla.org/mozilla-central/rev/8e5fd409bfeb
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox40: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
status-firefox38: --- → unaffected
status-firefox39: --- → affected
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.
Attachment #8589244 - Flags: approval-mozilla-aurora+
https://hg.mozilla.org/releases/mozilla-aurora/rev/7dee328009b7
status-firefox39: affected → fixed
You need to log in before you can comment on or make changes to this bug.