Closed Bug 1190324 Opened 5 years ago Closed 4 years ago

Test coverage for notifications extension API

Categories

(WebExtensions :: Untriaged, defect)

defect
Not set

Tracking

(firefox48 fixed)

RESOLVED FIXED
mozilla48
Iteration:
48.2 - Apr 4
Tracking Status
firefox48 --- fixed

People

(Reporter: gkrizsanits, Assigned: bsilverberg)

References

Details

(Whiteboard: [testing] triaged)

Attachments

(1 file)

Blocks: 1185459
This is still missing coverage for:

* Notifications with an |iconUrl| property.
* Notification observers being fired with the "alertfinished" topic.
* The |create| API method with the 1-arg form or without a notification ID.
* The |clear| API method.
* The |onClosed| event.


https://people.mozilla.org/~kmaglione/webextension-test-coverage/toolkit/components/extensions/ext-notifications.js.html
Component: Extension Compatibility → WebExtensions
Product: Firefox → Toolkit
Flags: blocking-webextensions-
Flags: blocking-webextensions- → blocking-webextensions+
I will work on this next.
Assignee: nobody → bob.silverberg
Status: NEW → ASSIGNED
Iteration: --- → 48.1 - Mar 21
Add coverage:

* Notification observers being fired with the "alertfinished" topic.
* The |create| API method without a notification ID.
* The |clear| API method.
* The |onClosed| event.

Review commit: https://reviewboard.mozilla.org/r/40069/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/40069/
Attachment #8730650 - Flags: review?(kmaglione+bmo)
Note that the coverage for Notifications with an |iconUrl| property is addressed in bug 1254291.
Comment on attachment 8730650 [details]
MozReview Request: Bug 1190324 - Test coverage for notifications extension API, r?kmag

https://reviewboard.mozilla.org/r/40069/#review36617

::: toolkit/components/extensions/ext-notifications.js:89
(Diff revision 1)
>    return {
>      notifications: {
>        create: function(notificationId, options) {
>          if (!notificationId) {
>            notificationId = nextId++;
> +          notificationId = notificationId.toString();

`notificationId = String(nextId++)`
Attachment #8730650 - Flags: review?(kmaglione+bmo) → review+
Comment on attachment 8730650 [details]
MozReview Request: Bug 1190324 - Test coverage for notifications extension API, r?kmag

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

> `notificationId = String(nextId++)`

This is fixed now (in the commit for adding the schema), but I also added a new test that checks the logic for a pre-existing notification. You may want to review that before I request checkin.
Comment on attachment 8730650 [details]
MozReview Request: Bug 1190324 - Test coverage for notifications extension API, r?kmag

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

::: toolkit/components/extensions/ext-notifications.js:88
(Diff revision 1)
>  extensions.registerSchemaAPI("notifications", "notifications", (extension, context) => {
>    return {
>      notifications: {
>        create: function(notificationId, options) {
>          if (!notificationId) {
>            notificationId = nextId++;

I think you may have rebased this in the wrong order.
https://reviewboard.mozilla.org/r/40069/#review36937

> I think you may have rebased this in the wrong order.

I thought I might have done something wrong too, but that file isn't even touched by this patch, and this is meant to land on top of https://reviewboard.mozilla.org/r/39783/#review36927, so when that happens everything will be ok, won't it?
Comment on attachment 8730650 [details]
MozReview Request: Bug 1190324 - Test coverage for notifications extension API, r?kmag

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40069/diff/3-4/
Please land *after* bug 1254291, for which I added checkin-needed about 10 hours ago.
Depends on: 1254291
Keywords: checkin-needed
Blocks: 1254359
Iteration: 48.1 - Mar 21 → 48.2 - Apr 4
Whiteboard: [testing] triaged
https://hg.mozilla.org/mozilla-central/rev/5851bac416b3
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.