1190320 - Test coverage for alarms 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 API is still missing coverage for:

* Alarms with a |when| property.
* Observer callbacks for canceled alarms being ignored.
* Extant alarms being cleared on extension shutdown.
* The |get|, |getAll|, and |clearAll| API methods.
* The one-argument forms of |alarms.create| and |alarms.clear| (although these should be replaced by schema checks).
* |alarms.clear| ignoring alarms without matching names.


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

Comment 2

[Bob Silverberg [:bsilverberg]]

I'm going to attempt to work on this. Wish me luck!

Comment 3

[Bob Silverberg [:bsilverberg]]

I've got a patch just about ready that adds all of the missing coverage above, except for the part about checking that alarms are cleared on extension shutdown. I'm not sure how to do that, i.e., check the state of alarms after the extension has been shutdown. Do you have any suggestions?

Comment 4

[Bob Silverberg [:bsilverberg]]

Attachment: MozReview Request: Bug 1190320 - Test coverage for alarms extension API, r?kmag

Add new alarm coverage:

* Alarms with a |when| property.
* Observer callbacks for canceled alarms being ignored.
* The one-argument forms of |alarms.create| and |alarms.clear|.
* |alarms.clear| ignoring alarms without matching names.

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

Comment 5

[Kris Maglione [:kmag]]

Comment on attachment 8722791 [details]
MozReview Request: Bug 1190320 - Test coverage for alarms extension API, r?kmag

https://reviewboard.mozilla.org/r/36243/#review32805

::: toolkit/components/extensions/test/mochitest/test_ext_alarms.html:69
(Diff revision 1)
> +    browser.test.log("running test_cleared_alarm_does_not_fire");

These aren't really necessary. The name of the task function is printed before each test is run.

::: toolkit/components/extensions/test/mochitest/test_ext_alarms.html:71
(Diff revision 1)
> +    chrome.alarms.onAlarm.addListener(function(alarm) {

Please use an arrow function. Same for the others.

Also, please use "browser." rather than "chrome."

::: toolkit/components/extensions/test/mochitest/test_ext_alarms.html:72
(Diff revision 1)
> +      browser.test.notifyFail("cleared alarm does not fire");

Just `browser.test.fail(`. `notifyPass` and `notifyFail` should always have the same argument.

::: toolkit/components/extensions/test/mochitest/test_ext_alarms.html:107
(Diff revision 1)
> +      browser.test.notifyFail("alarm fired within expected time");

`notifyFail` should have the same argument as `awaitFinish`. You can have a separate `browser.test.fail` and `browser.test.notifyFail`, though.

Also, this timeout should be canceled when you receive the alarm callback.

::: toolkit/components/extensions/test/mochitest/test_ext_alarms.html:131
(Diff revision 1)
> +    chrome.alarms.clear(ALARM_NAME + 1, wasCleared => {

Please use "1" to make it clear you're adding this to a string rather than a number.

Comment 6

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8722791 [details]
MozReview Request: Bug 1190320 - Test coverage for alarms extension API, r?kmag

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

Comment 7

[Bob Silverberg [:bsilverberg]]

Note that, as I commented above, I don't think I covered the case you described as:

* Extant alarms being cleared on extension shutdown.

How would I test that alarms are cleared after an extension is shutdown?

Comment 8

[Bob Silverberg [:bsilverberg]]

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

Comment 9

[Kris Maglione [:kmag]]

(In reply to Bob Silverberg [:bsilverberg] from comment #7)
> Note that, as I commented above, I don't think I covered the case you
> described as:
> 
> * Extant alarms being cleared on extension shutdown.
> 
> How would I test that alarms are cleared after an extension is shutdown?

Probably just by importing Extension.jsm and making sure the entries are gone. This has to be done from a chrome test, though.

Comment 10

[Bob Silverberg [:bsilverberg]]

This one seems to have eslint issues as well. Were there just some new rules added? I have eslint running on commit locally and it didn't catch these either. I just tried to rebase against fx-team to pick up the changes that you made to the other alarm test and I somehow messed it up totally. I seem to have lost most of my changes. I'm calling it a day for tonight, but I'll probably ask for some help with this tomorrow.

Comment 11

[Kris Maglione [:kmag]]

A rule for whitespace inside curly brackets was added last week.

Aside from that, try now runs ESLint checks by default. Before, you had to ask for them to be run.

Er, yeah, sorry about the rebase. I had to make some pretty noisy changes to fix the callback depth issue.

Comment 12

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8722791 [details]
MozReview Request: Bug 1190320 - Test coverage for alarms extension API, r?kmag

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

Comment 13

[Bob Silverberg [:bsilverberg]]

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

Comment 14

[Bob Silverberg [:bsilverberg]]

I fixed the eslint issues and rebased correctly (I think). Pushed to try again to see what happens.

Comment 15

[Bob Silverberg [:bsilverberg]]

Try looks good!

Comment 16

[Bob Silverberg [:bsilverberg]]

Oops, that was an older try push I was looking at. The most recent one is still at 53%, but looking good so far.

Comment 17

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/4d789e933b43

Comment 18

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/4d789e933b43