Adding tests for RTCDataChannel id attribute. #5742

taylor-b · 2017-04-30T04:48:10Z

When no ID argument is provided, and the DTLS role hasn't been
determined, the id attribute should return null.

After the DTLS role is determined by an answer being applied with
"a=setup:active" (or "passive"), the null IDs should be replaced with
either odd or even IDs, depending on the negotiated role.

This change is

When no ID argument is provided, and the DTLS role hasn't been determined, the id attribute should return `null`. After the DTLS role is determined by an answer being applied with "a=setup:active" (or "passive"), the null IDs should be replaced with either odd or even IDs, depending on the negotiated role.

wpt-pr-bot · 2017-04-30T04:48:18Z

Notifying @agouaillard, @alvestrand, @dontcallmedom, @foolip, @guidou, @henbos, and @phoglund. (Learn how reviewing works.)

taylor-b · 2017-04-30T04:49:14Z

This is the thing @foolip requested on w3c/webrtc-pc#1137, with an additional test for the odd/even ID selection after setRemoteDescription.

agouaillard · 2017-04-30T04:53:46Z

Read the thread, looked at the code. LGTM. As soon as @foolip approve, (and travis is happy) I will merge.

w3c-bots · 2017-04-30T04:56:53Z

View the complete job log.

Lint

Passed

w3c-bots · 2017-04-30T04:56:54Z

View the complete job log.

Firefox (nightly channel)

Testing web-platform-tests at revision 5805688
Using browser at version BuildID 20170507100239; SourceStamp 37a5b7f6f101df2eb292b1b6baaf1540c9920e20
Starting 10 test iterations
All results were stable

All results

2 tests ran

/webrtc/RTCDataChannel-id.html

Subtest	Results	Messages
	OK
`DTLS client uses odd data channel IDs`	FAIL	`assert_not_equals: got disallowed value 65535`
`DTLS server uses even data channel IDs`	FAIL	`assert_equals: id expected 0 but got 1`

/webrtc/RTCPeerConnection-createDataChannel.html

Subtest	Results	Messages
	OK
`createDataChannel required arguments`	PASS
`createDataChannel with closed connection`	PASS
`createDataChannel defaults`	FAIL	`RTCDataChannel is not defined`
`createDataChannel with label "foo"`	PASS
`createDataChannel with label null`	PASS
`createDataChannel with label undefined`	PASS
`createDataChannel with label lone surrogate`	PASS
`createDataChannel with ordered false`	PASS
`createDataChannel with ordered null/undefined`	PASS
`createDataChannel with maxPacketLifeTime 0`	FAIL	`assert_equals: expected (number) 0 but got (undefined) undefined`
`createDataChannel with maxRetransmits 0`	FAIL	`assert_equals: expected (number) 0 but got (undefined) undefined`
`createDataChannel with both maxPacketLifeTime and maxRetransmits`	FAIL	`assert_throws: function "() => pc.createDataChannel('', {\n maxPacketLifeTime: 0,\n maxRetransmits: 0\n })" threw object "InvalidParameterError: Both maxPacketLifeTime and maxRetransmits cannot be provided" that is not a DOMException SyntaxError: property "code" is equal to 0, expected 12`
`createDataChannel with protocol "foo"`	PASS
`createDataChannel with protocol null`	PASS
`createDataChannel with protocol undefined`	PASS
`createDataChannel with protocol lone surrogate`	PASS
`createDataChannel with negotiated true`	FAIL	`assert_equals: expected (boolean) true but got (undefined) undefined`
`createDataChannel with id -1`	FAIL	`assert_throws: function "() => pc.createDataChannel('', { id: id })" did not throw`
`createDataChannel with id 0`	PASS
`createDataChannel with id 1`	PASS
`createDataChannel with id 65534`	PASS
`createDataChannel with id 65535`	FAIL	`assert_throws: function "() => pc.createDataChannel('', { id: id })" did not throw`
`createDataChannel with id 65536`	FAIL	`assert_throws: function "() => pc.createDataChannel('', { id: id })" did not throw`
`createDataChannel with priority "high"`	FAIL	`assert_equals: expected (string) "high" but got (undefined) undefined`

