Closed
Bug 993783
Opened 10 years ago
Closed 10 years ago
Create a mochitest which exercises all the different events for PC.onsignalingstatechange
Categories
(Core :: WebRTC, defect)
Core
WebRTC
Tracking
()
RESOLVED
FIXED
mozilla31
People
(Reporter: drno, Assigned: onecyrenus)
Details
Attachments
(1 file, 2 obsolete files)
14.50 KB,
patch
|
drno
:
review+
|
Details | Diff | Splinter Review |
Currently the RTCPeerConnection.onsignalingstatechange event callback gets partially tested through the existing test_peerConnection_* mochitest test cases. But as Bug 989936 illustrate we are (or were) missing states. We should have one mochitest which exercises all 4 events for the onsignalingstatechange callback: stable, have-local-offer, have-remote-offer and closed (have-local-pranswer and have-remote-pranswer are optional, as they are not yet implemented in FF).
Comment 1•10 years ago
|
||
Agree, one comment on implementation though: This sounds like something that could be tested during every call rather than something that requires another call cycle of its own. Each new test_*.html mochitest we add for webrtc adds a whole new call, and with our test queue framework, we can perhaps avoid doing that.
Reporter | ||
Updated•10 years ago
|
Assignee: nobody → drno
Assignee | ||
Comment 2•10 years ago
|
||
jib: drno: To get the most amount of coverage I have written a patch that does the following. #1) stable, have-local-offer, have-remote-offer are tested in all the test cases using template.js (peerconnection / datachannel) #2) closed is implemented in a separate test case, and tested on both the local / remote ends. Close is implemented inside the peerconnection teardown, and the steeplechase tests assume that teardown is handled outside of the template. Attached is as far as i was able to take this, as there doesn't seem to be a fix available for the underlying issue. I have tested part 1, but part 2 doesn't work in my environment because onsignalingstatechange never throws the "closed" event.
Assignee | ||
Comment 3•10 years ago
|
||
https://mozqa.etherpad.mozilla.org/WebRTC-Backlog -- assuming this is my task ?
Assignee: drno → dclarke
Reporter | ||
Comment 4•10 years ago
|
||
(In reply to dclarke@mozilla.com [:onecyrenus] from comment #3) > https://mozqa.etherpad.mozilla.org/WebRTC-Backlog -- assuming this is my > task ? Yep. Don't worry about #2. I'm working on that in Bug 991877 and Bug 989936. I'm getting close to submit patches there. Once these are landed we can add the checks for closed separately.
Reporter | ||
Comment 5•10 years ago
|
||
So the patch for the "closed" event landed on central in Bug 989936. So you should be able to add this now and land it.
Assignee | ||
Comment 6•10 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=1a62f2c590f5, attached is the final patch i removed the closed event portion because it is covered by another bug.
Attachment #8407225 -
Attachment is obsolete: true
Attachment #8410836 -
Flags: review?(drno)
Reporter | ||
Comment 7•10 years ago
|
||
Comment on attachment 8410836 [details] [diff] [review] Bug993783.patch Review of attachment 8410836 [details] [diff] [review]: ----------------------------------------------------------------- I would like to get answers to my question regarding the change in set(Local|Remote)Description() before giving approval. ::: dom/media/tests/mochitest/pc.js @@ +602,1 @@ > check_next_test(); Wouldn't it be better to move the check_next_test() into the if condition as well, as check_next_test won't do anything if the wrong event fired. Should we at least log something about receiving unexpected events if not even have test fail in case of unexpected event? @@ +663,1 @@ > check_next_test(); Same comments/questions as for setLocalDescription() above. @@ +865,2 @@ > if (peer.signalingState === 'have-remote-offer') { > + ok(true, "signaling state is have remote offer"); These ok() calls should be converted to info(), or even better removed as they don't seem to provide much value outside of debugging this specific patch. @@ +915,5 @@ > check_next_test(); > }); > > + PeerConnectionTest.prototype.setLocalDescription.call(this, targetPeer, desc, > + state, Nit: no space at the end of the line. ::: dom/media/tests/mochitest/templates.js @@ +311,5 @@ > ], > [ > 'PC_LOCAL_SET_LOCAL_DESCRIPTION', > function (test) { > + test.setLocalDescription(test.pcLocal, test.pcLocal._last_offer, HAVE_LOCAL_OFFER, Nit: no space at the end of the line @@ +322,5 @@ > ], > [ > 'PC_REMOTE_SET_REMOTE_DESCRIPTION', > function (test) { > + test.setRemoteDescription(test.pcRemote, test.pcLocal._last_offer, HAVE_REMOTE_OFFER, Nit: no space at the end of the line @@ +354,5 @@ > ], > [ > 'PC_REMOTE_SET_LOCAL_DESCRIPTION', > function (test) { > + test.setLocalDescription(test.pcRemote, test.pcRemote._last_answer, STABLE, Nit: no space at the end of the line
Attachment #8410836 -
Flags: review?(drno) → review-
Assignee | ||
Comment 8•10 years ago
|
||
+ check_next_test moved within if, also added some logging. https://tbpl.mozilla.org/?tree=Try&rev=beb9d3e996e8
Attachment #8410836 -
Attachment is obsolete: true
Attachment #8412377 -
Flags: review?(drno)
Assignee | ||
Comment 9•10 years ago
|
||
if (peer.signalingState === 'have-remote-offer') {$ this.waitForInitialDataChannel(peer, desc, state, onSuccess);$ }$ These lines have been removed, they were just extra debug information I was using that made it into the patch.
Reporter | ||
Comment 10•10 years ago
|
||
Comment on attachment 8412377 [details] [diff] [review] Bug993783-1.patch Review of attachment 8412377 [details] [diff] [review]: ----------------------------------------------------------------- lgtm. thanks
Attachment #8412377 -
Flags: review?(drno) → review+
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed
Comment 11•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/5aac64564630
Flags: in-testsuite+
Keywords: checkin-needed
Comment 12•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/5aac64564630
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
You need to log in
before you can comment on or make changes to this bug.
Description
•