Closed Bug 1445261 Opened 2 years ago Closed 2 years ago

Replace promiseTopicObserved with TestUtils.topicObserved in browser_popupNotification_no_anchors.js

Categories

(Toolkit :: Notifications and Alerts, enhancement, P5)

Product:
Toolkit
Component:
Notifications and Alerts
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla62
Tracking Flags:
Tracking Status
firefox62 --- fixed

People

(Reporter: johannh, Assigned: alexandersonone, Mentored)

Details

(Keywords: good-first-bug)

Attachments

(1 file)

Bug 1445261 - Change promiseTopicObserved with TestUtils.topicObserved for one occurrence
59 bytes, text/x-review-board-request
johannh
: review+
Details 
This is a good first bug for newcomers to Firefox development.

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

We need to replace this occurrence: https://searchfox.org/mozilla-central/rev/99df6dad057b94c2ac4d8b12f7b0b3a7fdf25d04/browser/base/content/test/popupNotifications/browser_popupNotification_no_anchors.js#170

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_no_anchors.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 issue. Can i get this assigned?
Yup, thanks!
Assignee: nobody → sushma.siddamsetty
Status: NEW → ASSIGNED
Hi, are you still interested in working on this bug? Do you need any help? :)
Flags: needinfo?(sushma.siddamsetty)
Hi,there were unsolvable issues with my system in setting up Firefox. Sorry,can't work on this.
Flags: needinfo?(sushma.siddamsetty)
(In reply to Sushma Siddamsetty from comment #4)
> Hi,there were unsolvable issues with my system in setting up Firefox.
> Sorry,can't work on this.

Oh, ok. I'm sure we can solve any issue you encounter, but I understand if you don't want to spend more time on it.

Thanks!
Assignee: sushma.siddamsetty → nobody
Status: ASSIGNED → NEW
Hi Johann! I would like to work on this issue. Can I be assigned to it?
Hi there, of course, thank you!
Assignee: nobody → vraghubir0418
Status: NEW → ASSIGNED
Hi Johann. I am having issues with setting up the mozilla development environment on Windows. I have successfully installed Visual Studio 2017,Rust and I successfully ran the hg clone https://hg.mozilla.org/mozilla-central command. The ./mach bootstrap command runs smoothly but the ./mach/build command fails for me. Any recommendations for how to fix this build issue?
(In reply to vraghubir0418 from comment #8)
> Hi Johann. I am having issues with setting up the mozilla development
> environment on Windows. I have successfully installed Visual Studio
> 2017,Rust and I successfully ran the hg clone
> https://hg.mozilla.org/mozilla-central command. The ./mach bootstrap command
> runs smoothly but the ./mach/build command fails for me. Any recommendations
> for how to fix this build issue?

Hey, sorry for the slow reply, I was away during public holiday over here. What does your ./mach build command say when it fails?
Also, feel free to contact me on IRC (https://wiki.mozilla.org/IRC) if you like.
Are you still working on this? Feel free to ask for help if you need any!
Flags: needinfo?(vraghubir0418)
Unfortunately I could not get Mozilla to build on my laptop. Sorry for the inconvenience I have caused you. Could you please unassigned me from this bug?
Flags: needinfo?(vraghubir0418)
Sure, no worries.
Assignee: vraghubir0418 → nobody
Status: ASSIGNED → NEW
I'd like to work on this bug! I am working with :sfoster
That works for me, have fun :)
Assignee: nobody → alexandersonone
Mentor: jhofmann, prathikshaprasadsuman → sfoster
Status: NEW → ASSIGNED
Comment on attachment 8979321 [details]
Bug 1445261 - Change promiseTopicObserved with TestUtils.topicObserved for one occurrence

https://reviewboard.mozilla.org/r/245500/#review251602

Looks good. You just need to fix up the commit message. Can you also confirm you were able to run the test cleanly? And flag johannh for review by adding r?johannh to your commit message.

::: commit-message-ca13d:1
(Diff revision 1)
> +Bug 1445261 - Change promiseTopicObserved with testUtils.topObserved for

Typo - should be TestUtils.topicObserved
Yes, the test runs properly and outputs that the tests run successfully.
Comment on attachment 8979321 [details]
Bug 1445261 - Change promiseTopicObserved with TestUtils.topicObserved for one occurrence

https://reviewboard.mozilla.org/r/245500/#review251722

This seems good, but if I'm not mistaken we can also remove the function definition now:

https://searchfox.org/mozilla-central/rev/2aa42f2cab4a110edf21dd7281ac23a1ea8901f9/browser/base/content/test/popupNotifications/head.js#16

Since we're removing the last consumer.

Thanks!
Attachment #8979321 - Flags: review?(jhofmann)
I believe there are other uses in other files too relying on the same definition, should I delete those too?

https://searchfox.org/mozilla-central/search?q=promiseTopicObserved&path=
Nevermind, I just saw that there are no other in this folder
Comment on attachment 8979321 [details]
Bug 1445261 - Change promiseTopicObserved with TestUtils.topicObserved for one occurrence

https://reviewboard.mozilla.org/r/245500/#review252144

Looks great, thank you!
Attachment #8979321 - Flags: review?(jhofmann) → review+
Pushed by sfoster@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/71dbe00b841a
Change promiseTopicObserved with TestUtils.topicObserved for one occurrence r=johannh
https://hg.mozilla.org/mozilla-central/rev/71dbe00b841a
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
status-firefox62: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
You need to log in before you can comment on or make changes to this bug.