Closed Bug 797296 Opened 12 years ago Closed 11 years ago

B2G STK: write marionette tests for STK proactive commands

Categories

(Core :: DOM: Device Interfaces, defect)

x86_64
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla22

People

(Reporter: hsinyi, Assigned: johnshih.bugs)

References

Details

Attachments

(15 files, 47 obsolete files)

2.27 KB, patch
allstars.chh
: review+
Details | Diff | Splinter Review
2.15 KB, patch
johnshih.bugs
: review+
Details | Diff | Splinter Review
11.18 KB, patch
allstars.chh
: review+
Details | Diff | Splinter Review
5.00 KB, patch
allstars.chh
: review+
Details | Diff | Splinter Review
11.45 KB, patch
allstars.chh
: review+
Details | Diff | Splinter Review
9.22 KB, patch
allstars.chh
: review+
Details | Diff | Splinter Review
14.01 KB, patch
johnshih.bugs
: review+
Details | Diff | Splinter Review
11.59 KB, patch
johnshih.bugs
: review+
Details | Diff | Splinter Review
14.74 KB, patch
johnshih.bugs
: review+
Details | Diff | Splinter Review
9.48 KB, patch
johnshih.bugs
: review+
Details | Diff | Splinter Review
3.33 KB, patch
johnshih.bugs
: review+
Details | Diff | Splinter Review
6.09 KB, patch
johnshih.bugs
: review+
Details | Diff | Splinter Review
23.69 KB, patch
johnshih.bugs
: review+
Details | Diff | Splinter Review
17.49 KB, patch
johnshih.bugs
: review+
Details | Diff | Splinter Review
14.55 KB, patch
johnshih.bugs
: review+
Details | Diff | Splinter Review
We should have high-level API tests for STK features, such as sending STK responses and sending STK menu selection. Even more, setting up a call by STK or setting up an event list.
Depends on: 809284
CCing Al
As Bug 809294 supports STK proactive command.

This bug should include following STK features:
- send STK response.
- send STK envelope and envelope with status
- send "REPORT_STK_SERVICE_IS_RUNNGING'
- set/get STK Profile
- send 'session end' from icc

