Closed Bug 1466490 Opened Last year Closed Last year

Add multi-process xpcshell test for scalar semantics

Categories

(Toolkit :: Telemetry, enhancement, P2)

enhancement

Tracking

()

RESOLVED FIXED
mozilla62
Tracking Status
firefox62 --- fixed

People

(Reporter: Dexter, Assigned: janerik)

References

Details

Attachments

(4 files)

In bug 1454606 we landed the code to preserve the scalar semantics in GeckoView, during measurements deserialization. Test coverage for that was added through gtest, which does not support content processes yet.

We should add xpcshell test coverage for making sure we're correctly preserving the scalar semantics with multiple processes on GeckoView.
Blocks: 1454606, 1452550
Depends on: 1461965
Priority: -- → P2
Assignee: nobody → jrediger
Comment on attachment 8985539 [details]
Bug 1466490 - Actually record non-main scalars on the main process after deserialization/recording.

https://reviewboard.mozilla.org/r/251150/#review257408
Attachment #8985539 - Flags: review?(alessio.placitelli) → review+
Comment on attachment 8985540 [details]
Bug 1466490 - Set Telemetry preference on startup.

https://reviewboard.mozilla.org/r/251152/#review257410

::: toolkit/components/telemetry/TelemetryScalar.h:9
(Diff revision 1)
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  #ifndef TelemetryScalar_h__
>  #define TelemetryScalar_h__
>  
> +#include "nsTArray.h"

mh, why is this needed?
Comment on attachment 8985540 [details]
Bug 1466490 - Set Telemetry preference on startup.

https://reviewboard.mozilla.org/r/251152/#review257412

