Closed
Bug 1190324
Opened 9 years ago
Closed 8 years ago
Test coverage for notifications extension API
Categories
(WebExtensions :: Untriaged, defect)
WebExtensions
Untriaged
Tracking
(firefox48 fixed)
Tracking | Status | |
---|---|---|
firefox48 | --- | fixed |
People
(Reporter: gkrizsanits, Assigned: bsilverberg)
References
Details
(Whiteboard: [testing] triaged)
Attachments
(1 file)
Comment 1•8 years ago
|
||
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
Updated•8 years ago
|
Component: Extension Compatibility → WebExtensions
Product: Firefox → Toolkit
Updated•8 years ago
|
Flags: blocking-webextensions-
Updated•8 years ago
|
Flags: blocking-webextensions- → blocking-webextensions+
Assignee | ||
Comment 2•8 years ago
|
||
I will work on this next.
Assignee: nobody → bob.silverberg
Status: NEW → ASSIGNED
Iteration: --- → 48.1 - Mar 21
Assignee | ||
Comment 3•8 years ago
|
||
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)
Assignee | ||
Comment 4•8 years ago
|
||
Note that the coverage for Notifications with an |iconUrl| property is addressed in bug 1254291.
Comment 5•8 years ago
|
||
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+
Assignee | ||
Comment 6•8 years ago
|
||
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/
Assignee | ||
Comment 7•8 years ago
|
||
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.
Assignee | ||
Comment 8•8 years ago
|
||
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/
Comment 9•8 years ago
|
||
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.
Assignee | ||
Comment 10•8 years ago
|
||
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?
Assignee | ||
Comment 11•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=de794e145da1
Assignee | ||
Comment 12•8 years ago
|
||
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/
Assignee | ||
Comment 13•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7ce97832e038
Assignee | ||
Comment 14•8 years ago
|
||
Please land *after* bug 1254291, for which I added checkin-needed about 10 hours ago.
Depends on: 1254291
Keywords: checkin-needed
Assignee | ||
Updated•8 years ago
|
Iteration: 48.1 - Mar 21 → 48.2 - Apr 4
Updated•8 years ago
|
Whiteboard: [testing] triaged
Comment 15•8 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/5851bac416b3
Keywords: checkin-needed
Comment 16•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/5851bac416b3
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox48:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Updated•6 years ago
|
Product: Toolkit → WebExtensions
You need to log in
before you can comment on or make changes to this bug.
Description
•