Closed Bug 993783 Opened 6 years ago Closed 6 years ago

Create a mochitest which exercises all the different events for PC.onsignalingstatechange


(Core :: WebRTC, defect)

Not set





(Reporter: drno, Assigned: onecyrenus)



(1 file, 2 obsolete files)

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).
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.
Assignee: nobody → drno
Attached patch Bug993783.patch (obsolete) — Splinter Review
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. -- assuming this is my task ?
Assignee: drno → dclarke
(In reply to  [:onecyrenus] from comment #3)
> -- assuming this is my
> task ?

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.
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.
Attached patch Bug993783.patch (obsolete) — Splinter Review, 

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)
Comment on attachment 8410836 [details] [diff] [review]

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();
>        });
> +, targetPeer, desc, 
> +        state,

Nit: no space at the end of the line.

::: dom/media/tests/mochitest/templates.js
@@ +311,5 @@
>    ],
>    [
>      function (test) {
> +      test.setLocalDescription(test.pcLocal, test.pcLocal._last_offer, HAVE_LOCAL_OFFER, 

Nit: no space at the end of the line

@@ +322,5 @@
>    ],
>    [
>      function (test) {
> +      test.setRemoteDescription(test.pcRemote, test.pcLocal._last_offer, HAVE_REMOTE_OFFER, 

Nit: no space at the end of the line

@@ +354,5 @@
>    ],
>    [
>      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-
+ check_next_test moved within if, also added some logging.
Attachment #8410836 - Attachment is obsolete: true
Attachment #8412377 - Flags: review?(drno)
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.
Comment on attachment 8412377 [details] [diff] [review]

Review of attachment 8412377 [details] [diff] [review]:

lgtm. thanks
Attachment #8412377 - Flags: review?(drno) → review+
Keywords: checkin-needed
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
You need to log in before you can comment on or make changes to this bug.