All users were logged out of Bugzilla on October 13th, 2018

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

RESOLVED FIXED in mozilla18

Status

()

RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: rwood, Assigned: rwood)

Tracking

unspecified
mozilla18
All
Gonk (Firefox OS)
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

6 years ago
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.
(Assignee)

Comment 1

6 years ago
Created attachment 662319 [details] [diff] [review]
bug790991 patch
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-
(Assignee)

Comment 4

6 years ago
Created attachment 663029 [details] [diff] [review]
bug790991-fix patch

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
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
You need to log in before you can comment on or make changes to this bug.