Closed Bug 979134 Opened 6 years ago Closed 6 years ago

Rewrite mobileconnection test cases with Promise

Categories

(Firefox OS Graveyard :: RIL, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(tracking-b2g:backlog)

RESOLVED FIXED
2.0 S5 (4july)
tracking-b2g backlog

People

(Reporter: vicamo, Assigned: edgar)

References

Details

Attachments

(16 files, 18 obsolete files)

62 bytes, text/x-github-pull-request
hsinyi
: review+
Details | Review
3.75 KB, patch
vicamo
: review+
Details | Diff | Splinter Review
16.55 KB, patch
hsinyi
: review+
Details | Diff | Splinter Review
9.69 KB, patch
vicamo
: review+
Details | Diff | Splinter Review
8.04 KB, patch
vicamo
: review+
Details | Diff | Splinter Review
6.82 KB, patch
vicamo
: review+
Details | Diff | Splinter Review
6.79 KB, patch
vicamo
: review+
Details | Diff | Splinter Review
16.40 KB, patch
hsinyi
: review+
Details | Diff | Splinter Review
14.75 KB, patch
jessica
: review+
Details | Diff | Splinter Review
4.84 KB, patch
vicamo
: review+
Details | Diff | Splinter Review
6.96 KB, patch
vicamo
: review+
Details | Diff | Splinter Review
1.98 KB, patch
vicamo
: review+
Details | Diff | Splinter Review
6.06 KB, patch
vicamo
: review+
Details | Diff | Splinter Review
11.06 KB, patch
vicamo
: review+
Details | Diff | Splinter Review
3.34 KB, patch
edgar
: review+
Details | Diff | Splinter Review
5.40 KB, patch
vicamo
: review+
Details | Diff | Splinter Review
+++ This bug was initially created as a clone of Bug #957917 comment 37 +++
Assignee: nobody → vyang
Attachment #8399088 - Flags: review?(htsai)
Attachment #8399089 - Flags: review?(htsai)
Attachment #8399090 - Flags: review?(htsai)
Attachment #8399088 - Attachment description: part 1/N: share wrapDomRequestAsPromise → part 2/N: convert test_mobile_networks.js
Attachment #8399089 - Attachment description: part 2/N: convert test_mobile_networks.js → part 3/N: convert test_mobile_voice_state.js
Attachment #8399090 - Attachment description: part 3/N: convert test_mobile_voice_state.js → part 4/N: convert test_mobile_data_state.js
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.
Attachment #8399088 - Flags: review?(htsai)
Attachment #8399093 - Flags: review?(htsai)
Attachment #8399094 - Flags: review?(htsai)
add documentation.
Attachment #8399091 - Attachment is obsolete: true
Attachment #8399236 - Flags: review?(htsai)
1) documentation
2) correct reject parameter of selectNetwork & selectNetworkAutomatically
Attachment #8399088 - Attachment is obsolete: true
Attachment #8399238 - Flags: review?(htsai)
1) remove @param lines for getEmulatorGsmLocation() because it takes no arguments,
2) correct cid parsing in getEmulatorGsmLocation().
Attachment #8399093 - Attachment is obsolete: true
Attachment #8399093 - Flags: review?(htsai)
Attachment #8399239 - Flags: review?(htsai)
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.
Attachment #8399240 - Flags: review?(htsai)
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.
Whiteboard: [leave open]
Attachment #8399236 - Flags: review?(htsai) → review+
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
Attachment #8399238 - Flags: review?(htsai)
Attachment #8399089 - Flags: review?(htsai) → review+
Attachment #8399090 - Flags: review?(htsai) → review+
Attachment #8399239 - Flags: review?(htsai) → review+
Attachment #8399094 - Flags: review?(htsai) → review+
Re-gen from hg. No change.
Attachment #8399236 - Attachment is obsolete: true
Attachment #8402502 - Flags: review+
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 :)
Attachment #8399238 - Attachment is obsolete: true
Attachment #8402506 - Flags: review?(htsai)
Re-gen from hg. No change.
Attachment #8399089 - Attachment is obsolete: true
Attachment #8402508 - Flags: review+
Re-gen from hg. No change.
Attachment #8399090 - Attachment is obsolete: true
Attachment #8402510 - Flags: review+
Re-gen from hg. No change.
Attachment #8399239 - Attachment is obsolete: true
Attachment #8402511 - Flags: review+
Re-gen from hg. No change.
Attachment #8399094 - Attachment is obsolete: true
Attachment #8402512 - Flags: review+
Re-gen from hg. No change.
Attachment #8399240 - Attachment is obsolete: true
Attachment #8399240 - Flags: review?(htsai)
Attachment #8402513 - Flags: review?(htsai)
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.
Attachment #8402513 - Flags: review?(htsai)
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!
Attachment #8402506 - Flags: review?(htsai) → review+
Address previous comment and add one more case that aRoaming == true.
Attachment #8402513 - Attachment is obsolete: true
Attachment #8403225 - Flags: review?(htsai)
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 :)
Attachment #8403225 - Flags: review?(htsai) → review+
Attachment #8399241 - Flags: review?(htsai) → review+
Duplicate of this bug: 964680
(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
Put this bug into backlog.
blocking-b2g: --- → backlog
test_mobile_roaming_preference.js will be rewrote with Promise in bug 898445.
(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 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));
Attachment #8410151 - Flags: review?(vyang)
(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.
(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.
address review comment 36.
Attachment #8410151 - Attachment is obsolete: true
Attachment #8423593 - Flags: review?(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));
Attachment #8423593 - Flags: review?(vyang) → review+
Thanks, Vicamo.
Attachment #8423593 - Attachment is obsolete: true
Attachment #8424568 - Flags: review+
Comment on attachment 8424568 [details] [diff] [review]
(follow-up): convert test_mobile_data_connection.js : v3

https://hg.mozilla.org/mozilla-central/rev/3d50702c465a
Depends on: 1024374
Attachment #8443281 - Flags: review?(vyang)
Attachment #8443291 - Flags: review?(vyang)
Attachment #8443294 - Flags: review?(vyang)
Attachment #8443296 - Flags: review?(vyang)
Keeps original comments.
Attachment #8443323 - Attachment is obsolete: true
Attachment #8443325 - Flags: review?(vyang)
Attachment #8443326 - Flags: review?(vyang)
Attachment #8443327 - Flags: review?(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'
Attachment #8443281 - Flags: review?(vyang) → review-
Attachment #8443291 - Flags: review?(vyang) → review+
Attachment #8443294 - Flags: review?(vyang) → review+
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"
Attachment #8443296 - Flags: review?(vyang) → review+
Attachment #8443325 - Flags: review?(vyang) → review+
Attachment #8443326 - Flags: review?(vyang) → review+
Attachment #8443327 - Flags: review?(vyang) → review+
(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. :)
Address review comment #53:
1). Use an array for each test entry.
2). Correct typo: s/expecedError/expectedError/
Attachment #8443281 - Attachment is obsolete: true
Attachment #8443389 - Flags: review?(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.
Attachment #8443389 - Flags: review?(vyang) → review-
(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. :(
Address review comment #57:
- Use bind instead
Attachment #8443389 - Attachment is obsolete: true
Address nit in comment #54.
Attachment #8443296 - Attachment is obsolete: true
Attachment #8443537 - Flags: review+
remove trailing space
Attachment #8443531 - Attachment is obsolete: true
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
Attachment #8443542 - Flags: review?(vyang)
Assignee: vyang → echen
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: ';'
Attachment #8443542 - Flags: review?(vyang) → review+
Whiteboard: [leave open]
blocking-b2g: backlog → ---
You need to log in before you can comment on or make changes to this bug.