Add initial test for sending/receiving data for RTCDataChannel #6215
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
wpt-pr-bot
requested review from
rwaldron,
dontcallmedom,
agouaillard,
alvestrand,
youennf,
phoglund,
henbos and
guidou
|
Most of the tests are straightforward, other than I am not very sure of the behavior for the
When testing The current assumption for the test is that the earlier interpretation is correct, and the tests fail on current browser because synchronous sending is not allowed. Please correct me if I misunderstood anything. |
Firefox (nightly)Testing web-platform-tests at revision 384256d All results8 tests ran/webrtc/RTCDataChannel-bufferedAmount.html
/webrtc/RTCDataChannel-send.html
/webrtc/RTCPeerConnection-createAnswer.html
/webrtc/RTCPeerConnection-createOffer.html
/webrtc/RTCPeerConnection-ondatachannel.html
/webrtc/RTCPeerConnection-setLocalDescription.html
/webrtc/RTCPeerConnection-setRemoteDescription.html
/webrtc/RTCSctpTransport-constructor.html
|
Sauce (safari)Testing web-platform-tests at revision 384256d All results8 tests ran/webrtc/RTCDataChannel-bufferedAmount.html
/webrtc/RTCDataChannel-send.html
/webrtc/RTCPeerConnection-createAnswer.html
/webrtc/RTCPeerConnection-createOffer.html
/webrtc/RTCPeerConnection-ondatachannel.html
/webrtc/RTCPeerConnection-setLocalDescription.html
/webrtc/RTCPeerConnection-setRemoteDescription.html
/webrtc/RTCSctpTransport-constructor.html
|
|
*This report has been truncated because the total content is 124951 characters in length, which is in excess of GitHub.com's limit for comments (65536 characters). Chrome (unstable)Testing web-platform-tests at revision 384256d All results8 tests ran/webrtc/RTCDataChannel-bufferedAmount.html
/webrtc/RTCDataChannel-send.html
/webrtc/RTCPeerConnection-createAnswer.html
/webrtc/RTCPeerConnection-createOffer.html
/webrtc/RTCPeerConnection-ondatachannel.html
/webrtc/RTCPeerConnection-setLocalDescription.html
|
Sauce (MicrosoftEdge)Testing web-platform-tests at revision 384256d All results8 tests ran/webrtc/RTCDataChannel-bufferedAmount.html
/webrtc/RTCDataChannel-send.html
/webrtc/RTCPeerConnection-createAnswer.html
/webrtc/RTCPeerConnection-createOffer.html
/webrtc/RTCPeerConnection-ondatachannel.html
/webrtc/RTCPeerConnection-setLocalDescription.html
/webrtc/RTCPeerConnection-setRemoteDescription.html
/webrtc/RTCSctpTransport-constructor.html
|
|
@soareschen there is a conflict in this branch, but it's very minimal: // Helper function to exchange ice candidates between
// two local peer connections
function exchangeIceCandidates(pc1, pc2) {
<<<<<<< HEAD
=======
// private function
>>>>>>> 73395d471befa78497bf62ec21bcfd88074c5a78
|
I cannot find spec text to support this Based on my reading of:
I think that the implementations are just wrong, but I think we need some feedback from spec authors. Can you file an issue on webrtc-pc? Thanks! |
rwaldron
requested changes
Jun 12, 2017
| // Simple ASCII encoded string | ||
| const helloString = 'hello'; | ||
| // ASCII encoded buffer representation of the string | ||
| const helloBuffer = Uint8Array.of(0x68, 0x65, 0x6c, 0x6c, 0x6f); |
There was a problem hiding this comment.
If you wanted to avoid hand-writing these bytes, you could do this:
Uint8Array.from(helloString, char => char.charCodeAt(0));(That's not a blocker, just a thought)
There was a problem hiding this comment.
Technically we could use the TextEncoder to encode the ASCII and unicode strings. But I think hard coded byte values is better here, since the test strings are short. It avoids additional dependency and demonstrates that nothing can go wrong of using the encoder incorrectly.
There was a problem hiding this comment.
I think hard coded byte values is better here, since the test strings are short. It avoids additional dependency and demonstrates that nothing can go wrong of using the encoder incorrectly.
I agree with this rationale, 100%
webrtc/RTCDataChannel-send.html
Outdated
| const pc = new RTCPeerConnection(); | ||
| const channel = pc.createDataChannel('test'); | ||
| channel.close(); | ||
| assert_equals(channel.readyState, 'closing'); |
There was a problem hiding this comment.
This is failing with the following:
In Firefox Nightly:
assert_equals: expected "closing" but got "closed"
In Chrome Canary:
assert_equals: expected "closing" but got "connecting"
In Safari Developer Preview:
assert_equals: expected "closing" but got "closed"
(W/r to the actual state, I think Chrome is incorrect.)
readyState will not be "closing" after you've called the close() method. This is specified here:
An RTCDataChannel object's underlying data transport may be torn down in a non-abrupt manner by running the closing procedure. When that happens the user agent must, unless the procedure was initiated by the close method, queue a task that sets the object's readyState attribute to closing. This will eventually render the data transport closed.
(emphasis added)
There was a problem hiding this comment.
The closing procedure is initiated by the close method. So the step "queue a task that sets the object's readyState attribute to closing" is not applied.
Step 3 of close() says "Set channel's readyState attribute to closing.", so after synchronous return the readyState should be closing.
I wanted to write a RTCDataChannel-close.html in separate PR. Just happen to write this first and have to call close() in here as well.
webrtc/RTCDataChannel-send.html
Outdated
| // createDataChannelPair | ||
| // awaitMessage | ||
| // blobToArrayBuffer | ||
| // assertEqualsArrayBuffer |
There was a problem hiding this comment.
As much as I truly wish that the convention for assertion functions was camel-case, this breaks consistency with all of testharness.js and all of your own assertion functions.
webrtc/RTCDataChannel-send.html
Outdated
| channel.close(); | ||
| assert_equals(channel.readyState, 'closing'); | ||
| channel.send(helloString); | ||
| }, 'Calling send() when data channel is in closing state should succeed'); |
There was a problem hiding this comment.
Calling send() when data channel is in closing state should succeed
I don't understand this test message in the context of 6.2, step 4.
There was a problem hiding this comment.
If channel's underlying data transport is not established yet, or if the closing procedure has started, then abort these steps.
My understanding for this is that if readyState is not connected, the method is aborted without actually sending the data.
The case for "channel's underlying data transport is not established yet" is a bit curious, because step 2 have already guard against connecting state.
I think the reason it is abort in close state is because in the current version, any error sending would abruptly close the connection and the exception is swallowed.
I have raised some comment in w3c/webrtc-pc#1209 that if error sending is changed to throwing, then it should throw error if connection is closed as well. Perhaps this test can be deferred until that is resolved.
soareschen
force-pushed
the
rtcdatachannel-send
branch
from
73395d4
to
cce7d5f
Compare
I did a search and found w3c/webrtc-pc#1111. (Should have done this earlier) Basically my initial interpretation is correct:
But given that both of us get confused by the description in |
|
I have filed w3c/webrtc-pc#1376 to ask for improvement to |
soareschen
force-pushed
the
rtcdatachannel-send
branch
from
cce7d5f
to
14cab04
Compare
|
I have refactored the test for |
Awesome! Thanks for that follow up |
rwaldron
added a commit
to rwaldron/webrtc-pc
that referenced
this pull request
Jun 13, 2017
…bufferedAmount - web-platform-tests/wpt#6215 Signed-off-by: Rick Waldron <waldron.rick@gmail.com>
rwaldron
approved these changes
Jun 13, 2017
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This adds basic testing for sending / receiving messages between two data channels. It has a common commit with #6186 that make use of helper functions to connect between two peer connections.
For now the code only test on sending a few short messages without considering the cases for low buffered amount. It also doesn't test on closing the data channel, which will be tested in separate file.