989728 - [B2G] [RIL] split test_conference.js into smaller pieces to avoid timeout failure

Status: RESOLVED FIXED

(Firefox OS Graveyard :: RIL, defect)

Description

[Vicamo Yang [:vicamo][:vyang]]

+++ This bug was initially created as a clone of Bug #984289 +++

Split test_conference.js because it's becoming an orange factor more and more often.

https://tbpl.mozilla.org/?tree=B2g-Inbound&rev=bcb36bfe15b3
https://tbpl.mozilla.org/php/getParsedLog.php?id=36951930&tree=B2g-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=36719768&tree=B2g-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=36655947&tree=B2g-Inbound
...

Comment 1

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

Attachment: part 1 - move 'let conference;' to head.js

Comment 2

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

Attachment: part 2 - cleanup test_conference.js. Move global functions to head.js

Comment 3

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

Attachment: part 3a - cleanup other tests & receivedPending()

Comment 4

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

Attachment: part 3b - cleanup test_audiomanager_phonestate.js

Comment 5

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

Attachment: part 3x - cleanup test_dsds_normal_call.js

Comment 6

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

Attachment: part 3x v2 - cleanup test_dsds_normal_call.js

Correct the usage of gCheckAll().

Comment 7

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

Attachment: part 4 - split test_conference.js

Comment 8

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

Attachment: part 3d - cleanup test_dsds_connection_conflict.js

Comment 9

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

Attachment: part 2 v2 - cleanup test_conference.js. Move global functions to head.js

Add comments for helper functions.

Comment 10

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

Attachment: part 3c v3 - cleanup test_dsds_normal_call.js

Rebase only.

Comment 11

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

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

Comment 12

[Vicamo Yang [:vicamo][:vyang]]

Comment on attachment 8403870 [details] [diff] [review]
part 2 v2 - cleanup test_conference.js. Move global functions to head.js

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

::: dom/telephony/test/marionette/head.js
@@ +281,5 @@
> +    emulator.run("gsm list", function(result) {
> +        log("Call list is now: " + result);
> +        for (let i = 0; i < expectedCallList.length; ++i) {
> +        is(result[i], expectedCallList[i], "emulator calllist");
> +        }

nit: indentation.

@@ +310,5 @@
> +    checkConferenceStateAndCalls(conferenceState, conferenceCalls);
> +  }
> +
> +  /**
> +   * Supre convenient helper to check calls and state of mozTelephony and

nit: s/Supre/Super/

Comment 13

[Vicamo Yang [:vicamo][:vyang]]

Comment on attachment 8403872 [details] [diff] [review]
part 3c v3 - cleanup test_dsds_normal_call.js

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

::: dom/telephony/test/marionette/head.js
@@ +377,5 @@
>  
>        call.onalerting = function onalerting(event) {
>          call.onalerting = null;
>          log("Received 'onalerting' call event.");
> +        is(call.serviceId, serviceId);

nit: shouldn't this be next to |is(call.state, "dialing");|?

Comment 14

[Vicamo Yang [:vicamo][:vyang]]

Comment on attachment 8403872 [details] [diff] [review]
part 3c v3 - cleanup test_dsds_normal_call.js

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

This part is critical to run Marionette test cases in emulator-jb & kk.  "test_dsds_normal_call.js" always fails because we call dial() without that serviceId parameter.  This fails one test and then skips a call to 'mux modem 0'.  As a result, the active modem isn't set back to 0 and many many following test cases fail unexpectedly.

::: dom/telephony/test/marionette/head.js
@@ +361,5 @@
>     * Make an outgoing call.
>     *
>     * @param number
>     *        A string.
>     * @return A deferred promise.

nit: missing documentation for 'serviceId'.

Comment 15

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

Thanks for the review, Vicamo!

https://tbpl.mozilla.org/?tree=Try&rev=0ea210b15cc0 try green.

https://hg.mozilla.org/integration/b2g-inbound/rev/7348f3d4ec58 fold all the patches with comment 12, 13, 14 addressed. I also cleaned "conference.onerror" after receiving the onerror event.

Comment 16

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

(In reply to Hsin-Yi Tsai  [:hsinyi] from comment #15)
> Thanks for the review, Vicamo!
> 
> https://tbpl.mozilla.org/?tree=Try&rev=0ea210b15cc0 try green.
> 

https://tbpl.mozilla.org/?tree=Try&rev=bee9cda7fc4d this is the right link to latest try. Green as well.

> https://hg.mozilla.org/integration/b2g-inbound/rev/7348f3d4ec58 fold all the
> patches with comment 12, 13, 14 addressed. I also cleaned
> "conference.onerror" after receiving the onerror event.

Comment 17

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

Hsinyi,

Is it possible to merge setupConferenceTwoCalls, setupConferenceThreeCalls, setupConferenceFileCalls into one function, i.e., setupConferenceOfCalls([...]) ?

Comment 18

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

(In reply to Szu-Yu Chen [:aknow] from comment #17)
> Hsinyi,
> 
> Is it possible to merge setupConferenceTwoCalls, setupConferenceThreeCalls,
> setupConferenceFileCalls into one function, i.e.,
> setupConferenceOfCalls([...]) ?

Yes, I think we could do that. Any problem you met? And you want to have that done in this bug? If so, we'd better add a whiteboard [Leave open] because I already put my patch into inbound.

Comment 19

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

OK. We could file another bug for that.

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/7348f3d4ec58