Rewrite RTC ICE and DTLS transport tests with alternative dependencies (Rebased) #13886

soareschen · 2018-11-03T07:12:28Z

This is a rebased version of PR #9424.

The PR is updated to make use of async/await.
The code is simplified based on reviews in Rewrite RTC ICE and DTLS transport tests with alternative dependencies #9424.

alvestrand · 2018-11-03T07:28:11Z

webrtc/RTCDtlsTransport-getRemoteCertificates.html

+
+    for(const cert of certs) {
+      assert_true(cert instanceof ArrayBuffer,
+        'Expect certificate elements be instance of ArrayBuffer');


English: "Certificates should be represented as ArrayBuffers".

alvestrand · 2018-11-03T07:28:47Z

webrtc/RTCDtlsTransport-getRemoteCertificates.html

+  function validateConnectingDtlsTransport(dtlsTransport) {
+    if (dtlsTransport.state !== 'connected') {
+      assert_array_equals(dtlsTransport.getCertificates(), [],
+        'Expect DTLS certificates be initially empty until become connected');


English: "The DTLS certificate list should be empty before the transport is connected".

webrtc/RTCDtlsTransport-getRemoteCertificates.html

youennf

Nice!
Some nits and one question about webrtc/tools/.eslintrc.js below.

youennf · 2018-11-05T03:25:25Z

webrtc/RTCDtlsTransport-getRemoteCertificates.html

+  // The following helper functions are called from RTCTransport-helper.js:
+  //  waitForConnectedState
+  //  getDtlsTransportFromSctpTransport
+  //  getDtlsTransportsFromSenderReceiver


Do we need those comments?
I am fearing that they will be obsolete when/if we start updating the tests and forget to update these comments.

Since the helper functions are exposed as global variables, I thought it might be helpful for readers to find out where they come from. If people don't find it useful, we may remove them in new PRs.

webrtc/RTCDtlsTransport-getRemoteCertificates.html

webrtc/RTCTransport-helper.js

youennf · 2018-11-05T03:36:54Z

webrtc/tools/.eslintrc.js

@@ -111,6 +114,7 @@ module.exports = {
    getUserMediaTracksAndStreams: true,
    performOffer: true,
    Resolver: true,
+    addTrackOrTransceiver: true,


Ah, I did not know about that file.
Should I have updated it when adding some helper functions in RTCPeerConnection-helpers.js?

The lint tool was intended as quick way to find out common errors such as undefined variable without being flooded by warnings on the global helpers. It has not been updated for some time so running lint now is not very useful anyway. We probably should either add all global variables to it, or remove the lint tools from the repo.

Either we should enforce running the lint tool for every PR or we should remove it.
Do you think we should do with one or the other?

sideshowbarker · 2018-11-07T03:11:57Z

stability checker failing https://travis-ci.org/web-platform-tests/wpt/jobs/450733046

alvestrand

Found a few more nits that need fixing. The PC leak is important.

alvestrand · 2018-11-14T05:44:52Z

webrtc/RTCIceTransport.html

    assert_greater_than(candidates.length, 0,
      'Expect at least one ICE candidate returned from get*Candidates()');

    for(const candidate of candidates) {
-      assert_true(candidate instanceof RTCIceCandidate,
-        'Expect candidate elements to be instance of RTCIceCandidate');
+     assert_true(candidate instanceof RTCIceCandidate,


Here and below, indentation seems to have turned odd. Please fix.

alvestrand · 2018-11-14T05:46:32Z

webrtc/RTCPeerConnection-helper.js

+// for example would have it implemented but only usable in unified plan,
+// which means it would fail the tests when using default peer connection config
+function canAddTransceiver() {
+  const pc = new RTCPeerConnection();


Please close this PeerConnection in cleanup.

We should not feature detect in tests IMHO, as it ends up testing different things on different browsers.

addTransceiver is in the spec; Chrome should be running these tests with unified-plan enabled already.

soareschen · 2018-11-15T07:50:36Z

All issues fixed. Is there any reason the CI keep failing with Chrome and Firefox Nightly? Any way we can get this merged?

sideshowbarker · 2018-11-15T08:27:45Z

All issues fixed. Is there any reason the CI keep failing with Chrome and Firefox Nightly? Any way we can get this merged?

See #7660 and #14033. So we can merge this PR after review approval

youennf · 2018-11-15T15:02:32Z

webrtc/RTCPeerConnection-helper.js

+// on the supported method on a browser.
+//   - If addTransceiver(track) is usable, use that.
+//   - If addTrack(track, mediastream) is available, use that.
+function addTrackOrTransceiver(pc, track, mediaStream) {


Do we need this one?
It makes sense to support older tests for older browsers but for new tests/new constructs, we can probably hope to have transceivers available. Maybe we do want to test these new constructs with plan B as well, I guess this is fine.

Maybe we should rename it to addTransceiverOrTrack since we first try to add a transceiver and fall back to tracks otherwise.

I agree with @youennf this helper is unneeded, unwanted even, because it ends up testing different things on different browsers. I vote we call addTransceiver or addTrack directly, whichever we intend to test with.

youennf · 2018-11-15T15:02:37Z

webrtc/RTCPeerConnection-helper.js

+//   - If addTrack(track, mediastream) is available, use that.
+function addTrackOrTransceiver(pc, track, mediaStream) {
+  if (canAddTransceiver()) {
+    return pc.addTransceiver(track);


We should probably pass the mediaStream to addTransceiver so as to be consistent with addTrack

youennf · 2018-11-15T15:03:07Z

webrtc/RTCPeerConnection-helper.js

+    return pc.addTrack(track, mediaStream);
+  } else {
+    throw new Error('This test requires either addTrack or addTransceiver implemented');
+  }


Maybe we can simplify and just call addTrack without check. It will anyway throw.

jan-ivar

This may be subjective, but I'd prefer not organizing around helper functions the way this does.

jan-ivar · 2018-11-15T20:23:33Z

webrtc/RTCDtlsTransport-getRemoteCertificates.html

+    assert_greater_than(certs.length, 0,
+      'Expect DTLS transport to have at least one remote certificate when connected');
+
+    for(const cert of certs) {


for space (

jan-ivar · 2018-11-15T20:36:37Z

webrtc/RTCPeerConnection-helper.js

+// for example would have it implemented but only usable in unified plan,
+// which means it would fail the tests when using default peer connection config
+function canAddTransceiver() {
+  const pc = new RTCPeerConnection();


We should not feature detect in tests IMHO, as it ends up testing different things on different browsers.

addTransceiver is in the spec; Chrome should be running these tests with unified-plan enabled already.

jan-ivar · 2018-11-15T20:39:23Z

webrtc/RTCPeerConnection-helper.js

+// on the supported method on a browser.
+//   - If addTransceiver(track) is usable, use that.
+//   - If addTrack(track, mediastream) is available, use that.
+function addTrackOrTransceiver(pc, track, mediaStream) {


I agree with @youennf this helper is unneeded, unwanted even, because it ends up testing different things on different browsers. I vote we call addTransceiver or addTrack directly, whichever we intend to test with.

jan-ivar · 2018-11-15T21:03:35Z

webrtc/RTCTransport-helper.js

+  assert_true(dtlsTransport instanceof RTCDtlsTransport,
+    'Expect sctp.transport to be an RTCDtlsTransport');
+
+  return dtlsTransport;


I confess I'm personally not a fan of tests hidden in helpers and helpers that otherwise merely substitute JS for English—I prefer helpers that encapsulate whole sub-problems, usually with some output.

I find it much easier to read tests that call spec-documented APIs directly. Less to learn.

Others may disagree, but I'd rather see repetition in the tests themselves than too many helpers.

jan-ivar · 2018-11-15T21:04:58Z

webrtc/RTCTransport-helper.js

+  assert_true(iceTransport instanceof RTCIceTransport,
+    'Expect dtlsTransport.iceTransport to be an RTCIceTransport');
+
+  return iceTransport;


ditto with this one.

jan-ivar · 2018-11-15T21:10:40Z

webrtc/RTCTransport-helper.js

+// in pc.There may be more than one DTLSTransport if there are
+// multiple underlying transports, such as when not using max-bundle,
+// or when not using RTCP multiplexing.
+function getDtlsTransportsFromSenderReceiver(pc) {


These helpers collectively abstract away the peer connection APIs we're testing. That may be the right approach for application development, but for writing API tests, I prefer repetition operating on the APIs we're actually testing.

This applies to the remaining helpers here as well.

My 2 cents.

I think it depends on how complicated it is to figure how to perform an operation correctly. There are many things in WebRTC that are not as straightforward as "I want to do X, therefore I'd just call method Y".

I have removed the helpers and make them inline. The inline transport extractors are simplified and not doing any thorough checking or assertions anymore. Hope this will be more readable, easier to verify correctness, and easy for copy/paste for new test cases.

jan-ivar · 2018-11-15T21:20:26Z

These are tests meant to enforce that all browsers behave the same way. We want them readable like prose.

soareschen · 2018-11-20T05:18:11Z

Added suggested changes. Just want to point out that the motivation for addTrackOrTransceiver came out when we were back in the "old" days when addTransceiver was not well supported in most browsers. At that time people were concerned of the tests on interfaces such as RTCRtpSender are failing because they are using addTransceiver, even though browsers have implemented alternative methods addTrack or addStream.

The tests here are neither testing addTrack nor addTransceiver. I prefer not have helpers like addTrackOrTransceiver just to make more tests passing. But I also prefer people not raising this or similar issues because different browsers have different methods implemented, and they all want the tests to pass.

Let's hope history never repeat again for issues similar to whether to use addStream / addTrack(track) / addTrack(track, mediaStream) / addTransceiver to make tests pass, and everyone can move on to just assume addTransceiver / addTrack are there.

jan-ivar

Thanks for addressing this, I like this direction! Some nits and comments, particularly around the tests around number of transports seems wrong.

jan-ivar · 2018-11-20T13:08:13Z

webrtc/RTCDtlsTransport-getRemoteCertificates.html

+      ...pc2.getReceivers()
+    ]
+    .map(senderReceiver => senderReceiver.transport)
+    .filter(dtlsTransport => dtlsTransport); // filter null/undefined


Why allow null and undefined? All sender/receiver.transport objects should be created at this point, no?

Applies throughout.

I probably misremembered or referred to older spec where some transport may be null and still spec-compliant. Removed that assumption now.

jan-ivar · 2018-11-20T13:10:50Z

webrtc/RTCDtlsTransport-getRemoteCertificates.html

+    .map(senderReceiver => senderReceiver.transport)
+    .filter(dtlsTransport => dtlsTransport); // filter null/undefined
+
+    assert_greater_than_equal(dtlsTransports.length, 2, 'expect to find at least 2 RTCDtlsTransports');


This doesn't match what you had before, which used new Set() instead of an array, and then tested the total number of transports in that set, allowing for duplicates, matching how I read transport: "When bundling is used, multiple RTCRtpReceiver objects will share one transport".

I interpret the spec here to mean multiple .transport attributes may point to the exact same JavaScript object when bundling is used. It should work if we use:

const dtlsTransports1 = new Set([...pc1.getSenders(), ...pc1.getReceivers()]);

...and test for dtlsTransports1.size >= 1, but we probably shouldn't mix pc1 and pc2 in the same set as these should never share transport objects. That would be a bug, I think.

Yes the semantics kind of changed a bit, as I tried to avoid copy-pasting too much code when inlining. Though the original test don't really test that the transport objects in different pcs are different though, and validating that is a bit complicated and redundant.

I have simplified the code to just skip all the checkings. They just assume the browser is spec compliant and create a transport object after setting local description. The consequence is mainly error messages in tests is more obscure, for trying to access fields of a null or undefined object for current implementations.

jan-ivar · 2018-11-20T14:58:50Z

webrtc/RTCDtlsTransport-getRemoteCertificates.html

+    }
+
+    await waitForConnectedState(pc1);
+    await waitForConnectedState(pc2);


Nit: Fyi, my eyes burn a bit when I see this pattern, as it looks like a potential race on the surface. I'd prefer:

await Promise.all([waitForConnectedState(pc1), waitForConnectedState(pc2)]);

or

await Promise.all([pc1, pc2].map(waitForConnectedState));

I know it's fine as is, but from its name alone, I would not have expected waitForConnectedState to check for state synchronously:

async function waitForConnectedState(pc) { if (pc.connectionState === 'connected') { return; }

Maybe I'm used to similarly named functions that don't—they too are useful, just different, as they wait for the next time a particular state rolls around.

So while this is fine as is, I worry about people new to async/await copying this pattern. Tests often serve as examples. My 2 cents.

Applies throughout.

Changed it to use Promise.all(). I don't think it matter in this case, as the operations are just waiting for another concurrent operation that are already running.

Async functions always return a promise. So the caller should still resume asynchronously unless they are ignoring the promise.

So while this is fine as is, I worry about people new to async/await copying this pattern. Tests often serve as examples. My 2 cents.

IMO most of the test code are pretty bad examples for writing good application code, even though they may be considered good test code. If we want WPT to serve as examples for using the spec APIs, they should probably written in a much different way.

jan-ivar · 2018-11-20T15:06:23Z

webrtc/RTCIceTransport.html

+    .filter(dtlsTransport => dtlsTransport) // filter null/undefined
+    .map(dtlsTransport => dtlsTransport.iceTransport);
+
+    assert_greater_than_equal(iceTransports.length, 2, 'expect to find at least 2 RTCIceTransports');


Nit: Wouldn't it be fewer lines to just assert each of the two pcN.sctp.transport.iceTransports?

jan-ivar · 2018-11-20T15:10:51Z

webrtc/RTCIceTransport.html

+    await pc1.setLocalDescription(offer)
+    await pc2.setRemoteDescription(offer)
+    const answer = await pc2.createAnswer()
+    await pc1.setRemoteDescription(answer)


Tip: could be inlined, for fewer names polluting the cognitive stack space:

await pc1.setRemoteDescription(await pc2.createAnswer());

Also, missing ; semicolons at EOLs here.

jan-ivar · 2018-11-20T15:28:50Z

webrtc/RTCTransport-helper.js

+      if (connectionState === 'connected') {
+        resolve();
+      } else if (['closed', 'failed'].includes(connectionState)) {
+        reject(new Error(`one of DTLS/ICE transports transition to unexpected state ${connectionState}`));


I advocate wrapping non-promise code as tightly as possible, which means avoiding any kind of application code inside promise constructors.

The WPT test harness adds a second reason: the awkward t.step_func() wrapper is needed to correlate spurious event handlers with a particular test, and it's missing here. Unfortunately, this would mean adding t as an additional argument to waitForConnectedState, which gets ugly.

Instead, I would recommend something like this:

while (pc.connectionState != 'connected') { await new Promise(r => pc.addEventListener('connectionstatechange', r, {once: true})); if (['closed', 'failed'].includes(pc.connectionState)) { throw new Error(`DTLS/ICE transition to unexpected state ${pc.connectionState}`); } }

This way, context is preserved by promise_test and we don't need t.

I agree with your proposition that inside the promise constructor the code should be as minimal as possible. However I do not agree with the proposed solution. Given that the timing for promise resolution is non-deterministic, you may miss an event firing if after the first event, a second event fires before the continuation following the await is executed. Instead of keeping the promise constructor absolute minimal, I think the better way to interpret is that code inside the promise constructor should be more carefully scrutinized, and having less code is a way for verifying the correctness.

I have added a try catch inside the callback to make sure the promise does not get unresolved forever due to exceptions. There are definitely better ways to do this, but given the many conflicting constraints we have for writing tests, I think that should be enough for this particular case.

alvestrand · 2018-11-29T15:57:44Z

@soareschen will you update?

foolip · 2018-12-20T12:16:45Z

This PR is blocked on the required "Travis CI - Pull Request" check after #14499. In order to trigger it, I will close and reopen this PR.

foolip · 2018-12-20T12:26:34Z

This PR is running into a Taskcluster infra issue, see #14618. If, when this is reviewed and ready to merge, there's still trouble with Taskcluster, please poke me, @Hexcles or @jgraham and we can merge it anyway.

alvestrand · 2018-12-20T15:46:05Z

There's still a lot of dependency on sctp.transport.
Given the pattern of implementation, it seems better to get the transport via senders/receivers.

alvestrand · 2019-04-11T14:34:33Z

Closing as "likely obsolete".

wpt-pr-bot added webrtc wg-webrtc labels Nov 3, 2018

wpt-pr-bot requested review from agouaillard, alvestrand, dontcallmedom, guidou, henbos, phoglund, rwaldron, snuggs and youennf November 3, 2018 07:12

soareschen mentioned this pull request Nov 3, 2018

Rewrite RTC ICE and DTLS transport tests with alternative dependencies #9424

Closed

alvestrand reviewed Nov 3, 2018

View reviewed changes

foolip mentioned this pull request Nov 3, 2018

Enable Azure Pipelines #13818

Closed

alvestrand approved these changes Nov 4, 2018

View reviewed changes

youennf approved these changes Nov 5, 2018

View reviewed changes

jugglinmike mentioned this pull request Nov 7, 2018

Verify correctness of Taskcluster PR checks #13194

Closed

youennf approved these changes Nov 7, 2018

View reviewed changes

soareschen force-pushed the rtcicetransport-dep-2 branch from 5150b9b to c48c817 Compare November 7, 2018 08:57

alvestrand requested changes Nov 14, 2018

View reviewed changes

wpt-pr-bot requested a review from jan-ivar November 15, 2018 04:32

youennf reviewed Nov 15, 2018

View reviewed changes

jan-ivar reviewed Nov 15, 2018

View reviewed changes

jan-ivar requested changes Nov 20, 2018

View reviewed changes

soareschen added 19 commits December 10, 2018 16:17

Decouple dependencies for RTCIceTransport tests

a142f6a

Rewrite ICE and DTLS transport tests with alternative dependencies

30f9931

Add in comments

a2f3af7

Refactor RTCIceTransport tests

8b63545

Fix review feedback

52203a2

Remove console.warn to pass lint check

8301d94

Update code to work with async/await and newer browsers

2f00092

Fix review feedback

ce08934

Fix grammar

7691a9f

Fix minor semantics in waitForConnectedState

bb08190

Fix nit

d851654

Fix more nit

9f516dd

Rename dtlsTransport.transport to .iceTransport

f34413b

Remove addTrackOrTransceiver and use addTransceiver directly

75f280b

Remove getDtls/IceTransports helpers

3478f68

Fix senderReceiver array usage

5fbc84d

Don't need to filter null dtlsTransport.iceTransport

10055fe

assert_greater_than_equal

85497d7

Simplify code based on review feedback

d6d0c2e

soareschen force-pushed the rtcicetransport-dep-2 branch from 5dffb44 to d6d0c2e Compare December 10, 2018 09:32

Ensure waitForConnectedState always either resolve regardless of error

7a49b27

foolip closed this Dec 20, 2018

foolip reopened this Dec 20, 2018

This was referenced Dec 20, 2018

Taskcluster checkout step can fail due to "The remote end hung up unexpectedly" #14617

Closed

Taskcluster checkout step can fail due to "Couldn't find remote ref" #14618

Closed

alvestrand closed this Apr 11, 2019

Rewrite RTC ICE and DTLS transport tests with alternative dependencies (Rebased) #13886

Rewrite RTC ICE and DTLS transport tests with alternative dependencies (Rebased) #13886

Conversation

soareschen commented Nov 3, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

youennf left a 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

Choose a reason for hiding this comment

sideshowbarker commented Nov 7, 2018

alvestrand left a 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

soareschen commented Nov 15, 2018

sideshowbarker commented Nov 15, 2018

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

jan-ivar left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jan-ivar commented Nov 15, 2018

soareschen commented Nov 20, 2018

jan-ivar left a comment • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jan-ivar Nov 20, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alvestrand commented Nov 29, 2018

foolip commented Dec 20, 2018

foolip commented Dec 20, 2018

alvestrand commented Dec 20, 2018

alvestrand commented Apr 11, 2019

jan-ivar left a comment •

edited

Loading

jan-ivar Nov 20, 2018 •

edited

Loading