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

There are two occurrences that need to be replaced: https://searchfox.org/mozilla-central/search?q=waitforcondition&path=browser_devices_get_user_media_multi_process.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_multi_process.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 I would like to work on this. Please assign this bug to me!

Comment 2

[Trisha Gupta [:trisha]]

Also, can I get a link to the github repository of the code for this?

Comment 3

[Trisha Gupta [:trisha]]

92: await TestUtils.waitForCondition(() => !webrtcUI.showCameraIndicator);
195: await TestUtils.waitForCondition(() => webrtcUI.showCameraIndicator);

Do these changes look sound enough?

Comment 4

[Johann Hofmann [:johannh]]

(In reply to Trisha from comment #1)
> Hey I would like to work on this. Please assign this bug to me!

Yup, sorry for the delay.

(In reply to Trisha from comment #2)
> Also, can I get a link to the github repository of the code for this?

The GitHub repo lives at https://github.com/mozilla/gecko-dev, but it's read-only so you can't submit patches there, you'll have to attach them here.

(In reply to Trisha from comment #3)
> 92: await TestUtils.waitForCondition(() => !webrtcUI.showCameraIndicator);
> 195: await TestUtils.waitForCondition(() => webrtcUI.showCameraIndicator);
> 
> Do these changes look sound enough?

That looks great to me. You can read up more on how to submit that change here: https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/How_to_Submit_a_Patch

Comment 5

[Trisha Gupta [:trisha]]

Attachment: browser_devices_get_user_media_multi_process.patch

I have created the patch for your review. Please let me know what changes need to be made! Thanks :)

Comment 6

[Johann Hofmann [:johannh]]

Comment on attachment 8957456 [details] [diff] [review]
browser_devices_get_user_media_multi_process.patch

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

This looks great, thanks!

I'm going to set the checkin-needed flag in order to get this landed.

We usually do a run on our try server (https://wiki.mozilla.org/ReleaseEngineering/TryServer) to make sure all tests are still passing, but in this case I think we can skip that.

Comment 7

[Trisha Gupta [:trisha]]

(In reply to Johann Hofmann [:johannh] from comment #6)
> Comment on attachment 8957456 [details] [diff] [review]
> browser_devices_get_user_media_multi_process.patch
> 
> Review of attachment 8957456 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This looks great, thanks!
> 
> I'm going to set the checkin-needed flag in order to get this landed.
> 
> We usually do a run on our try server
> (https://wiki.mozilla.org/ReleaseEngineering/TryServer) to make sure all
> tests are still passing, but in this case I think we can skip that.

Thanks a lot! So I don't have to do anything more on this bug, right?

Comment 8

[Ryan VanderMeulen (PTO, back July 27)]

Please attach a patch in the proper format with appropriate commit information so it can be committed.

Comment 9

[Trisha Gupta [:trisha]]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #8)
> Please attach a patch in the proper format with appropriate commit
> information so it can be committed.

Thank you. I'm new to Bugzilla and even though I went through all documentation provided, I'm facing trouble in understanding how exactly to create a patch and include commit information using MozReview. I need help there! I have my account and requisites downloaded and installed. Please, can you guide me a bit? Thanks!

Comment 10

[Johann Hofmann [:johannh]]

(In reply to Trisha from comment #9)
> (In reply to Ryan VanderMeulen [:RyanVM] from comment #8)
> > Please attach a patch in the proper format with appropriate commit
> > information so it can be committed.
> 
> Thank you. I'm new to Bugzilla and even though I went through all
> documentation provided, I'm facing trouble in understanding how exactly to
> create a patch and include commit information using MozReview. I need help
> there! I have my account and requisites downloaded and installed. Please,
> can you guide me a bit? Thanks!

First of all, sorry for not checking the commit message in my review.

I assume you exported your changeset using hg diff? You can use hg commit to create a commit and it will open an editor to write your commit message. The correct message format is:

Bug Number - Short description of change. r=reviewer

so for this one you could write

Bug 1443866 - Use TestUtils.waitForCondition in browser_devices_get_user_media_multi_process.js. r=johannh

Let me now if you need more help :)

Thanks!

Comment 11

[Trisha Gupta [:trisha]]

Attachment: bug1443866.patch

Please check if this is the correct commit message and patch format. Thanks a lot!

Comment 12

[Trisha Gupta [:trisha]]

Carrying forward r+ from previous patch

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/42afef8cda32
Use TestUtils.waitForCondition in browser_devices_get_user_media_multi_process.js. r=johannh, Trisha

Comment 14

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out for failing browser-chrome's browser/base/content/test/webrtc/browser_devices_get_user_media_multi_process.js:

https://hg.mozilla.org/integration/mozilla-inbound/rev/5ba6adc700a5b4d8d11c3e9616c5ea7853072446

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=1fd2ee90a6705f4fdf1dc88d3d2e3a911d5cad3f&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=167456061&repo=mozilla-inbound

[task 2018-03-12T17:40:26.845Z] 17:40:26     INFO - TEST-PASS | browser/base/content/test/webrtc/browser_devices_get_user_media_multi_process.js | webrtcUI wants the mic indicator hidden - 
[task 2018-03-12T17:40:26.846Z] 17:40:26     INFO - TEST-PASS | browser/base/content/test/webrtc/browser_devices_get_user_media_multi_process.js | 2 active camera streams - 
[task 2018-03-12T17:40:26.847Z] 17:40:26     INFO - TEST-PASS | browser/base/content/test/webrtc/browser_devices_get_user_media_multi_process.js | 2 active streams - 
[task 2018-03-12T17:40:26.847Z] 17:40:26     INFO - removing the second tab
[task 2018-03-12T17:40:26.847Z] 17:40:26     INFO - Buffered messages finished
[task 2018-03-12T17:40:26.848Z] 17:40:26     INFO - TEST-UNEXPECTED-FAIL | browser/base/content/test/webrtc/browser_devices_get_user_media_multi_process.js | Uncaught exception - undefined - timed out after 50 tries.

Comment 15

[Johann Hofmann [:johannh]]

Comment on attachment 8958074 [details] [diff] [review]
bug1443866.patch

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

::: browser/base/content/test/webrtc/browser_devices_get_user_media_multi_process.js
@@ +192,4 @@
>      await BrowserTestUtils.removeTab(tab);
>  
>      // Check that we still show the sharing indicators for the first tab's stream.
> +    await TestUtils.waitForCondition(() => !webrtcUI.showCameraIndicator);

This line seems to have been mis-copy-pasted, it should not have a !.

Can you fix it up and request review again? :)

Thanks!

Comment 16

[Trisha Gupta [:trisha]]

Attachment: bug1443866.patch

Sorry for the typo. Made the relevant changes.

Comment 17

[Johann Hofmann [:johannh]]

Comment on attachment 8958371 [details] [diff] [review]
bug1443866.patch

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

Thanks!

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8c5d0d2498ea
Use TestUtils.waitForCondition in browser_devices_get_user_media_multi_process.js. r=johannh

Comment 19

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/8c5d0d2498ea