1213785 - Intermittent script timed out in test_icc_contact_add.js and test_icc_contact_update.js

Status: RESOLVED FIXED

(Firefox OS Graveyard :: RIL, defect)

Description

[Edgar Chen [:edgar]]

Recent try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8ccd09f7dfa4&exclusion_profile=false&group_state=expanded

I believe it is because we added some new test data in bug 1122376 (accessing icc contact is really slow), we could try to rewrite the test a bit to make it faster or increase the timeout value.

Comment 1

[Edgar Chen [:edgar]]

Thanks for taking this, John.

Comment 2

[John Dai[:jdai]]

Attachment: Fixed test_icc_contact_add.js and test_icc_contact_update.js testcases timeout.

In this patch, I moved out read contact method which is most time-consuming part to make tests faster.

Hi Edgar, May I have your review of this patch? Thanks.

Comment 3

[Edgar Chen [:edgar]]

Comment on attachment 8679282 [details] [diff] [review]
Fixed test_icc_contact_add.js and test_icc_contact_update.js testcases timeout.

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

::: dom/icc/tests/marionette/test_icc_contact_add.js
@@ +43,5 @@
>        is(aResult.tel[0].value, aMozContact.tel[0].value.substring(0, 40));
>        // We only support SIM in emulator, so we don't have anr and email field.
>        ok(aResult.tel.length == 1);
>        ok(!aResult.email);
>  

Nit: remove extra space

@@ +58,5 @@
> +function removeContacts(aIcc, aContacts, aType, aPin2) {
> +  log("removeContacts: type=" + aType + ", pin2=" + aPin2);
> +  let promise = Promise.resolve();
> +  let contact;
> +  let start = aContacts.length - TEST_ADD_DATA.length;

Is it possible that the caller passes the contacts that we actually want to remove, instead of doing this filtering.

@@ +87,5 @@
> +      let start = aResult.length - TEST_ADD_DATA.length;
> +
> +      for (let i = start; i < aResult.length ; i++) {
> +        let contact = aResult[i];
> +        let aMozContact = TEST_ADD_DATA[i - start];

Just drop the unreferenced contacts from |aResult| then we don't have shift the index.

e.g.

aResult = aResult.split(aResult.length - TEST_ADD_DATA.length);
for (let i = 0; i < TEST_ADD_DATA.length; i++) {
 ....
}

@@ +111,5 @@
> +  promise =  promise.then(() => testAddContacts(icc, "adn"))
> +     // Test add fdn contacts
> +    .then(() => testAddContacts(icc, "fdn", "0000"))
> +    // Test add fdn contacts without passing pin2
> +    .then(() => testAddContacts(icc, "fdn"));

For test without passing pin2, I think we don't need to do with all test data, just one is enough, since they are all the same case actually.

@@ +117,1 @@
>    return promise;

return Promise.resolve()
 .then(....)
 ....
 .then(....);

::: dom/icc/tests/marionette/test_icc_contact_update.js
@@ +95,5 @@
> +    // Get ICC contact for checking expect contacts
> +    promise = promise.then(() => aIcc.readContacts(aType))
> +    .then((aResult) => {
> +      for (let i = 0; i < TEST_UPDATE_DATA.length ; i++) {
> +        let aMozContact = TEST_UPDATE_DATA[i];

No `a` prefix for local variables.

@@ +129,5 @@
>      })
> +    // Test update adn contacts
> +    .then(() => testUpdateContacts(icc, "adn", adnContacts))
> +    // Test update fdn contacts
> +    .then(() => testUpdateContacts(icc, "fdn", fdnContacts))

Ditto: For test without passing pin2, I think we don't need to do with all test data, just one is enough, since they are all the same case actually.

@@ +130,5 @@
> +    // Test update adn contacts
> +    .then(() => testUpdateContacts(icc, "adn", adnContacts))
> +    // Test update fdn contacts
> +    .then(() => testUpdateContacts(icc, "fdn", fdnContacts))
> +    // Test update fdn contacts without passing pin2

I guess the comment was put in wrong place.

Comment 4

[John Dai[:jdai]]

Attachment: Fixed test_icc_contact_add.js and test_icc_contact_update.js testcases timeout. v2

Addressed comment 3.

Hi Edgar, May I have your review of this patch? Thanks.

Comment 5

[Edgar Chen [:edgar]]

Comment on attachment 8681045 [details] [diff] [review]
Fixed test_icc_contact_add.js and test_icc_contact_update.js testcases timeout. v2

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

r=me with comment addressed. Thank you.

::: dom/icc/tests/marionette/test_icc_contact_add.js
@@ +75,5 @@
> +
> +    promise = promise.then(() => testAddContact(aIcc, aType, test_data, aPin2));
> +  }
> +
> +  if (aType === "adn" || (aType === "fdn" && aPin2)) {

Since no pin2 case won't call testAddContacts(), we don't need this check.

@@ +83,5 @@
> +      aResult = aResult.slice(aResult.length - TEST_ADD_DATA.length);
> +
> +      for (let i = 0; i < aResult.length ; i++) {
> +        let contact = aResult[i];
> +        let mozContact = TEST_ADD_DATA[i];

s/mozContact/expectedResult/

::: dom/icc/tests/marionette/test_icc_contact_update.js
@@ +94,2 @@
>  
> +  if (aType === "adn" || (aType === "fdn" && aPin2)) {

Ditto, Since no pin2 case won't be here, we don't need this check.

@@ +96,5 @@
> +    // Get ICC contact for checking expect contacts
> +    promise = promise.then(() => aIcc.readContacts(aType))
> +    .then((aResult) => {
> +      for (let i = 0; i < TEST_UPDATE_DATA.length; i++) {
> +        let mozContact = TEST_UPDATE_DATA[i];

s/mozContact/expectedResult/

Comment 6

[John Dai[:jdai]]

Attachment: Fixed test_icc_contact_add.js and test_icc_contact_update.js testcases timeout. r=echen

Addressed comment 5 and carry on r+.

Try link:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=46f49d327b01&exclusion_profile=false

Comment 7

[John Dai[:jdai]]

Try link:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=cc5ad1bc3671&exclusion_profile=false

Comment 8

[Pulsebot]

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

Comment 9

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/2f14feb271d6

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/2f14feb271d6

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/1091f16dde9b