-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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
More Tests for WebRTC Data Channels #13499
Conversation
Update to current spec version Fix missing import to RTCPeerConnection-helper.js Test .bufferedAmount and .bufferedAmountLowThreshold Fix invalid use of 'label' in RTCDataChannelInit Add negotiated true and id null test Add reusing an id that is in use tests
description has been updated)
options are synchronized correctly across peers. Add optional 'channelLabel' and 'channelOptions' to `createDataChannelPair`
Once canSendSize has been determined, require it to be available in further tests that depend on it Fix canSendSize checks (explicitly check for the 0 case) Fix remoteMMS > canSendSize test for the 0 case Update descriptions to make them more precise
Add a test that tries to create the maximum number of data channels until no ids are free anymore after the connection has been established. Add a test that checks if channel IDs are being reassigned after exhaustion in case a channel has been closed. Add a test that tries to create the theoretical maximum number of data channels and ensure that violation of the odd/even rule does not occur. Add a test that tries to create the theoretical maximum number of data channels before the connection has been established (and check if some of them are properly teared down).
Add test for violation of odd/even rule with mixed channels (negotiated & DCEP-negotiated)
… have a strong reference to the RTCPeerConnection instance anyway
Move RTCPeerConnection.close tests into their own file
Update to current spec version Add assert for 'state' Fix the assert for 'maxMessageSize' A bit of cleanup
…fires with the correct state Add an `IceCandidateQueue` helper class which allows you hold back ICE candidates conveniently until `exchangeIceCandidates` is called
…no data channels are being created
…no SCTP transport is being negotiated
…ons and comments only)
…rocedure and the onclose event Add some timeout and fix missing unreachable checks for RTCDataChannel-onopen
…ide of the range of the negotiated amount of SCTP streams
…damountlow event and the threshold value itself
Fix missing assert_unreached in RTCDataChannel-close Fix invalid creation of a data channel in RTCDataChannel-close
Add send test for DataView Add test for invalid binary type Add test for changing the binary type on-the-fly Add tests for sending and receiving n bytes Add test for closing a channel with outstanding data
…send Remove datachannel-emptystring
…age size and expects a TypeError in case the message is larger than that
Minor refactoring
Split sending outstanding messages before a channel may become closed tests into two (close on the local/remote side) in RTCDataChannel-send-receive Add DCEP-channel ID reuse test to RTCPeerConnection-createDataChannel
…tLowThreshold Add a test which enqueues 10 messages
Looking at #11029 (comment) and #11120, WPT seems to go away from setting timeout in individual tests. |
Magnificent... This will require a lot of tedious refactoring and I don't have time for that before next year. |
Note to self: Needs updating to some tests due to w3c/webrtc-pc#1999 |
@youennf will try to get the impractical (multi-minute) tests disabled or removed; we should merge-and-iterate on the rest. |
@alvestrand The main problem seems to be the design choice now (local timeouts for each test, disabled global timeout). Since I have found a little bit of time, a suggestion: I'll refactor one of the larger test groups and you folks tell me if it looks good to you. Then I'll proceed with refactoring the rest. Sounds good? |
Current plan is to split this PR as smaller pieces:
|
To do notes (mostly for myself):
|
Originally from web-platform-tests#13499
|
||
// We use the queue to hold back candidates, so no connection can be established | ||
const pc1Queue = new IceCandidateQueue(pc1); | ||
const pc2Queue = new IceCandidateQueue(pc2); |
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.
This being the lone use of IceCandidateQueue, I propose using a promise as a simple queue here instead:
promise_test(async (t) => {
const pc1 = new RTCPeerConnection();
const pc2 = new RTCPeerConnection();
t.add_cleanup(() => pc1.close());
t.add_cleanup(() => pc2.close());
// We use a queue to hold back candidates, so no connection can be established
let flush;
const queue = new Promise(r => flush = r);
pc1.onicecandidate = e => queue.then(() => pc2.addIceCandidate(e.candidate));
pc2.onicecandidate = e => queue.then(() => pc1.addIceCandidate(e.candidate));
// Negotiate SCTP transport
pc1.createDataChannel('test');
await doSignalingHandshake(pc1, pc2);
for (const pc of [pc1, pc2]) {
assert_equals(pc.sctp.state, 'connecting',
'RTCSctpTransport should be in the connecting state');
}
flush(); // Let the peers connect
await Promise.all([pc1, pc2].map(async pc => {
await new Promise(r => pc.sctp.onstatechange = r);
assert_equals(pc.sctp.state, 'connected');
}));
}
This should also fix that the test doesn't bail before both pc
s have had their statechange
event tested.
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.
@lgrahl Odd, looks like github had some old unsubmitted review comments cached here. The last comment is the one I wanted you to look at.
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.
@youennf ^
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'm fine with implementing that for this specific test case.
Should I resolve the rest of the comments or are these still valid?
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.
The first unresolved comment thread is the one about splitting this up. Presumably that's still happening? I just commented here because I couldn't find this test in the newer PRs. If we split it up and avoid modifying exchangeIceCandidates to take a queue, then I'm happy.
Status Dec 18: Waiting for split-up PRs to land, then we'll close this one. |
* Update RTCPeerConnection-helper.js according changes proposed in #13499 * Modernize doSignalingHandshake
…stonly Automatic update from web-platform-tests Update RTCPeerConnection-helper.js (#14417) * Update RTCPeerConnection-helper.js according changes proposed in web-platform-tests/wpt#13499 * Modernize doSignalingHandshake -- wpt-commits: b75b876e7d5843582f21e5b52c54d509dffb6da0 wpt-pr: 14417
…stonly Automatic update from web-platform-tests Update RTCPeerConnection-helper.js (#14417) * Update RTCPeerConnection-helper.js according changes proposed in web-platform-tests/wpt#13499 * Modernize doSignalingHandshake -- wpt-commits: b75b876e7d5843582f21e5b52c54d509dffb6da0 wpt-pr: 14417
…stonly Automatic update from web-platform-tests Update RTCPeerConnection-helper.js (#14417) * Update RTCPeerConnection-helper.js according changes proposed in web-platform-tests/wpt#13499 * Modernize doSignalingHandshake -- wpt-commits: b75b876e7d5843582f21e5b52c54d509dffb6da0 wpt-pr: 14417
…stonly Automatic update from web-platform-tests Update RTCPeerConnection-helper.js (#14417) * Update RTCPeerConnection-helper.js according changes proposed in web-platform-tests/wpt#13499 * Modernize doSignalingHandshake -- wpt-commits: b75b876e7d5843582f21e5b52c54d509dffb6da0 wpt-pr: 14417
…stonly Automatic update from web-platform-tests Update RTCPeerConnection-helper.js (#14417) * Update RTCPeerConnection-helper.js according changes proposed in web-platform-tests/wpt#13499 * Modernize doSignalingHandshake -- wpt-commits: b75b876e7d5843582f21e5b52c54d509dffb6da0 wpt-pr: 14417 UltraBlame original commit: 118c5c41cfd03c68da65ed6775072450154c86dc
…stonly Automatic update from web-platform-tests Update RTCPeerConnection-helper.js (#14417) * Update RTCPeerConnection-helper.js according changes proposed in web-platform-tests/wpt#13499 * Modernize doSignalingHandshake -- wpt-commits: b75b876e7d5843582f21e5b52c54d509dffb6da0 wpt-pr: 14417 UltraBlame original commit: 603e8e8097b742115dca2e195f59f65618b97ac0
…stonly Automatic update from web-platform-tests Update RTCPeerConnection-helper.js (#14417) * Update RTCPeerConnection-helper.js according changes proposed in web-platform-tests/wpt#13499 * Modernize doSignalingHandshake -- wpt-commits: b75b876e7d5843582f21e5b52c54d509dffb6da0 wpt-pr: 14417 UltraBlame original commit: 118c5c41cfd03c68da65ed6775072450154c86dc
…stonly Automatic update from web-platform-tests Update RTCPeerConnection-helper.js (#14417) * Update RTCPeerConnection-helper.js according changes proposed in web-platform-tests/wpt#13499 * Modernize doSignalingHandshake -- wpt-commits: b75b876e7d5843582f21e5b52c54d509dffb6da0 wpt-pr: 14417 UltraBlame original commit: 603e8e8097b742115dca2e195f59f65618b97ac0
…stonly Automatic update from web-platform-tests Update RTCPeerConnection-helper.js (#14417) * Update RTCPeerConnection-helper.js according changes proposed in web-platform-tests/wpt#13499 * Modernize doSignalingHandshake -- wpt-commits: b75b876e7d5843582f21e5b52c54d509dffb6da0 wpt-pr: 14417 UltraBlame original commit: 118c5c41cfd03c68da65ed6775072450154c86dc
…stonly Automatic update from web-platform-tests Update RTCPeerConnection-helper.js (#14417) * Update RTCPeerConnection-helper.js according changes proposed in web-platform-tests/wpt#13499 * Modernize doSignalingHandshake -- wpt-commits: b75b876e7d5843582f21e5b52c54d509dffb6da0 wpt-pr: 14417 UltraBlame original commit: 603e8e8097b742115dca2e195f59f65618b97ac0
Closing this PR on the assumption that split-up PRs have landed. |
Description
This PR updates all data channel related tests to reflect the current spec (at the time of writing, that was revision 1cc5bfc3ff18741033d804c4a71f7891242fb5b3).
Updated/Fixed Tests
New Tests
DOMString
orUint8Array
)undefined
,null
,0
,65535
or4294967295
)1024
,16384
,65536
,131072
,262144
,524288
,1048576
,16777216
or33554432
)Comments
Only few helper functions have been changed but everything is either backwards compatible or the affected tests have been updated. FWIW, I have carefully tested new test cases (if that was possible) on Firefox and Chrome by temporarily commenting out asserts and in some cases even by throwing in adapter (
RTCSctpTransport.maxMessageSize
for example).Towards WebRTC implementations: A bunch of new issues are revealed by these tests. I'm at a tight time schedule, so I can't file them for you at the moment. This listing here should help you in finding new or updated test cases, so you can track and file new issues in your bug trackers. /cc @nils-ohlmeier @jan-ivar. Please also forward this to whoever is responsible for Google's, Safari's and Edge's (:joy_cat:) data channel implementation.
And I apologise for creating this rather large PR.