Closed Bug 1373453 Opened 2 years ago Closed 2 years ago

Switch test_TelemetryEnvironment.js over to webextensions where possible

Categories

(Toolkit :: Telemetry, enhancement)

55 Branch
enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla56
Tracking Status
firefox56 --- fixed

People

(Reporter: aswan, Assigned: aswan)

References

Details

Attachments

(2 files)

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.
Attachment #8878251 - Flags: review?(kmaglione+bmo)
Attachment #8878252 - Flags: review?(alessio.placitelli)
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 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 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 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?
Attachment #8878251 - Flags: review?(kmaglione+bmo) → review+
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 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...
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.
Flags: needinfo?(alessio.placitelli)
Comment on attachment 8878252 [details]
Bug 1373453 Part 2 Convert extensions in telemetry unit tests to webextensions

https://reviewboard.mozilla.org/r/149590/#review154948
Attachment #8878252 - Flags: review?(alessio.placitelli) → review+
Flags: needinfo?(alessio.placitelli)
Pushed by alessio.placitelli@gmail.com:
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
https://hg.mozilla.org/mozilla-central/rev/91c2ce06c69b
https://hg.mozilla.org/mozilla-central/rev/26d62a1ac0e3
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.