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

RESOLVED FIXED in Firefox 26

Status

Firefox OS
RIL
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: aknow, Assigned: aknow)

Tracking

unspecified
1.2 FC (16sep)
x86_64
Gonk (Firefox OS)

Firefox Tracking Flags

(firefox26 fixed, firefox27 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

4 years ago
[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
(Assignee)

Updated

4 years ago
Depends on: 916719
Thanks for the bug and reporting. :) Looks like you got the root cause of 'gsm clear,' then I vote for option 2).
(Assignee)

Comment 2

4 years ago
Created attachment 805815 [details] [diff] [review]
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)
Attachment #805815 - Flags: review?(htsai)
(Assignee)

Updated

4 years ago
Attachment #805815 - Flags: review?(htsai)
(Assignee)

Comment 3

4 years ago
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".
Attachment #805815 - Flags: review?(htsai)
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 '+'.
Attachment #805815 - Flags: review?(htsai)
(Assignee)

Comment 5

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

Comment 6

4 years ago
Created attachment 806423 [details] [diff] [review]
#2 Fix cancelExistingCalls
Attachment #805815 - Attachment is obsolete: true
Attachment #806423 - Flags: review?(htsai)
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.
Attachment #806423 - Flags: review?(htsai) → review+
(Assignee)

Comment 8

4 years ago
Created attachment 806468 [details] [diff] [review]
[Final] Fix cancelExistingCalls. r=hsinyi
Attachment #806423 - Attachment is obsolete: true
(Assignee)

Comment 9

4 years ago
https://tbpl.mozilla.org/?tree=Try&rev=d90cc73f9cdb
Keywords: checkin-needed
https://hg.mozilla.org/integration/b2g-inbound/rev/ba6395f2dc6e
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/ba6395f2dc6e
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → 1.2 FC (16sep)
I would like to uplift this patch to mozilla-aurora; it applies cleanly.  Any objections, :aknow or :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.
(Assignee)

Comment 14

4 years ago
(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.
https://hg.mozilla.org/releases/mozilla-aurora/rev/d7a44a73156b
status-firefox26: --- → fixed
status-firefox27: --- → fixed
You need to log in before you can comment on or make changes to this bug.