Closed Bug 991877 Opened 6 years ago Closed 6 years ago

Add a test for PeerConnection.close()

Categories

(Core :: WebRTC: Networking, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla32

People

(Reporter: drno, Assigned: drno)

References

Details

Attachments

(1 file, 5 obsolete files)

We should have a test case which verifies the functionality around RTCPeerConnection.close()
Assignee: nobody → drno
Note: this implementation is known to fail on slower system as the ICEGatheringState does not switch over fast enough.
Attached patch Add test case for PC.close() (obsolete) — Splinter Review
Attachment #8401526 - Attachment is obsolete: true
Depends on: 991368
Try run: https://tbpl.mozilla.org/?tree=Try&rev=d4b371956401
Attachment #8401544 - Attachment is obsolete: true
Attachment #8402122 - Flags: review?(rjesup)
Attachment #8402122 - Flags: review?(jsmith)
Comment on attachment 8402122 [details] [diff] [review]
A new test case for PeerConnection.close()

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

r- but easily modified

::: dom/media/tests/mochitest/test_peerConnection_close.html
@@ +65,5 @@
> +        pc = null;
> +
> +        SimpleTest.finish();
> +      }, 1000);
> +    }, 1000);

These timeouts are likely too short, if they're needed at all (see similar issues with other tests recently, especially on B2G emulator debug).  Either it will never happen (so either make it a good percentage of mochitest timeout time, or let the test time out), or it will happen in which case we don't really care in this context how long it took.  If we care a little about timeliness, we can log if it's longer than N when it happens.
Attachment #8402122 - Flags: review?(rjesup) → review-
(In reply to Randell Jesup [:jesup] from comment #5)
> These timeouts are likely too short, if they're needed at all (see similar
> issues with other tests recently, especially on B2G emulator debug).  Either
> it will never happen (so either make it a good percentage of mochitest
> timeout time, or let the test time out), or it will happen in which case we
> don't really care in this context how long it took.  If we care a little
> about timeliness, we can log if it's longer than N when it happens.

As you can see from this line in the test
  todo(signalstatechanged, "Closing the connection should create a callback");
the callback right now never happen. Which is also documented in Bug 989936.
So if I have a callback sitting their waiting and then just another timer to ensure the callback fires in X time that is one thing.
As the callback right now never fires. We only have three options:
1) Wait for the callback to get implemented properly
2) Have a single long timeout (and sure I can make the timeout longer - but then the test would run that long every single time)
3) Implement a interval based polling (to save time on the faster platforms)
And obviously we could and should replace #2 or #3 with #1 once the callback is properly implemented.

In case of the second timer I actually wait because I do not expected to get a callback (as not state transition should happen). So there I can only wait for X time. If you think 1s is not long enough in that case I'll be happy to increase the value, but again each test would then run that long.
Comment on attachment 8402122 [details] [diff] [review]
A new test case for PeerConnection.close()

Can we really implement this mochitest reliably without fixing bug 989936? It feels like the implementation of this test is blocked on the resolution of bug 989936, as I'm not sure if timeouts are going to be reliable here.
Attachment #8402122 - Flags: review?(jsmith)
Depends on: 989936
Note: this patch depends on landing the patch in 989936. It only tests the locally generated close event for now.
Attachment #8402122 - Attachment is obsolete: true
Attachment #8407959 - Flags: review?(rjesup)
Attachment #8407959 - Flags: review?(jsmith)
Comment on attachment 8407959 [details] [diff] [review]
Patch for testing the PC.close() behavior

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

::: dom/media/tests/mochitest/test_peerConnection_close.html
@@ +84,5 @@
> +    // This is only a shortcut to prevent a mochitest timeout in case the event does not fire
> +    var eTimeout = setTimeout(function() {
> +      ok(signalstatechanged, "Failed to receive expected onsignalingstatechange event in 30s");
> +      SimpleTest.finish();
> +    }, 30000);

let's use at least 60s.  Sometimes the testers can be very slow (b2g emulator), and all we really care about is not hitting the global timeout in the unlikely case of an error.
Attachment #8407959 - Flags: review?(rjesup) → review+
Comment on attachment 8407959 [details] [diff] [review]
Patch for testing the PC.close() behavior

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

review- mainly for the intermittent failure risk.