::: toolkit/components/telemetry/tests/unit/test_GeckoView_ScalarSemantics.js:25
(Diff revision 1)
> + * Run a file in the content process.
> + * @param aFileName - The file to execute in the content process.
> + * @return {Promise} A promise resolved after the execution in the other process
> + *         finishes.
> + */
> +async function run_in_child(aFileName) {

Let's add all the shared functions to a new header file, "head_GeckoView.js".

::: toolkit/components/telemetry/tests/unit/test_GeckoView_ScalarSemantics.js:41
(Diff revision 1)
> +
> +/**
> + * Builds a promise to wait for the GeckoView data loading to finish.
> + * @return {Promise} A promise resolved when the data loading finishes.
> + */
> +function waitGeckoViewLoadComplete() {

Ditto

::: toolkit/components/telemetry/tests/unit/test_GeckoView_ScalarSemantics.js:57
(Diff revision 1)
> + * snapshot of the relevant process.
> + * @param aScalarName - The name of the scalar to look for.
> + * @param aProcessName - The name of the process to look in.
> + * @param aKeyed - Whether or not to look in keyed snapshots.
> + */
> +async function waitForSnapshotData(aScalarName, aProcessName, aKeyed) {

Ditto

::: toolkit/components/telemetry/tests/unit/test_GeckoView_ScalarSemantics.js:81
(Diff revision 1)
> +  let env = Cc["@mozilla.org/process/environment;1"].getService(Ci.nsIEnvironment);
> +  env.set("MOZ_ANDROID_DATA_DIR", profileDir.path);
> +});
> +
> +add_task(async function test_MultiprocessScalarSemantics() {
> +  Telemetry.clearScalars();

Can you add a brief comment about the purpose of this test? Maybe just copy the example from the docs

::: toolkit/components/telemetry/tests/unit/xpcshell.ini:27
(Diff revision 1)
>    system.xpi
>    restartless.xpi
>  
>  [test_GeckoView.js]
>  skip-if = os == "android" # Disabled due to crashes (see bug 1331366)
>  head =

Reference the header file here (and in the other test)
Comment on attachment 8985675 [details]
Bug 1466490 - Extract common test functionality into a header.

https://reviewboard.mozilla.org/r/251210/#review257544
Attachment #8985675 - Flags: review?(alessio.placitelli) → review+
Comment on attachment 8985540 [details]
Bug 1466490 - Set Telemetry preference on startup.

https://reviewboard.mozilla.org/r/251152/#review257556
Attachment #8985540 - Flags: review?(alessio.placitelli) → review+
Pushed by jrediger@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e3891caf027f
Actually record non-main scalars on the main process after deserialization/recording. r=Dexter
https://hg.mozilla.org/integration/autoland/rev/95048dd368eb
Extract common test functionality into a header. r=Dexter
https://hg.mozilla.org/integration/autoland/rev/5f13ed9de8a3
Add multi-process xpcshell test for scalar semantics. r=Dexter
Lint fixed. Looking into test failure, might be blocked by one of the other issues we encountered this week around (de)serialization.
Flags: needinfo?(jrediger)
Attachment #8985540 - Flags: review+ → review?(alessio.placitelli)
Comment on attachment 8985540 [details]
Bug 1466490 - Set Telemetry preference on startup.

https://reviewboard.mozilla.org/r/251152/#review258128

::: toolkit/components/telemetry/tests/unit/head_GeckoView.js:8
(Diff revision 4)
>  */
>  "use strict";
>  
>  ChromeUtils.import("resource://gre/modules/PromiseUtils.jsm", this);
>  ChromeUtils.import("resource://gre/modules/Services.jsm", this);
> +ChromeUtils.import("resource://gre/modules/TelemetryController.jsm", this);

We are not exporting this module on GeckoView, so we should probably not be importing this (even though these xpcshell tests, for now, run on other platforms).

The best way forward would be to move the code to a shared location (e.g. `TelemetryUtils.jsm`?) and use it in both places. Let's not block on this for this bug, but please file a new bug and leave a comment in the code mentioning its number.

::: toolkit/components/telemetry/tests/unit/head_GeckoView.js:69
(Diff revision 4)
> +
> +if (runningInParent) {
> +  // Set logging preferences for all the tests.
> +  Services.prefs.setCharPref("toolkit.telemetry.log.level", "Trace");
> +  // Telemetry archiving should be on.
> +  Services.prefs.setBoolPref(TelemetryUtils.Preferences.ArchiveEnabled, true);

This is not needed for GeckoView.

::: toolkit/components/telemetry/tests/unit/head_GeckoView.js:71
(Diff revision 4)
> +  // Set logging preferences for all the tests.
> +  Services.prefs.setCharPref("toolkit.telemetry.log.level", "Trace");
> +  // Telemetry archiving should be on.
> +  Services.prefs.setBoolPref(TelemetryUtils.Preferences.ArchiveEnabled, true);
> +  // Telemetry xpcshell tests cannot show the infobar.
> +  Services.prefs.setBoolPref(TelemetryUtils.Preferences.BypassNotification, true);

This is not needed for GeckoView.

::: toolkit/components/telemetry/tests/unit/head_GeckoView.js:73
(Diff revision 4)
> +  // Telemetry archiving should be on.
> +  Services.prefs.setBoolPref(TelemetryUtils.Preferences.ArchiveEnabled, true);
> +  // Telemetry xpcshell tests cannot show the infobar.
> +  Services.prefs.setBoolPref(TelemetryUtils.Preferences.BypassNotification, true);
> +  // FHR uploads should be enabled.
> +  Services.prefs.setBoolPref(TelemetryUtils.Preferences.FhrUploadEnabled, true);

This is not needed for GeckoView.

::: toolkit/components/telemetry/tests/unit/head_GeckoView.js:77
(Diff revision 4)
> +  // FHR uploads should be enabled.
> +  Services.prefs.setBoolPref(TelemetryUtils.Preferences.FhrUploadEnabled, true);
> +  // Many tests expect the shutdown and the new-profile to not be sent on shutdown
> +  // and will fail if receive an unexpected ping. Let's globally disable these features:
> +  // the relevant tests will enable these prefs when needed.
> +  Services.prefs.setBoolPref(TelemetryUtils.Preferences.ShutdownPingSender, false);

This is not needed for GeckoView.

::: toolkit/components/telemetry/tests/unit/head_GeckoView.js:78
(Diff revision 4)
> +  Services.prefs.setBoolPref(TelemetryUtils.Preferences.FhrUploadEnabled, true);
> +  // Many tests expect the shutdown and the new-profile to not be sent on shutdown
> +  // and will fail if receive an unexpected ping. Let's globally disable these features:
> +  // the relevant tests will enable these prefs when needed.
> +  Services.prefs.setBoolPref(TelemetryUtils.Preferences.ShutdownPingSender, false);
> +  Services.prefs.setBoolPref(TelemetryUtils.Preferences.ShutdownPingSenderFirstSession, false);

This is not needed for GeckoView.

::: toolkit/components/telemetry/tests/unit/head_GeckoView.js:80
(Diff revision 4)
> +  // and will fail if receive an unexpected ping. Let's globally disable these features:
> +  // the relevant tests will enable these prefs when needed.
> +  Services.prefs.setBoolPref(TelemetryUtils.Preferences.ShutdownPingSender, false);
> +  Services.prefs.setBoolPref(TelemetryUtils.Preferences.ShutdownPingSenderFirstSession, false);
> +  Services.prefs.setBoolPref("toolkit.telemetry.newProfilePing.enabled", false);
> +  Services.prefs.setBoolPref(TelemetryUtils.Preferences.FirstShutdownPingEnabled, false);

This is not needed for GeckoView.

::: toolkit/components/telemetry/tests/unit/head_GeckoView.js:82
(Diff revision 4)
> +  Services.prefs.setBoolPref(TelemetryUtils.Preferences.ShutdownPingSender, false);
> +  Services.prefs.setBoolPref(TelemetryUtils.Preferences.ShutdownPingSenderFirstSession, false);
> +  Services.prefs.setBoolPref("toolkit.telemetry.newProfilePing.enabled", false);
> +  Services.prefs.setBoolPref(TelemetryUtils.Preferences.FirstShutdownPingEnabled, false);
> +  // Turn off Health Ping submission.
> +  Services.prefs.setBoolPref(TelemetryUtils.Preferences.HealthPingEnabled, false);

This is not needed for GeckoView.

::: toolkit/components/telemetry/tests/unit/head_GeckoView.js:89
(Diff revision 4)
> +  // Non-unified Telemetry (e.g. Fennec on Android) needs the preference to be set
> +  // in order to enable Telemetry.
> +  if (Services.prefs.getBoolPref(TelemetryUtils.Preferences.Unified, false)) {
> +    Services.prefs.setBoolPref(TelemetryUtils.Preferences.OverridePreRelease, true);
> +  } else {
> +    Services.prefs.setBoolPref(TelemetryUtils.Preferences.TelemetryEnabled, true);

I don't think we need the `Unified` check, that's Fennec specific. If we need `TelemetryEnabled` to be true, let's always set it to true.

::: toolkit/components/telemetry/tests/unit/head_GeckoView.js:93
(Diff revision 4)
> +  } else {
> +    Services.prefs.setBoolPref(TelemetryUtils.Preferences.TelemetryEnabled, true);
> +  }
> +}
> +
> +TelemetryController.testInitLogging();

Do we really need this?
Comment on attachment 8986243 [details]
Bug 1466490 - Add multi-process xpcshell test for scalar semantics.

https://reviewboard.mozilla.org/r/251634/#review258134
Attachment #8986243 - Flags: review?(alessio.placitelli) → review+
Comment on attachment 8985540 [details]
Bug 1466490 - Set Telemetry preference on startup.

https://reviewboard.mozilla.org/r/251152/#review258152
Attachment #8985540 - Flags: review?(alessio.placitelli) → review+
Pushed by jrediger@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/6870eb4e9daa
Actually record non-main scalars on the main process after deserialization/recording. r=Dexter
https://hg.mozilla.org/integration/autoland/rev/cd3ecb04a7bc
Extract common test functionality into a header. r=Dexter
https://hg.mozilla.org/integration/autoland/rev/d282d6a15965
Set Telemetry preference on startup. r=Dexter
https://hg.mozilla.org/integration/autoland/rev/eb0d3d9a16c6
Add multi-process xpcshell test for scalar semantics. r=Dexter
You need to log in before you can comment on or make changes to this bug.