Closed Bug 1213785 Opened 4 years ago Closed 4 years ago

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

Categories

(Firefox OS Graveyard :: RIL, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(firefox45 fixed, b2g-v2.5 fixed)

RESOLVED FIXED
FxOS-S11 (13Nov)
Tracking Status
firefox45 --- fixed
b2g-v2.5 --- fixed

People

(Reporter: edgar, Assigned: jdai)

References

Details

Attachments

(1 file, 2 obsolete files)

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.
Assignee: nobody → jdai
Blocks: 1081529
Thanks for taking this, John.
Blocks: 1212262
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.
Attachment #8679282 - Flags: review?(echen)
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.
Attachment #8679282 - Flags: review?(echen)
Addressed comment 3.

Hi Edgar, May I have your review of this patch? Thanks.
Attachment #8679282 - Attachment is obsolete: true
Attachment #8681045 - Flags: review?(echen)
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/
Attachment #8681045 - Flags: review?(echen) → review+
https://hg.mozilla.org/mozilla-central/rev/2f14feb271d6
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → FxOS-S11 (13Nov)
You need to log in before you can comment on or make changes to this bug.