1159134 - [B2G][STK] Add Test Coverage for STK related requests.

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Emulator, defect)

Description

[Bevis Tseng[:bevis][:btseng](Exited)]

[Tracking Requested - why for this release]:

To ensure there won't be any regression after bug 1114938 (Refactor STK with IPDL) is fixed, I'd like to improve our test coverage for STK related requests defined MozIcc.webidl [1].

However, currently, there is no way to know what STK response exactly be sent into the modem for the verification.

AFAIK, the AT command of +CSIM is available in TS 27.007 to send raw APDU as a generic SIM IO for EF access, STK responses, and EAP/AKA authentications. [2] 

Hence, we could wrap the hex data of REQUEST_STK_SET_PROFILE, REQUEST_STK_SEND_ENVELOPE_COMMAND, REQUEST_STK_SEND_TERMINAL_RESPONSE into the APDU[3] with +CSIM and with an additional console command to peek the hex data from test case to verdict the test result.

The command APDU structure is defined as followed in [3]:
  Code  | Length |  Description
--------+--------+-------------------------------------
  CLA   |   1    | Class of Instruction
  INS   |   1    | Instruction Code
  P1    |   1    | Instruction Parameter 1
  P2    |   1    | Instruction Parameter 2
  Lc    |  0-1   | Length of Command Data
  Data  |  Lc    | Command Data String
  Le    |  0-1   | Maximum number of data bytes 
        |        | expected in response of the command

[1] https://dxr.mozilla.org/mozilla-central/source/dom/webidl/MozIcc.webidl#183-234
[2] http://www.etsi.org/deliver/etsi_ts/127000_127099/127007/08.05.00_60/ts_127007v080500p.pdf
[3] http://www.etsi.org/deliver/etsi_ts/102200_102299/102221/09.02.00_60/ts_102221v090200p.pdf

Comment 1

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Part 1: Map STK related requests into +CSIM command.

Hi Edgar,

May I have your review for this changes?

Thanks!

Comment 2

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Part 2: Add new console command to peek last stk request.

Hi Edgar,

May I have your review for this change?

Thanks!

Comment 3

[Edgar Chen [:edgar]]

Per offline discussion, please also help to attach the test you wanna to add in this bug as well. Thank you.

Comment 4

[Bevis Tseng[:bevis][:btseng](Exited)]

