-
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 #10468
Conversation
f34c94b
to
c0217c6
Compare
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
Since I've started adding tests based on new PRs in https://github.com/w3c/webrtc-pc, I'll need to bump the spec revision. Will do that once all of them have been merged. |
Add explicit *negotiated false* and *negotiated true and id not defined* test cases
Move an existing test case from RTCDataChannel-send-receive
…e it work in Safari
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 is a pretty massive CL, but seems straightforward (once you accept the "await test.done" paradigm).
I'd like to have it merged, but would like to see approval from someone who feels comfortable approving changes to testharness.js.
|
||
// Disable global timeout | ||
// IMPORTANT: You need to add a timeout to *every test* of this file! | ||
setup({ explicit_timeout: true }); |
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.
Why is the default timeout inappropriate here?
I see that a 5 second timeout is used, while the default timeout is 10 seconds. Saving 5 seconds in the failure case seems hardly worth doing.
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 problem is that there's a varying amount of test cases onto which one constant global timeout is applied, so it's 10 or 60 seconds globally for all test cases no matter how many you have. This workaround allows us to apply a local timeout to each test case.
|
||
await t.done_promise; | ||
}, 'Stress-test with multiple channels sending and receiving using various data sources', { | ||
timeout: 420000 // yep, 7m to be safe |
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 would suggest leaving the long-duration stress test in its own file, to minimize the use of explicit timeouts. It's the only place in the CL with a timeout that is longer than the default.
Thanks Harald. The |
Closing because of messy flags. |
-> #13499 |
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.
frontend | git fast-import []
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 @taylor-b
And I apologise for creating this rather large PR.