Closed Bug 1443861 Opened 2 years ago Closed 2 years ago

Replace promiseWaitForCondition with TestUtils.waitForCondition in browser_devices_get_user_media_anim.js

Categories

(Firefox :: Site Permissions, enhancement, P5)

60 Branch
enhancement

Tracking

()

RESOLVED FIXED
Firefox 63
Tracking Status
firefox60 --- wontfix
firefox61 --- wontfix
firefox62 --- wontfix
firefox63 --- fixed

People

(Reporter: johannh, Assigned: arshadkazmi42, Mentored)

Details

(Keywords: good-first-bug)

Attachments

(1 file)

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

promiseWaitForCondition in the browser_devices_get_user_media_anim.js test file can be replaced by the TestUtils.waitForCondition[0] utility function.

The code in question is here: https://searchfox.org/mozilla-central/rev/efce4ceddfbe5fe4e2d74f1aed93894bada9b273/browser/base/content/test/webrtc/browser_devices_get_user_media_anim.js#62

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/webrtc/browser_devices_get_user_media_anim.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/efce4ceddfbe5fe4e2d74f1aed93894bada9b273/testing/modules/TestUtils.jsm#114
Hey! Can i work on this?
Assignee: nobody → hritvi2801
Status: NEW → ASSIGNED
Can u please tell me from where to start :). Thankyou
Flags: needinfo?(jhofmann)
Hey, sorry, this slipped through the cracks for me. What exactly do you need help with? As mentioned, the instructions are here: https://developer.mozilla.org/en-US/docs/Introduction

The whole process of setting up is a bit lengthy at the beginning, so it doesn't really make sense for me to write it down again. I'm happy to answer any questions or help troubleshoot any problems you may encounter following that document...

Thanks!
Flags: needinfo?(jhofmann)
Hi Hritvi, I'm going to unassign you for now. If you're still interested in taking up this bug, please let me know!
Assignee: hritvi2801 → nobody
Status: ASSIGNED → NEW
Hey Johann! I am a newcomer to the FireFox development and was wondering if I can be assigned to this bug?
(In reply to vraghubir0418 from comment #5)
> Hey Johann! I am a newcomer to the FireFox development and was wondering if
> I can be assigned to this bug?

Hi! I just assigned you to bug 1445261, I hope that works as a good first bug for now! Feel free to attack this one once you're done with the other bug, otherwise I also have plenty of good beginner bugs in the backlog. Just let me know!

Thanks!
Johann, 

I am in the same boat as vraghublr! I have build firefox per the instructions and am looking to tackle a first bug. Can I be assigned to this one or a different one?

Thanks!
There you go, please let me know if you have any questions!
Assignee: nobody → mitchellslavens
Status: NEW → ASSIGNED
Johann,

So I committed a change and pushed it using hg push review. I think my commit message may have been incorrect, though. When I view the log it doesn't show the format "Bug - 1443861" like the others do. I did not format it like I should have. What should I do at this point? 

Thanks for the help.
Johann,

Any word on this? I'd like to start working on another bug if I did this one properly.

Thanks
(In reply to Mitchell from comment #10)
> Johann,
> 
> Any word on this? I'd like to start working on another bug if I did this one
> properly.
> 
> Thanks

Hi Mitchell, sorry for the delay, had a long public easter holiday :)

What does your commit message say right now? You can always amend it using

hg commit --amend

and do hg push review again. Otherwise you can just attach the patch manually using Bugzilla.

If you run into other issues it might be good to contact me on IRC (https://wiki.mozilla.org/IRC). Keep in mind that my timezone is UTC+2.
Hi Mitchell, are you still working on this? :)
Flags: needinfo?(mitchellslavens)
Assignee: mitchellslavens → nobody
Status: ASSIGNED → NEW
Flags: needinfo?(mitchellslavens)
Johann, 

Sorry about that, I have been very busy. When I tried to append my commit message I ended up running into some issue. I should be able to work on a bug this Sunday at the earliest. If this can remain assigned to me until I get back to town I can work on this one. Otherwise I can look for a new one to work on.

Thanks
No worries, let me know if you need help!
Assignee: nobody → mitchellslavens
Status: NEW → ASSIGNED
Johann,

At the moment and when I try to use hg I get the following error. "ignoring unknown working parent e9a85d0aa974" I know I must have done something to mess up my repo. Any suggestions on how to fix this?

Thanks,

Mitchell Slavens
Not sure what it is with this bug for me. I'm really sorry for missing this, it's usually best to set needinfo? flags on questions. I remember answering you on IRC though, but you might not have caught it. I'm not 100% sure where that message comes from, but you should check whether it's an actual error or just a warning (in which case you can still do "hg commit" and "hg export" and so on).

It's probably really best to tackle this on IRC so please feel free to contact me again any time.

Thanks
Unassigning for inactivity, let me know if anyone wants to pick this one up.
Assignee: mitchellslavens → nobody
Status: ASSIGNED → NEW
I would like to work on this.
Flags: needinfo?(jhofmann)
Assignee: nobody → arshadkazmi42
So I tried doing this change but getting error saying 

`Uncaught exception - at chrome://mochitests/content/browser/browser/base/content/test/webrtc/browser_devices_get_user_media_anim.js:62 - TypeError: TestUtils.waitForCondition[0] is not a functio`


I have made this change

Changed this

`await promiseWaitForCondition(() => !tab.getAttribute("sharing"));`

to this

`await TestUtils.waitForCondition[0](() => !tab.getAttribute("sharing"));`

I event tried doing this 


`await BrowserTestUtils.waitForCondition[0](() => !tab.getAttribute("sharing"));`

Is there something which I am missing here?
Hey, thanks for helping out!

I see that I was a little ambiguous in comment 0, the "[0]" in this case was just a reference to the URL at the bottom, not part of the JS syntax. So if you change "TestUtils.waitForCondition[0]" to "TestUtils.waitForCondition" it should work fine :)
Flags: needinfo?(jhofmann)
Oh. Yes. Just seen your full comment now. By mad.
Made that change and tested the script. Pushing code for review to Phabricator
Typo spotted :P

