1373453 - Switch test_TelemetryEnvironment.js over to webextensions where possible

Status: RESOLVED FIXED

(Toolkit :: Telemetry, enhancement)

Description

[Andrew Swan [:aswan]]

test_TelemetryEnvironment.js uses several legacy extensions.  With the upcoming webextensions-only policy in 57, we should switch those that make sense over to webextensions both to match how this code will run in the field and to ensure that the test will still work in automation.

Comment 1

[Andrew Swan [:aswan]]

Attachment: Bug 1373453 Part 1 Move repeated webextension startup check to AddonTestUtils

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

Comment 2

[Andrew Swan [:aswan]]

Attachment: Bug 1373453 Part 2 Convert extensions in telemetry unit tests to webextensions

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

Comment 3

[Alessio Placitelli [:Dexter]]

Comment on attachment 8878252 [details]
Bug 1373453 Part 2 Convert extensions in telemetry unit tests to webextensions

https://reviewboard.mozilla.org/r/149590/#review154384

Thanks for this patch, it looks good! I just have a question (see below), but we should be there ;)

::: commit-message-11899:2
(Diff revision 1)
> +Bug 1373453 Part 2 Convert extensions in telemetry unit tests to webextensions
> +

nit: could you add a couple of lines explaining why this is needed after the first one?

::: toolkit/components/telemetry/tests/unit/test_TelemetryEnvironment.js:1019
(Diff revision 1)
>    await checkpointPromise;
>    assertCheckpoint(2);
>    Assert.ok(!(ADDON_ID in TelemetryEnvironment.currentEnvironment.addons.activeAddons));
>  
>    checkpointPromise = registerCheckpointPromise(3);
> +  let startupPromise = AddonTestUtils.webextensionStartup(ADDON_ID);

Mh, just to make sure about what's happening, why is this needed here? Isn't

```
addon.userDisabled = false;
```

enough anymore? If so, why was it enough in the previous check a few lines above?

Comment 4

[Andrew Swan [:aswan]]

Comment on attachment 8878252 [details]
Bug 1373453 Part 2 Convert extensions in telemetry unit tests to webextensions

https://reviewboard.mozilla.org/r/149590/#review154512

::: commit-message-11899:2
(Diff revision 1)
> +Bug 1373453 Part 2 Convert extensions in telemetry unit tests to webextensions
> +

