Closed Bug 790991 Opened 7 years ago Closed 7 years ago

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

Categories

(Core :: DOM: Device Interfaces, defect)

All
Gonk (Firefox OS)
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla18

People

(Reporter: rwood, Assigned: rwood)

Details

Attachments

(1 file, 1 obsolete file)

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.
Attached patch bug790991 patch (obsolete) — Splinter Review
Attachment #662319 - Flags: review?(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.
Attachment #662319 - Flags: review?(jgriffin) → review+
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.
Attachment #662319 - Flags: review-
Thank you for the feedback guys, appreciated.  Changes made and replacement patch uploaded.
Attachment #662319 - Attachment is obsolete: true
Attachment #663029 - Flags: review?(jgriffin)
Comment on attachment 663029 [details] [diff] [review]
bug790991-fix patch

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

lgtm!
Attachment #663029 - Flags: review?(jgriffin) → review+
https://hg.mozilla.org/mozilla-central/rev/b061c2316d0c
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
You need to log in before you can comment on or make changes to this bug.