Replace promiseTopicObserved with TestUtils.topicObserved in browser_popupNotification_no_anchors.js

RESOLVED FIXED in Firefox 62

Status

()

enhancement
P5
normal
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: johannh, Assigned: alexandersonone, Mentored)

Tracking

({good-first-bug})

unspecified
mozilla62
Points:
---

Firefox Tracking Flags

(firefox62 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_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?
(Reporter)

Comment 2

a year ago
Yup, thanks!
Assignee: nobody → sushma.siddamsetty
Status: NEW → ASSIGNED
(Reporter)

Comment 3

a year ago
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)
(Reporter)

Comment 5

a year 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

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

Comment 7

a year ago
Hi there, of course, thank you!
Assignee: nobody → vraghubir0418
Status: NEW → ASSIGNED

Comment 8

a year 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

a year 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

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

Comment 11

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

Comment 12

a year 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

a year ago
Sure, no worries.
Assignee: vraghubir0418 → nobody
Status: ASSIGNED → NEW
(Assignee)

Comment 14

a year ago
I'd like to work on this bug! I am working with :sfoster
(Reporter)

Comment 15

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

Comment 17

a year 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

a year 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

a year 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

a year 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

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

Comment 26

a year 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

a year 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

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