979134 - Rewrite mobileconnection test cases with Promise

Status: RESOLVED FIXED

(Firefox OS Graveyard :: RIL, defect)

Description

[Vicamo Yang [:vicamo][:vyang]]

+++ This bug was initially created as a clone of Bug #957917 comment 37 +++

Comment 1

[Vicamo Yang [:vicamo][:vyang]]

Attachment: part 2/N: convert test_mobile_networks.js

Comment 2

[Vicamo Yang [:vicamo][:vyang]]

Attachment: part 3/N: convert test_mobile_voice_state.js

Comment 3

[Vicamo Yang [:vicamo][:vyang]]

Attachment: part 4/N: convert test_mobile_data_state.js

Comment 4

[Vicamo Yang [:vicamo][:vyang]]

Attachment: part 1/N: share wrapDomRequestAsPromise

Comment 5

[Vicamo Yang [:vicamo][:vyang]]

Comment on attachment 8399088 [details] [diff] [review]
part 2/N: convert test_mobile_networks.js

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

::: dom/mobileconnection/tests/marionette/head.js
@@ +273,5 @@
>  
>    return deferred.promise;
>  }
>  
> +function getNetworks() {

TODO: documentation.

Comment 6

[Vicamo Yang [:vicamo][:vyang]]

Attachment: part 5/N: convert test_mobile_voice_location.js

Comment 7

[Vicamo Yang [:vicamo][:vyang]]

Attachment: part 6/N: convert test_mobile_data_location.js

Comment 8

[Vicamo Yang [:vicamo][:vyang]]

Attachment: part 1/N: share wrapDomRequestAsPromise : v2

add documentation.

Comment 9

[Vicamo Yang [:vicamo][:vyang]]

Attachment: part 2/N: convert test_mobile_networks.js : v2

1) documentation
2) correct reject parameter of selectNetwork & selectNetworkAutomatically

Comment 10

[Vicamo Yang [:vicamo][:vyang]]

Attachment: part 5/N: convert test_mobile_voice_location.js : v2

1) remove @param lines for getEmulatorGsmLocation() because it takes no arguments,
2) correct cid parsing in getEmulatorGsmLocation().

Comment 11

[Vicamo Yang [:vicamo][:vyang]]

Attachment: part 7/N: convert test_mobile_operator_names.js

1) split test_mobile_operator_names.js into three parts because it causes some intermittent failures
2) we used to have a 3-second idle wait for each roaming test. We have 8 of them, so it costs 24 seconds in total.  Now I find the root cause and replace it with a waitForManagerEvent('voicechange').  This reduces the execution time down to less than 3 seconds on my desktop.

Comment 12

[Vicamo Yang [:vicamo][:vyang]]

Attachment: Github PR for external/qemu: GSM cell ID mask should be 0xFFFFFFF

Comment 13

[Vicamo Yang [:vicamo][:vyang]]

There are numerous test cases for mobile connection.  I think I cannot convert them all at once because there are still other important tasks to do.  So maybe we can have some progress every few days.

Comment 14

[Vicamo Yang [:vicamo][:vyang]]

try part 1~7: https://tbpl.mozilla.org/?tree=Try&rev=7d5369f16bca

Comment 15

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Comment on attachment 8399238 [details] [diff] [review]
part 2/N: convert test_mobile_networks.js : v2

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

::: dom/mobileconnection/tests/marionette/head.js
@@ +315,5 @@
> + */
> +function selectNetwork(aNetwork) {
> +  let request = mobileConnection.selectNetwork(aNetwork);
> +  return wrapDomRequestAsPromise(request)
> +    .then(null, () => request.error);

We cannot have this line otherwise the reject handler in, e.g. testSelectExistingNetworkManual, won't be called. And the promise rejection failure can't be caught.

@@ +351,5 @@
> + */
> +function selectNetworkAutomatically() {
> +  let request = mobileConnection.selectNetworkAutomatically();
> +  return wrapDomRequestAsPromise(request)
> +    .then(null, () => request.error);

ditto.

::: dom/mobileconnection/tests/marionette/test_mobile_networks.js
@@ +59,4 @@
>  }
>  
> +function testSelectNetwork(aNetwork, aValidator) {
> +  log("Selecting TelKila manually");

What if "aNetwork" isn't Telkila? The log message seems not right for all the cases.

@@ +87,5 @@
>    }
>  }
>  
> +function testSelectNetworkErrors(aHomeNetwork, aRoamingNetwork) {
> +  throwsException(() => mobileConnection.selectNetwork(null));

When I looked at throwsException, something came into my mind: is it better to fire req.onerror better than throwing an exception? But I know this is out of the scope of this bug. :)

