Replace promiseWaitForCondition with TestUtils.waitForCondition in browser_devices_get_user_media_screen.js

RESOLVED FIXED in Firefox 61

Status

()

enhancement
P5
normal
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: johannh, Assigned: trisha, Mentored)

Tracking

({good-first-bug})

60 Branch
Firefox 61
Points:
---

Firefox Tracking Flags

(firefox61 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

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

Comment 1

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

Comment 2

a year ago
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. :)
Assignee: nobody → guptatrisha97
Mentor: jhofmann
Status: NEW → ASSIGNED
(Assignee)

Comment 3

a year ago
Replaced promiseWaitForCondition with TestUtils.waitForCondition(6 occurrences) in browser_devices_get_user_media_screen.js
Attachment #8957530 - Flags: review?(jhofmann)
(Reporter)

Comment 4

a year ago
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? :)
Attachment #8957530 - Flags: review?(jhofmann) → review-
(Assignee)

Comment 5

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

Comment 6

a year ago
Added 100 as third parameter and gave the second parameter default value of 100.
Attachment #8957530 - Attachment is obsolete: true
Attachment #8958121 - Flags: review?(jhofmann)
(Reporter)

Comment 7

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

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

Thank you!
Attachment #8958121 - Flags: review?(jhofmann) → review+
(Reporter)

Updated

a year ago
Keywords: checkin-needed

Comment 8

a year ago
Pushed by nbeleuzu@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b0344b00dda4
Replace promiseWaitForCondition with TestUtils.waitForCondition in browser_devices_get_user_media_screen.js. r=johannh
Keywords: checkin-needed

Comment 9

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