(In reply to Edgar Chen [:edgar][:echen] from comment #3)
> Per offline discussion, please also help to attach the test you wanna to add
> in this bug as well. Thank you.

Sure, I'll move the related test cases I've done locally for bug 1114938 to this one for further review.

Comment 5

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Part 3: Fix improper CTLV encoding in STK Responses

This patch include the fix of improper CTLV encoding in STK related request.

Hi Edgar,

May I have your review for these changes?

Thanks!

Comment 6

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Part 4: Add Test Cases for STK related Requests.

Add Marionette test cases for STK related requests.

Comment 7

[Edgar Chen [:edgar]]

Since I am working on the branching/porting things (see bug 1163968) now, please expect a delayed response and you probably need to rebase and provide a new PR after it's done.

Comment 8

[Edgar Chen [:edgar]]

Comment on attachment 8604024 [details] [review]
Part 1: Map STK related requests into +CSIM command.

Looks good, but master is for emulator-kk now, please also send a PR to b2g-ril_v7 [1] which is for emualtor-ics. Thank you.

[1] https://github.com/mozilla-b2g/platform_hardware_ril/commits/b2g-ril_v7

Comment 9

[Edgar Chen [:edgar]]

Comment on attachment 8604025 [details] [review]
Part 2: Add new console command to peek last stk request.

Please see my comments on github. And emulator-ics now moves to b2g-ics branch, please create a new PR to b2g-ics branch. Thank you.

Comment 10

[Edgar Chen [:edgar]]

Comment on attachment 8605638 [details] [diff] [review]
Part 3: Fix improper CTLV encoding in STK Responses

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

Please revise the commit title: Just start with `Part 1` for the gecko patch. Thank you.

Comment 11

[Edgar Chen [:edgar]]

Comment on attachment 8605639 [details] [diff] [review]
Part 4: Add Test Cases for STK related Requests.

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

Nice. Thank you.

::: dom/icc/tests/marionette/head.js
@@ +155,5 @@
> + *
> + * @return A deferred promise.
> + */
> +function peekLastRequestedApdu() {
> +  let cmd = "stk lastapdu";

Unused variable.

Comment 12

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Patch: (RIL_ICS) Map STK related requests into +CUSATT/+CUSATE command.

Comment 13

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Patch: (RIL_KK) Map STK related requests into +CUSATT/+CUSATE command.

Comment 14

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Patch: (RIL_ICS) Add new console command to peek last stk request.

Comment 15

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Patch: (RIL_KK) Add new console command to peek last stk request.

Comment 16

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: (v2) Part 1: Fix improper CTLV encoding in STK Responses. r=echen

rename as part#1 patch in gecko.

Comment 17

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: (v2) Part 2: Add Test Cases for STK related Requests. r=echen

rename as part#2 patch in gecko.

Comment 18

[Edgar Chen [:edgar]]

I just found the latest version of AT command spec (v12.8.0) has already defined commands for terminal response and envelope command [1], +CUSATT and +CUSATE. What about using those commands, instead of generic SIM IO? (Sorry, I should point this out early)

[1] http://www.etsi.org/deliver/etsi_ts/127000_127099/127007/12.08.00_60/ts_127007v120800p.pdf

Comment 19

[Bevis Tseng[:bevis][:btseng](Exited)]

(In reply to Edgar Chen [:edgar][:echen] from comment #18)
> I just found the latest version of AT command spec (v12.8.0) has already
> defined commands for terminal response and envelope command [1], +CUSATT and
> +CUSATE. What about using those commands, instead of generic SIM IO? (Sorry,
> I should point this out early)
> 
> [1]
> http://www.etsi.org/deliver/etsi_ts/127000_127099/127007/12.08.00_60/
> ts_127007v120800p.pdf

Never mind.
It's my mistake for not checking the latest version of this Spec.
It make our implementation more clear if we adopt these 2 UAT specific AT commands.

Thanks for pointing out this. :)

Comment 20

[Bevis Tseng[:bevis][:btseng](Exited)]

Comment on attachment 8610445 [details] [review]
Patch: (RIL_ICS) Map STK related requests into +CUSATT/+CUSATE command.

Per comment 18, I'll update this patch with new approach instead.

Comment 21

[Bevis Tseng[:bevis][:btseng](Exited)]

Comment on attachment 8610446 [details] [review]
Patch: (RIL_KK) Map STK related requests into +CUSATT/+CUSATE command.

Per comment 18, I'll update this patch with new approach instead.

Comment 22

[Bevis Tseng[:bevis][:btseng](Exited)]

Comment on attachment 8610447 [details] [review]
Patch: (RIL_ICS) Add new console command to peek last stk request.

Per comment 18, I'll update this patch with new approach instead.

Comment 23

[Bevis Tseng[:bevis][:btseng](Exited)]

Comment on attachment 8610461 [details] [review]
Patch: (RIL_KK) Add new console command to peek last stk request.

Per comment 18, I'll update this patch with new approach instead.

Comment 24

[Bevis Tseng[:bevis][:btseng](Exited)]

Comment on attachment 8610445 [details] [review]
Patch: (RIL_ICS) Map STK related requests into +CUSATT/+CUSATE command.

update implementation with +CUSATT/+CUSATE command.

Comment 25

[Bevis Tseng[:bevis][:btseng](Exited)]

Comment on attachment 8610446 [details] [review]
Patch: (RIL_KK) Map STK related requests into +CUSATT/+CUSATE command.

update implementation with +CUSATT/+CUSATE command.

Comment 26

[Bevis Tseng[:bevis][:btseng](Exited)]

Comment on attachment 8610447 [details] [review]
Patch: (RIL_ICS) Add new console command to peek last stk request.

update pull request with +CUSATT/+CUSATE adopted.

Comment 27

[Bevis Tseng[:bevis][:btseng](Exited)]

Comment on attachment 8610461 [details] [review]
Patch: (RIL_KK) Add new console command to peek last stk request.

update pull request with +CUSATT/+CUSATE adopted.

Comment 28

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Part 1: Fix improper CTLV encoding in STK Responses. r=echen

update final patch.

Comment 29

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Part 2: Add Test Cases for STK related Requests. r=echen

update final patch.

Comment 30

[Bevis Tseng[:bevis][:btseng](Exited)]

try server result is green:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=be8f4b0679d5

Comment 31

[Edgar Chen [:edgar]]

Comment on attachment 8610445 [details] [review]
Patch: (RIL_ICS) Map STK related requests into +CUSATT/+CUSATE command.

Thank you.

Comment 32

[Edgar Chen [:edgar]]

Comment on attachment 8610447 [details] [review]
Patch: (RIL_ICS) Add new console command to peek last stk request.

Looks good. Thank you.

Comment 33

[Bevis Tseng[:bevis][:btseng](Exited)]

Set checkin-needed for emulator parts at 1st stage and keep this bug open to set checkin-needed again for gecko parts at 2nd stage.

Comment 34

[Bevis Tseng[:bevis][:btseng](Exited)]

sorry, not aware of the nits to be addressed.
I'll update the patch again in emulator parts.

Comment 35

[Bevis Tseng[:bevis][:btseng](Exited)]

Address nits in github.

Set checkin-needed for emulator parts at 1st stage and 
keep this bug open to set checkin-needed again for gecko parts at 2nd stage.

Comment 36

[Edgar Chen [:edgar]]

I will help to merge, thank you.

Comment 37

[Edgar Chen [:edgar]]

platform_hardware_ril:
master: https://github.com/mozilla-b2g/platform_hardware_ril/commit/de4bfffbbc2aabe5b5eca485e459da75e49097e2
b2g-ril_v7: https://github.com/mozilla-b2g/platform_hardware_ril/commit/aac9cc4bb94cf720baf8f7ee419b4d76ac86b1ac

Comment 38

[Edgar Chen [:edgar]]

platform_external_qemu:

b2g-ics:
https://github.com/mozilla-b2g/platform_external_qemu/commit/10b3daf0093db94c64e78a72ac43a93b68976087

b2g-kitkat:
https://github.com/mozilla-b2g/platform_external_qemu/commit/c4d9746e5f1a3a2e6cb53d59d5d721e9888cd2e1

Comment 39

[Bevis Tseng[:bevis][:btseng](Exited)]

try server result is green:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=99b44a488587

set checkin-needed for landing gecko patches. 
i.e., attachment #8612164 [details] [diff] [review] and attachment 8612166 [details] [diff] [review].

Comment 40

[Pulsebot]

https://hg.mozilla.org/integration/b2g-inbound/rev/ec84531589e4
https://hg.mozilla.org/integration/b2g-inbound/rev/abeb01323218

Comment 41

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/ec84531589e4
https://hg.mozilla.org/mozilla-central/rev/abeb01323218