::: dom/media/tests/mochitest/pc.js
@@ +492,5 @@
>   */
>  PeerConnectionTest.prototype.close = function PCT_close(onSuccess) {
>    info("Closing peer connections. Connection state=" + this.connected);
>  
> +  signalingstatechangeClose = function (state) {

Nit - this patch needs to use camel case throughout each file for variable naming. Also, we need a "var" before this.

@@ +594,5 @@
>      }
>    }
>  
> +  peer.onsignalingstatechange = function (state) {
> +    //info(peer + ": 'onsignalingstatechange' event registered, signalingState: " + peer.signalingState);

Nit - we should remove this dead code

::: dom/media/tests/mochitest/test_peerConnection_close.html
@@ +26,5 @@
> +    pc.onsignalingstatechange = function(aEvent) {
> +      signalstatechanged = true;
> +      is(aEvent, "closed", "onsignalingstatechange event is 'closed'");
> +      is(pc.signalingState, "closed", "Event callback signalingState is 'closed'");
> +      is(pc.iceConnectionState, "closed", "Event callback iceConnectionState is 'closed'");

Nit - signalingState & iceConnectionState would have already been checked at this point after close() is called outside of this event handler. I think we only need one check here - so we can remove the other one.

@@ +95,5 @@
> +    is(pc.signalingState, "closed", "Final signalingState is 'closed'");
> +    is(pc.iceConnectionState, "closed", "Final iceConnectionState is 'closed'");
> +    is(exception, null, "closing the connection raises no exception");
> +
> +    if (signalstatechanged) {

This might lead to intermittent failures because events can choose to fire arbitrarily sometime in the future (i.e. we don't have direct have control over this). I think what we should do here instead is move this into the onsignalingstatechange event handler itself to ensure that this runs more deterministically.

@@ +102,5 @@
> +
> +    // destroy the PC object to be safe
> +    pc = null;
> +
> +    SimpleTest.finish();

I think we need to do these cleanup steps (i.e. pc = null & this) at the end of the event handler, as it's currently possible that this test could finish without completing the event handler.
Attachment #8407959 - Flags: review?(jsmith) → review-
This version can handle closure from outside the event callback (current behavior) as well as within the event callback (in case our implementation behavior should change in the future).

Try run: https://tbpl.mozilla.org/?tree=Try&rev=9da326f362ef
Attachment #8407959 - Attachment is obsolete: true
Attachment #8423242 - Flags: review?(rjesup)
Attachment #8423242 - Flags: review?(jsmith)
Blocks: 850325
Comment on attachment 8423242 [details] [diff] [review]
Patch for testing the PC.close() behavior

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

::: dom/media/tests/mochitest/test_peerConnection_close.html
@@ +8,5 @@
> +<body>
> +<pre id="test">
> +<script type="application/javascript">
> +  createHTML({
> +    bug: "834153",

Nit - bug # should be 991877

@@ +14,5 @@
> +  });
> +
> +  runTest(function () {
> +    var pc = new mozRTCPeerConnection();
> +    var signalstatechanged = false;

Nit - camel case

@@ +85,5 @@
> +        SimpleTest.finish();
> +      }
> +    }
> +
> +    // This is only a shortcut to prevent a mochitest timeout in case the 

Nit - trailing whitespace

@@ +102,5 @@
> +      pc.close();
> +    } catch (e) {
> +      exception = e;
> +    }
> +    is(exception, null, "closing the connection raises no exception");

Is anything below this point of code really needed? We could just call clearTimeout right after the event handler fires (right after line 26).
Attachment #8423242 - Flags: review?(jsmith) → review+
Addressed NIT's and added clearTimeout() in the signal handler.
Carry forward r+=jsmith.
Attachment #8423242 - Attachment is obsolete: true
Attachment #8423242 - Flags: review?(rjesup)
Attachment #8423463 - Flags: review?(rjesup)
Comment on attachment 8423463 [details] [diff] [review]
Patch for testing the PC.close() behavior

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

::: dom/media/tests/mochitest/test_peerConnection_close.html
@@ +40,5 @@
> +      is(pc.signalingState, "closed", "Final signalingState stays at 'closed'");
> +      is(pc.iceConnectionState, "closed", "Final iceConnectionState stays at 'closed'");
> +
> +      // TODO: according to the webrtc spec all of these should throw InvalidStateError's
> +      //       instead they seem to throw simple Error's

File a bug on this please and reference here
Attachment #8423463 - Flags: review?(rjesup) → review+
Opened bug 1011586 for the wrong exception on closed connections.
Keywords: checkin-needed
See Also: → 1011586
https://hg.mozilla.org/mozilla-central/rev/d12fa5b0c0f9
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
You need to log in before you can comment on or make changes to this bug.