Closed Bug 989728 Opened 6 years ago Closed 5 years ago

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

Categories

(Firefox OS Graveyard :: RIL, defect)

x86_64
Linux
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
1.4 S5 (11apr)

People

(Reporter: vicamo, Assigned: hsinyi)

References

Details

Attachments

(7 files, 3 obsolete files)

Assignee: nobody → htsai
Summary: [B2G] [RIL] add marionette-webapi tests for conference error → [B2G] [RIL] split test_conference.js into smaller pieces to avoid timeout failure
Target Milestone: --- → 1.4 S5 (11apr)
Correct the usage of gCheckAll().
Attachment #8403167 - Attachment is obsolete: true
Add comments for helper functions.
Attachment #8403163 - Attachment is obsolete: true
Rebase only.
Attachment #8403176 - Attachment is obsolete: true
Attachment #8403872 - Attachment description: part 3x v3 - cleanup test_dsds_normal_call.js → part 3c v3 - cleanup test_dsds_normal_call.js
Attachment #8403180 - Attachment description: part 3y - cleanup test_dsds_connection_conflict.js → part 3d - cleanup test_dsds_connection_conflict.js
Attachment #8403162 - Flags: review?(vyang)
Attachment #8403870 - Flags: review?(vyang)
Attachment #8403164 - Flags: review?(vyang)
Attachment #8403166 - Flags: review?(vyang)
Attachment #8403180 - Flags: review?(vyang)
Attachment #8403872 - Flags: review?(vyang)
Attachment #8403178 - Flags: review?(vyang)
Attachment #8403162 - Flags: review?(vyang) → review+
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/
Attachment #8403870 - Flags: review?(vyang) → review+
Attachment #8403164 - Flags: review?(vyang) → review+
Attachment #8403166 - Flags: review?(vyang) → review+
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");|?
Attachment #8403872 - Flags: review?(vyang) → review+
Attachment #8403180 - Flags: review?(vyang) → review+
Attachment #8403178 - Flags: review?(vyang) → review+
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'.
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.
(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.
Hsinyi,

Is it possible to merge setupConferenceTwoCalls, setupConferenceThreeCalls, setupConferenceFileCalls into one function, i.e., setupConferenceOfCalls([...]) ?
(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.
OK. We could file another bug for that.
https://hg.mozilla.org/mozilla-central/rev/7348f3d4ec58
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.