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)
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.
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
Comment 2•12 years ago
|
||
(In reply to Yoshi Huang[:yoshi][:allstars.chh] from comment #1) > As Bug 809294 supports STK proactive command. wrong bug id?
Updated•12 years ago
|
Updated•12 years ago
|
Assignee: nobody → atsai
Attachment #709568 -
Flags: review?
Attachment #709569 -
Flags: review?
Attachment #709570 -
Flags: review?
Attachment #709571 -
Flags: review?
Attachment #709572 -
Flags: review?
Attachment #709573 -
Flags: review?
Attachment #709574 -
Flags: review?
Assignee | ||
Comment 10•11 years ago
|
||
Attachment #709575 -
Flags: review?
Assignee | ||
Comment 11•11 years ago
|
||
Attachment #709576 -
Flags: review?
Assignee | ||
Comment 12•11 years ago
|
||
Assignee | ||
Comment 13•11 years ago
|
||
Attachment #709578 -
Flags: review?
Assignee | ||
Comment 14•11 years ago
|
||
Attachment #709579 -
Flags: review?
Assignee | ||
Comment 15•11 years ago
|
||
Attachment #709580 -
Flags: review?
Assignee | ||
Comment 16•11 years ago
|
||
Attachment #709581 -
Flags: review?
Assignee | ||
Comment 17•11 years ago
|
||
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.
Blocks: b2g-stk
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)
No longer depends on: 816926
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
Assignee | ||
Comment 26•11 years ago
|
||
(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.
Assignee | ||
Comment 27•11 years ago
|
||
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?
Assignee | ||
Comment 29•11 years ago
|
||
(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?
Assignee | ||
Comment 33•11 years ago
|
||
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.
Assignee | ||
Comment 34•11 years ago
|
||
Attachment #709569 -
Attachment is obsolete: true
Attachment #709569 -
Flags: review?(allstars.chh)
Assignee | ||
Comment 35•11 years ago
|
||
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+
Assignee | ||
Comment 39•11 years ago
|
||
update
Attachment #709630 -
Attachment is obsolete: true
Attachment #711184 -
Flags: review?(allstars.chh)
Assignee | ||
Comment 40•11 years ago
|
||
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+
Assignee | ||
Comment 42•11 years ago
|
||
update. (with test pass)
Attachment #709570 -
Attachment is obsolete: true
Attachment #709570 -
Flags: review?(allstars.chh)
Attachment #711190 -
Flags: review?(allstars.chh)
Assignee | ||
Comment 43•11 years ago
|
||
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+
Assignee | ||
Comment 48•11 years ago
|
||
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.
Assignee | ||
Comment 51•11 years ago
|
||
add check of command.options.eventList
Attachment #711190 -
Attachment is obsolete: true
Assignee | ||
Comment 53•11 years ago
|
||
update
Assignee | ||
Comment 54•11 years ago
|
||
update (with test pass)
Attachment #709572 -
Attachment is obsolete: true
Attachment #711646 -
Attachment is obsolete: true
Attachment #709572 -
Flags: review?(allstars.chh)
Assignee | ||
Comment 55•11 years ago
|
||
update (with test pass)
Attachment #709573 -
Attachment is obsolete: true
Attachment #709573 -
Flags: review?(allstars.chh)
Attachment #711647 -
Flags: review?(allstars.chh)
Attachment #711648 -
Flags: review?(allstars.chh)
Attachment #711650 -
Flags: review?(allstars.chh)
Assignee | ||
Comment 57•11 years ago
|
||
update (with test pass)
Attachment #709574 -
Attachment is obsolete: true
Attachment #709574 -
Flags: review?(allstars.chh)
Assignee | ||
Comment 58•11 years ago
|
||
Attachment #711662 -
Attachment is obsolete: true
Assignee | ||
Comment 59•11 years ago
|
||
Attachment #711650 -
Attachment is obsolete: true
Attachment #711650 -
Flags: review?(allstars.chh)
Attachment #711665 -
Flags: review?(allstars.chh)
Attachment #711663 -
Flags: review?(allstars.chh)
Assignee | ||
Comment 60•11 years ago
|
||
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 #709579 -
Flags: review?(allstars.chh)
Attachment #709580 -
Flags: review?(allstars.chh)
Attachment #709581 -
Flags: review?(allstars.chh)
Attachment #709582 -
Flags: review?(allstars.chh)
Assignee | ||
Comment 69•11 years ago
|
||
Attachment #711700 -
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)
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)
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+
Assignee | ||
Comment 81•11 years ago
|
||
(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?
Assignee | ||
Comment 82•11 years ago
|
||
add the check of userClear, isHighPriority
Attachment #711702 -
Attachment is obsolete: true
Attachment #711734 -
Flags: review?(allstars.chh)
Assignee | ||
Comment 83•11 years ago
|
||
add check of url
Attachment #711706 -
Attachment is obsolete: true
Attachment #711738 -
Flags: review?(allstars.chh)
Assignee | ||
Comment 84•11 years ago
|
||
(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 #711722 -
Flags: review+
Attachment #711734 -
Flags: review?(allstars.chh) → review+
Attachment #711738 -
Flags: review?(allstars.chh) → review+
Assignee | ||
Comment 85•11 years ago
|
||
Check array instead of single element
Attachment #711647 -
Attachment is obsolete: true
Attachment #715331 -
Flags: review?(allstars.chh)
Assignee | ||
Comment 86•11 years ago
|
||
add check of isAlphabet, isUSC2, isYesNoRequested.
Attachment #711705 -
Attachment is obsolete: true
Assignee | ||
Comment 87•11 years ago
|
||
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+
Assignee | ||
Comment 89•11 years ago
|
||
add check of items[].
Attachment #711714 -
Attachment is obsolete: true
Assignee | ||
Comment 90•11 years ago
|
||
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)
Assignee | ||
Comment 91•11 years ago
|
||
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+
Assignee | ||
Comment 93•11 years ago
|
||
fix nit
Attachment #711210 -
Attachment is obsolete: true
Attachment #715356 -
Flags: review+
Attachment #715335 -
Flags: review?(allstars.chh) → review+
Assignee | ||
Comment 94•11 years ago
|
||
fix nit.
Attachment #711648 -
Attachment is obsolete: true
Attachment #715359 -
Flags: review+
Assignee | ||
Comment 95•11 years ago
|
||
fix nit.
Attachment #711665 -
Attachment is obsolete: true
Attachment #715361 -
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+
Assignee | ||
Comment 98•11 years ago
|
||
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
Assignee | ||
Comment 101•11 years ago
|
||
update.
Attachment #715354 -
Attachment is obsolete: true
Attachment #715365 -
Flags: review+
Assignee | ||
Comment 102•11 years ago
|
||
update.
Attachment #715333 -
Attachment is obsolete: true
Attachment #715367 -
Flags: review+
Assignee | ||
Comment 103•11 years ago
|
||
Attachment #715367 -
Attachment is obsolete: true
Attachment #715369 -
Flags: review+
Assignee | ||
Comment 104•11 years ago
|
||
update.
Attachment #715344 -
Attachment is obsolete: true
Attachment #715371 -
Flags: review+
Assignee | ||
Comment 105•11 years ago
|
||
update.
Attachment #715352 -
Attachment is obsolete: true
Attachment #715374 -
Flags: review+
Assignee | ||
Comment 106•11 years ago
|
||
Attachment #711663 -
Attachment is obsolete: true
Attachment #715362 -
Attachment is obsolete: true
Attachment #715377 -
Flags: review+
Assignee | ||
Comment 107•11 years ago
|
||
I've tested the patches on try server, please see https://tbpl.mozilla.org/?tree=Try&rev=87f8b4b8c0fc http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/jshih@mozilla.com-87f8b4b8c0fc/try-linux/
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.
Assignee | ||
Comment 110•11 years ago
|
||
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?
Assignee | ||
Comment 112•11 years ago
|
||
ok, I'll check it and do another test
Assignee | ||
Comment 113•11 years ago
|
||
New test run. https://tbpl.mozilla.org/?tree=Try&rev=15684a551bbc
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 ?
Depends on: 847256
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
Assignee | ||
Comment 117•11 years ago
|
||
Hi, the test is here. https://tbpl.mozilla.org/?tree=Try&rev=7c2034aa9aa5
Please also apply patch from Bug 847683 for the uploadssymbols error.
Comment 119•11 years ago
|
||
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
Assignee | ||
Comment 120•11 years ago
|
||
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
Comment 121•11 years ago
|
||
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
Assignee | ||
Comment 122•11 years ago
|
||
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.
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?changeset=0b9505f9af26 Thanks for your hard work, John.
Comment 125•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/df8f39f4e927 https://hg.mozilla.org/mozilla-central/rev/a999b78a26bd https://hg.mozilla.org/mozilla-central/rev/deef21121046 https://hg.mozilla.org/mozilla-central/rev/85a99a931072 https://hg.mozilla.org/mozilla-central/rev/60240a6d40e9 https://hg.mozilla.org/mozilla-central/rev/322bfcca74c2 https://hg.mozilla.org/mozilla-central/rev/23e438b0264c https://hg.mozilla.org/mozilla-central/rev/42424b62ca23 https://hg.mozilla.org/mozilla-central/rev/d75651cff1be https://hg.mozilla.org/mozilla-central/rev/7ce9fa20fb19 https://hg.mozilla.org/mozilla-central/rev/3b9b3ba143eb https://hg.mozilla.org/mozilla-central/rev/29707091ab83 https://hg.mozilla.org/mozilla-central/rev/2f6ef14fa3b5 https://hg.mozilla.org/mozilla-central/rev/35d864904312 https://hg.mozilla.org/mozilla-central/rev/0b9505f9af26
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
You need to log in
before you can comment on or make changes to this bug.
Description
•