1190325 - Test coverage for runtime extension API

Status: RESOLVED FIXED

(WebExtensions :: Untriaged, defect)

Description

[Gabor Krizsanits (INACTIVE)]

https://wiki.mozilla.org/Browser_Extensions#List_of_supported_APIs

Comment 1

[Kris Maglione [:kmag]]

This is still missing coverage for:

* The |onStartup| event.
* The 2-4 arg forms of the |sendMessage| API method.
* The |sendMessage| and |connect| API methods with an explicit |extensionId| property.
* The |getManifest| method.



https://people.mozilla.org/~kmaglione/webextension-test-coverage/toolkit/components/extensions/ext-runtime.js.html

Comment 2

[Bob Silverberg [:bsilverberg]]

Looking at the most recent coverage report at [1], it looks like now we are only missing coverage for:

* The |onStartup| event.
* The |openOptionsPage| method when |options_ui| is missing from the manifest.

I will start working on those.

[1] https://people.mozilla.org/~kmaglione/webextension-test-coverage/toolkit/components/extensions/ext-runtime.js.html

Comment 3

[Bob Silverberg [:bsilverberg]]

Oh, also:

* The |openOptionsPage| method when there is no browser window. (from [2], assuming that is also meant to be covered by this bug)

[2] https://people.mozilla.org/~kmaglione/webextension-test-coverage/browser/components/extensions/ext-desktop-runtime.js.html

Comment 4

[Bob Silverberg [:bsilverberg]]

Attachment: MozReview Request: Bug 1190325 - Test coverage for runtime extension API, r?kmag

Add coverage for:
* The |openOptionsPage| method when |options_ui| is missing from the manifest.
* The |openOptionsPage| method when there is no browser window.
* The |onStartup| event.

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

Comment 5

[Bob Silverberg [:bsilverberg]]

https://reviewboard.mozilla.org/r/44579/#review41301

::: browser/components/extensions/test/browser/browser_ext_runtime_openOptionsPage.js:261
(Diff revision 1)
> +
> +  yield extension.awaitFinish("options-no-manifest");
> +  yield extension.unload();
> +});
> +
> +add_task(function* test_options_no_window() {

Note: This test "works" in that all the assertions pass, but it causes a problem for the test runner at the end of the test when the window is closed. Do you have any suggestions for testing this error condition and also not breaking the test runner?

::: toolkit/components/extensions/schemas/runtime.json
(Diff revision 1)
>        }
>      ],
>      "events": [
>        {
>          "name": "onStartup",
> -        "unsupported": true,

I discovered that `onStartup` is currently labelled `unsupported` and our docs concur with this. I assume, as the code exists and it's listed as needing coverage, that that is an error and this really should be listed as supported, which is why I made this change.

::: toolkit/components/extensions/test/mochitest/test_ext_runtime_onstartup.html:20
(Diff revision 1)
> +
> +add_task(function* test_onstartup() {
> +  function background() {
> +    browser.runtime.onStartup.addListener(function listener() {
> +      browser.runtime.onStartup.removeListener(listener);
> +      browser.test.notifyPass("runtime.onstartup");

This doesn't work. The listener never gets called, which doesn't surprise me as the docs suggest that this will get called when a profile is loaded which already includes the extension. I am submitting this as what I've done thus far, so comments can be added. Is there a way to create a profile that contains the test extension and then load that into a running instance of Firefox in order to test this functionality?

Comment 6

[Kris Maglione [:kmag]]

https://reviewboard.mozilla.org/r/44579/#review41301

> Note: This test "works" in that all the assertions pass, but it causes a problem for the test runner at the end of the test when the window is closed. Do you have any suggestions for testing this error condition and also not breaking the test runner?

I'd say skip it for now. The only sensible way I can think of doing this is to use an xpcshell test, but they're not allowed to actually open windows.

> I discovered that `onStartup` is currently labelled `unsupported` and our docs concur with this. I assume, as the code exists and it's listed as needing coverage, that that is an error and this really should be listed as supported, which is why I made this change.

There's some skeleton code, but it's not really supported.

> This doesn't work. The listener never gets called, which doesn't surprise me as the docs suggest that this will get called when a profile is loaded which already includes the extension. I am submitting this as what I've done thus far, so comments can be added. Is there a way to create a profile that contains the test extension and then load that into a running instance of Firefox in order to test this functionality?

No profile is necessary. The problem is that the event currently doesn't work.

Comment 7

[Kris Maglione [:kmag]]

Comment on attachment 8738557 [details]
MozReview Request: Bug 1190325 - Test coverage for runtime extension API, r?kmag

https://reviewboard.mozilla.org/r/44579/#review41411

The no manifest test looks good, but I think we're going to have to live without the other two for now.

::: browser/components/extensions/test/browser/browser_ext_runtime_openOptionsPage.js:299
(Diff revision 1)
> +
> +  let {WindowManager} = Cu.import("resource://gre/modules/Extension.jsm", {});
> +
> +  let window = WindowManager.topWindow;
> +  yield extension.awaitMessage("ready");
> +  yield BrowserTestUtils.closeWindow(window);

We can't close the initial browser window in browser chrome tests.

::: toolkit/components/extensions/test/mochitest/mochitest.ini:52
(Diff revision 1)
>  skip-if = (os == 'android' || buildapp == 'b2g') # port.sender.tab is undefined on b2g. Bug 1258975 on android.
>  [test_ext_runtime_connect2.html]
>  skip-if = (os == 'android' || buildapp == 'b2g') # port.sender.tab is undefined on b2g. Bug 1258975 on android.
>  [test_ext_runtime_disconnect.html]
>  [test_ext_runtime_getPlatformInfo.html]
> +[test_ext_runtime_onstartup.html]

`onStartup` is not currently implemented.

Comment 8

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8738557 [details]
MozReview Request: Bug 1190325 - Test coverage for runtime extension API, r?kmag

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

Comment 9

[Bob Silverberg [:bsilverberg]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ede7044edc9d

Comment 10

[Bob Silverberg [:bsilverberg]]

Kris, in your comments it sounds like you've signed off on this (with only the one test included), but it's still awaiting an official r+. Once that's done it's ready for check in.

Comment 11

[Kris Maglione [:kmag]]

Comment on attachment 8738557 [details]
MozReview Request: Bug 1190325 - Test coverage for runtime extension API, r?kmag

https://reviewboard.mozilla.org/r/44579/#review41583

Comment 12

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/99a4af2f94e8

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/99a4af2f94e8