Replace promiseTopicObserved with TestUtils.topicObserved in browser_popupNotification_no_anchors.js

RESOLVED FIXED in Firefox 62

Status

()

Product:
Toolkit
Component:
Notifications and Alerts
Importance:
P5
normal
Status:
RESOLVED FIXED
Reported:
10 months ago
Modified:
8 months ago

People

(Reporter: johannh, Assigned: alexandersonone, Mentored)

Tracking

({good-first-bug})

Version:
unspecified
Target:
mozilla62
Keywords:
good-first-bug
Points:
---

Firefox Tracking Flags

(firefox62 fixed)

Details

Attachments

(1 attachment)

Bug 1445261 - Change promiseTopicObserved with TestUtils.topicObserved for one occurrence
59 bytes, text/x-review-board-request
johannh
: review+
Details
(Reporter)

Description

•
10 months ago 
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

Comment 1

•
10 months ago 
Hi. I would like to work on this issue. Can i get this assigned?
(Reporter)

Comment 2

•
10 months ago 
Yup, thanks!
Assignee: nobody → sushma.siddamsetty
Status: NEW → ASSIGNED
(Reporter)

Comment 3

•
10 months ago 
Hi, are you still interested in working on this bug? Do you need any help? :)
Flags: needinfo?(sushma.siddamsetty)

Comment 4

•
10 months ago 
Hi,there were unsolvable issues with my system in setting up Firefox. Sorry,can't work on this.
Flags: needinfo?(sushma.siddamsetty)
(Reporter)

Comment 5

•
10 months ago 
(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

Comment 6

•
10 months ago 
Hi Johann! I would like to work on this issue. Can I be assigned to it?
(Reporter)

Comment 7

•
10 months ago 
Hi there, of course, thank you!
Assignee: nobody → vraghubir0418
Status: NEW → ASSIGNED

Comment 8

•
10 months ago 
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?
(Reporter)

Comment 9

•
10 months ago 
(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?
(Reporter)

Comment 10

•
10 months ago 
Also, feel free to contact me on IRC (https://wiki.mozilla.org/IRC) if you like.
(Reporter)

Comment 11

•
10 months ago 
Are you still working on this? Feel free to ask for help if you need any!
Flags: needinfo?(vraghubir0418)

Comment 12

•
9 months ago 
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)
(Reporter)

Comment 13

•
9 months ago 
Sure, no worries.
Assignee: vraghubir0418 → nobody
Status: ASSIGNED → NEW
(Assignee)

Comment 14

•
8 months ago 
I'd like to work on this bug! I am working with :sfoster
(Reporter)

Comment 15

•
8 months ago 
That works for me, have fun :)
Assignee: nobody → alexandersonone
Mentor: jhofmann, prathikshaprasadsuman → sfoster
Status: NEW → ASSIGNED
Comment hidden (mozreview-request)

Comment 17

•
8 months ago
mozreview-review 
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
(Assignee)

Comment 18

•
8 months ago 
Yes, the test runs properly and outputs that the tests run successfully.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Reporter)

Comment 22

•
8 months ago
mozreview-review 
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)
(Assignee)

Comment 23

•
8 months ago 
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=
(Assignee)

Comment 24

•
8 months ago 
Nevermind, I just saw that there are no other in this folder
Comment hidden (mozreview-request)
(Reporter)

Comment 26

•
8 months ago
mozreview-review 
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+

Comment 27

•
8 months ago 
Pushed by sfoster@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/71dbe00b841a
Change promiseTopicObserved with TestUtils.topicObserved for one occurrence r=johannh

Comment 28

•
8 months ago
bugherder 
https://hg.mozilla.org/mozilla-central/rev/71dbe00b841a
Status: ASSIGNED → RESOLVED
Last Resolved: 8 months 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.