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

webrtc: improve Firefox compability of RTCDTMFHelper #9941

Merged
merged 1 commit into from Mar 11, 2018

Conversation

@fippo
Copy link
Contributor

fippo commented Mar 9, 2018

does not check canInsertDTMF which is not implemented in Firefox yet.

webrtc: improve Firefox compability of RTCDTMFHelper
does not check canInsertDTMF which is not implemented in Firefox yet.
@fippo
Copy link
Contributor Author

fippo left a comment

@alvestrand PTAL. You don't want @jan-ivar to be sad about tests that fail when they could work ;-)

// Wait until dtmfSender.canInsertDTMF becomes true.
// Up to 150 ms has been observed in test. Wait 1 second
// in steps of 10 ms.
// Note: Using a short timeout and rejected promise in order to
// make test return a clear error message on failure.
return new Promise((resolve, reject) => {
let counter = 0;
let checkfunc = function() {
step_timeout(function checkCanInsertDTMF() {

This comment has been minimized.

Copy link
@fippo

fippo Mar 9, 2018

Author Contributor

drive-by style change

This comment has been minimized.

Copy link
@alvestrand

alvestrand Mar 11, 2018

Contributor

Yeah, @henbos commented on this busy-looping too. The reason to have a timeout here is that if the overall timeout triggers, the error message gets lost.

This comment has been minimized.

Copy link
@fippo

fippo Mar 11, 2018

Author Contributor

kept the busy-loop here, just changed the style ;-)

});
}).then(() => {
assert_true(dtmfSender.canInsertDTMF,

This comment has been minimized.

Copy link
@fippo

fippo Mar 9, 2018

Author Contributor

this assertion used to be always true, the promise would have been rejected otherwise.

This comment has been minimized.

Copy link
@alvestrand

alvestrand Mar 11, 2018

Contributor

Yeah, this is a leftover from the version before the waiting promise was inserted.

@w3c-bots

This comment has been minimized.

Copy link

w3c-bots commented Mar 9, 2018

Build PASSED

Started: 2018-03-09 08:35:19
Finished: 2018-03-09 08:47:58

Failing Jobs

  • safari:11.0

Unstable Results

Browser: "Safari 11.0" (failures allowed)

View in: WPT PR Status | TravisCI

Test Subtest Results Messages
/webrtc/RTCDTMFSender-ontonechange.https.html   TIMEOUT: 2
OK: 8
  Calling insertDTMF() in the middle of tonechange events should cause future tonechanges to be updated to new tones FAIL: 8
assert_unreached: Unexpected promise rejection: Error: Invalid constraint Reached unreachable code
  Calling insertDTMF('') in the middle of tonechange events should stop future tonechange events from firing FAIL: 8
assert_unreached: Unexpected promise rejection: Error: Invalid constraint Reached unreachable code
  Calling insertDTMF() multiple times in the middle of tonechange events should cause future tonechanges to be updated the last provided tones FAIL: 8
assert_unreached: Unexpected promise rejection: Error: Invalid constraint Reached unreachable code
  insertDTMF('') should not fire any tonechange event, including for '' tone FAIL: 8
assert_unreached: Unexpected promise rejection: Error: Invalid constraint Reached unreachable code
  insertDTMF with comma should delay next tonechange event for a constant 2000ms FAIL: 8
assert_unreached: Unexpected promise rejection: Error: Invalid constraint Reached unreachable code
  insertDTMF() with default duration and intertoneGap should fire tonechange events at the expected time FAIL: 8
assert_unreached: Unexpected promise rejection: Error: Invalid constraint Reached unreachable code
  insertDTMF() with duration less than 40 should be clamped to 40 FAIL: 8
assert_unreached: Unexpected promise rejection: Error: Invalid constraint Reached unreachable code
  insertDTMF() with explicit duration and intertoneGap should fire tonechange events at the expected time FAIL: 8
assert_unreached: Unexpected promise rejection: Error: Invalid constraint Reached unreachable code
  insertDTMF() with interToneGap less than 30 should be clamped to 30 FAIL: 8
assert_unreached: Unexpected promise rejection: Error: Invalid constraint Reached unreachable code
  insertDTMF() with transceiver stopped in the middle should stop future tonechange events from firing FAIL: 8
assert_unreached: Unexpected promise rejection: Error: Invalid constraint Reached unreachable code
  Setting transceiver.currentDirection to recvonly in the middle of tonechange events should stop future tonechange events from firing FAIL: 8
undefined is not an object (evaluating 'dtmfSender.addEventListener')
@alvestrand
Copy link
Contributor

alvestrand left a comment

Looks good. We need to test canInsertDtmf behavior too, but helpers shouldn't try to be tests.

// Wait until dtmfSender.canInsertDTMF becomes true.
// Up to 150 ms has been observed in test. Wait 1 second
// in steps of 10 ms.
// Note: Using a short timeout and rejected promise in order to
// make test return a clear error message on failure.
return new Promise((resolve, reject) => {
let counter = 0;
let checkfunc = function() {
step_timeout(function checkCanInsertDTMF() {

This comment has been minimized.

Copy link
@alvestrand

alvestrand Mar 11, 2018

Contributor

Yeah, @henbos commented on this busy-looping too. The reason to have a timeout here is that if the overall timeout triggers, the error message gets lost.

});
}).then(() => {
assert_true(dtmfSender.canInsertDTMF,

This comment has been minimized.

Copy link
@alvestrand

alvestrand Mar 11, 2018

Contributor

Yeah, this is a leftover from the version before the waiting promise was inserted.

@alvestrand alvestrand merged commit 9c9d8f2 into web-platform-tests:master Mar 11, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@fippo fippo deleted the fippo:better-dtmf-test-helper branch Mar 12, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.