1191237 - [Telephony] test_TelephonyUtils.js fails intermittently

Status: RESOLVED FIXED

(Firefox OS Graveyard :: RIL, defect)

Description

[Ben Hsu [:HoPang]]

|test_TelephonyUtils.js| now fails intermittently on both local tests and try-servers. After a short investigation, I think there is a little defect in current implementation. We should resolve the promise returned by |waitForStateChanged()| only when the call states meet our expectation. For instance, now we recieve a |callStateChanged| from |TelephonyService| when the call state becomes alerting, and we shouldn't resolve the promise here, otherwise the testcase fails.

Comment 1

[Edgar Chen [:edgar]]

I also met this failure on emulator-l. Besides the defect of test script, there is also a problem regarding to the |callStateChanged| event: even if the call state wasn't changed, TelephonyService will still notify |callStateChanged| with empty |TelephonyCallInfo|.

Comment 2

[Ben Hsu [:HoPang]]

Attachment: Part 1: Enhance |TelephonyService.js|

Since the original debug message printed in the function already can tell the reason we early-return the funciton if needed, I don't print other debug message here.

Comment 3

[Ben Hsu [:HoPang]]

Attachment: Part 2: Stablize |test_TelephonyUtils.js|

Comment 4

[Ben Hsu [:HoPang]]

(In reply to Edgar Chen [:edgar][:echen] from comment #1)
> I also met this failure on emulator-l. Besides the defect of test script,
> there is also a problem regarding to the |callStateChanged| event: even if
> the call state wasn't changed, TelephonyService will still notify
> |callStateChanged| with empty |TelephonyCallInfo|.

Nice catch! I've writen a patch for that. Do you mind trying the two patches to check whether they fixed this issue on emulator-l?

Comment 5

[Edgar Chen [:edgar]]

(In reply to Ben Hsu [:bhsu] from comment #4)
> Nice catch! I've writen a patch for that. Do you mind trying the two patches
> to check whether they fixed this issue on emulator-l?

It works good on emulator-l, thank you.

Comment 6

[Ben Hsu [:HoPang]]

Hi Aknow,

May I have your review for both of the two  patches?

Comment 7

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

Comment on attachment 8643617 [details] [diff] [review]
Part 1: Enhance |TelephonyService.js|

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

::: dom/telephony/gonk/TelephonyService.js
@@ +2337,5 @@
>     */
>    _handleCallStateChanged: function(aClientId, aCalls) {
>      if (DEBUG) debug("handleCallStateChanged: " + JSON.stringify(aCalls));
>  
> +    if (aCalls.length == 0) {

using ===

Comment 8

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

Comment on attachment 8643618 [details] [diff] [review]
Part 2: Stablize |test_TelephonyUtils.js|

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

::: dom/telephony/test/marionette/test_TelephonyUtils.js
@@ +32,5 @@
>      });
>    });
>  }
>  
> +function waitForStateChanged(aFilter) {

'filter' is not a good name.
People are unsure about what will it do when aFilter(...) return true. Maybe keep it or filter it out. So Please find a better name for that, e.g., "aPredicate".

@@ +69,5 @@
>    return dial()
>      .then(() => {
>        is(TelephonyUtils.hasAnyCalls(), true, "hasAnyCalls");
>        is(TelephonyUtils.hasConnectedCalls(), false, "hasConnectedCalls");
>      })

We need to have more precise steps here.

dial
wait for alerting state
remote party accept
wait for connected state
check

@@ +78,1 @@
>        emulator.runCmd("gsm accept " + number);

Otherwise, in some cases, we might send out "gsm accept" too fast.

Comment 9

[Ben Hsu [:HoPang]]

Attachment: Part 1: Enhance |TelephonyService.js| (V2). r=aknow

Comment 10

[Ben Hsu [:HoPang]]

Attachment: Part 2: Stablize |test_TelephonyUtils.js| (V2). r=aknow

In addition to the previous enhancement, I adopt |startTest()| defined in |head.js| to start this testcase, since the radio might be not ready and thus fails the testcase when running this testcase individually.

Comment 11

[Ben Hsu [:HoPang]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c07244e79672

Comment 12

[Pulsebot]

https://hg.mozilla.org/integration/b2g-inbound/rev/9ba92e1ffa2d
https://hg.mozilla.org/integration/b2g-inbound/rev/8fdce5ed302a

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/9ba92e1ffa2d
https://hg.mozilla.org/mozilla-central/rev/8fdce5ed302a