w3c-bots · 2017-04-30T04:56:54Z

View the complete job log.

Chrome (unstable channel)

Testing web-platform-tests at revision 5805688
Using browser at version 60.0.3088.3 dev
Starting 10 test iterations
All results were stable

All results

2 tests ran

/webrtc/RTCDataChannel-id.html

Subtest	Results	Messages
	OK
`DTLS client uses odd data channel IDs`	PASS
`DTLS server uses even data channel IDs`	PASS

/webrtc/RTCPeerConnection-createDataChannel.html

Subtest	Results	Messages
	OK
`createDataChannel required arguments`	PASS
`createDataChannel with closed connection`	PASS
`createDataChannel defaults`	FAIL	`assert_equals: maxPacketLifeTime expected (object) null but got (undefined) undefined`
`createDataChannel with label "foo"`	PASS
`createDataChannel with label null`	PASS
`createDataChannel with label undefined`	PASS
`createDataChannel with label lone surrogate`	PASS
`createDataChannel with ordered false`	PASS
`createDataChannel with ordered null/undefined`	PASS
`createDataChannel with maxPacketLifeTime 0`	FAIL	`assert_equals: expected (number) 0 but got (undefined) undefined`
`createDataChannel with maxRetransmits 0`	PASS
`createDataChannel with both maxPacketLifeTime and maxRetransmits`	FAIL	`assert_throws: function "() => pc.createDataChannel('', {\n maxPacketLifeTime: 0,\n maxRetransmits: 0\n })" did not throw`
`createDataChannel with protocol "foo"`	PASS
`createDataChannel with protocol null`	PASS
`createDataChannel with protocol undefined`	PASS
`createDataChannel with protocol lone surrogate`	PASS
`createDataChannel with negotiated true`	PASS
`createDataChannel with id -1`	PASS
`createDataChannel with id 0`	PASS
`createDataChannel with id 1`	PASS
`createDataChannel with id 65534`	FAIL	`Failed to execute 'createDataChannel' on 'RTCPeerConnection': RTCDataChannel is not supported`
`createDataChannel with id 65535`	FAIL	`assert_throws: function "() => pc.createDataChannel('', { id: id })" threw object "NotSupportedError: Failed to execute 'createDataChannel' on 'RTCPeerConnection': RTCDataChannel is not supported" ("NotSupportedError") expected object "TypeError" ("TypeError")`
`createDataChannel with id 65536`	PASS
`createDataChannel with priority "high"`	FAIL	`assert_equals: expected (string) "high" but got (undefined) undefined`

taylor-b · 2017-04-30T05:38:21Z

The new tests are failing on Firefox because it looks like Firefox only assigns the ID after the DTLS (and SCTP?) handshake completes. But the spec currently says it should be assigned by setting an answer. This was discussed briefly in a virtual interim, and changed in this PR: w3c/webrtc-pc#1038

But there wasn't a lot of discussion. @jesup, @jan-ivar, any opinions? Should the spec be more loose about when the ID is assigned?

agouaillard · 2017-05-01T04:36:41Z

This is a spec question. If the test as is tests the current spec draft, I am tempted to merge it in, and let the discussion happen in the spec repository. If / when there is a spec change, then this test will be modified accordingly. The risk is that any test could be otherwise stay on hold waiting for the final version of the spec. Unless there is a strong objection, I will merge it Tuesday evening end of day pacific time.

…

Sent from my iPhone

