Skip to content
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

Test for a no-media call. #1783

Merged
merged 5 commits into from
May 12, 2015

Conversation

alvestrand
Copy link
Contributor

This test verifies that a connection is established for a call where
no media is added, but the initiator offers to receive video.

The iceconnectionstate = connected indicator is used to determine
that the connection is established.

(This test submission is also a test on the review process for submitting new tests. Please give feedback as appropriate.)

This test verifies that a connection is established for a call where
no media is added, but the initiator offers to receive video.

The iceconnectionstate = connected indicator is used to determine
that the connection is established.
@hoppipolla-critic-bot
Copy link

Critic review: https://critic.hoppipolla.co.uk/r/4832

This is an external review system which you may optionally use for the code review of your pull request.

In order to help critic track your changes, please do not make in-place history rewrites (e.g. via git rebase -i or git commit --amend) when updating this pull request.

<script src="/resources/testharnessreport.js"></script>
<script src="/common/vendor-prefix.js"
data-prefixed-objects=
'[{"ancestors":["navigator"], "name":"getUserMedia"},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

getUserMedia is not used, so no need to enable its prefixing

@dontcallmedom
Copy link
Contributor

I've taken a first pass at commenting on the test case; also, the test times out in firefox (whereas it passes in Chrome) — not sure why yet.

Used wrappers for the rest of callbacks, added TODO about using
a promise-based API, addressed other comments.
@alvestrand
Copy link
Contributor Author

PTAL

// Note: This test should have been as below, but second connection
// does not reach "completed". This is noted as an issue in the spec.
// https://github.com/w3c/webrtc-pc/issues/224
if (gFirstConnection.iceConnectionState == 'completed' &&
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

since w3c/webrtc-pc#224 is now closed (or about to be), that needs an update; if "completed" && "completed" is the expected outcome, we should only keep that version.

(I've since determined that the timeout in firefox I reported separately is due to it being stuck in "connected" / "connected" apparently)

Test now runs in Firefox, except that Firefox doesn't implement the
oniceconnectionstatechange callback.

There is a display on the test page for the state info, and the comment
explaining the reason for early test termination has been expanded.
@alvestrand
Copy link
Contributor Author

PTAL.

Restored the prefixing, and also added a little more polish (as well as removed the video display tags that we don't use). One more Firefox issue found and bypassed (it doesn't queue createAnswer behind setLocalDescription).

Also displayed what the status is at test end.

<script src="/common/vendor-prefix.js"
data-prefixed-objects=
'[{"ancestors":["window"], "name":"RTCPeerConnection"},
{"ancestors":["window"], "name":"RTCSessionDescription"}]'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

still misses one for RTCIceCandidate for Firefox sake

@dontcallmedom
Copy link
Contributor

we're nearly there :)

@alvestrand
Copy link
Contributor Author

Great - I now have a test that works in Firefox 37.0.2 too - the last fix did it!
Wonderful!

dontcallmedom added a commit that referenced this pull request May 12, 2015
@dontcallmedom dontcallmedom merged commit 25cf28d into web-platform-tests:master May 12, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants