1169159 - Add basic test coverage for Telemetry child payloads

Status: RESOLVED FIXED

(Toolkit :: Telemetry, defect)

Description

[Georg Fritzsche [:gfritzsche]]

Bug 1156857 has nearly complete tests:
* attachment 8599270 [details] [diff] [review]
* attachment 8599262 [details] [diff] [review]

Comment 1

[Alessio Placitelli [:Dexter]]

Attachment: part 1 - Make xpcshells run_test_in_child() and do_await_remote_message() return promises

This is basically like the original patch, I've just changed a comment and added a statement (outlined in the diff file).

Comment 2

[Alessio Placitelli [:Dexter]]

Attachment: part 1 - Diff

Comment 3

[Alessio Placitelli [:Dexter]]

Attachment: diff1.diff

Comment 4

[Alessio Placitelli [:Dexter]]

Attachment: part 2 - Add test coverage for child payload

Comment 5

[Alessio Placitelli [:Dexter]]

Attachment: diff part 2

Comment 6

[Alessio Placitelli [:Dexter]]

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 7

[Georg Fritzsche [:gfritzsche]]

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"

Comment 8

[Alessio Placitelli [:Dexter]]

Attachment: part 1 - Make xpcshells run_test_in_child() and do_await_remote_message() return promises - v2

Updated the commit message.

Comment 9

[Alessio Placitelli [:Dexter]]

Attachment: part 2 - Add test coverage for child payload - v2

This patch removes unneded changes to TelemetrySession (removes |setupContent| and drops the changes to |setupContentProcess|).

Comment 10

[Alessio Placitelli [:Dexter]]

Attachment: part 1 - Make xpcshells run_test_in_child() and do_await_remote_message() return promises - v3

This fixes the test fallout (callers of |do_await_remote_message| don't have to explicitly call do_test_finished).

Comment 11

[Alessio Placitelli [:Dexter]]

Attachment: part 2 - Add test coverage for child payload - v3

This fixes some other test fallout by introducing a light setup for content processes in tests.

Comment 12

[Alessio Placitelli [:Dexter]]

Attachment: part 3 - Refactor enableTelemetryRecording

This refactors enableTelemetryRecording so that it works without assuming particular default values.

Comment 13

[Georg Fritzsche [: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.

Comment 14

[Georg Fritzsche [: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.

Comment 15

[Alessio Placitelli [:Dexter]]

Attachment: part 3 - Refactor enableTelemetryRecording - v2

Comment 16

[Alessio Placitelli [:Dexter]]

Attachment: part 2 - Add test coverage for child payload - v3

Updated to fix the usage of |do_await_remote_message| in tests.

Comment 17

[Georg Fritzsche [: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 ..."

Comment 18

[Alessio Placitelli [:Dexter]]

Attachment: part 3 - Refactor enableTelemetryRecording - v3

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c56f5cf8c3f5

Comment 19

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/dd3e7d950ad5
https://hg.mozilla.org/integration/fx-team/rev/1a73250cd62d
https://hg.mozilla.org/integration/fx-team/rev/eeb332cac64e

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/dd3e7d950ad5
https://hg.mozilla.org/mozilla-central/rev/1a73250cd62d
https://hg.mozilla.org/mozilla-central/rev/eeb332cac64e