By mad => My Bad

:P :P
Added code for review in Phabricator.

https://phabricator.services.mozilla.com/D4293

I am not sure whom to add as reviewer. Should I add you?
Flags: needinfo?(jhofmann)
(In reply to Arshad Kazmi from comment #24)
> Added code for review in Phabricator.
> 
> https://phabricator.services.mozilla.com/D4293
> 
> I am not sure whom to add as reviewer. Should I add you?

Yup, that works, thanks!
Flags: needinfo?(jhofmann)
Added you as reviewer!
Comment on attachment 9004054 [details]
Bug 1443861 - Replace promiseWaitForCondition with TestUtils.waitForCondition

Johann Hofmann [:johannh] has approved the revision.
Attachment #9004054 - Flags: review+
Thanks :)

This is my first approved patch :)
What are the next steps here? do i have to do anything else?
Flags: needinfo?(jhofmann)
Congrats :)

To get this checked in, you'll have to set the checkin-needed keyword in Bugzilla. Since you probably don't have editbugs access yet, I can do it for you.

Let me know if you want to work on something else!
Flags: needinfo?(jhofmann)
Keywords: checkin-needed
Thanks for adding checkin-needed keyword.

And Yes. I want to work on more bugs. 

I have added two more patches in Phabricator for review.

I am still looking for bugs which I can work on. Do you have anything in your mind which I can take up?
Flags: needinfo?(jhofmann)
(In reply to Arshad Kazmi from comment #31)
> Thanks for adding checkin-needed keyword.
> 
> And Yes. I want to work on more bugs. 
> 
> I have added two more patches in Phabricator for review.
> 
> I am still looking for bugs which I can work on. Do you have anything in
> your mind which I can take up?

Great! How about bug 1486313? It's more cleanup work but that's a great way to get into the contributing workflow.
Flags: needinfo?(jhofmann)
https://hg.mozilla.org/mozilla-central/rev/d97d4b1e2491
Status: NEW → RESOLVED
Closed: 2 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 63
Pushed by apavel@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8d42de3d9245
Replace promiseWaitForCondition with TestUtils.waitForCondition
:johannh.

Sure I will look into that bug
You need to log in before you can comment on or make changes to this bug.