On 30 Apr 2017, at 1:38 pm, Taylor Brandstetter ***@***.***> wrote: The new tests are failing on Firefox because it looks like Firefox only assigns the ID after the DTLS (and SCTP?) handshake completes. But the spec currently says it should be assigned by setting an answer. This was discussed briefly in a virtual interim, and changed in this PR: w3c/webrtc-pc#1038 But there wasn't a lot of discussion. @jesup, @jan-ivar, any opinions? Should the spec be more loose about when the ID is assigned? I ― You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

jan-ivar · 2017-05-01T19:34:57Z

webrtc/RTCPeerConnection-createDataChannel.html

+    const answer = new RTCSessionDescription({
+      type: "answer",
+      sdp: pc.localDescription.sdp.replace("actpass", "active")
+    });


@agouaillard By that standard we shouldn't use the legacy RTCSessionDescription constructor ;)

jesup · 2017-05-01T20:22:58Z

webrtc/RTCPeerConnection-createDataChannel.html

+    // and should use even data channel IDs, according to rtcweb-data-channel.
+    assert_equals(channel.id, 0, 'id');
+    const another_channel = pc.createDataChannel('another');
+    assert_equals(another_channel.id, 2, 'id');


I don't think the spec says the next channel must be '2', just that it must be even.

jesup · 2017-05-01T20:23:22Z

webrtc/RTCPeerConnection-createDataChannel.html

