1470208 - Add an explicit automated test for the telemetry histogram related to the storage.local data migration failures

Status: RESOLVED FIXED

(WebExtensions :: Storage, enhancement, P3)

Description

[Luca Greco [:rpl] [:luca] [:lgreco]]

In Bug 1465129 we are adding a telemetry histogram to keep an eye on the outcome of the storage.local data migrations, the patch contains automated tests for checking that the "success" category is incremented in the new histogram when a migration is completed and the IDB backend is being enabled, but there is currently no automated test for the "failure" category.

I've a test locally for testing the "failure" scenario, but it is not yet part of that patch mostly because I'm not really happy about the way that "failure" scenario is triggered in that test and I'd like to find a better way to trigger it.

In the meantime I'm going to attach to this bugzilla issue the version of the test that I'm running locally.

Comment 1

[Luca Greco [:rpl] [:luca] [:lgreco]]

Attachment: Bug 1470208 - Add an automated test for the telemetry collected on storage.local data migration failures.

Review commit: https://reviewboard.mozilla.org/r/252074/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/252074/

Comment 2

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8986827 [details]
Bug 1470208 - Add an automated test for the telemetry collected on storage.local data migration failures.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/252074/diff/1-2/

Comment 3

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8986827 [details]
Bug 1470208 - Add an automated test for the telemetry collected on storage.local data migration failures.

https://reviewboard.mozilla.org/r/252072/#review258708

::: toolkit/components/extensions/test/xpcshell/test_ext_storage_idb_data_migration.js:253
(Diff revision 2)
> +  assertTelemetryAddonDetails(EXTENSION_ID, {
> +    backend: "JSONFile",
> +    dataMigrateError: "DataCloneError: The object could not be cloned.",
> +  });
> +
> +  await extension.unload();
> +
> +  assertMigrationHistogramCount("success", 0);
> +  assertMigrationHistogramCount("failure", 1);
> +});

This additional test case verify that the collected per-addon details (backend and the last data migration error message) are the expected one and that the failure category of the data migration histogram is incremented when a data migration fails.

Comment 4

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8986827 [details]
Bug 1470208 - Add an automated test for the telemetry collected on storage.local data migration failures.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/252074/diff/2-3/

Comment 5

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8986827 [details]
Bug 1470208 - Add an automated test for the telemetry collected on storage.local data migration failures.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/252074/diff/3-4/

Comment 6

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8986827 [details]
Bug 1470208 - Add an automated test for the telemetry collected on storage.local data migration failures.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/252074/diff/4-5/

Comment 7

[Code Review Bot [:reviewbot]]

Comment on attachment 8986827 [details]
Bug 1470208 - Add an automated test for the telemetry collected on storage.local data migration failures.

https://reviewboard.mozilla.org/r/252074/#review260454


Code analysis found 1 defect in this patch:
 - 1 defect found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: toolkit/components/extensions/test/xpcshell/test_ext_storage_idb_data_migration.js:15
(Diff revision 5)
>  ChromeUtils.import("resource://gre/modules/TelemetryController.jsm");
>  
>  const {
>    ExtensionStorageIDB,
>    DataMigrationTelemetry,
>  } = ChromeUtils.import("resource://gre/modules/ExtensionStorageIDB.jsm");

Error: Cu.import imports into variables and into global scope. [eslint: mozilla/no-import-into-var-and-global]

Comment 8

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8986827 [details]
Bug 1470208 - Add an automated test for the telemetry collected on storage.local data migration failures.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/252074/diff/5-6/

Comment 9

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8986827 [details]
Bug 1470208 - Add an automated test for the telemetry collected on storage.local data migration failures.

https://reviewboard.mozilla.org/r/252074/#review263184

Comment 10

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8986827 [details]
Bug 1470208 - Add an automated test for the telemetry collected on storage.local data migration failures.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/252074/diff/6-7/

Comment 11

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8986827 [details]
Bug 1470208 - Add an automated test for the telemetry collected on storage.local data migration failures.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/252074/diff/7-8/

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/aeb7d06d9638
Add an automated test for the telemetry collected on storage.local data migration failures. r=mixedpuppy

Comment 13

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/aeb7d06d9638