1159622 - [Icc] Split test_icc_contact.js into smaller tests

Status: RESOLVED FIXED

(Firefox OS Graveyard :: RIL, defect)

Description

[Edgar Chen [:edgar]]

Now we put all the contact tests (reading, updating, adding ..) into one script, we could consider to split it to smaller tests. This may also help to prevent the intermittent script timed out [1][2].

[1] https://treeherder.mozilla.org/logviewer.html#?job_id=7003504&repo=try
[2] https://treeherder.mozilla.org/logviewer.html#?job_id=6997588&repo=try

Comment 1

[Edgar Chen [:edgar]]

Thanks for taking this, John.
One more thing, as we are planning to enable OOP marionette tests, all the tests could be ran twice (OOP and in-process). Please also help to add some mechanism to recover sim contact once a test is finished (e.g. remove added contact ...), otherwise the second run may get unexpected result. Thank you.

Comment 2

[John Dai[:jdai]]

Attachment: Split test_icc_contact.js into read contact and add contact.

Comment 3

[Wesley Huang [:wesley_huang] (EPM) (NI me)]

[Tracking Requested - why for this release]:

Comment 4

[Edgar Chen [:edgar]]

Comment on attachment 8600825 [details] [diff] [review]
Split test_icc_contact.js into read contact and add contact.

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

Please see my comments below, Thank you.

::: dom/icc/tests/marionette/test_icc_contact_add.js
@@ +1,4 @@
> +/* Any copyright is dedicated to the Public Domain.
> + * http://creativecommons.org/publicdomain/zero/1.0/ */
> +
> +MARIONETTE_TIMEOUT = 60000;

Give a longer timeout for this test, maybe 90000.

@@ +38,5 @@
> +      }
> +    });
> +}
> +
> +function tearDownContact(aIcc, aContactId, aType, aPin2) {

s/tearDown/remove/

@@ +44,5 @@
> +  let contact = {
> +    id: aIcc.iccInfo.iccid + aContactId,
> +  };
> +
> +  return aIcc.updateContact(aType, contact, aPin2)

ICC Contacts API takes mozContact [1] as argument. |contact| should be a mozContact instance. 
However the id of mozContact is a readonly attribute, you are unable to create a mozContact (e.g. new mozContact({.....}) with a specific id.
So, you have to cache the added contact and clear up it before passing into update API for removing.

[1] https://dxr.mozilla.org/mozilla-central/source/dom/webidl/Contacts.webidl#62

@@ +49,5 @@
> +    .then((aResult) => {
> +      // Get ICC contact for checking new contact
> +      return aIcc.readContacts(aType)
> +        .then((aResult) => {
> +          // There are 4 SIM contacts which are harded in emulator

Don't need to check removing result, we could just assume the removing is working as expected in this test.
The removing functionality should be secured in bug 914659, though we don't have it yet :p.

@@ +75,5 @@
> +    .then(() => testAddContact(icc, "fdn", "0000"))
> +    // Test add fdn contacts without passing pin2
> +    .then(() => testAddContact(icc, "fdn"))
> +    .then(() => tearDownContact(icc, "5", "adn"))
> +    .then(() => tearDownContact(icc, "5", "fdn"))

This should also belong to bug 914659.

::: dom/icc/tests/marionette/test_icc_contact_read.js
@@ +8,5 @@
> +  log("testReadContacts: type=" + aType);
> +  let iccId = aIcc.iccInfo.iccid;
> +  return aIcc.readContacts(aType)
> +    .then((aResult) => {
> +      is(Array.isArray(aResult), true);

Also check the number of contacts.

is(aResult.length, 4, "Check number of contact");

And please help to add some comments about these contacts are hardcorded in emulator.

Comment 5

[John Dai[:jdai]]

Attachment: Split test_icc_contact.js into read contact and add contact.(V2)

Address comment 4.

Comment 6

[John Dai[:jdai]]

Attachment: Split test_icc_contact.js into read contact and add contact.(V3)

Hi Edgar,
The previous patch is incompleted, and I recreated a new one.
Could you help me review this patch? Thank you.

Comment 7

[Edgar Chen [:edgar]]

Comment on attachment 8603915 [details] [diff] [review]
Split test_icc_contact.js into read contact and add contact.(V3)

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

r=me with comments addressed, thank you.

::: dom/icc/tests/marionette/test_icc_contact_add.js
@@ +23,5 @@
> +      return aIcc.readContacts(aType)
> +        .then((aResult) => {
> +          let index = aResult.length - 1;
> +
> +          is(aResult[index].name[0], aMozContact.name[0]);

Nit: cache the last contact.

let contact = aResult[aResult.length - 1];
is(contact.name[0], aMozContact.name[0]);

@@ +29,5 @@
> +          is(aResult[index].id, aIcc.iccInfo.iccid + aResult.length);
> +
> +          return aResult[index].id;
> +        }, (aError) => {
> +          ok(false, "Cannot get " + aType + " contacts: " + aError.name);

Don't catch the reject, let startTestBase() [1] handle it.

[1] https://dxr.mozilla.org/mozilla-central/source/dom/icc/tests/marionette/head.js#432-433

Comment 8

[John Dai[:jdai]]

Attachment: Split test_icc_contact.js into read contact and add contact.(Final) r=echen

Try looks good:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9f60cc1607cb

Comment 9

[Edgar Chen [:edgar]]

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

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ebbc5a23f950