[Telephony] test_TelephonyUtils.js fails intermittently

RESOLVED FIXED in Firefox 42

Status

defect
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: bhsu, Assigned: bhsu)

Tracking

unspecified
FxOS-S5 (21Aug)
ARM
Gonk (Firefox OS)
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox42 fixed)

Details

Attachments

(2 attachments, 2 obsolete attachments)

|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.
Assignee: nobody → bhsu
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|.
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.
Attachment #8643618 - Attachment description: 11911237-2.patch → Part 2: Stablize |test_TelephonyUtils.js|
Attachment #8643617 - Attachment description: Part 1: Enhance TelephonyService.js → Part 1: Enhance |TelephonyService.js|
(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?
(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.
Attachment #8643617 - Flags: review?(szchen)
Attachment #8643618 - Flags: review?(szchen)
Hi Aknow,

May I have your review for both of the two  patches?
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 ===
Attachment #8643617 - Flags: review?(szchen) → review+
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.
Attachment #8643618 - Flags: review?(szchen) → review+
Attachment #8643617 - Attachment is obsolete: true
Attachment #8644462 - Flags: review+
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.
Attachment #8643618 - Attachment is obsolete: true
Attachment #8644467 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/9ba92e1ffa2d
https://hg.mozilla.org/mozilla-central/rev/8fdce5ed302a
Status: NEW → RESOLVED
Closed: 4 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → FxOS-S5 (21Aug)
You need to log in before you can comment on or make changes to this bug.