Closed Bug 1169159 Opened 4 years ago Closed 4 years ago

Add basic test coverage for Telemetry child payloads

Categories

(Toolkit :: Telemetry, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla41
Tracking Status
firefox41 --- fixed

People

(Reporter: gfritzsche, Assigned: Dexter)

References

Details

Attachments

(3 files, 10 obsolete files)

4.29 KB, patch
Dexter
: review+
Details | Diff | Splinter Review
13.10 KB, patch
Dexter
: review+
gfritzsche
: feedback+
Details | Diff | Splinter Review
2.39 KB, patch
Dexter
: review+
Details | Diff | Splinter Review
No longer blocks: 1156857
Depends on: 1156857
This is basically like the original patch, I've just changed a comment and added a statement (outlined in the diff file).
Assignee: nobody → alessio.placitelli
Status: NEW → ASSIGNED
Attachment #8614628 - Flags: review?(gfritzsche)
Attached patch part 1 - Diff (obsolete) — Splinter Review
Attached patch diff1.diff (obsolete) — Splinter Review
Attachment #8614629 - Attachment is obsolete: true
Attachment #8614683 - Flags: review?(gfritzsche)
Attached patch diff part 2 (obsolete) — Splinter Review
A note about part 2: most of the changes in the diff are due to rebasing. I've only added |setupContent| and changed |setupContentProcess| a bit.
Comment on attachment 8614628 [details] [diff] [review]
part 1 - Make xpcshells run_test_in_child() and do_await_remote_message() return promises

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

I guess this is now "r=ted,gfritzsche"
Attachment #8614628 - Flags: review?(gfritzsche) → review+
Updated the commit message.
Attachment #8614628 - Attachment is obsolete: true
Attachment #8615935 - Flags: review+
This patch removes unneded changes to TelemetrySession (removes |setupContent| and drops the changes to |setupContentProcess|).
Attachment #8614683 - Attachment is obsolete: true
Attachment #8614685 - Attachment is obsolete: true
Attachment #8614683 - Flags: review?(gfritzsche)
Attachment #8616557 - Flags: review?(gfritzsche)
Attachment #8616557 - Flags: review?(gfritzsche) → review+
This fixes the test fallout (callers of |do_await_remote_message| don't have to explicitly call do_test_finished).
Attachment #8614681 - Attachment is obsolete: true
Attachment #8615935 - Attachment is obsolete: true
Attachment #8620967 - Flags: review+
Attachment #8620967 - Flags: feedback?(gfritzsche)
This fixes some other test fallout by introducing a light setup for content processes in tests.
Attachment #8616557 - Attachment is obsolete: true
Attachment #8620969 - Flags: review+
Attachment #8620969 - Flags: feedback?(gfritzsche)
This refactors enableTelemetryRecording so that it works without assuming particular default values.
Attachment #8620970 - Flags: review?(gfritzsche)
Comment on attachment 8620970 [details] [diff] [review]
part 3 - Refactor enableTelemetryRecording

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

::: toolkit/components/telemetry/TelemetryController.jsm
@@ +595,5 @@
>  
>  #ifdef MOZILLA_OFFICIAL
> +    // If we can't send data, no point in initializing observers etc.
> +    // Only do this for official builds so that e.g. developer builds
> +    // still enable Telemetry based on prefs.

This is not actually controlling whether we send data.
Let's fix the comment here.

@@ +605,3 @@
>  #endif
>  
> +    if (!Telemetry.canRecordExtended) {

We don't need the if here. We can just always log what we set canRecordBase and *Extended to.
Attachment #8620970 - Flags: review?(gfritzsche)
Comment on attachment 8620967 [details] [diff] [review]
part 1 - Make xpcshells run_test_in_child() and do_await_remote_message() return promises - v3

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

::: testing/xpcshell/head.js
@@ +1282,5 @@
> +          resolve();
> +          if (optionalCallback) {
> +            optionalCallback();
> +          }
> +          do_test_finished();

If we always call do_test_finished() we can only use do_await_remote_message() once and won't be able to add other test tasks later.
Let's make the behavior at least a bit more sane.
Attachment #8620967 - Flags: feedback?(gfritzsche)
Attachment #8620970 - Attachment is obsolete: true
Attachment #8620995 - Flags: review?(gfritzsche)
Attachment #8615935 - Attachment is obsolete: false
Attachment #8620967 - Attachment is obsolete: true
Updated to fix the usage of |do_await_remote_message| in tests.
Attachment #8620969 - Attachment is obsolete: true
Attachment #8620969 - Flags: feedback?(gfritzsche)
Attachment #8621010 - Flags: review+
Attachment #8621010 - Flags: feedback?(gfritzsche)
Comment on attachment 8620995 [details] [diff] [review]
part 3 - Refactor enableTelemetryRecording - v2

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

::: toolkit/components/telemetry/TelemetryController.jsm
@@ +595,5 @@
>  
>  #ifdef MOZILLA_OFFICIAL
> +    // If we're not on official telemetry nor in test mode, don't record the extended
> +    // telemetry data. We only do this for official builds so that e.g. developer builds
> +    // still enable Telemetry based on prefs.

"Enable extended telemetry if:
* the telemetry preference is set and
* this is an official buil or we are in test-mode
We only do the latter check for official ..."
Attachment #8620995 - Flags: review?(gfritzsche) → review+
Attachment #8621010 - Flags: feedback?(gfritzsche) → feedback+
https://hg.mozilla.org/mozilla-central/rev/dd3e7d950ad5
https://hg.mozilla.org/mozilla-central/rev/1a73250cd62d
https://hg.mozilla.org/mozilla-central/rev/eeb332cac64e
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
You need to log in before you can comment on or make changes to this bug.