1509246 - Create mochitests for existing WebExtension APIs

Status: RESOLVED FIXED

Description

[Geoff Lankow (:darktrojan)]

We're nearly ready to run mochitests on TaskCluster, so I've created some tests for the addressBooks, browserAction, and composeAction APIs.

Comment 1

[Geoff Lankow (:darktrojan)]

Attachment: 1509246-webext-tests-1.diff

Comment 2

[Geoff Lankow (:darktrojan)]

Attachment: 1509246-webext-tests-2.diff

Comment 3

[Geoff Lankow (:darktrojan)]

Attachment: 1509246-webext-tests-3.diff

Comment 4

[Geoff Lankow (:darktrojan)]

Comment on attachment 9029375 [details] [diff] [review]
1509246-webext-tests-3.diff

You might as well review this one too.

Comment 5

[Magnus Melin [:mkmelin]]

Comment on attachment 9029375 [details] [diff] [review]
1509246-webext-tests-3.diff

Review of attachment 9029375 [details] [diff] [review]:
-----------------------------------------------------------------

::: mail/components/extensions/test/browser/browser_ext_addressBooksUI.js
@@ +18,5 @@
> +
> +    await awaitMessage("check", 0);
> +
> +    await browser.addressBooks.openUI();
> +    await awaitMessage("check", 1);

it could be useful for these tests to have more descriptions of what they are doing, so people don't have to read the code that carefully ;)

::: mail/components/extensions/test/browser/browser_ext_composeAction.js
@@ +70,5 @@
> +    composeWindow.close();
> +  }
> +}
> +
> +let account;

why isn't this inside the function?

::: mail/components/extensions/test/browser/head.js
@@ +36,5 @@
> +async function promiseAnimationFrame(win = window) {
> +  await new Promise(resolve => win.requestAnimationFrame(resolve));
> +
> +  let {tm} = Services;
> +  return new Promise(resolve => tm.dispatchToMainThread(resolve));

Why not Services.tm.dispatchToMainThread directly?

Comment 6

[Geoff Lankow (:darktrojan)]

Attachment: 1509246-webext-tests-4.diff

A few small updates. I made a patch of the pref changes needed to run this stuff locally, and put it here: https://gist.github.com/darktrojan/3474b57149635c322dd8b3e59cdfa89f.

Comment 7

[Geoff Lankow (:darktrojan)]

Attachment: 1509246-webext-tests-5.diff

Managed to get rid of a lot of flakiness. I think it still fails every now and then, but as far as I can tell, that's because of something dodgy in the WebExtensions framework.

Comment 8

[Magnus Melin [:mkmelin]]

Comment on attachment 9032316 [details] [diff] [review]
1509246-webext-tests-5.diff

Review of attachment 9032316 [details] [diff] [review]:
-----------------------------------------------------------------

::: mail/components/extensions/test/browser/browser_ext_browserAction.js
@@ +76,5 @@
> +    },
> +    manifest: {
> +      applications: {
> +        gecko: {
> +          id: "[e-mail]",

might be better to use @example.com

::: mail/components/extensions/test/browser/browser_ext_composeAction.js
@@ +1,5 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +let account;

what's up with this?

Comment 9

[Geoff Lankow (:darktrojan)]

> might be better to use @example.com

Okay.

> what's up with this?

It's assigned in one function and used in another and I don't want to pass it around all over the place. What's the problem?

Comment 10

[Magnus Melin [:mkmelin]]

I see. Perhaps best to use the gAccount convention to make that clearer.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/4b4bc20aa6de
Create mochitests for existing WebExtension APIs; fix some nits found; r=mkmelin