Closed
Bug 864118
Opened 11 years ago
Closed 10 years ago
Enhance existing WebRTC mochitest framework to test against a TURN server
Categories
(Core :: WebRTC, defect, P3)
Core
WebRTC
Tracking
()
RESOLVED
FIXED
mozilla35
People
(Reporter: jsmith, Assigned: drno)
References
(Blocks 1 open bug)
Details
(Whiteboard: [WebRTC][blocking-webrtc-][turn][p=2, s=fx33])
Attachments
(1 file, 2 obsolete files)
6.28 KB,
patch
|
bwc
:
review+
|
Details | Diff | Splinter Review |
We've landed support for TURN, but we're not running mochitest automation against it. This bug tracks the work to enhance the WebRTC mochitest automation to allow it to test against a TURN server vs. STUN server in a configurable manner. Note - a lot of the tests will have some similarities running under STUN and TURN, so we need to be mindful of test reusability here.
Reporter | ||
Updated•11 years ago
|
Priority: -- → P3
Reporter | ||
Updated•11 years ago
|
Whiteboard: [WebRTC][blocking-webrtc-][turn]
Updated•10 years ago
|
Whiteboard: [WebRTC][blocking-webrtc-][turn] → [WebRTC][blocking-webrtc-][turn][p=2]
Target Milestone: --- → mozilla32
Comment 2•10 years ago
|
||
After discussing with Nils and Randell, we're going to move this to Fx 33.
No longer blocks: 970426
Target Milestone: mozilla32 → mozilla33
Updated•10 years ago
|
Whiteboard: [WebRTC][blocking-webrtc-][turn][p=2] → [WebRTC][blocking-webrtc-][turn][p=2, s=fx33]
Updated•10 years ago
|
Target Milestone: mozilla33 → mozilla35
Assignee | ||
Comment 3•10 years ago
|
||
Simple verification of what the RTCP stats tell us which ICE candidates are in use.
Attachment #8517067 -
Flags: review?(docfaraday)
Assignee | ||
Comment 4•10 years ago
|
||
Try run: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=3138bbd22d77 BTW this is mostly for running in our new QA lab where some machines are setup to require TURN.
Assignee | ||
Comment 5•10 years ago
|
||
Fixed bugs from previous try run. New try run: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=8b9d850ba5b5
Attachment #8517067 -
Attachment is obsolete: true
Attachment #8517067 -
Flags: review?(docfaraday)
Attachment #8517158 -
Flags: review?(docfaraday)
Comment 6•10 years ago
|
||
Comment on attachment 8517158 [details] [diff] [review] bug_864118_add_turn_verification.patch Review of attachment 8517158 [details] [diff] [review]: ----------------------------------------------------------------- Need some fixes here I think. ::: dom/media/tests/mochitest/pc.js @@ +2573,5 @@ > + */ > + checkStatsIceConnectionType : function PCW_checkStatsIceConnectionType(stats) > + { > + if (Object.keys(stats).length === 0) { > + info("checkStatsIceConnectionType called with emtpy stats"); s/emtpy/empty/ @@ +2585,5 @@ > + lId = stats[name].localCandidateId; > + rId = stats[name].remoteCandidateId; > + } > + }); > + info("Veryfying: local=" + JSON.stringify(stats[lId]) + s/Veryfying/Verifying/ @@ +2593,5 @@ > + var lIp = stats[lId].ipAddress; > + var rIp = stats[rId].ipAddress; > + if ((this.configuration) && (typeof this.configuration.iceServers !== 'undefined')) { > + info("Ice Server configured"); > + var serverIp = this.configuration.iceServers[0].url.split(':')[1]; Hmm. This leaves a gotcha in place if we ever try to use an FQDN, but this is primarily intended for use in the lab, so maybe ok. I would add a note to the config-file warning about this. @@ +2595,5 @@ > + if ((this.configuration) && (typeof this.configuration.iceServers !== 'undefined')) { > + info("Ice Server configured"); > + var serverIp = this.configuration.iceServers[0].url.split(':')[1]; > + ok((lType === "relayed" || rType === "relayed") || > + (lIp === serverIp || rIp === serverIp), "One peer uses a relay"); I think that second '||' was intended to be an '&&'? Even so, I'm a little worried about the intent here. Even if we use a TURN server, we will only select pairs that use it if there is no direct connectivity. Are we assuming that a configured TURN server implies that we're doing a test that will prevent direct connectivity? Additionally, we still have the relayed-classified-as-peer-reflexive corner-case to worry about. @@ +2600,5 @@ > + } else { > + info("P2P configured"); > + ok(((lType !== "relayed") && (rType !== "relayed")), "Pure peer to peer call without a relay"); > + } > + info("done"); Do we need this? ::: dom/media/tests/mochitest/templates.js @@ +23,5 @@ > if (typeof test._remote_answer !== 'undefined') { > dump("ERROR: SDP answer: " + test._remote_answer.sdp.replace(/[\r]/g, '')); > } > > + if ((test.pcLocal) && (typeof test.pcLocal._local_ice_candidates !== 'undefined')) { This typeof check seems strange; what about _remote_ice_candidates and _ice_candidates_to_add? Should those not be logged if there are no local? Are we willing to log these as "undefined"? I'd be ok with logging "undefined" for all of these, but that's just me. @@ +28,5 @@ > + dump("pcLocal._local_ice_candidates: " + JSON.stringify(test.pcLocal._local_ice_candidates) + "\n"); > + dump("pcLocal._remote_ice_candidates: " + JSON.stringify(test.pcLocal._remote_ice_candidates) + "\n"); > + dump("pcLocal._ice_candidates_to_add: " + JSON.stringify(test.pcLocal._ice_candidates_to_add) + "\n"); > + } > + if ((test.pcRemote) && (typeof test.pcRemote._local_ice_candidates !== 'undefined')) { Same question here.
Attachment #8517158 -
Flags: review?(docfaraday) → review-
Assignee | ||
Comment 7•10 years ago
|
||
Addressed some of bwc's comments. Try run: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=8694d6877e8a
Attachment #8517158 -
Attachment is obsolete: true
Attachment #8521012 -
Flags: review?(docfaraday)
Assignee | ||
Comment 8•10 years ago
|
||
(In reply to Byron Campen [:bwc] from comment #6) > @@ +2593,5 @@ > > + var lIp = stats[lId].ipAddress; > > + var rIp = stats[rId].ipAddress; > > + if ((this.configuration) && (typeof this.configuration.iceServers !== 'undefined')) { > > + info("Ice Server configured"); > > + var serverIp = this.configuration.iceServers[0].url.split(':')[1]; > > Hmm. This leaves a gotcha in place if we ever try to use an FQDN, but this > is primarily intended for use in the lab, so maybe ok. I would add a note to > the config-file warning about this. Yes, it is for our lab use cases only. I added a note. > @@ +2595,5 @@ > > + if ((this.configuration) && (typeof this.configuration.iceServers !== 'undefined')) { > > + info("Ice Server configured"); > > + var serverIp = this.configuration.iceServers[0].url.split(':')[1]; > > + ok((lType === "relayed" || rType === "relayed") || > > + (lIp === serverIp || rIp === serverIp), "One peer uses a relay"); > > I think that second '||' was intended to be an '&&'? Even so, I'm a little > worried about the intent here. Even if we use a TURN server, we will only > select pairs that use it if there is no direct connectivity. Are we assuming > that a configured TURN server implies that we're doing a test that will > prevent direct connectivity? Additionally, we still have the > relayed-classified-as-peer-reflexive corner-case to worry about. Just filed bug 1097333 for the relay vs. reflexive thing. The '||' is intentionally, because it only serves as a backup for the case that the relayed candidate got wrongly categorized as peer reflexive. Only in that case I want to compare IP addresses. > ::: dom/media/tests/mochitest/templates.js > @@ +23,5 @@ > > if (typeof test._remote_answer !== 'undefined') { > > dump("ERROR: SDP answer: " + test._remote_answer.sdp.replace(/[\r]/g, '')); > > } > > > > + if ((test.pcLocal) && (typeof test.pcLocal._local_ice_candidates !== 'undefined')) { > > This typeof check seems strange; what about _remote_ice_candidates and > _ice_candidates_to_add? Should those not be logged if there are no local? > Are we willing to log these as "undefined"? I'd be ok with logging > "undefined" for all of these, but that's just me. The main idea behind the check is to verify that these queues have been generated by the trickle ICE test functions (they don't exist if no trickle ICE is used). And the existence of one of them as treated as indicator that all three got added/exist. I guess because of the JSON.stringify() calls around them it should be safe to call them even if they are not defined...
Comment 9•10 years ago
|
||
Comment on attachment 8521012 [details] [diff] [review] bug_864118_add_turn_verification.patch Review of attachment 8521012 [details] [diff] [review]: ----------------------------------------------------------------- lgtm
Attachment #8521012 -
Flags: review?(docfaraday) → review+
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed
Comment 10•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/9e7f014425c1
Keywords: checkin-needed
Comment 11•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/9e7f014425c1
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Updated•9 years ago
|
Flags: qe-verify-
You need to log in
before you can comment on or make changes to this bug.
Description
•