Closed Bug 848662 Opened 11 years ago Closed 11 years ago

B2G RIL: Marionette tests for reading FDN contact

Categories

(Core :: DOM: Device Interfaces, defect)

Product:
Core
Component:
DOM: Device Interfaces
Platform:
x86_64
Linux
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla24

People

(Reporter: allstars.chh, Assigned: gsvelto)

References

Details

Attachments

(1 file, 2 obsolete files)

[PATCH v2] Add marionette tests for FDN contacts manipulation
3.90 KB, patch
Details | Diff | Splinter Review 
Add test case for reading FDN contact.
Also need tests for writing FDN contact.
I'll be taking this as I need to test FDN functionality and the best approach is definitely to add some tests to our harness. I had just a couple of questions:

- Should I add the new tests to dom/icc/tests/test_icc_contacts.js or should I add a separate file specifically for FDN? Since that functionality belongs to the MozIccManager I'd be more keen on adding the tests to the existing file.

- Modifying FDN numbers requires a PIN2 code so one needs to be hard-coded in the tests; I'm using '0000' on my device for simplicity so I was tempted of hard-coding that one. However this will cause the tests to fail on every device that uses a different PIN2 code, would that be a problem for automated testing? If that's the case then another approach would be to put the tests in a separate file and have them disabled by default since they wouldn't be able to run w/o the proper PIN2 code.
Assignee: nobody → gsvelto
Status: NEW → ASSIGNED
Flags: needinfo?(allstars.chh)
(In reply to Gabriele Svelto [:gsvelto] from comment #2) 
> - Should I add the new tests to dom/icc/tests/test_icc_contacts.js 
Yes

> - Modifying FDN numbers requires a PIN2 code so one needs to be hard-coded
> in the tests; I'm using '0000' on my device for simplicity so I was tempted
> of hard-coding that one. However this will cause the tests to fail on every
> device that uses a different PIN2 code, would that be a problem for
> automated testing? If that's the case then another approach would be to put
> the tests in a separate file and have them disabled by default since they
> wouldn't be able to run w/o the proper PIN2 code.

Marionette runs on emulator, and I think emulator doesn't check PIN2 when writing. So maybe you don't have to worry about this.
Flags: needinfo?(allstars.chh)
(In reply to Yoshi Huang[:allstars.chh][:yoshi] from comment #3)
> Marionette runs on emulator, and I think emulator doesn't check PIN2 when
> writing. So maybe you don't have to worry about this.

I've successfully managed to run the existing tests on the emulator, it took me a while because I was first trying to get them to work on the device and that wasn't really working very well (marionette and gaia don't seem to like each other running at the same time).

Anyway I've laid down a plan for this bug, this should come in two parts:

- First of all I have to add some hard-coded FDN numbers in the emulator in order to be able to test readContacts() and all. This should be similar to the work done in bug 833174. I'll prepare a pull request for mozilla-b2g/platform_external_qemu with the relevant code (which will probably look a lot like https://github.com/mozilla-b2g/platform_external_qemu/pull/20/files#L0R761)

- Once this is done I'll prepare a patch with the tests themselves and land it once the QEMU bits are in place

Just one final question, dom/icc/tests/marionett/test_icc_card_lock.js does check for regular "pin" lock functionality but does not exercise "fdn" locking/unlocking. Should I add checks for that too? Or should I open a separate bug for that?
(In reply to Gabriele Svelto [:gsvelto] from comment #4)
> - First of all I have to add some hard-coded FDN numbers in the emulator in
> order to be able to test readContacts() and all. This should be similar to
> the work done in bug 833174. I'll prepare a pull request for
> mozilla-b2g/platform_external_qemu with the relevant code (which will
> probably look a lot like
> https://github.com/mozilla-b2g/platform_external_qemu/pull/20/files#L0R761)
> 
I already done this in Bug 845216

> 
> Just one final question, dom/icc/tests/marionett/test_icc_card_lock.js does
> check for regular "pin" lock functionality but does not exercise "fdn"
> locking/unlocking. Should I add checks for that too? Or should I open a
> separate bug for that?

Can't this be done by updateContact('fdn', contact, pin2)?

But I am not sure the PIN2 support on emulator. Edgar Chen did some work on it.
(In reply to Yoshi Huang[:allstars.chh][:yoshi] from comment #5)
> I already done this in Bug 845216

Great! I had been looking at a v1-train checkout so I missed your addition to QEMU. I was already wading through the 3GPP spec to figure out the format so you just saved me a lot of time :)

> Can't this be done by updateContact('fdn', contact, pin2)?

Yes, it does, however we have a separate test for the setCardLock() method so I was wondering if we want to cover that too. Looking through the nsIDOMMozIccManager interface I see that we don't have tests covering unlockCardLock() which might also be worthy covering in another bug (and which also includes FDN/PIN2 functionality).

> But I am not sure the PIN2 support on emulator. Edgar Chen did some work on
> it.

Thanks for the pointer, I'll ping him then if I encounter any problems.
(In reply to Yoshi Huang[:allstars.chh][:yoshi] from comment #5)
> (In reply to Gabriele Svelto [:gsvelto] from comment #4)
> > - First of all I have to add some hard-coded FDN numbers in the emulator in
> > order to be able to test readContacts() and all. This should be similar to
> > the work done in bug 833174. I'll prepare a pull request for
> > mozilla-b2g/platform_external_qemu with the relevant code (which will
> > probably look a lot like
> > https://github.com/mozilla-b2g/platform_external_qemu/pull/20/files#L0R761)
> > 
> I already done this in Bug 845216
> 
> > 
> > Just one final question, dom/icc/tests/marionett/test_icc_card_lock.js does
> > check for regular "pin" lock functionality but does not exercise "fdn"
> > locking/unlocking. Should I add checks for that too? Or should I open a
> > separate bug for that?
> 
> Can't this be done by updateContact('fdn', contact, pin2)?
> 
> But I am not sure the PIN2 support on emulator. Edgar Chen did some work on
> it.

Emulator does not support PIN2 currently.
(In reply to Edgar Chen [:edgar][:echen] from comment #7)
> Emulator does not support PIN2 currently.

I see... do we already have a bug for that? If not I'll open one myself.
Blocks: 883821
(In reply to Gabriele Svelto [:gsvelto] from comment #6)
> > Can't this be done by updateContact('fdn', contact, pin2)?
> 
> Yes, it does, however we have a separate test for the setCardLock() method
> so I was wondering if we want to cover that too. Looking through the
> nsIDOMMozIccManager interface I see that we don't have tests covering
> unlockCardLock() which might also be worthy covering in another bug (and
> which also includes FDN/PIN2 functionality).
> 
I agree we also need setCardLock to enable FDN.
The reason that we dont have tests for it is emulator doesn't have good support for SIM Locks. 
Bug 860585 have some tests for card lock.
But it doesn't cover PIN2 yet.

I think we don't have bugs for PIN2 on emulator yet.
Feel free to file one.
Depends on: 884305
This patch adds tests for FDN functionality; currently it does not work yet as the emulator lacks support for locking/unlocking the FDN mode.
Attachment #764829 - Flags: feedback?(allstars.chh)
Comment on attachment 764829 [details] [diff] [review]
[WIP PATCH] Add marionette tests for FDN contacts manipulation

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

::: dom/icc/tests/marionette/test_icc_contact.js
@@ +7,5 @@
>  
>  let icc = navigator.mozIccManager;
>  ok(icc instanceof MozIccManager, "icc is instanceof " + icc.constructor);
>  
> +function testImportAdnContacts() {

Maybe rename it to testReadContacts.
Because in Gaia, 'import' also implies to store those contacts to contact DB, which we don't actually.

Also how do you think if we could merge readADN and readFDN functions together by adding another parameter, contactType. And possibly another optional parameter 'pin2'.
So we won't need two functions doing the same thing.

@@ +75,5 @@
> +  };
> +};
> +
> +function testImportFdnContacts() {
> +  let fdnUnlockRequest = icc.unlockCardLock(

For reading FDN, you don't have to supply PIN2 nor unlock PIN2.
In case you want to do 'update fdn', you don't even have to unlock PIN2 first.
You could supply pin2 as the parameter for updateContact.

@@ +116,5 @@
> +
> +function testAddFdnContact() {
> +  let fdnUnlockRequest = icc.unlockCardLock(
> +    {lockType: "fdn",
> +     pin2: "0000"});

As I've explained above, this is wrong.
Attachment #764829 - Flags: feedback?(allstars.chh)
(In reply to Yoshi Huang[:allstars.chh][:yoshi] from comment #11)
> Maybe rename it to testReadContacts.
> Because in Gaia, 'import' also implies to store those contacts to contact
> DB, which we don't actually.

Yes, that would be clearer.

> Also how do you think if we could merge readADN and readFDN functions
> together by adding another parameter, contactType. And possibly another
> optional parameter 'pin2'.
> So we won't need two functions doing the same thing.

I'll do that.

> For reading FDN, you don't have to supply PIN2 nor unlock PIN2.
> In case you want to do 'update fdn', you don't even have to unlock PIN2
> first.
> You could supply pin2 as the parameter for updateContact.

I was reading through the specs and TS 51.055 section 10.5.2 states that I need PIN1 access for reading and PIN2 access for writing entries so I could skip that part and use the |pin2| field directly as you suggest but shouldn't I unlock PIN1 first? Or do we assume the emulator's SIM is already PIN1-unlocked at the beginning of the test?

I'll refresh the patch with your suggestions leaving out the PIN1 part for now as I'm not sure if it's needed or not.
(In reply to Gabriele Svelto [:gsvelto] from comment #12)
> 
> I was reading through the specs and TS 51.055 section 10.5.2 states that I
> need PIN1 access for reading and PIN2 access for writing entries so I could
> skip that part and use the |pin2| field directly as you suggest but
> shouldn't I unlock PIN1 first? Or do we assume the emulator's SIM is already
> PIN1-unlocked at the beginning of the test?
> 

Now for simplicity we don't enable PIN on emulator.
But if you still want to check is PIN enabled when running your test, you could read cardState.
(I forgot wheter it's in IccManager or MobileConnection)

But I think it's trival so I just leave you decide you'd like to do it or not.
Updated patch addressing your comments. I didn't add the PIN1-unlocking part as it doesn't really add anything interesting to the test. The tests passes correctly even though we don't have PIN2 support in the emulator because we don't check for it either.
Attachment #764829 - Attachment is obsolete: true
Attachment #765327 - Flags: review?(allstars.chh)
Comment on attachment 765327 [details] [diff] [review]
[PATCH] Add marionette tests for FDN contacts manipulation

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

::: dom/icc/tests/marionette/test_icc_contact.js
@@ +49,5 @@
> +  if (type === "adn") {
> +    updateRequest = icc.updateContact(type, contact);
> +  } else { // type === "fdn"
> +    updateRequest = icc.updateContact(type, contact, pin2);
> +  }

seems 
let updateRequest = icc.updateContact(type, contact, pin2);
is enough
Attachment #765327 - Flags: review?(allstars.chh) → review+
Updated patch as per comment 15. I'll push to try as soon as the tree is open again; thanks for the review and your help :)
Attachment #765327 - Attachment is obsolete: true
Try run is green:

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

The patch was r=yoshi and it turns out it doesn't need the functionality I'm adding in bug 884305 (yet) so it can land right away.
No longer depends on: 884305
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/2cecdcdd4c77
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: