1100200 - [MarionetteTest] Organize all telephony tests

Status: RESOLVED FIXED

(Firefox OS Graveyard :: RIL, defect)

Description

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

1. Migrate all test cases to promise version and utilize head.js
2. Sort the list in manifest.ini
3. Re-examine all the disabled tests and try to enable them.

Comment 1

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

Attachment: (WIP) Migrate to promise version

Comment 2

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

Attachment: Part 1: Migrate to promise version

Sorry for the huge patch.....

Comment 3

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

Attachment: Part 2: Merge test cases into test_incoming_basic_operations.js

Comment 4

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

Attachment: Part 3: Merge test cases into test_incoming_connecting_hangup.js

Comment 5

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

Attachment: Part 4: Merge test cases into test_outgoing_basic_operations.js

Comment 6

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

Comment on attachment 8524409 [details] [diff] [review]
Part 1: Migrate to promise version

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

r=me with 
1) the comment for test_incoming_already_connected.js addressed,
2) have patches for another part to clean up test_emergency.js, test_outgoing_radio_off.js and test_outgoing_error_state.js

Details in-line, thank you!

::: dom/telephony/test/marionette/head.js
@@ +1067,5 @@
>    this.gDelay = delay;
>    this.gWaitForEvent = waitForEvent;
> +  this.gWaitForCallsChangedEvent = waitForCallsChangedEvent;
> +  this.gWaitForNamedStateEvent = waitForNamedStateEvent;
> +  this.gWaitForStateChangeEvent = waitForStateChangeEvent;

These three are added in bug 1093014. Rebase needed.

::: dom/telephony/test/marionette/test_emergency.js
@@ +11,5 @@
> +    .then(call => outCall = call)
> +    .then(() => gRemoteAnswer(outCall))
> +    .then(() => gHangUp(outCall))
> +    .catch(error => ok(false, "Promise reject: " + error))
> +    .then(finish);

Let's have another part for merging this and part of test_outgoing_error_state.js. 

Please add one more case for |gDialEmergency("A Bad Number")| to test_emergency.js.

::: dom/telephony/test/marionette/test_emergency_label.js
@@ +9,5 @@
>  function setEccListProperty(list) {
>    log("Set property ril.ecclist: " + list);
>  
> +  // We should wrap empty |list| by ''. Otherwise, the entire command will be
> +  // "setprop ril.ecclist" which causus the command error.

Fixed in bug 1099053. Rebase needed.

::: dom/telephony/test/marionette/test_incoming_already_connected.js
@@ +27,5 @@
> +
> +    // Answer incoming call; original outgoing call should be held
> +    .then(() => gAnswer(inCall))
> +    .then(() => gCheckAll(inCall, [outCall, inCall], "", [],
> +                          [outInfo.held, inInfo.active]))

Be careful of potential timing issues b/w incall.active and outcall.held. Let's do what has been done in bug 1093014, thank you.

::: dom/telephony/test/marionette/test_outgoing_error_state.js
@@ +22,1 @@
>      .then(() => gDialEmergency(number))

Please have another part for the comment:
Let's move this case to test_emergency.js, and expand test_outgoing_radio_off.js to cover |gDialEmergency(a_normal_number) when radio off|. Then, it's fine to remove the whole test_outgoing_error_state.js.

Comment 7

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

Comment on attachment 8524410 [details] [diff] [review]
Part 2: Merge test cases into test_incoming_basic_operations.js

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

r=me with comment addressed.

::: dom/telephony/test/marionette/manifest.ini
@@ +31,1 @@
>  [test_incoming_onstatechange.js]

Don't forget to add " test_incoming_basic_operations.js " to manifest.ini.

Comment 8

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

Comment on attachment 8524412 [details] [diff] [review]
Part 4: Merge test cases into test_outgoing_basic_operations.js

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

Truly nice!

r=me with comments addressed, thank you~

::: dom/telephony/test/marionette/manifest.ini
@@ +43,2 @@
>  [test_outgoing_onstatechange.js]
>  [test_outgoing_radio_off.js]

Don't forget |test_outgoing_basic_operations.js |.

::: dom/telephony/test/marionette/test_incoming_basic_operations.js
@@ +36,5 @@
> +
> +function remoteHangUp() {
> +  return gRemoteHangUp(inCall)
> +    .then(() => gCheckAll(null, [], "", [], []));
> +}

We don't really need this, right :)

Comment 9

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

Attachment: Part 5: Merge emergency and radio-off related tests

Comment 10

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

try looks good
https://treeherder.mozilla.org/#/jobs?repo=try&revision=aad92d4bf44f

Comment 11

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

https://hg.mozilla.org/integration/b2g-inbound/rev/7ea0cf5b6bc6
https://hg.mozilla.org/integration/b2g-inbound/rev/b5572f21d6ab
https://hg.mozilla.org/integration/b2g-inbound/rev/426b1a22e1ab
https://hg.mozilla.org/integration/b2g-inbound/rev/6e009bb139fd
https://hg.mozilla.org/integration/b2g-inbound/rev/a0d08d9c9748

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/7ea0cf5b6bc6
https://hg.mozilla.org/mozilla-central/rev/b5572f21d6ab
https://hg.mozilla.org/mozilla-central/rev/426b1a22e1ab
https://hg.mozilla.org/mozilla-central/rev/6e009bb139fd
https://hg.mozilla.org/mozilla-central/rev/a0d08d9c9748