The remaining requests are more complicated
- call setup
- RIL_REQUEST_STK_HANDLE_CALL_SETUP_REQUESTED_FROM_SIM
(In reply to Yoshi Huang[:yoshi][:allstars.chh] from comment #1)
> As Bug 809294 supports STK proactive command.

wrong bug id?
Blocks: 816926
No longer blocks: 816926
Depends on: 816926
Assignee: nobody → atsai
Attached patch Part 1: Test STK Refresh. (obsolete) — Splinter Review
Attachment #709568 - Flags: review?
Attached patch Part 2: Test STK Poll Off. (obsolete) — Splinter Review
Attachment #709569 - Flags: review?
Attachment #709570 - Flags: review?
Attached patch Part 4: Test STK Setup Call. (obsolete) — Splinter Review
Attachment #709571 - Flags: review?
Attached patch Part 5: Test STK Send SS. (obsolete) — Splinter Review
Attachment #709572 - Flags: review?
Attached patch Part 6: Test STK Send USSD. (obsolete) — Splinter Review
Attachment #709573 - Flags: review?
Attached patch Part 7: Test STK Send SMS. (obsolete) — Splinter Review
Attachment #709574 - Flags: review?
Attached patch Part 8: Test STK Send DTMF. (obsolete) — Splinter Review
Attachment #709575 - Flags: review?
Attached patch Part 9: Test STK Launch Browser. (obsolete) — Splinter Review
Attachment #709576 - Flags: review?
Attached patch Part 10: Test STK Display Text. (obsolete) — Splinter Review
Attached patch Part 11: Test STK Get Inkey. (obsolete) — Splinter Review
Attachment #709578 - Flags: review?
Attached patch Part 12: Test STK Get Input. (obsolete) — Splinter Review
Attachment #709579 - Flags: review?
Attached patch Part 13: Test STK Select Item. (obsolete) — Splinter Review
Attachment #709580 - Flags: review?
Attached patch Part 14: Test STK Setup Menu. (obsolete) — Splinter Review
Attachment #709581 - Flags: review?
Attachment #709582 - Flags: review?
Attachment #709577 - Flags: review?
Comment on attachment 709568 [details] [diff] [review]
Part 1: Test STK Refresh.

Please update the description for that patch, i.e. "Part 1: Test STK Refresh".
And you should send r? to me, allstars.chh@mozilla.

And in the bug title, please use Capital case, 
i.e.
"Bug 797296 - Part 1: Test STK Refresh"
Attachment #709568 - Attachment description: bug_797296_part1.patch → Part 1: Test STK Refresh.
Comment on attachment 709568 [details] [diff] [review]
Part 1: Test STK Refresh.

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

I think you also need to update the manifest.ini as well.

::: dom/icc/tests/marionette/test_stk_refresh.js
@@ +7,5 @@
> +
> +let icc = navigator.mozMobileConnection.icc;
> +ok(icc instanceof MozIccManager, "icc is instanceof " + icc.constructor);
> +
> +function testRefresh(cmd, exp) {

Is exp short for 'expect' ?
'exp' is too ambitious please use a better name here.

@@ +16,5 @@
> +  runNextTest();
> +}
> +
> +let tests = [
> +  

nit, extra line.

@@ +19,5 @@
> +let tests = [
> +  
> +  {command: "d0108103010101820281829205013f002fe2",
> +   func: testRefresh,
> +   exp: {name: "pull_off_112",

Why the name is pull off here?
I though this test is for Refresh.

And what does 112 mean here?

@@ +23,5 @@
> +   exp: {name: "pull_off_112",
> +         commandQualifier: 0x01}},
> +  {command: "d009810301010482028182",
> +   func: testRefresh,
> +   exp: {name: "pull_off_151",

same here.
Attachment #709568 - Flags: review?
Since John is working on this, assign this bug to him.
Assignee: atsai → jshih
I'll file another bug for other STK features mentioned in comment 1.
Summary: B2G STK: write marionette tests for STK features → B2G STK: write marionette tests for STK proactive commands
(In reply to Vicamo Yang [:vicamo][:vyang] from comment #2)
> (In reply to Yoshi Huang[:yoshi][:allstars.chh] from comment #1)
> > As Bug 809294 supports STK proactive command.
> 
> wrong bug id?

Sorry I notice this until now, the bug should be 809284.
Attachment #709569 - Attachment description: bug_797296_part2.patch → Part2: Test STK Poll Off.
Attachment #709569 - Flags: review? → review?(allstars.chh)
Attachment #709568 - Flags: review?(allstars.chh)
Attachment #709569 - Attachment description: Part2: Test STK Poll Off. → Part 2: Test STK Poll Off.
Attachment #709570 - Attachment description: bug_797296_part3.patch → Part 3: Test STK Setup Event List.
Attachment #709570 - Flags: review? → review?(allstars.chh)
Attachment #709571 - Attachment description: bug_797296_part4.patch → Part 4: Test STK Setup Call.
Attachment #709571 - Flags: review? → review?(allstars.chh)
Attachment #709572 - Attachment description: bug_797296_part5.patch → Part 5: Test STK Send SS.
Attachment #709572 - Flags: review? → review?(allstars.chh)
Attachment #709573 - Attachment description: bug_797296_part6.patch → Part 6: Test STK Send USSD.
Attachment #709573 - Flags: review? → review?(allstars.chh)
Attachment #709574 - Attachment description: bug_797296_part7.patch → Part 7: Test STK Send SMS.
Attachment #709574 - Flags: review? → review?(allstars.chh)
Attachment #709575 - Attachment description: bug_797296_part8.patch → Part 8: Test STK Send DTMF.
Attachment #709575 - Flags: review? → review?(allstars.chh)
Attachment #709576 - Attachment description: bug_797296_part9.patch → Part 9: Test STK Launch Browser.
Attachment #709576 - Flags: review? → review?(allstars.chh)
Attachment #709577 - Attachment description: bug_797296_part10.patch → Part 10: Test STK Display Text.
Attachment #709577 - Flags: review? → review?(allstars.chh)
Attachment #709578 - Attachment description: bug_797296_part11.patch → Part 11: Test STK Get Inkey.
Attachment #709578 - Flags: review? → review?(allstars.chh)
Attachment #709579 - Attachment description: bug_797296_part12.patch → Part 12: Test STK Get Input.
Attachment #709579 - Flags: review? → review?(allstars.chh)
Attachment #709580 - Attachment description: bug_797296_part13.patch → Part 13: Test STK Select Item.
Attachment #709580 - Flags: review? → review?(allstars.chh)
Attachment #709581 - Attachment description: bug_797296_part14.patch → Part 14: Test STK Setup Menu.
Attachment #709581 - Flags: review? → review?(allstars.chh)
I suggest you finish Part 1 first otherwise you need to do the same thing for all your patches.
Attachment #709582 - Attachment description: bug_797296_part15.patch → Part 15: Test STK Setup Idle Mode Text.
Attachment #709582 - Flags: review? → review?(allstars.chh)
Comment on attachment 709568 [details] [diff] [review]
Part 1: Test STK Refresh.

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

Please address my previous comments first before sending r?
Attachment #709568 - Flags: review?(allstars.chh) → review-
These patches share some code for sending STK PDU. When Bug 805838 is landed, we could reuse those code instead of repeating it in each test script.
Depends on: 805838
(In reply to Yoshi Huang[:yoshi][:allstars.chh] from comment #19)
> Comment on attachment 709568 [details] [diff] [review]
> Part 1: Test STK Refresh.
> 
> Review of attachment 709568 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I think you also need to update the manifest.ini as well.
> 
> ::: dom/icc/tests/marionette/test_stk_refresh.js
> @@ +7,5 @@
> > +
> > +let icc = navigator.mozMobileConnection.icc;
> > +ok(icc instanceof MozIccManager, "icc is instanceof " + icc.constructor);
> > +
> > +function testRefresh(cmd, exp) {
> 
> Is exp short for 'expect' ?
> 'exp' is too ambitious please use a better name here.
> 
> @@ +16,5 @@
> > +  runNextTest();
> > +}
> > +
> > +let tests = [
> > +  
> 
> nit, extra line.
> 
> @@ +19,5 @@
> > +let tests = [
> > +  
> > +  {command: "d0108103010101820281829205013f002fe2",
> > +   func: testRefresh,
> > +   exp: {name: "pull_off_112",
> 
> Why the name is pull off here?
> I though this test is for Refresh.
> 
> And what does 112 mean here?

I just followed the naming in the existing test cases (test-stkutil.c)
so I'm not sure what the correct naming/number for each event. 

> 
> @@ +23,5 @@
> > +   exp: {name: "pull_off_112",
> > +         commandQualifier: 0x01}},
> > +  {command: "d009810301010482028182",
> > +   func: testRefresh,
> > +   exp: {name: "pull_off_151",
> 
> same here.
Attached patch Part 1: Test STK Refresh. (obsolete) — Splinter Review
Attachment #709568 - Attachment is obsolete: true
Attachment #709626 - Flags: review?(allstars.chh)
(In reply to John Shih from comment #26)
> 
> I just followed the naming in the existing test cases (test-stkutil.c)
> so I'm not sure what the correct naming/number for each event. 
> 

Seems it's just a name for the test, can you rename it to simpler or meaningful one?
(In reply to Yoshi Huang[:yoshi][:allstars.chh] from comment #28)
> (In reply to John Shih from comment #26)
> > 
> > I just followed the naming in the existing test cases (test-stkutil.c)
> > so I'm not sure what the correct naming/number for each event. 
> > 
> 
> Seems it's just a name for the test, can you rename it to simpler or
> meaningful one?

So can I just order by number (e.g, xxx_1. xxx_2, xxx_3 ...) ?
Comment on attachment 709626 [details] [diff] [review]
Part 1: Test STK Refresh.

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

Please address comment 18 and comment 19 first before sending r?
This is for the second time I said this.
Attachment #709626 - Flags: review?(allstars.chh) → review-
(In reply to John Shih from comment #29)
> So can I just order by number (e.g, xxx_1. xxx_2, xxx_3 ...) ?
Sound okay.
Please do this.
Comment on attachment 709626 [details] [diff] [review]
Part 1: Test STK Refresh.

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

Please address comment 18 and comment 19 first before sending r?
This is for the second time I said this.

::: dom/icc/tests/marionette/test_stk_refresh.js
@@ +10,5 @@
> +
> +function testRefresh(cmd, expect) {
> +  log("STK CMD " + JSON.stringify(cmd));
> +  is(cmd.typeOfCommand, icc.STK_CMD_REFRESH, expect.name);
> +  is(cmd.commandQualifier, exp.commandQualifier, expect.name);

TEST-UNEXPECTED-FAIL | test_stk_refresh.js | JavascriptException: ReferenceError: exp is not defined.

Please make sure you run the tests before sending r?
Attached patch Part 1: Test STK Refresh. (obsolete) — Splinter Review
I've changed the naming to xxx_cmd_# and fixed the typo
but sadly I cannot pass this case

got
 
raise ScriptTimeoutException(message=message, status=status, stacktrace=stacktrace)
TEST-UNEXPECTED-FAIL | test_stk_refresh.js | ScriptTimeoutException: timed out
Attachment #709626 - Attachment is obsolete: true
Attachment #709630 - Attachment description: Test STK Refresh. → Part 1: Test STK Refresh.
Attached patch Part 2: Test STK Poll Off. (obsolete) — Splinter Review
Attachment #709569 - Attachment is obsolete: true
Attachment #709569 - Flags: review?(allstars.chh)
Comment on attachment 709634 [details] [diff] [review]
Part 2: Test STK Poll Off.

Since I can't pass STK Refresh, please review this one first
thanks
Attachment #709634 - Flags: review?(allstars.chh)
Comment on attachment 709630 [details] [diff] [review]
Part 1: Test STK Refresh.

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

::: dom/icc/tests/marionette/test_stk_refresh.js
@@ +19,5 @@
> +let tests = [
> +  {command: "d0108103010101820281829205013f002fe2",
> +   func: testRefresh,
> +   expect: {name: "refresh_cmd_1",
> +         commandQualifier: 0x01}},

nit, align this line.

@@ +23,5 @@
> +         commandQualifier: 0x01}},
> +  {command: "d009810301010482028182",
> +   func: testRefresh,
> +   expect: {name: "refresh_cmd_2",
> +         commandQualifier: 0x04}}

ditto
Attachment #709634 - Flags: review?(allstars.chh) → review?(allstars.chh)
Comment on attachment 709634 [details] [diff] [review]
Part 2: Test STK Poll Off.

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

This patch is much better now.

Please update bug comment, "Bug 797296", not "bug 797296"
There's a rule for this.
https://developer.mozilla.org/en-US/docs/Developer_Guide/Committing_Rules_and_Responsibilities

Please address nits below, and add r=me.

thanks

::: dom/icc/tests/marionette/test_stk_poll_off.js
@@ +7,5 @@
> +
> +let icc = navigator.mozMobileConnection.icc;
> +ok(icc instanceof MozIccManager, "icc is instanceof " + icc.constructor);
> +
> +function testPollOff(cmd, expect) {

For consistency, please rename cmd to command,
and all occurrences below.

@@ +19,5 @@
> +let tests = [
> +  {command: "d009810301040082028182",
> +   func: testPollOff,
> +   expect: {name: "pull_off_cmd_1",
> +         commandQualifier: 0x00}}

nit, align this.
Attachment #709634 - Flags: review?(allstars.chh) → review+
Comment on attachment 709630 [details] [diff] [review]
Part 1: Test STK Refresh.

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

Please update patch comment and add r=me.

::: dom/icc/tests/marionette/test_stk_refresh.js
@@ +7,5 @@
> +
> +let icc = navigator.mozMobileConnection.icc;
> +ok(icc instanceof MozIccManager, "icc is instanceof " + icc.constructor);
> +
> +function testRefresh(cmd, expect) {

For consistency please rename cmd to command, and all occurrences below.
Attachment #709630 - Flags: review+
update
Attachment #709630 - Attachment is obsolete: true
Attachment #711184 - Flags: review?(allstars.chh)
update
Attachment #709634 - Attachment is obsolete: true
Attachment #711187 - Flags: review?(allstars.chh)
Comment on attachment 711184 [details] [diff] [review]
Part 1: Test STK Refresh.

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

I already gave you r+ so you don't have to r? again.
BTW, my bugzilla email is allstars.chh@mozilla.com, gmail.com is my personal mail
Attachment #711184 - Flags: review?(allstars.chh) → review+
update. (with test pass)
Attachment #709570 - Attachment is obsolete: true
Attachment #709570 - Flags: review?(allstars.chh)
Attachment #711190 - Flags: review?(allstars.chh)
Attached patch Part 4: Test STK Setup Call. (obsolete) — Splinter Review
update (with test pass)
Attachment #709571 - Attachment is obsolete: true
Attachment #709571 - Flags: review?(allstars.chh)
Attachment #711192 - Flags: review?(allstars.chh)
Attachment #711187 - Flags: review?(allstars.chh) → review+
Attachment #711190 - Flags: review?(allstars.chh) → review?(allstars.chh)
Comment on attachment 711190 [details] [diff] [review]
Part 3: Test STK Setup Event List.

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

::: dom/icc/tests/marionette/test_stk_setup_event_list.js
@@ +11,5 @@
> +function testSetupEventList(command, expect) {
> +  log("STK CMD " + JSON.stringify(command));
> +  is(command.typeOfCommand, icc.STK_CMD_SET_UP_EVENT_LIST, expect.name);
> +  is(command.commandQualifier, expect.commandQualifier, expect.name);
> +

We should also test command.options.eventList here

@@ +39,5 @@
> +            commandQualifier: 0x00}},
> +  {command: "d00c810301050082028182990107",
> +   func: testSetupEventList,
> +   expect: {name: "setup_event_list_cmd_6",
> +            commandQualifier: 0x00}}                      

nit, trailing ws.
Attachment #711190 - Flags: review?(allstars.chh)
Comment on attachment 711192 [details] [diff] [review]
Part 4: Test STK Setup Call.

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

::: dom/icc/tests/marionette/test_stk_setup_call.js
@@ +11,5 @@
> +function testSetupCall(command, expect) {
> +  log("STK CMD " + JSON.stringify(command));
> +  is(command.typeOfCommand, icc.STK_CMD_SET_UP_CALL, expect.name);
> +  is(command.commandQualifier, expect.commandQualifier, expect.name);
> +  if(typeof command.options.confirmMessage != "undefined") {

if (command.options.confirmMessage) seems enough.

@@ +82,5 @@
> +  {command: "d04c81030110008202818385165365742075702063616c6c2049636f6e20332e342e318609911032042143651c2c9e02000185165365742075702063616c6c2049636f6e20332e342e329e020001",
> +   func: testSetupCall,
> +   expect: {name: "setup_call_cmd_13",
> +            commandQualifier: 0x00,
> +            text: "Set up call Icon 3.4.1"}},   

nit, trailing space.

@@ +112,5 @@
> +  {command: "d02c810301100082028183850e434f4e4649524d4154494f4e20328609911032042143651c2c850643414c4c2032",
> +   func: testSetupCall,
> +   expect: {name: "setup_call_cmd_19",
> +            commandQualifier: 0x00,
> +            text: "CONFIRMATION 2"}},    

ditto.

@@ +242,5 @@
> +  {command: "d02081030110008202818385058030eb003186079110320421436585058030eb0032",
> +   func: testSetupCall,
> +   expect: {name: "setup_call_cmd_45",
> +            commandQualifier: 0x00,
> +            text: "ル1"}},                                               

ditto.
Attachment #711192 - Flags: review?(allstars.chh) → review+
Attached patch Part 4: Test STK Setup Call. (obsolete) — Splinter Review
update
Attachment #711192 - Attachment is obsolete: true
Attached patch Part 4: Test STK Setup Call. (obsolete) — Splinter Review
update
Attachment #711206 - Attachment is obsolete: true
Attached patch Part 4: Test STK Setup Call. (obsolete) — Splinter Review
Attachment #711209 - Attachment is obsolete: true
Attachment #711210 - Flags: review+
Comment on attachment 709572 [details] [diff] [review]
Part 5: Test STK Send SS.

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

::: dom/icc/tests/marionette/test_stk_send_ss.js
@@ +58,5 @@
> +         title: "Basic Icon"}},
> +  {command: "d01d810301110082028183890e91aa120a214365870921436587b99e020101",
> +   func: testSendSS,
> +   exp: {name: "send_ss_241",
> +         commandQualifier: 0x00}},

This test doesn't have title,
however title is required in SEND_SS.
Can you remove this test?

@@ +153,5 @@
> +  {command: "d02d810301110082028183851054657874204174747269627574652033891091aa120a214365870921436587a901fb",
> +   func: testSendSS,
> +   exp: {name: "send_ss_473",
> +         commandQualifier: 0x00,
> +         title: "Toolkit Select 3"}},

The title seems wrong here.
Comment on attachment 709573 [details] [diff] [review]
Part 6: Test STK Send USSD.

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

::: dom/icc/tests/marionette/test_stk_send_ussd.js
@@ +43,5 @@
> +         title: "once a RELEASE COMPLETE message containing the USSD Return Result message not containing an error has been received from the network, the ME shall inform the SIM that the command has"}},
> +  {command: "d0448103011200820281838a39f041e19058341e9149e592d9743ea151e9945ab55eb1596d2b2c1e93cbe6333aad5eb3dbee373c2e9fd3ebf63b3eaf6fc564335acd76c3e560",
> +   func: testSendUSSD,
> +   exp: {name: "send_ussd_171",
> +         commandQualifier: 0x00}},

No title.

@@ +67,5 @@
> +         title: "Basic Icon"}},
> +  {command: "d0488103011200820281838a39f041e19058341e9149e592d9743ea151e9945ab55eb1596d2b2c1e93cbe6333aad5eb3dbee373c2e9fd3ebf63b3eaf6fc564335acd76c3e5609e020101",
> +   func: testSendUSSD,
> +   exp: {name: "send_ussd_241",
> +         commandQualifier: 0x00}},

No title here.

@@ +152,5 @@
> +  {command: "d05c8103011200820281838510546578742041747472696275746520318a39f041e19058341e9149e592d9743ea151e9945ab55eb1596d2b2c1e93cbe6333aad5eb3dbee373c2e9fd3ebf63b3eaf6fc564335acd76c3e560d004001020b4",
> +   func: testSendUSSD,
> +   exp: {name: "send_ussd_471",
> +         commandQualifier: 0x00,
> +         title: "Toolkit Select 1"}},

The title seems wrong here.
add check of command.options.eventList
Attachment #711190 - Attachment is obsolete: true
update
Attachment #711645 - Attachment is obsolete: true
update
Attached patch Part 5: Test STK Send SS. (obsolete) — Splinter Review
update (with test pass)
Attachment #709572 - Attachment is obsolete: true
Attachment #711646 - Attachment is obsolete: true
Attachment #709572 - Flags: review?(allstars.chh)
Attached patch Part 6: Test STK Send USSD. (obsolete) — Splinter Review
update (with test pass)
Attachment #709573 - Attachment is obsolete: true
Attachment #709573 - Flags: review?(allstars.chh)
Attached patch Part 6: Test STK Send USSD. (obsolete) — Splinter Review
update
Attachment #711649 - Attachment is obsolete: true
Attachment #711647 - Flags: review?(allstars.chh)
Attachment #711648 - Flags: review?(allstars.chh)
Attachment #711650 - Flags: review?(allstars.chh)
Attached patch Part 7: Test STK Send SMS. (obsolete) — Splinter Review
update (with test pass)
Attachment #709574 - Attachment is obsolete: true
Attachment #709574 - Flags: review?(allstars.chh)
Attached patch Part 7: Test STK Send SMS. (obsolete) — Splinter Review
Attachment #711662 - Attachment is obsolete: true
Attached patch Part 6: Test STK Send USSD. (obsolete) — Splinter Review
Attachment #711650 - Attachment is obsolete: true
Attachment #711650 - Flags: review?(allstars.chh)
Attachment #711665 - Flags: review?(allstars.chh)
Attachment #711663 - Flags: review?(allstars.chh)
Attached patch Part 8: Test STK Send DTMF. (obsolete) — Splinter Review
update (with test pass)
Attachment #709575 - Attachment is obsolete: true
Attachment #709575 - Flags: review?(allstars.chh)
Attachment #711669 - Flags: review?(allstars.chh)
Comment on attachment 711647 [details] [diff] [review]
Part 3: Test STK Setup Event List.

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

::: dom/icc/tests/marionette/test_stk_setup_event_list.js
@@ +28,5 @@
> +  {command: "d00d81030105008202818299020507",
> +   func: testSetupEventList,
> +   expect: {name: "setup_event_list_cmd_2",
> +            commandQualifier: 0x00,
> +            eventList: 5}},

eventList is an array,
and in this case the array has two elements [5, 7],
so the test in testSetupEventLust should test with the contents of eventList.
Attachment #711647 - Flags: review?(allstars.chh)
Attachment #711648 - Flags: review?(allstars.chh) → review+
Attachment #711663 - Flags: review?(allstars.chh) → review+
Attachment #711665 - Flags: review?(allstars.chh) → review+
Attachment #711669 - Flags: review?(allstars.chh) → review+
Comment on attachment 709576 [details] [diff] [review]
Part 9: Test STK Launch Browser.

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

::: dom/icc/tests/marionette/test_stk_launch_browser.js
@@ +7,5 @@
> +
> +let icc = navigator.mozMobileConnection.icc;
> +ok(icc instanceof MozIccManager, "icc is instanceof " + icc.constructor);
> +
> +function testLaunchBrowser(cmd, exp) {

please rename these parameters.

@@ +17,5 @@
> +  runNextTest();
> +}
> +
> +let tests = [
> +  

nit, spaces.
also many of them below.
Attachment #709576 - Flags: review?(allstars.chh)
Comment on attachment 709576 [details] [diff] [review]
Part 9: Test STK Launch Browser.

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

what about manifest.ini?
Comment on attachment 709577 [details] [diff] [review]
Part 10: Test STK Display Text.

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

Same problem here.
Attachment #709577 - Flags: review?(allstars.chh)
Comment on attachment 709578 [details] [diff] [review]
Part 11: Test STK Get Inkey.

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

Same
Attachment #709578 - Flags: review?(allstars.chh)
Attachment #709580 - Flags: review?(allstars.chh)
Attachment #709581 - Flags: review?(allstars.chh)
Attachment #709582 - Flags: review?(allstars.chh)
Attached patch Part 9: Test STK Launch Browser (obsolete) — Splinter Review
update
Attachment #709576 - Attachment is obsolete: true
Attached patch Part 10: Test STK Display Text. (obsolete) — Splinter Review
update
Attachment #709577 - Attachment is obsolete: true
Attached patch Part 11: Test STK Get Inkey. (obsolete) — Splinter Review
update
Attachment #709578 - Attachment is obsolete: true
Attached patch Part 9: Test STK Launch Browser (obsolete) — Splinter Review
Attachment #711700 - Attachment is obsolete: true
Attached patch Part 12: Test STK Get Input. (obsolete) — Splinter Review
update
Attachment #709579 - Attachment is obsolete: true
Attachment #711702 - Flags: review?(allstars.chh)
Attachment #711705 - Flags: review?(allstars.chh)
Attachment #711706 - Flags: review?(allstars.chh)
Attachment #711709 - Flags: review?(allstars.chh)
Attached patch Part 13: Test STK Select Item. (obsolete) — Splinter Review
update
Attachment #709580 - Attachment is obsolete: true
Attached patch Part 14: Test STK Setup Menu. (obsolete) — Splinter Review
update
Attachment #709581 - Attachment is obsolete: true
Attachment #711702 - Flags: review?(allstars.chh) → review+
Attachment #711705 - Flags: review?(allstars.chh) → review+
Comment on attachment 711706 [details] [diff] [review]
Part 9: Test STK Launch Browser

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

::: dom/icc/tests/marionette/test_stk_launch_browser.js
@@ +14,5 @@
> +  is(command.commandQualifier, expect.commandQualifier, expect.name);
> +  if (command.options.confirmMessage) {
> +    is(command.options.confirmMessage, expect.text, expect.name);
> +  }
> +

Can you also the command.options.url?
Attachment #711706 - Flags: review?(allstars.chh)
update
Attachment #709582 - Attachment is obsolete: true
Attachment #711714 - Flags: review?(allstars.chh)
Attachment #711718 - Flags: review?(allstars.chh)
Attachment #711722 - Flags: review?(allstars.chh)
Comment on attachment 711705 [details] [diff] [review]
Part 11: Test STK Get Inkey.

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

Cancel the r+, as I found out we'd test more properties here.

::: dom/icc/tests/marionette/test_stk_get_inkey.js
@@ +12,5 @@
> +  log("STK CMD " + JSON.stringify(command));
> +  is(command.typeOfCommand, icc.STK_CMD_GET_INKEY, expect.name);
> +  is(command.commandQualifier, expect.commandQualifier, expect.name);
> +  is(command.options.text, expect.text, expect.name);
> +

For GET_INKEY
we should also check command.options.{isAlphabet, isUCS2, and isYesNoRequired};
Attachment #711705 - Flags: review+
Comment on attachment 711709 [details] [diff] [review]
Part 12: Test STK Get Input.

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

The same with GET_INKEY, we should test those isAlpha, isUCS2,.. properties.
(Those properties are extracted from Command Qualifier).
Attachment #711709 - Flags: review?(allstars.chh)
Comment on attachment 711714 [details] [diff] [review]
Part 13: Test STK Select Item.

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

Can we also test item[] ?
Attachment #711714 - Flags: review?(allstars.chh)
Comment on attachment 711718 [details] [diff] [review]
Part 14: Test STK Setup Menu.

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

Same with SELECT_ITEM
Attachment #711718 - Flags: review?(allstars.chh)
Comment on attachment 711722 [details] [diff] [review]
Part 15: Test STK Setup Idle Mode Text.

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

::: dom/icc/tests/marionette/test_stk_setup_idle_mode_text.js
@@ +12,5 @@
> +  log("STK CMD " + JSON.stringify(command));
> +  is(command.typeOfCommand, icc.STK_CMD_SET_UP_IDLE_MODE_TEXT, expect.name);
> +  is(command.commandQualifier, expect.commandQualifier, expect.name);
> +  is(command.options.text, expect.text, expect.name);
> +

Can we also test isHighPriority, userClear and responseNeeded?
Attachment #711722 - Flags: review?(allstars.chh)
Comment on attachment 711702 [details] [diff] [review]
Part 10: Test STK Display Text.

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

Cancel the r+ as we'd also test other properties in DISPLAY_TEXT.

::: dom/icc/tests/marionette/test_stk_display_text.js
@@ +12,5 @@
> +  log("STK CMD " + JSON.stringify(command));
> +  is(command.typeOfCommand, icc.STK_CMD_DISPLAY_TEXT, expect.name);
> +  is(command.options.text, expect.text, expect.name);
> +  is(command.commandQualifier, expect.commandQualifier, expect.name);
> +

We should also test isHighPriority, userClear and responseNeed here.
Attachment #711702 - Flags: review+
(In reply to Yoshi Huang[:yoshi][:allstars.chh] from comment #79)
> Comment on attachment 711722 [details] [diff] [review]
> Part 15: Test STK Setup Idle Mode Text.
> 
> Review of attachment 711722 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/icc/tests/marionette/test_stk_setup_idle_mode_text.js
> @@ +12,5 @@
> > +  log("STK CMD " + JSON.stringify(command));
> > +  is(command.typeOfCommand, icc.STK_CMD_SET_UP_IDLE_MODE_TEXT, expect.name);
> > +  is(command.commandQualifier, expect.commandQualifier, expect.name);
> > +  is(command.options.text, expect.text, expect.name);
> > +
> 
> Can we also test isHighPriority, userClear and responseNeeded?

I got response in the form below:
INFO STK CMD {"commandNumber":1,"typeOfCommand":40,"commandQualifier":0,"rilMessageType":"stkcommand","options":{"text":"Idle Mode Text 2"}} Fri Feb 08 2013 08:17:13 GMT+0000 (GMT)

how can I get the fields you mention?
add the check of userClear, isHighPriority
Attachment #711702 - Attachment is obsolete: true
Attachment #711734 - Flags: review?(allstars.chh)
add check of url
Attachment #711706 - Attachment is obsolete: true
Attachment #711738 - Flags: review?(allstars.chh)
(In reply to Yoshi Huang[:yoshi][:allstars.chh] from comment #61)
> Comment on attachment 711647 [details] [diff] [review]
> Part 3: Test STK Setup Event List.
> 
> Review of attachment 711647 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/icc/tests/marionette/test_stk_setup_event_list.js
> @@ +28,5 @@
> > +  {command: "d00d81030105008202818299020507",
> > +   func: testSetupEventList,
> > +   expect: {name: "setup_event_list_cmd_2",
> > +            commandQualifier: 0x00,
> > +            eventList: 5}},
> 
> eventList is an array,
> and in this case the array has two elements [5, 7],
> so the test in testSetupEventLust should test with the contents of eventList.

Can you provide a proper way to test the contents of array? (I wonder if for loop is valid for test cases)
or can I just compare their length?
thanks
Attachment #711734 - Flags: review?(allstars.chh) → review+
Attachment #711738 - Flags: review?(allstars.chh) → review+
Check array instead of single element
Attachment #711647 - Attachment is obsolete: true
Attachment #715331 - Flags: review?(allstars.chh)
Attached patch Part 11: Test STK Get Inkey. (obsolete) — Splinter Review
add check of isAlphabet, isUSC2, isYesNoRequested.
Attachment #711705 - Attachment is obsolete: true
add check of isAlphabet, isUCS2, isPacked, hideInput.
Attachment #711709 - Attachment is obsolete: true
Comment on attachment 715331 [details] [diff] [review]
Part 3: Test STK Setup Event List.

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

::: dom/icc/tests/marionette/test_stk_setup_event_list.js
@@ +11,5 @@
> +function testSetupEventList(command, expect) {
> +  log("STK CMD " + JSON.stringify(command));
> +  is(command.typeOfCommand, icc.STK_CMD_SET_UP_EVENT_LIST, expect.name);
> +  is(command.commandQualifier, expect.commandQualifier, expect.name);
> +  for(index in command.options.eventList) {

nit, space after for
Attachment #715331 - Flags: review?(allstars.chh) → review+
Attached patch Part 13: Test STK Select Item. (obsolete) — Splinter Review
add check of items[].
Attachment #711714 - Attachment is obsolete: true
Attached patch Part 14: Test STK Setup Menu. (obsolete) — Splinter Review
add check of items[].
Attachment #711718 - Attachment is obsolete: true
Attachment #715333 - Flags: review?(allstars.chh)
Attachment #715335 - Flags: review?(allstars.chh)
Attachment #715344 - Flags: review?(allstars.chh)
Attachment #715352 - Flags: review?(allstars.chh)
fix nit
Attachment #715331 - Attachment is obsolete: true
Attachment #715354 - Flags: review+
Comment on attachment 715333 [details] [diff] [review]
Part 11: Test STK Get Inkey.

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

::: dom/icc/tests/marionette/test_stk_get_inkey.js
@@ +20,5 @@
> +    is(command.options.isUCS2, expect.isUCS2, expect.name);
> +  }
> +  if (command.options.isYesNoRequested) {
> +    is(command.options.isYesNoRequested, expect.isYesNoRequested, expect.name);
> +  }

nit, add an extra line before runNextTest
Attachment #715333 - Flags: review?(allstars.chh) → review+
fix nit
Attachment #711210 - Attachment is obsolete: true
Attachment #715356 - Flags: review+
Attachment #715335 - Flags: review?(allstars.chh) → review+
fix nit.
Attachment #711648 - Attachment is obsolete: true
Attachment #715359 - Flags: review+
fix nit.
Attachment #711665 - Attachment is obsolete: true
Attachment #715361 - Flags: review+
Attached patch Part 7: Test STK Send SMS. (obsolete) — Splinter Review
fix nit.
Attachment #715362 - Flags: review+
Comment on attachment 715344 [details] [diff] [review]
Part 13: Test STK Select Item.

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

::: dom/icc/tests/marionette/test_stk_select_item.js
@@ +12,5 @@
> +  log("STK CMD " + JSON.stringify(command));
> +  is(command.typeOfCommand, icc.STK_CMD_SELECT_ITEM, expect.name);
> +  is(command.commandQualifier, expect.commandQualifier, expect.name);
> +  is(command.options.title, expect.title, expect.name);
> +  for (index in command.options.items) {

let index ..
Attachment #715344 - Flags: review?(allstars.chh) → review+
fix nit.
Attachment #711669 - Attachment is obsolete: true
Attachment #715364 - Flags: review+
Comment on attachment 715352 [details] [diff] [review]
Part 14: Test STK Setup Menu.

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

::: dom/icc/tests/marionette/test_stk_setup_menu.js
@@ +12,5 @@
> +  log("STK CMD " + JSON.stringify(command));
> +  is(command.typeOfCommand, icc.STK_CMD_SET_UP_MENU, expect.name);
> +  is(command.commandQualifier, expect.commandQualifier, expect.name);
> +  is(command.options.title, expect.title, expect.name);
> +  for (index in command.options.items) {

let index
Attachment #715352 - Flags: review?(allstars.chh) → review+
Hi John,
Could you run try server on B2G platform?

thanks
update.
Attachment #715354 - Attachment is obsolete: true
Attachment #715365 - Flags: review+
Attached patch Part 11: Test STK Get Inkey. (obsolete) — Splinter Review
update.
Attachment #715333 - Attachment is obsolete: true
Attachment #715367 - Flags: review+
Attachment #715367 - Attachment is obsolete: true
Attachment #715369 - Flags: review+
update.
Attachment #715344 - Attachment is obsolete: true
Attachment #715371 - Flags: review+
update.
Attachment #715352 - Attachment is obsolete: true
Attachment #715374 - Flags: review+
Attachment #711663 - Attachment is obsolete: true
Attachment #715362 - Attachment is obsolete: true
Attachment #715377 - Flags: review+
Cool, I'll help to push them when the test is finished,
also next time you'd run try only on B2G platform.
Otherwise sheriff would complain.
Import script for common code I filed Bug 843455 to address it.
Blocks: 843455
No longer depends on: 805838
little error on previous test,
so I push another test. (rename the part 15 patch to correct one)

https://tbpl.mozilla.org/?tree=Try&rev=59f25c918d58
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/jshih@mozilla.com-59f25c918d58
John, your 'try' result is failed, although I don't know what went wrong,
Can you run it again on 'B2G only' platform?
ok, I'll check it and do another test
CCing Griffin on this,
Hi, Griffin
In the Part 4 patch, there are some Chinese and Japanese characters in that test, we ran it well on our local PCs, but when we run try server, it fails with error message "UnicodeEncodeError: 'ascii' codec can't encode character u'\u30eb' in position 131: ordinal not in range(128)", (see comment 113)

Is this a known issue for marionette ?
Filed Bug 847256 for the TBPL failure,

John Can you comment out those failed tests and add comments on it?

Like


/*
 TBPL UnicodeEncodeError, See Bug 847256.
 {command:...
  func:...
  expect:...
 }
*/

thanks
Hi, John
Can you try the patch from Bug 847256 with your patches and run the try server again?
thanks
Hi, 
the test is here.
https://tbpl.mozilla.org/?tree=Try&rev=7c2034aa9aa5
Please also apply patch from Bug 847683 for the uploadssymbols error.
Try run for acedca11d51c is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=acedca11d51c
Results (out of 1 total builds):
    failure: 1
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/yhuang@mozilla.com-acedca11d51c
The patch in Bug 847683 seems strange..
I can't find the file "b2g_config.py" in the path "mozilla/" 
This makes the import fail
Try run for b6aa786f5bbb is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=b6aa786f5bbb
Results (out of 1 total builds):
    failure: 1
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/yhuang@mozilla.com-b6aa786f5bbb
Hi, I've push a test again and seems all pass
please see
https://tbpl.mozilla.org/?tree=Try&rev=42ea5b7953b6

p.s. there is a fail unrelated to my patch
Hi, John 
Thanks.
Once Bug 847256 is landed I'll commit your patches.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: