-
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
Rewrite RTC ICE and DTLS transport tests with alternative dependencies (Rebased) #13886
Changes from 1 commit
a142f6a
30f9931
a2f3af7
8b63545
52203a2
8301d94
2f00092
ce08934
7691a9f
bb08190
d851654
9f516dd
f34413b
75f280b
3478f68
5fbc84d
10055fe
85497d7
d6d0c2e
7a49b27
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
- Loading branch information
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -476,14 +476,29 @@ async function exchangeOfferAndListenToOntrack(t, caller, callee) { | |
const ontrackPromise = addEventListenerPromise(t, callee, 'track'); | ||
await exchangeOffer(caller, callee); | ||
return ontrackPromise; | ||
} | ||
|
||
// Feature detect whether we can call add transceiver on the target browser. | ||
// We cannot simply test if the addTransceiver method exist, as Chrome | ||
// 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(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please close this PeerConnection in cleanup. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
try { | ||
pc.addTransceiver('audio'); | ||
return true; | ||
} catch (err) { | ||
return false; | ||
} | ||
} | ||
|
||
// A dependency agnostic way of adding track depending | ||
// on the supported method on a browser. Uses addTransceiver(track) | ||
// first if available, otherwise use addTrack(track, mediaStream) | ||
// or falls back to the legacy addStream method (which has been removed | ||
// from the spec). | ||
function addTrackOrTransceiver(pc, track, mediaStream) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we need this one? Maybe we should rename it to addTransceiverOrTrack since we first try to add a transceiver and fall back to tracks otherwise. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
if (typeof pc.addTransceiver === 'function') { | ||
if (canAddTransceiver()) { | ||
return pc.addTransceiver(track); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should probably pass the mediaStream to addTransceiver so as to be consistent with addTrack |
||
} else if (typeof pc.addTrack === 'function') { | ||
// Note: The mediaStream argument is optional in the spec, but we require | ||
|
@@ -542,8 +557,9 @@ async function createTrackAndStreamWithCleanup(t, kind = 'audio') { | |
function findTransceiverForSender(pc, sender) { | ||
const transceivers = pc.getTransceivers(); | ||
for (let i = 0; i < transceivers.length; ++i) { | ||
if (transceivers[i].sender == sender) | ||
if (transceivers[i].sender == sender) { | ||
return transceivers[i]; | ||
} | ||
} | ||
return null; | ||
} |
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.
Tip: could be inlined, for fewer names polluting the cognitive stack space:
Also, missing
;
semicolons at EOLs here.