916695 - B2G RIL: telephony related marionette tests are broken after test_conference.js

Status: RESOLVED FIXED

(Firefox OS Graveyard :: RIL, defect)

Description

[Szu-Yu Chen [:aknow] (inactive)]

[To reproduce]

Modify dom/telephony/test/marionette/manifest.ini
Move [test_conference.js] to the 1st one. Then run the test for whole manifest

Ex:
[test_conference.js]    
[test_crash_emulator.js]
...

[Root cause]

There are some existed call after test_conference.js. However, in next test, we didn't correctly clear the existed call.

This is the code snippet we used to clear the call in every test case. [1]

function cancelExistingCalls(callList) {
  if (callList.length && callList[0] != "OK") {
    // Existing calls remain; get rid of the next one in the list
    nextCall = callList.shift().split(' ')[2].trim();

Check the log

INFO Initial call list: outbound to  5555551111 : active,inbound from 5555552222 : held,inbound from 5555553333 : held,OK
INFO Cancelling existing call ''

Beware of this line 'outbound to  5555551111 : active'
                                ^^ two space here

So after |callList.shift().split(' ')|
> a = 'outbound to  5555551111 : active,'
> a.split(' ')
[ 'outbound', 'to', '', '5555551111', ':', 'active,' ]
      0         1    2       3 

We are using the wrong index to get the call number.

From emulator, we use the following two string as the prefix [2].

"outbound to "
"inbound from"

I guess we are intent to let them have the same width. So under this scenario, we may have two <space> before the number in outbound and only one <space> before the number in inbound case.

[Solution]

(1) use regex for the split (tiny change for all the test case)

   nextCall = callList.shift().split(' ')[2].trim();
=> nextCall = callList.shift().split(/\s+/)[2].trim();

The solution is verified and could correctly fix this problem.

(2) now we have "gsm clear", re-write each test case by using this command

I have try this one. However, running "gsm clear" in next test case crashes the emulator. This might be another issue and need further study.


[1] http://dxr.mozilla.org/mozilla-central/source/dom/telephony/test/marionette/test_crash_emulator.js#l26
[2] https://github.com/mozilla-b2g/platform_external_qemu/blob/e804997a08c1cc85b0f5dbcdfa7d40e57269c91e/android/console.c#L1422

Comment 1

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

Thanks for the bug and reporting. :) Looks like you got the root cause of 'gsm clear,' then I vote for option 2).

Comment 2

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: Fix cancelExistingCalls

Use option 1.

I will file another bug for option 2 modification, which is not applicable right now because the state of telephony is not correctly restored after "gsm clear".
=> depends on bug 916722 (bug 915638)

Comment 3

[Szu-Yu Chen [:aknow] (inactive)]

Comment on attachment 805815 [details] [diff] [review]
Fix cancelExistingCalls

Hi Hsinyi,

Restart the review.

Just change my mind. I prefer to fix this problem by option 1 (using correct split() ) now.
Later, when Bug 805838 is ready, I will file another bug to move these two parts to head.js
  (1) clear call
  (2) verify initial state

Then we could include the header and run it as the test setup for each test. At that time, we could also change the usage to "gsm clear".

Comment 4

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

Comment on attachment 805815 [details] [diff] [review]
Fix cancelExistingCalls

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

Looks good but please remember to improve test_outgoing_radio_off.js as well. Thank you.

::: dom/telephony/test/marionette/test_incoming_remote_cancel.js
@@ +88,5 @@
>  
>  function cancelIncoming(){
>    log("Remote party cancelling call before it is answered.");
>  
> +  // We get no 'disconnecting' event when remote party cancels+/hangs-up call

I believe the way of using '/' is fine. No need to add '+'.

Comment 5

[Szu-Yu Chen [:aknow] (inactive)]

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #4)
> Comment on attachment 805815 [details] [diff] [review]
> Fix cancelExistingCalls
> 
> Review of attachment 805815 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good but please remember to improve test_outgoing_radio_off.js as
> well. Thank you.
> 
> ::: dom/telephony/test/marionette/test_incoming_remote_cancel.js
> @@ +88,5 @@
> >  
> >  function cancelIncoming(){
> >    log("Remote party cancelling call before it is answered.");
> >  
> > +  // We get no 'disconnecting' event when remote party cancels+/hangs-up call
> 
> I believe the way of using '/' is fine. No need to add '+'.

Thank you. It's a false positive caused by my lazy search/replace...

Comment 6

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: #2 Fix cancelExistingCalls

Comment 7

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

Comment on attachment 806423 [details] [diff] [review]
#2 Fix cancelExistingCalls

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

Thanks for catching these up!

::: dom/telephony/test/marionette/test_outgoing_radio_off.js
@@ +122,5 @@
>    sendToEmulator("gsm clear", function(result) {
>      is(result[0], "OK");
> +
> +    waitFor(function() { dial("0912345678"); },
> +            function() { return telephony.calls.length === 0; });

Let's use the same style as bug 915638 comment 17.

Comment 8

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: [Final] Fix cancelExistingCalls. r=hsinyi

Comment 9

[Szu-Yu Chen [:aknow] (inactive)]

https://tbpl.mozilla.org/?tree=Try&rev=d90cc73f9cdb

Comment 10

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

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

Comment 11

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 12

[Jonathan Griffin (:jgriffin)]

I would like to uplift this patch to mozilla-aurora; it applies cleanly.  Any objections, :aknow or :hsinyi?

Comment 13

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

(In reply to Jonathan Griffin (:jgriffin) from comment #12)
> I would like to uplift this patch to mozilla-aurora; it applies cleanly. 
> Any objections, :aknow or :hsinyi?

I don't see harms. Uplifting sounds good to me. In addition to this, I'd suggest uplifting Bug 916719 as well.

Comment 14

[Szu-Yu Chen [:aknow] (inactive)]

(In reply to Hsin-Yi Tsai  [:hsinyi] from comment #13)
> (In reply to Jonathan Griffin (:jgriffin) from comment #12)
> > I would like to uplift this patch to mozilla-aurora; it applies cleanly. 
> > Any objections, :aknow or :hsinyi?
> 
> I don't see harms. Uplifting sounds good to me. In addition to this, I'd
> suggest uplifting Bug 916719 as well.

Agree with hsinyi.

Comment 15

[Jonathan Griffin (:jgriffin)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/d7a44a73156b