908012 - B2G RIL: Add telephony marionette tests for conference call

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect)

Comment 1

[Hsin-Yi Tsai (she/her) | away [:hsinyi]]

WIP: https://github.com/hsinyi/releases-mozilla-central/tree/bugzilla/908012

Comment 2

[Hsin-Yi Tsai (she/her) | away [:hsinyi]]

Attachment: 908012.patch

Comment 3

[Hsin-Yi Tsai (she/her) | away [:hsinyi]]

Attachment: 908012.patch

Addressed some nits.

Comment 4

[Hsin-Yi Tsai (she/her) | away [:hsinyi]]

Attachment: 908012.patch

Comment 5

[Vicamo Yang [:vicamo][:vyang]]

Comment on attachment 799977 [details] [diff] [review]
908012.patch

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

Thank you, but I'd like to have another run :)

::: dom/telephony/test/marionette/test_conference.js
@@ +30,5 @@
> +  log("Verifying initial state.");
> +  ok(telephony);
> +  is(telephony.active, null);
> +  ok(telephony.calls);
> +  is(telephony.calls.length, 0);

I'd like to have a convenient function to track |telephony.active|, |telephony.calls|, |conference.calls| changes every time you perform an action.

  function checkStates(telephonyActive, telephonyCalls, conferenceState, conferenceCalls) { ... }

@@ +31,5 @@
> +  ok(telephony);
> +  is(telephony.active, null);
> +  ok(telephony.calls);
> +  is(telephony.calls.length, 0);
> +  calls = telephony.calls;

nit: |calls| is assigned but never referenced.

@@ +33,5 @@
> +  ok(telephony.calls);
> +  is(telephony.calls.length, 0);
> +  calls = telephony.calls;
> +
> +  runEmulatorCmd("gsm clear", function(result) {

<Copied from my comment in bug 821579>
Please have a utility function for this.  See test_filter_mixed.js[1].  Or this case may, not very often, result in an intermittent failure like bug 903058.

[1]: https://mxr.mozilla.org/mozilla-central/source/dom/mobilemessage/tests/marionette/test_filter_mixed.js#16

@@ +48,5 @@
> +  is(outgoingCall.state, "dialing");
> +
> +  is(outgoingCall, telephony.active);
> +  is(telephony.calls.length, 1);
> +  is(telephony.calls[0], outgoingCall);

So we can replace these three calls with |checkStates(outgoingCall, [outgoingCall], '', null);|.

@@ +51,5 @@
> +  is(telephony.calls.length, 1);
> +  is(telephony.calls[0], outgoingCall);
> +
> +  outgoingCall.onalerting = function(event) {
> +    log("Received 'onalerting' call event.");

disconnect onalerting event handler?

@@ +171,5 @@
> +function conferenceAddTwoCalls() {
> +  log("Creating a conference call.");
> +
> +  ok(conference);
> +  is(conference.calls.length, 0);

It looks like |telephony.conference| is instantiated along with |telephony| itself.  So, we should move this check into |verifyInitialState()| and check |conference.calls.length| there, too.

@@ +196,5 @@
> +  };
> +
> +  conference.onconnected = function(event) {
> +    log("Received 'connected' event for the conference call.");
> +    conference.onconnected = null;

Is there a fixed order of conference oncallchanged and onconnected events?  We can have a |is(conference.oncallchanged, null);| to make sure these events come in order.  So applies to outgoingCall onstatechange and ongroupchange events.  And incomingCall, of course.

Comment 6

[Hsin-Yi Tsai (she/her) | away [:hsinyi]]

Attachment: 908012.patch (v2)

Review comments addressed.

Comment 7

[Hsin-Yi Tsai (she/her) | away [:hsinyi]]

try looks good! https://tbpl.mozilla.org/?tree=Try&rev=45f569850d32

Comment 8

[Hsin-Yi Tsai (she/her) | away [:hsinyi]]

https://hg.mozilla.org/integration/b2g-inbound/rev/980c155d507e

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/980c155d507e