1443861 - Replace promiseWaitForCondition with TestUtils.waitForCondition in browser_devices_get_user_media_anim.js

Status: RESOLVED FIXED

(Firefox :: Site Permissions, enhancement, P5)

Description

[Johann Hofmann [:johannh]]

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

Comment 1

[Hritvi Bhandari]

Hey! Can i work on this?

Comment 2

[Hritvi Bhandari]

Can u please tell me from where to start :). Thankyou

Comment 3

[Johann Hofmann [:johannh]]

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!

Comment 4

[Johann Hofmann [:johannh]]

Hi Hritvi, I'm going to unassign you for now. If you're still interested in taking up this bug, please let me know!

Comment 5

[vraghubir0418]

Hey Johann! I am a newcomer to the FireFox development and was wondering if I can be assigned to this bug?

Comment 6

[Johann Hofmann [:johannh]]

(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!

Comment 7

[Mitchell]

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!

Comment 8

[Johann Hofmann [:johannh]]

There you go, please let me know if you have any questions!

Comment 9

[Mitchell]

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.

Comment 10

[Mitchell]

Johann,

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

Thanks

Comment 11

[Johann Hofmann [:johannh]]

(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.

Comment 12

[Johann Hofmann [:johannh]]

Hi Mitchell, are you still working on this? :)

Comment 13

[Mitchell]

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

Comment 14

[Johann Hofmann [:johannh]]

No worries, let me know if you need help!

Comment 15

[Mitchell]

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

Comment 16

[Johann Hofmann [:johannh]]

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

Comment 17

[Johann Hofmann [:johannh]]

Unassigning for inactivity, let me know if anyone wants to pick this one up.

Comment 18

[Arshad Kazmi [:arshadkazmi42]]

I would like to work on this.

Comment 19

[Arshad Kazmi [: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?

Comment 20

[Johann Hofmann [:johannh]]

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 :)

Comment 21

[Arshad Kazmi [:arshadkazmi42]]

Oh. Yes. Just seen your full comment now. By mad.
Made that change and tested the script. Pushing code for review to Phabricator

Comment 22

[Arshad Kazmi [:arshadkazmi42]]

Typo spotted :P

By mad => My Bad

:P :P

Comment 23

[Arshad Kazmi [:arshadkazmi42]]

Attachment: Bug 1443861 - Replace promiseWaitForCondition with TestUtils.waitForCondition

Comment 24

[Arshad Kazmi [:arshadkazmi42]]

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?

Comment 25

[Johann Hofmann [:johannh]]

(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!

Comment 26

[Arshad Kazmi [:arshadkazmi42]]

Added you as reviewer!

Comment 27

[Johann Hofmann [:johannh]]

Comment on attachment 9004054 [details]
Bug 1443861 - Replace promiseWaitForCondition with TestUtils.waitForCondition

Johann Hofmann [:johannh] has approved the revision.

Comment 28

[Arshad Kazmi [:arshadkazmi42]]

Thanks :)

This is my first approved patch :)

Comment 29

[Arshad Kazmi [:arshadkazmi42]]

What are the next steps here? do i have to do anything else?

Comment 30

[Johann Hofmann [:johannh]]

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!

Comment 31

[Arshad Kazmi [:arshadkazmi42]]

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?

Comment 32

[Johann Hofmann [:johannh]]

(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.

Comment 33

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/d97d4b1e2491

Comment 34

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8d42de3d9245
Replace promiseWaitForCondition with TestUtils.waitForCondition

Comment 35

[Arshad Kazmi [:arshadkazmi42]]

:johannh.

Sure I will look into that bug