Replace promiseWaitForCondition with TestUtils.waitForCondition in browser_devices_get_user_media_multi_process.js

RESOLVED FIXED in Firefox 61

Status

()

P5
normal
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: johannh, Assigned: trisha, Mentored)

Tracking

({good-first-bug})

60 Branch
Firefox 61
good-first-bug
Points:
---

Firefox Tracking Flags

(firefox61 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

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

Comment 1

a year ago
Hey I would like to work on this. Please assign this bug to me!
(Assignee)

Comment 2

a year ago
Also, can I get a link to the github repository of the code for this?
(Assignee)

Comment 3

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

Do these changes look sound enough?
(Reporter)

Comment 4

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

Comment 5

a year ago
I have created the patch for your review. Please let me know what changes need to be made! Thanks :)
Attachment #8957456 - Flags: review?(jhofmann)
(Reporter)

Comment 6

a year ago
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.
Attachment #8957456 - Flags: review?(jhofmann) → review+
(Reporter)

Updated

a year ago
Keywords: checkin-needed
(Assignee)

Comment 7

a year ago
(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?
Please attach a patch in the proper format with appropriate commit information so it can be committed.
Flags: needinfo?(guptatrisha97)
Keywords: checkin-needed
(Assignee)

Comment 9

a year ago
(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!
Flags: needinfo?(guptatrisha97)
(Reporter)

Comment 10

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

Comment 11

a year ago
Posted patch bug1443866.patch (obsolete) — Splinter Review
Please check if this is the correct commit message and patch format. Thanks a lot!
(Assignee)

Updated

a year ago
Attachment #8958074 - Flags: review+
(Assignee)

Updated

a year ago
Keywords: good-first-bug → checkin-needed
(Assignee)

Updated

a year ago
Attachment #8957456 - Attachment is obsolete: true
(Assignee)

Updated

a year ago
Keywords: good-first-bug
(Assignee)

Comment 12

a year ago
Carrying forward r+ from previous patch

Comment 13

a year ago
Pushed by csabou@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/42afef8cda32
Use TestUtils.waitForCondition in browser_devices_get_user_media_multi_process.js. r=johannh, Trisha
Keywords: checkin-needed
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.
Flags: needinfo?(guptatrisha97)
(Reporter)

Comment 15

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

Comment 16

a year ago
Sorry for the typo. Made the relevant changes.
Attachment #8958074 - Attachment is obsolete: true
Flags: needinfo?(guptatrisha97)
Attachment #8958371 - Flags: review?(jhofmann)
(Reporter)

Comment 17

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

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

Thanks!
Attachment #8958371 - Flags: review?(jhofmann) → review+
(Reporter)

Updated

a year ago
Keywords: checkin-needed

Comment 18

a year ago
Pushed by nbeleuzu@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8c5d0d2498ea
Use TestUtils.waitForCondition in browser_devices_get_user_media_multi_process.js. r=johannh
Keywords: checkin-needed

Comment 19

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