Replace promiseTopicObserved with TestUtils.topicObserved in browser_popupNotification_2.js

RESOLVED FIXED in Firefox 61

Status

()

enhancement
P5
normal
RESOLVED FIXED
Last year
Last year

People

(Reporter: johannh, Assigned: accakks, Mentored)

Tracking

({good-first-bug})

unspecified
mozilla61
Points:
---

Firefox Tracking Flags

(firefox61 fixed)

Details

Attachments

(1 attachment)

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
Hi I would like to work on this!
Assignee: nobody → aakanksha.jain8
Status: NEW → ASSIGNED
Attachment #8958018 - Flags: review?(prathikshaprasadsuman)
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-
Okay, I'll do the needful ASAP!
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 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-
Keywords: checkin-needed
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
https://hg.mozilla.org/mozilla-central/rev/bbc2321850f8
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
You need to log in before you can comment on or make changes to this bug.