Replace promiseTopicObserved with TestUtils.topicObserved in browser_popupNotification_2.js

RESOLVED FIXED in Firefox 61

Status

()

P5
normal
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: johannh, Assigned: accakks, Mentored)

Tracking

({good-first-bug})

unspecified
mozilla61
good-first-bug
Points:
---

Firefox Tracking Flags

(firefox61 fixed)

Details

Attachments

(1 attachment)

(Reporter)

Description

a year ago
This is a good first bug for newcomers to Firefox development.

promiseTopicObserved in the browser_popupNotification_2.js test file can be replaced by the TestUtils.topicObserved[0] utility function.

There is only one occurrence in that file we need to replace: https://searchfox.org/mozilla-central/rev/588d8120aa11738657da93e09a03378bcd1ba8ec/browser/base/content/test/popupNotifications/browser_popupNotification_2.js#178

For instructions on how to get your local build of Firefox up and running and submit your patch, see https://developer.mozilla.org/en-US/docs/Introduction.

You can run this test with the ./mach mochitest command:

./mach mochitest browser/base/content/test/popupNotifications/browser_popupNotification_2.js

Please leave a comment if you would like to be assigned to this bug and feel free to ask questions here or via IRC if you're stuck.

[0] https://searchfox.org/mozilla-central/rev/588d8120aa11738657da93e09a03378bcd1ba8ec/testing/modules/TestUtils.jsm#56
(Assignee)

Comment 1

a year ago
Hi I would like to work on this!

Updated

a year ago
Assignee: nobody → aakanksha.jain8
Status: NEW → ASSIGNED
(Assignee)

Updated

a year ago
Attachment #8958018 - Flags: review?(prathikshaprasadsuman)

Comment 3

a year ago
mozreview-review
Comment on attachment 8958018 [details]
Bug 1444372 - Replace promiseTopicObserved with TestUtils.topicObserved in browser_popupNotification_2.js.

https://reviewboard.mozilla.org/r/226960/#review232772

Thank you for picking this up!

You need to make some small changes and request review again. Once you make the changes locally, you can amend your changeset by doing hg commit --amend -m"your_commit_message_goes_here".

Another thing to note here is that commit messages need to be formatted in a certain way. Here's how we do it:

<bug_number> - <commit_message>. r?<reviewer's_irc_nickname>

In this case the commit message will be:

"Bug 1444372 - Replace promiseTopicObserved with TestUtils.topicObserved in browser_popupNotification_2.js. r?prathiksha"

Formatting your commit message this way will also automatically set the review flag to '?' and notify the reviewer that they have a review request. :)

::: browser/base/content/test/popupNotifications/browser_popupNotification_2.js:178
(Diff revision 1)
>    { id: "Test#7",
>      async run() {
>        let notifyObj = new BasicNotification(this.id);
>        notifyObj.anchorID = "geo-notification-icon";
>        notifyObj.addOptions({neverShow: true});
> -      let promiseTopic = promiseTopicObserved("PopupNotifications-updateNotShowing");
> +      let promiseTopic = TestUtils.topicObserved;

This should be TestUtils.topicObserved("PopupNotifications-updateNotShowing"). Looking at the definition here (https://searchfox.org/mozilla-central/rev/588d8120aa11738657da93e09a03378bcd1ba8ec/testing/modules/TestUtils.jsm#56), we need to correctly pass "PopupNotifications-updateNotShowing" as the "topic" argument.
Attachment #8958018 - Flags: review?(prathikshaprasadsuman) → review-
(Assignee)

Comment 4

a year ago
Okay, I'll do the needful ASAP!
(Reporter)

Comment 5

a year ago
mozreview-review
Comment on attachment 8958018 [details]
Bug 1444372 - Replace promiseTopicObserved with TestUtils.topicObserved in browser_popupNotification_2.js.

https://reviewboard.mozilla.org/r/226960/#review233106

Seems like Prathiksha is handling this :)
Attachment #8958018 - Flags: review?(jhofmann)
Comment hidden (mozreview-request)

Comment 7

a year ago
mozreview-review
Comment on attachment 8958018 [details]
Bug 1444372 - Replace promiseTopicObserved with TestUtils.topicObserved in browser_popupNotification_2.js.

https://reviewboard.mozilla.org/r/226960/#review233442

Looks great, thank you!

We usually also do a try run to make sure that the patch does not break anything. In this case, it looks we don't need a try run since your patch is pretty straightforward and it's unlikely to break anything. I'll also set the checkin-needed flag for you this time. This flag signals that we want the patch merged. When you set this flag, someone will eventually come and check the patch in for us. :)
Attachment #8958018 - Flags: review?(prathikshaprasadsuman) → review-

Updated

a year ago
Keywords: checkin-needed

Comment 8

a year ago
Pushed by archaeopteryx@coole-files.de:
https://hg.mozilla.org/integration/mozilla-inbound/rev/bbc2321850f8
Replace promiseTopicObserved with TestUtils.topicObserved in browser_popupNotification_2.js. r=prathiksha
Keywords: checkin-needed

Comment 9

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/bbc2321850f8
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox61: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
You need to log in before you can comment on or make changes to this bug.