Just to make sure I understand, you mean why this patch needs to come after part 1?  (It uses a test function that is added in part 1, I can add that in a longer comment if it isn't clear by itself)

Comment 5

[Alessio Placitelli [:Dexter]]

Comment on attachment 8878252 [details]
Bug 1373453 Part 2 Convert extensions in telemetry unit tests to webextensions

https://reviewboard.mozilla.org/r/149590/#review154512

> Just to make sure I understand, you mean why this patch needs to come after part 1?  (It uses a test function that is added in part 1, I can add that in a longer comment if it isn't clear by itself)

Nope, I mean we cannot simply keep the non webextensions in the tests

Comment 6

[Andrew Swan [:aswan]]

Comment on attachment 8878252 [details]
Bug 1373453 Part 2 Convert extensions in telemetry unit tests to webextensions

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

Comment 7

[Kris Maglione [:kmag]]

Comment on attachment 8878251 [details]
Bug 1373453 Part 1 Move repeated webextension startup check to AddonTestUtils

https://reviewboard.mozilla.org/r/149588/#review154614

::: toolkit/mozapps/extensions/internal/AddonTestUtils.jsm:1251
(Diff revision 1)
> +   * Helper to wait for a webextension to completely start
> +   *
> +   * @param {string} id
> +   *        An optional extension id to look for

Please end sentences with periods.

::: toolkit/mozapps/extensions/internal/AddonTestUtils.jsm:1253
(Diff revision 1)
>    },
> +
> +  /**
> +   * Helper to wait for a webextension to completely start
> +   *
> +   * @param {string} id

Should be `@param {string} [id]`

::: toolkit/mozapps/extensions/internal/AddonTestUtils.jsm:1259
(Diff revision 1)
> +   *        An optional extension id to look for
> +   *
> +   * @returns {Promise<Extension>}
> +   *           A promise that resolves with the extension, once it is started.
> +   */
> +  webextensionStartup(id) {

Please name this `promiseWebExtensionStartup` for consistency with other methods.

And please also remove the same-named method from head_addons.js and use this instead.

::: toolkit/mozapps/extensions/internal/AddonTestUtils.jsm:1261
(Diff revision 1)
> +   * @returns {Promise<Extension>}
> +   *           A promise that resolves with the extension, once it is started.
> +   */
> +  webextensionStartup(id) {
> +    return new Promise(resolve => {
> +      Management.on("startup", function listener(event, extension) {

I think we probably want "ready" rather than "startup".

::: toolkit/mozapps/extensions/test/xpcshell/test_webextension_icons.js:69
(Diff revision 1)
>  async function testRetinaIconsetParsing(manifest) {
>    await promiseWriteWebManifestForExtension(manifest, profileDir);
>  
>    await Promise.all([
>      promiseRestartManager(),
> -    manifest.theme || promiseAddonStartup(),
> +    manifest.theme || AddonTestUtils.webextensionStartup(),

We know the ID we're looking for everywhere in this file, so can we please pass it explicitly?

Comment 8

[Andrew Swan [:aswan]]

Comment on attachment 8878251 [details]
Bug 1373453 Part 1 Move repeated webextension startup check to AddonTestUtils

https://reviewboard.mozilla.org/r/149588/#review154614

> Please end sentences with periods.

This isn't actually a sentence, but sure...

Comment 9

[Andrew Swan [:aswan]]

Comment on attachment 8878251 [details]
Bug 1373453 Part 1 Move repeated webextension startup check to AddonTestUtils

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

Comment 10

[Andrew Swan [:aswan]]

Comment on attachment 8878252 [details]
Bug 1373453 Part 2 Convert extensions in telemetry unit tests to webextensions

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

Comment 11

[Andrew Swan [:aswan]]

Comment on attachment 8878252 [details]
Bug 1373453 Part 2 Convert extensions in telemetry unit tests to webextensions

https://reviewboard.mozilla.org/r/149590/#review154384

> Mh, just to make sure about what's happening, why is this needed here? Isn't
> 
> ```
> addon.userDisabled = false;
> ```
> 
> enough anymore? If so, why was it enough in the previous check a few lines above?

setting `addon.userDisabled = false;` enables the addon but the bootstrap extension that this test used previously started completely synchronously.  The webextension startup process is asynchronous so if we uninstall it before it has finished startup, bad things can happen.  This should all be handled by the addons manager of course, but until it is I think being explicit in cases like this isn't bad.  Happy to add a comment to clarify if you think its warranted though...

Comment 12

[Andrew Swan [:aswan]]

Comment on attachment 8878251 [details]
Bug 1373453 Part 1 Move repeated webextension startup check to AddonTestUtils

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

Comment 13

[Andrew Swan [:aswan]]

Comment on attachment 8878252 [details]
Bug 1373453 Part 2 Convert extensions in telemetry unit tests to webextensions

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

Comment 14

[Andrew Swan [:aswan]]

The try run linked from reviewboard is green and I think I addressed the review comments.  I'm going to be offline for a few days, if this looks okay can you either land it from reviewboard or add checkin-needed so it doesn't get out of date while I'm gone?  Thanks.

Comment 15

[Alessio Placitelli [:Dexter]]

Comment on attachment 8878252 [details]
Bug 1373453 Part 2 Convert extensions in telemetry unit tests to webextensions

https://reviewboard.mozilla.org/r/149590/#review154948

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/91c2ce06c69b
Part 1 Move repeated webextension startup check to AddonTestUtils r=kmag
https://hg.mozilla.org/integration/autoland/rev/26d62a1ac0e3
Part 2 Convert extensions in telemetry unit tests to webextensions r=Dexter

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/91c2ce06c69b
https://hg.mozilla.org/mozilla-central/rev/26d62a1ac0e3