@@ +103,5 @@
>    // When the current network is selected again, the DOMRequest's onsuccess
>    // should be called, but the network shouldn't actually change
>  
>    // Telkila should be the currently selected network
>    log("Selecting TelKila (should already be selected");

We are trying to modualize the test case. It's better to have a more general log and comment. That said, Telkila might not be the existing network. Please:

log("Selecting " + aRoamingNetwork.longName + " (should already be selected)");

Also, please have one more check before 'selectNetwork'
  is(aRoamingNetwork.longName, mobileConnection.voice.network.longName)
so that we make sure the pre-condition is correct.

@@ +108,2 @@
>  
>    let voiceChanged = false;

Never referenced. We are fine to remove this flag.

@@ +108,5 @@
>  
>    let voiceChanged = false;
> +  function voiceChange() {
> +    let network = mobileConnection.voice.network;
> +    if (network.longName !== "TelKila") {

We should make this condition more general, i.e. 
network.longName !== aRoamingNetwork.longName

@@ +110,5 @@
> +  function voiceChange() {
> +    let network = mobileConnection.voice.network;
> +    if (network.longName !== "TelKila") {
> +      voiceChanged = true;
> +      ok(false, "voicechange event emitted");

I like the original message which provides clear information.

@@ +120,5 @@
> +  return selectNetwork(aRoamingNetwork)
> +    .then(function resolve() {
> +      let deferred = Promise.defer();
> +
> +      // Give the voicechange event another opportunity to fire

I'd like to add one more check :
is(aRoamingNetwork.longName, mobileConnection.voice.network.longName);

@@ +140,2 @@
>  
>    let voiceChanged = false;

ditto.

@@ +151,4 @@
>  
> +  return selectNetworkAutomatically()
> +    .then(function resolve() {
> +      let deferred = Promise.defer();

ditto: have one more check

Comment 16

[Vicamo Yang [:vicamo][:vyang]]

Attachment: part 1/N: share wrapDomRequestAsPromise : v3

Re-gen from hg. No change.

Comment 17

[Vicamo Yang [:vicamo][:vyang]]

Attachment: part 2/N: convert test_mobile_networks.js : v3

Address previous comment.  I also add a few more comments/checks to make the flow more clearer.  Please feel free to comment if you feel like more.  Thank you :)

Comment 18

[Vicamo Yang [:vicamo][:vyang]]

Attachment: part 3/N: convert test_mobile_voice_state.js : v2

Re-gen from hg. No change.

Comment 19

[Vicamo Yang [:vicamo][:vyang]]

Attachment: part 4/N: convert test_mobile_data_state.js : v2

Re-gen from hg. No change.

Comment 20

[Vicamo Yang [:vicamo][:vyang]]

Attachment: part 5/N: convert test_mobile_voice_location.js : v3

Re-gen from hg. No change.

Comment 21

[Vicamo Yang [:vicamo][:vyang]]

Attachment: part 6/N: convert test_mobile_data_location.js : v2

Re-gen from hg. No change.

Comment 22

[Vicamo Yang [:vicamo][:vyang]]

Attachment: part 7/N: convert test_mobile_operator_names.js : v2

Re-gen from hg. No change.

Comment 23

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Comment on attachment 8402513 [details] [diff] [review]
part 7/N: convert test_mobile_operator_names.js : v2

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

::: dom/mobileconnection/tests/marionette/test_mobile_operator_names_roaming.js
@@ +36,5 @@
> +    // We should not have voicechange here, but, yes, we do.
> +    .then(() => setEmulatorOperatorNamesAndWait("roaming", aLongName, aShortName))
> +
> +    .then(() => setEmulatorVoiceDataStateAndWait("voice", "roaming"))
> +    .then(() => check(aLongName, aShortName))

From the perspective of test(), we don't know in advance which value of "aLongName" and "aShortName" is going to be assigned. That is, it doesn't guarantee that network.voice.roaming is definitely false in check(). Please have one more argument 'aRoaming' is test() and check(). Thank you.

Comment 24

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Comment on attachment 8402506 [details] [diff] [review]
part 2/N: convert test_mobile_networks.js : v3

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

I like this!

Comment 25

[Vicamo Yang [:vicamo][:vyang]]

Attachment: part 7/N: convert test_mobile_operator_names.js : v3

Address previous comment and add one more case that aRoaming == true.

Comment 26

[Vicamo Yang [:vicamo][:vyang]]

marionette only: https://tbpl.mozilla.org/?tree=Try&rev=08538da2c20c

Comment 27

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Comment on attachment 8403225 [details] [diff] [review]
part 7/N: convert test_mobile_operator_names.js : v3

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

Thank you :)

Comment 28

[Vicamo Yang [:vicamo][:vyang]]

https://hg.mozilla.org/integration/b2g-inbound/rev/370b65e58512
https://hg.mozilla.org/integration/b2g-inbound/rev/d0493d130538
https://hg.mozilla.org/integration/b2g-inbound/rev/3d3335a00765
https://hg.mozilla.org/integration/b2g-inbound/rev/fb0c8c9f331a
https://hg.mozilla.org/integration/b2g-inbound/rev/d59cfd039be7
https://hg.mozilla.org/integration/b2g-inbound/rev/9b9144df3a8a
https://hg.mozilla.org/integration/b2g-inbound/rev/2384ee2298f0

Comment 30

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/370b65e58512
https://hg.mozilla.org/mozilla-central/rev/d0493d130538
https://hg.mozilla.org/mozilla-central/rev/3d3335a00765
https://hg.mozilla.org/mozilla-central/rev/fb0c8c9f331a
https://hg.mozilla.org/mozilla-central/rev/d59cfd039be7
https://hg.mozilla.org/mozilla-central/rev/9b9144df3a8a
https://hg.mozilla.org/mozilla-central/rev/2384ee2298f0

Comment 31

[Vicamo Yang [:vicamo][:vyang]]

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #12)
> Created attachment 8399241 [details] [review]
> Github PR for external/qemu: GSM cell ID mask should be 0xFFFFFFF

Merged on Github: https://github.com/mozilla-b2g/platform_external_qemu/commit/35fe717e430548b3de058d63c73c8357ec41520a

Comment 32

[Ken Chang[:kenkai]]

Put this bug into backlog.

Comment 33

[Edgar Chen [:edgar]]

test_mobile_roaming_preference.js will be rewrote with Promise in bug 898445.

Comment 34

[Edgar Chen [:edgar]]

(In reply to Edgar Chen [:edgar][:echen] from comment #33)
> test_mobile_roaming_preference.js will be rewrote with Promise in bug 898445.

And test_mobile_preferred_network_type.js will be done in bug 898445, too.

Comment 35

[Jessica Jong [:jessica] (inactive)]

Attachment: part 8/N: convert test_mobile_data_connection.js : v1

Comment 36

[Vicamo Yang [:vicamo][:vyang]]

Comment on attachment 8410151 [details] [diff] [review]
part 8/N: convert test_mobile_data_connection.js : v1

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

"Bug 979134 (follow-up): ..."

::: dom/mobileconnection/tests/marionette/test_mobile_data_connection.js
@@ +10,5 @@
> +  if (mobileConnection.data.connected == connected) {
> +    log("data.connected is now " + mobileConnection.data.connected);
> +    deferred.resolve();
> +    return;
> +  }

Don't create a Promise if not necessary.  You can simply return nothing if it's already connected.

  if (mobileConnection.data.connected == connected) {
    return;
  }

  let deferred = Promise.defer();
  ...

@@ +15,3 @@
>  
> +  return Promise.resolve()
> +    .then(() => waitForManagerEvent("datachange"))

|waitForManagerEvent()| always returns a Promise, so you can have:

  return waitForManagerEvent("datachange")
    .then(....);

@@ +47,3 @@
>  
> +  return Promise.resolve()
> +    .then(() => setDataEnabledAndWait(true));

nit: just |return setDataEnabledAndWait(true);|

@@ +55,5 @@
> +  // When data registration is unregistered, all data calls will be
> +  // automatically deactivated.
> +  return Promise.resolve()
> +    .then(() => runEmulatorCmdSafe("gsm data unregistered"))
> +    .then(() => waitForDataState(false));

You must install event listener *before* issuing commands that trigger the event.  Any reason you can't reuse |setEmulatorVoiceDataStateAndWait('data', 'unregistered')| here?

@@ +65,5 @@
> +  // When data registration is registered, data call will be (re)activated by
> +  // gecko if ril.data.enabled is set to true.
> +  return Promise.resolve()
> +    .then(() => runEmulatorCmdSafe("gsm data home"))
> +    .then(() => waitForDataState(true));

ditto.  |return setEmulatorVoiceDataStateAndWait('data', 'home');|

@@ +85,5 @@
>  
>    // Data should be re-connected as we enabled data roaming.
> +  return Promise.resolve()
> +    .then(() => setDataRoamingEnabled(true))
> +    .then(() => waitForDataState(true));

Ditto.  Please add another API |setDataRoamingEnabledAndWait()|.  See |setDataEnabledAndWait()|.

@@ +93,4 @@
>    log("Turn data off.");
>  
> +  return Promise.resolve()
> +    .then(() => setDataEnabledAndWait(false));

nit: return setDataEnabledAndWait(false);

@@ +108,5 @@
> +    .then(function() {
> +      // Restore test environment.
> +      return Promise.resolve()
> +        .then(() => setEmulatorRoamingAndWait(false))
> +        .then(() => setDataRoamingEnabled(false));

nit: just do:

  // Restore test environment.
  .then(() => setEmulatorRoamingAndWait(false))
  .then(() => setDataRoamingEnabled(false));

Comment 37

[Edgar Chen [:edgar]]

(In reply to Edgar Chen [:edgar][:echen] from comment #34)
> (In reply to Edgar Chen [:edgar][:echen] from comment #33)
> > test_mobile_roaming_preference.js will be rewrote with Promise in bug 898445.
> 
> And test_mobile_preferred_network_type.js will be done in bug 898445, too.

I am going to rewrite test_mobile_mmi.js with Promise in bug 929701.

Comment 38

[Jessica Jong [:jessica] (inactive)]

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #36)
> Comment on attachment 8410151 [details] [diff] [review]
> part 8/N: convert test_mobile_data_connection.js : v1
> 
> Review of attachment 8410151 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> "Bug 979134 (follow-up): ..."
> 
> ::: dom/mobileconnection/tests/marionette/test_mobile_data_connection.js
> @@ +10,5 @@
> > +  if (mobileConnection.data.connected == connected) {
> > +    log("data.connected is now " + mobileConnection.data.connected);
> > +    deferred.resolve();
> > +    return;
> > +  }
> 
> Don't create a Promise if not necessary.  You can simply return nothing if
> it's already connected.
> 
>   if (mobileConnection.data.connected == connected) {
>     return;
>   }
> 
>   let deferred = Promise.defer();
>   ...

Got it, will revise this, thank you.

> 
> @@ +15,3 @@
> >  
> > +  return Promise.resolve()
> > +    .then(() => waitForManagerEvent("datachange"))
> 
> |waitForManagerEvent()| always returns a Promise, so you can have:
> 
>   return waitForManagerEvent("datachange")
>     .then(....);

Got it, will revise this, thank you.

> 
> @@ +47,3 @@
> >  
> > +  return Promise.resolve()
> > +    .then(() => setDataEnabledAndWait(true));
> 
> nit: just |return setDataEnabledAndWait(true);|

Will do, thank you.

> 
> @@ +55,5 @@
> > +  // When data registration is unregistered, all data calls will be
> > +  // automatically deactivated.
> > +  return Promise.resolve()
> > +    .then(() => runEmulatorCmdSafe("gsm data unregistered"))
> > +    .then(() => waitForDataState(false));
> 
> You must install event listener *before* issuing commands that trigger the
> event.  Any reason you can't reuse |setEmulatorVoiceDataStateAndWait('data',
> 'unregistered')| here?

Yes, I can make use of 'setEmulatorVoiceDataStateAndWait()'.
About the event listener, my idea was to check the state before listening to events, cause there might not be event change in some cases (for this case and all the cases in this test file there will be event change though). I will reconsider this, thank you.

> 
> @@ +65,5 @@
> > +  // When data registration is registered, data call will be (re)activated by
> > +  // gecko if ril.data.enabled is set to true.
> > +  return Promise.resolve()
> > +    .then(() => runEmulatorCmdSafe("gsm data home"))
> > +    .then(() => waitForDataState(true));
> 
> ditto.  |return setEmulatorVoiceDataStateAndWait('data', 'home');|

Yes, I can make use of 'setEmulatorVoiceDataStateAndWait()', thank you.

> 
> @@ +85,5 @@
> >  
> >    // Data should be re-connected as we enabled data roaming.
> > +  return Promise.resolve()
> > +    .then(() => setDataRoamingEnabled(true))
> > +    .then(() => waitForDataState(true));
> 
> Ditto.  Please add another API |setDataRoamingEnabledAndWait()|.  See
> |setDataEnabledAndWait()|.

Mmm.. data roaming setting is a little bit different from data enabled setting.. data roaming does not always change the data state, it depends on the data enabled state and roaming state to take effect, so should we consider all these or should we leave the tester to decide what to wait for?

> 
> @@ +93,4 @@
> >    log("Turn data off.");
> >  
> > +  return Promise.resolve()
> > +    .then(() => setDataEnabledAndWait(false));
> 
> nit: return setDataEnabledAndWait(false);

Will do, thank you.

> 
> @@ +108,5 @@
> > +    .then(function() {
> > +      // Restore test environment.
> > +      return Promise.resolve()
> > +        .then(() => setEmulatorRoamingAndWait(false))
> > +        .then(() => setDataRoamingEnabled(false));
> 
> nit: just do:
> 
>   // Restore test environment.
>   .then(() => setEmulatorRoamingAndWait(false))
>   .then(() => setDataRoamingEnabled(false));

Will do, thank you.

Comment 39

[Jessica Jong [:jessica] (inactive)]

Attachment: (follow-up): convert test_mobile_data_connection.js : v2

address review comment 36.

Comment 40

[Vicamo Yang [:vicamo][:vyang]]

Comment on attachment 8423593 [details] [diff] [review]
(follow-up): convert test_mobile_data_connection.js : v2

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

Two nits.  Thank you!

::: dom/mobileconnection/tests/marionette/test_mobile_data_connection.js
@@ +60,5 @@
> +
> +  let promises = [];
> +  promises.push(waitForManagerEvent("datachange"));
> +  promises.push(setDataRoamingEnabled(false));
> +  promises.push(setEmulatorRoamingAndWait(true));

Actually I think all that we need is:

  return setEmulatorRoamingAndWait(true)
    .then(() => checkOrWaitForDataState(false));

in this function, because data romaing should has been disabled and we have checked that in verifyInitialState().  And we don't need an extra |waitForManagerEvent("datachange")|, because it has been included in |setEmulatorRoamingAndWait(true)|.

@@ +73,5 @@
> +  log("Enable data roaming while roaming.");
> +
> +  let promises = [];
> +  promises.push(waitForManagerEvent("datachange"));
> +  promises.push(setDataRoamingEnabled(true));

Ditto.  Thanks to that |checkOrWaitForDataState(true)| function, we can have an equivalent but shorter version here:

  return setDataRoamingEnabled(true)
    .then(() => checkOrWaitForDataState(true));

Comment 41

[Jessica Jong [:jessica] (inactive)]

Attachment: (follow-up): convert test_mobile_data_connection.js : v3

Thanks, Vicamo.

Comment 42

[Jessica Jong [:jessica] (inactive)]

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

checkin-needed for attachment 8424568 [details] [diff] [review].

Comment 43

[Edgar Chen [:edgar]]

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

Comment 44

[Ed Morley [:emorley]]

Comment on attachment 8424568 [details] [diff] [review]
(follow-up): convert test_mobile_data_connection.js : v3

https://hg.mozilla.org/mozilla-central/rev/3d50702c465a

Comment 45

[Edgar Chen [:edgar]]

Attachment: (follow-up): convert test_call_barring_change_password.js, v1

Comment 46

[Edgar Chen [:edgar]]

Attachment: (follow-up): convert test_call_barring_get_option.js, v1

Comment 47

[Edgar Chen [:edgar]]

Attachment: (follow-up): convert test_call_barring_set_error.js, v1

Comment 48

[Edgar Chen [:edgar]]

Attachment: (follow-up): convert test_mobile_icc_change.js, v1

Comment 49

[Edgar Chen [:edgar]]

Attachment: (follow-up): convert test_last_known_network.js, v1

Comment 50

[Edgar Chen [:edgar]]

Attachment: (follow-up): convert test_last_known_network.js, v2

Keeps original comments.

Comment 51

[Edgar Chen [:edgar]]

Attachment: (follow-up): convert test_mobile_set_radio.js, v1

Comment 52

[Edgar Chen [:edgar]]

Attachment: (follow-up): convert test_mobile_signal_strength.js, v1

Comment 53

[Vicamo Yang [:vicamo][:vyang]]

Comment on attachment 8443281 [details] [diff] [review]
(follow-up): convert test_call_barring_change_password.js, v1

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

::: dom/mobileconnection/tests/marionette/test_call_barring_change_password.js
@@ +6,2 @@
>  
> +const TEST_DATA = [

Can we have an array for each entry? I think it will be more clear.

  // <pin>, <new pin>, <expected error>
  [null, '0000', 'InvalidPassword'],
  ['0000', null, 'InvalidPassword'],
  ...

@@ +11,5 @@
> +    options: {
> +      pin: null,
> +      newPin: '0000'
> +    },
> +    expecedError: "InvalidPassword"

'expectedError'

Comment 54

[Vicamo Yang [:vicamo][:vyang]]

Comment on attachment 8443296 [details] [diff] [review]
(follow-up): convert test_mobile_icc_change.js, v1

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

::: dom/mobileconnection/tests/marionette/test_mobile_icc_change.js
@@ +6,3 @@
>  
> +// The emulator's hard coded iccid value.
> +const ICCID = 89014103211118510720;

nit: "89014103211118510720"

Comment 55

[Edgar Chen [:edgar]]

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #53)
> Comment on attachment 8443281 [details] [diff] [review]
> (follow-up): convert test_call_barring_change_password.js, v1
> 
> Review of attachment 8443281 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> :::
> dom/mobileconnection/tests/marionette/test_call_barring_change_password.js
> @@ +6,2 @@
> >  
> > +const TEST_DATA = [
> 
> Can we have an array for each entry? I think it will be more clear.

Thanks for your review, Vicamo.
Will do this in next version. :)

Comment 56

[Edgar Chen [:edgar]]

Attachment: (follow-up): convert test_call_barring_change_password.js, v2

Address review comment #53:
1). Use an array for each test entry.
2). Correct typo: s/expecedError/expectedError/

Comment 57

[Vicamo Yang [:vicamo][:vyang]]

Comment on attachment 8443389 [details] [diff] [review]
(follow-up): convert test_call_barring_change_password.js, v2

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

::: dom/mobileconnection/tests/marionette/test_call_barring_change_password.js
@@ +42,5 @@
> +startTestCommon(function() {
> +  let promise = Promise.resolve();
> +  for (let i = 0; i < TEST_DATA.length; i++) {
> +    let data = TEST_DATA[i];
> +    promise = promise.then(testChangeCallBarringPassword.apply(null, data));

This invokes testChangeCallBarringPassword() multiple times in one loop, so it's probably wrong.

Comment 58

[Edgar Chen [:edgar]]

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #57)
> Comment on attachment 8443389 [details] [diff] [review]
> (follow-up): convert test_call_barring_change_password.js, v2
> 
> Review of attachment 8443389 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> :::
> dom/mobileconnection/tests/marionette/test_call_barring_change_password.js
> @@ +42,5 @@
> > +startTestCommon(function() {
> > +  let promise = Promise.resolve();
> > +  for (let i = 0; i < TEST_DATA.length; i++) {
> > +    let data = TEST_DATA[i];
> > +    promise = promise.then(testChangeCallBarringPassword.apply(null, data));
> 
> This invokes testChangeCallBarringPassword() multiple times in one loop, so
> it's probably wrong.

Ah, yes, should use bind() instead. :(

Comment 59

[Edgar Chen [:edgar]]

Attachment: (follow-up): convert test_call_barring_change_password.js, v3

Address review comment #57:
- Use bind instead

Comment 60

[Edgar Chen [:edgar]]

Attachment: (follow-up): convert test_mobile_icc_change.js, v2, r=vicamo

Address nit in comment #54.

Comment 61

[Edgar Chen [:edgar]]

Attachment: (follow-up): convert test_call_barring_change_password.js, v4

remove trailing space

Comment 62

[Edgar Chen [:edgar]]

Comment on attachment 8443542 [details] [diff] [review]
(follow-up): convert test_call_barring_change_password.js, v4

Hi Vicamo, could you help to review again? Thank you

Comment 63

[Vicamo Yang [:vicamo][:vyang]]

Comment on attachment 8443542 [details] [diff] [review]
(follow-up): convert test_call_barring_change_password.js, v4

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

Thank you!

::: dom/mobileconnection/tests/marionette/head.js
@@ +589,5 @@
> + */
> + function changeCallBarringPassword(aOptions) {
> +  let request = mobileConnection.changeCallBarringPassword(aOptions);
> +  return wrapDomRequestAsPromise(request)
> +    .then(null, () => { throw request.error });

nit: ';'

Comment 64

[Edgar Chen [:edgar]]

Try result: https://tbpl.mozilla.org/?tree=Try&rev=3059b02f1596

Comment 65

[Edgar Chen [:edgar]]

https://hg.mozilla.org/integration/b2g-inbound/rev/08f2cc21824a
https://hg.mozilla.org/integration/b2g-inbound/rev/8bb2d4f94f29
https://hg.mozilla.org/integration/b2g-inbound/rev/f5986a992460
https://hg.mozilla.org/integration/b2g-inbound/rev/abdd53cd4eee
https://hg.mozilla.org/integration/b2g-inbound/rev/9b909febb9c5
https://hg.mozilla.org/integration/b2g-inbound/rev/870166b18260
https://hg.mozilla.org/integration/b2g-inbound/rev/85ee59e6eb34

Comment 66

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/08f2cc21824a
https://hg.mozilla.org/mozilla-central/rev/8bb2d4f94f29
https://hg.mozilla.org/mozilla-central/rev/f5986a992460
https://hg.mozilla.org/mozilla-central/rev/abdd53cd4eee
https://hg.mozilla.org/mozilla-central/rev/9b909febb9c5
https://hg.mozilla.org/mozilla-central/rev/870166b18260
https://hg.mozilla.org/mozilla-central/rev/85ee59e6eb34