790991 - [WebAPI] WebTelephony: Develop a test to verify swapping held and active calls

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[Robert Wood [:rwood]]

Create a new WebTelephony WebAPI test as follows: Have two calls, one active and one held. Then swap the calls, so the active one becomes held and the held call becomes the active one.

Comment 1

[Robert Wood [:rwood]]

Attachment: bug790991 patch

Comment 2

[Jonathan Griffin (:jgriffin)]

Comment on attachment 662319 [details] [diff] [review]
bug790991 patch

Review of attachment 662319 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good!

::: dom/telephony/test/marionette/test_swap_held_and_active.js
@@ +203,5 @@
> +    is(telephony.calls[0], outgoingCall);
> +    is(telephony.calls[1], incomingCall);
> +
> +    // Wait 250ms before verifying the swap
> +    setTimeout(verifySwap, 250);

Does the test fail without this?  Does the onheld event always occur after the onconnected one, or does it appear not to be deterministic?  If it varies you could eliminate potential timing issues by replacing this setTimeout with something like:

if (gotHeld && gotOnConnected) { verifySwap(); }

in both the onconnected and onheld event handlers, so that whichever event happens last will trigger it.

Comment 3

[Philipp von Weitershausen [:philikon]]

Comment on attachment 662319 [details] [diff] [review]
bug790991 patch

Review of attachment 662319 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/telephony/test/marionette/test_swap_held_and_active.js
@@ +203,5 @@
> +    is(telephony.calls[0], outgoingCall);
> +    is(telephony.calls[1], incomingCall);
> +
> +    // Wait 250ms before verifying the swap
> +    setTimeout(verifySwap, 250);

I agree with jgriffin here. This is by far not a complicated asynchronous scenario. Synchronizing those two events (if the order is non-deterministic) should be no problem. Timeouts should only ever be the last resort. Generally they're a really bad idea and a great way to introduce random oranges.

Comment 4

[Robert Wood [:rwood]]

Attachment: bug790991-fix patch

Thank you for the feedback guys, appreciated.  Changes made and replacement patch uploaded.

Comment 5

[Jonathan Griffin (:jgriffin)]

Comment on attachment 663029 [details] [diff] [review]
bug790991-fix patch

Review of attachment 663029 [details] [diff] [review]:
-----------------------------------------------------------------

lgtm!

Comment 6

[Jonathan Griffin (:jgriffin)]

https://hg.mozilla.org/mozilla-central/rev/b061c2316d0c