1444054 - Replace promiseWaitForCondition with TestUtils.waitForCondition in browser_devices_get_user_media_screen.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_screen.js test file can be replaced by the TestUtils.waitForCondition[0] utility function.

These occurrences need to be replaced: https://searchfox.org/mozilla-central/search?q=promiseWaitForCondition&path=browser_devices_get_user_media_screen.js

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

[Trisha Gupta [:trisha]]

Hey, since I am already working on bug 1443866, can I also work on this as they seem a bit similar?

Comment 2

[Johann Hofmann [:johannh]]

Sure, thank you! Note that I open these types of bugs for folks who are totally new to Firefox development, once you have solved one or two of them you should reach out to me to find more challenging bugs to work on. :)

Comment 3

[Trisha Gupta [:trisha]]

Attachment: browser_devices_get_user_media_screen.patch

Replaced promiseWaitForCondition with TestUtils.waitForCondition(6 occurrences) in browser_devices_get_user_media_screen.js

Comment 4

[Johann Hofmann [:johannh]]

Comment on attachment 8957530 [details] [diff] [review]
browser_devices_get_user_media_screen.patch

Review of attachment 8957530 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good, just one tiny thing to correct:

::: browser_devices_get_user_media_screen.js
@@ +67,4 @@
>      menulist.getItemAtIndex(2).doCommand();
>      ok(!document.getElementById("webRTC-all-windows-shared").hidden,
>         "the 'all windows will be shared' warning should now be visible");
> +    await TestUtils.waitForCondition(() => !document.getElementById("webRTC-preview").hidden, 100);

The promiseWaitForCondition function had "retryTimes" as second parameter, while it's the third in TestUtils.waitForCondition:

https://searchfox.org/mozilla-central/rev/588d8120aa11738657da93e09a03378bcd1ba8ec/browser/base/content/test/webrtc/head.js#33
https://searchfox.org/mozilla-central/rev/588d8120aa11738657da93e09a03378bcd1ba8ec/testing/modules/TestUtils.jsm#114

Can you please make it so that we pass 100 as the third parameter, instead? :)

Comment 5

[Trisha Gupta [:trisha]]

(In reply to Johann Hofmann [:johannh] from comment #4)
> Comment on attachment 8957530 [details] [diff] [review]
> browser_devices_get_user_media_screen.patch
> 
> Review of attachment 8957530 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This looks good, just one tiny thing to correct:
> 
> ::: browser_devices_get_user_media_screen.js
> @@ +67,4 @@
> >      menulist.getItemAtIndex(2).doCommand();
> >      ok(!document.getElementById("webRTC-all-windows-shared").hidden,
> >         "the 'all windows will be shared' warning should now be visible");
> > +    await TestUtils.waitForCondition(() => !document.getElementById("webRTC-preview").hidden, 100);
> 
> The promiseWaitForCondition function had "retryTimes" as second parameter,
> while it's the third in TestUtils.waitForCondition:
> 
> https://searchfox.org/mozilla-central/rev/
> 588d8120aa11738657da93e09a03378bcd1ba8ec/browser/base/content/test/webrtc/
> head.js#33
> https://searchfox.org/mozilla-central/rev/
> 588d8120aa11738657da93e09a03378bcd1ba8ec/testing/modules/TestUtils.jsm#114
> 
> Can you please make it so that we pass 100 as the third parameter, instead?
> :)

I see. So do you want me to add the "interval" parameter(set it to default value 100) as second parameter in the TestUtils.waitForCondition? This way I can have the retryTimes to be 100 as third parameter. Is this what you mean?

Comment 6

[Trisha Gupta [:trisha]]

Attachment: bug1444054.patch

Added 100 as third parameter and gave the second parameter default value of 100.

Comment 7

[Johann Hofmann [:johannh]]

Comment on attachment 8958121 [details] [diff] [review]
bug1444054.patch

Review of attachment 8958121 [details] [diff] [review]:
-----------------------------------------------------------------

Thank you!

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b0344b00dda4
Replace promiseWaitForCondition with TestUtils.waitForCondition in browser_devices_get_user_media_screen.js. r=johannh

Comment 9

[Andreea Pavel [:apavel]]

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