+  .then(function() {
+    // Since the remote description had a "passive" DTLS role, we're the client
+    // and should use even data channel IDs, according to rtcweb-data-channel.
+    assert_equals(channel.id, 0, 'id');


I don't think the spec says this must be 0, just that it must be even

jesup · 2017-05-01T20:23:56Z

webrtc/RTCPeerConnection-createDataChannel.html

+  .then(function() {
+    // Since the remote description had an "active" DTLS role, we're the server
+    // and should use odd data channel IDs, according to rtcweb-data-channel.
+    assert_equals(channel.id, 1, 'id');


I don't think the spec says this must be 1, just that it must be odd:
"If the side is the DTLS client, it MUST choose an even
Stream Identifier, if the side is the DTLS server, it MUST choose an
odd one. "

Fixed. Now only testing that the IDs are odd, and distinct from each other.

jesup · 2017-05-01T20:24:08Z

webrtc/RTCPeerConnection-createDataChannel.html

+    // and should use odd data channel IDs, according to rtcweb-data-channel.
+    assert_equals(channel.id, 1, 'id');
+    const another_channel = pc.createDataChannel('another');
+    assert_equals(another_channel.id, 3, 'id');


I don't think the spec says this must be 3, just that it must be odd

jesup · 2017-05-01T20:30:07Z

The interim discussion was on "what do we return before the id is established", and agreed null made more sense than the current impl's 65535.

Assigning the ID when setting the answer would require we inform the DataChannel code of the DTLS roles that will be negotiated, assuming one is guaranteed to know that at Answer time. Right now as stated we (firefox) assign them on DTLS negotiation, which gates us being able to actually send the Open packets on a specific id.

w3c-bots · 2017-05-01T21:16:29Z

These tests are now available on w3c-test.org

taylor-b · 2017-05-01T21:32:25Z

@jesup

Assigning the ID when setting the answer would require we inform the DataChannel code of the DTLS roles that will be negotiated, assuming one is guaranteed to know that at Answer time.

Since the answer contains the answerer's selected role, this should be guaranteed. Would it be feasible to change Firefox's behavior? Or should the spec's requirements be made more loose (for example, "the ID will be assigned at some point before readyState changes to "open"")?

foolip · 2017-05-03T12:51:25Z

webrtc/RTCPeerConnection-createDataChannel.html

  assert_equals(channel.priority, 'low', 'priority');
 }, 'createDataChannel defaults');

+async_test(test => {


Can you move this test into RTCDataChannel-id.html or similar? This file is for the createDataChannel method without further poking at the returned RTCDataChannel.

foolip · 2017-05-03T12:54:23Z

webrtc/RTCPeerConnection-createDataChannel.html

+  .then(function() {
+    // Turn our own offer SDP into valid answer SDP by setting the DTLS role to
+    // "active".
+    const answer = new RTCSessionDescription({


setRemoteDescription takes a RTCSessionDescriptionInit dictionary, so just passing the object (dictionary) directly should work?

foolip · 2017-05-03T13:19:42Z

webrtc/RTCPeerConnection-createDataChannel.html

+    assert_not_equals(channel.id, another_channel.id);
+    test.done();
+  })
+  .catch(test.step_func(function(e) {


This test would probably be slightly smaller using promise_test, which would avoid the need for this.

foolip · 2017-05-03T13:20:17Z

webrtc/RTCPeerConnection-createDataChannel.html

+  const pc = new RTCPeerConnection;
+  const channel = pc.createDataChannel('');
+  pc.createOffer()
+  .then(function(offer) {


This could be offer => pc.setLocalDescription(offer)

taylor-b · 2017-05-07T22:24:36Z

@foolip Addressed your comments; take another look.

wpt-pr-bot added webrtc wg-webrtc labels Apr 30, 2017

jan-ivar reviewed May 1, 2017

View reviewed changes

jesup suggested changes May 1, 2017

View reviewed changes

Allow any distinct odd/even IDs; don't require "0, 2, 4, ..."

1f5e055

foolip requested changes May 3, 2017

View reviewed changes

Move data channel ID tests to new file, follow foolip's other advice.

97c3ec1

foolip approved these changes May 9, 2017

View reviewed changes

foolip merged commit ce56f0e into web-platform-tests:master May 9, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding tests for RTCDataChannel id attribute. #5742

Adding tests for RTCDataChannel id attribute. #5742

taylor-b commented Apr 30, 2017 •

edited by wpt-reviewable-bot

wpt-pr-bot commented Apr 30, 2017

taylor-b commented Apr 30, 2017

agouaillard commented Apr 30, 2017 •

edited

w3c-bots commented Apr 30, 2017 •

edited

w3c-bots commented Apr 30, 2017 •

edited

w3c-bots commented Apr 30, 2017 •

edited

taylor-b commented Apr 30, 2017 •

edited

agouaillard commented May 1, 2017 via email

jan-ivar May 1, 2017

jesup May 1, 2017

jesup May 1, 2017

jesup May 1, 2017

taylor-b May 1, 2017

jesup May 1, 2017

jesup commented May 1, 2017

w3c-bots commented May 1, 2017

taylor-b commented May 1, 2017

foolip May 3, 2017

foolip May 3, 2017

foolip May 3, 2017

foolip May 3, 2017

taylor-b commented May 7, 2017

Adding tests for RTCDataChannel id attribute. #5742

Adding tests for RTCDataChannel id attribute. #5742

Conversation

taylor-b commented Apr 30, 2017 • edited by wpt-reviewable-bot

wpt-pr-bot commented Apr 30, 2017

taylor-b commented Apr 30, 2017

agouaillard commented Apr 30, 2017 • edited

w3c-bots commented Apr 30, 2017 • edited

Lint

w3c-bots commented Apr 30, 2017 • edited

Firefox (nightly channel)

All results

w3c-bots commented Apr 30, 2017 • edited

Chrome (unstable channel)

All results

taylor-b commented Apr 30, 2017 • edited

agouaillard commented May 1, 2017 via email

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jesup commented May 1, 2017

w3c-bots commented May 1, 2017

taylor-b commented May 1, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

taylor-b commented May 7, 2017

taylor-b commented Apr 30, 2017 •

edited by wpt-reviewable-bot

agouaillard commented Apr 30, 2017 •

edited

w3c-bots commented Apr 30, 2017 •

edited

w3c-bots commented Apr 30, 2017 •

edited

w3c-bots commented Apr 30, 2017 •

edited

taylor-b commented Apr 30, 2017 •

edited