New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add initial test for RTCPeerConnection ondatachannel #6186
Add initial test for RTCPeerConnection ondatachannel #6186
Conversation
Firefox (nightly)Testing web-platform-tests at revision 9d556ec All results6 tests ran/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 9d556ec All results6 tests ran/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 118977 characters in length, which is in excess of GitHub.com's limit for comments (65536 characters). Chrome (unstable)Testing web-platform-tests at revision 9d556ec All results6 tests ran/webrtc/RTCPeerConnection-createAnswer.html
/webrtc/RTCPeerConnection-createOffer.html
/webrtc/RTCPeerConnection-ondatachannel.html
/webrtc/RTCPeerConnection-setLocalDescription.html
|
Sauce (MicrosoftEdge)Testing web-platform-tests at revision 9d556ec All results6 tests ran/webrtc/RTCPeerConnection-createAnswer.html
/webrtc/RTCPeerConnection-createOffer.html
/webrtc/RTCPeerConnection-ondatachannel.html
/webrtc/RTCPeerConnection-setLocalDescription.html
/webrtc/RTCPeerConnection-setRemoteDescription.html
/webrtc/RTCSctpTransport-constructor.html
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Small fix. I know you're not done yet, but I've begun updating the coverage spec as well: https://rwaldron.github.io/webrtc-pc/#rtcdatachannel (I'll keep tracking as you progress)
// Helper function to exchange ice candidates between | ||
// two local peer connections | ||
function exchangeIceCandidates(pc1, pc2) { | ||
function doExchange(localPc, remotePc) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Move doExchange
outside of exchangeIceCandidates
(there are no closures)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know. But without proper module system I don't really want to expose the private helper function globally. Putting it as an inner function helps indicate that it is private without going through too much hacks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, that's a fair argument ;P
|
||
// The following helper functions are called from RTCPeerConnection-helper.js: | ||
// exchangeIceCandidates | ||
// doSignalingHandshake |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
b459b50
to
5302c6f
Compare
I have rebased and added a few more tests. I also removed the tests on the connection states. Turns out my assumption was wrong and the underlying transports can be of any state regardless of whether the upper layer transport is connected. Will figure other way to test the peer connection states. This is now ready for review and merge. |
Great work! |
This is a work in progress.(edit: done) I added one basic test for establishing data channel connection with a remote peer and fire adatachannel
event.I also make use of this to test the state change of the peer connection andRTC*Transport
objects. The idea is that for thedatachannel
event to be fired on a remote peer, it must first establish an SCTP connection and send the data channel negotiation through aDATA_CHANNEL_OPEN
message. In other words by that time theRTCDtlsTransport
andRTCIceTransport
should have all been in connected state.Other than that, since the test connection only have oneRTCDtlsTransport
and oneRTCIceTransport
and we know their final state, we can also test on the peer connection state.There is only one thing that I am not very certain. Currently running on the test on Chrome, theRTCIceConnectionState
isconnected
instead ofcompleted
. I am not sure if under TrickleICE, a DTLS connection can be established while the ICE agent is still attempting connection with each candidate pair. The current assumption for the test is that this is allowed, and the assertion allows the finalRTCIceConnectionState
to be eitherconnected
orcompleted
. Would appreciate if anyone familar with this can confirm or correct me if I am wrong.(Edit: assumption was wrong and test is removed)