[B2G][Emulator] Support call barring

RESOLVED FIXED in Firefox 40

Status

Firefox OS
Emulator
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: edgar, Assigned: HoPang)

Tracking

unspecified
2.2 S10 (17apr)
x86
Linux
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(tracking-b2g:backlog, firefox40 fixed)

Details

Attachments

(4 attachments, 6 obsolete attachments)

(Reporter)

Description

4 years ago
Enhance emulator to support call barring.
(Reporter)

Updated

3 years ago
Depends on: 1083650
(Reporter)

Updated

3 years ago
Blocks: 1100774
(Assignee)

Updated

3 years ago
Assignee: nobody → bhsu
blocking-b2g: --- → backlog
(Assignee)

Comment 1

3 years ago
Created attachment 8575133 [details] [diff] [review]
Part 1: Restore the service class (ril_worker.js)

In this patch, I restore |serviceClass| on receiving the AT response of query facility locks, which is replaced when sending the query request.
Attachment #8575133 - Flags: review?(szchen)
Attachment #8575133 - Flags: review?(szchen) → review+
(Assignee)

Comment 2

3 years ago
Created attachment 8575141 [details] [diff] [review]
Part 2: Update the existing testcases

In this patch, I removed the for loop, and use a chained promise structure to execute the test functions directly. By doing this, I think the code can be more readable. In addition to the structure changes, I also remove the cases should successfully pass, since those cases are tested massively in a new testcase.
Attachment #8575141 - Flags: review?(szchen)
(Assignee)

Comment 3

3 years ago
Created attachment 8575146 [details] [diff] [review]
Part 3: Create a new testcase

In many places in this patch, "CBP' are used as the abbreviation for "Call Barring Program". I am not quite sure whether it makes the code hard to be read. If there is any suggestion, I'll be very appreciated.
Attachment #8575146 - Flags: review?(szchen)
Comment on attachment 8575141 [details] [diff] [review]
Part 2: Update the existing testcases

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

::: dom/mobileconnection/tests/marionette/test_call_barring_get_option.js
@@ +3,5 @@
>  
>  MARIONETTE_TIMEOUT = 60000;
>  MARIONETTE_HEAD_JS = "head.js";
>  
> +function testGetCallBarringOption(aExpectedError, aOptions) {

Add a log for |aOptions|. That will help us identify what we are testing now.

@@ +8,3 @@
>    return getCallBarringOption(aOptions)
>      .then(function resolve(aResult) {
> +      ok(!aExpectedError, "should be rejected");

Why not just use |false|?
Are you going to pass a falsy value for aExpectedError to test the successful case?

::: dom/mobileconnection/tests/marionette/test_call_barring_set_error.js
@@ +3,5 @@
>  
>  MARIONETTE_TIMEOUT = 60000;
>  MARIONETTE_HEAD_JS = "head.js";
>  
> +function testSetCallBarringOption(aExpectedError, aOptions) {

Add a log for |aOptions|. That will help us identify what we are testing now.

@@ +6,5 @@
>  
> +function testSetCallBarringOption(aExpectedError, aOptions) {
> +  return setCallBarringOption(aOptions)
> +    .then(function resolve() {
> +      ok(!aExpectedError, "should be rejected");

Why not just use |ok(false)|?
Attachment #8575141 - Flags: review?(szchen) → review+
(Assignee)

Comment 5

3 years ago
Created attachment 8575162 [details] [diff] [review]
Part 3: Create a new testcase (V2)

On finishing of this testcase, we should disable all call barring programs.
Attachment #8575146 - Attachment is obsolete: true
Attachment #8575146 - Flags: review?(szchen)
Attachment #8575162 - Flags: review?(szchen)
Comment on attachment 8575162 [details] [diff] [review]
Part 3: Create a new testcase (V2)

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

It's ok but I'd like to review the revised version again.

::: dom/mobileconnection/tests/marionette/test_call_barring_basic_operations.js
@@ +12,5 @@
> +const AI = MozMobileConnection.CALL_BARRING_PROGRAM_ALL_INCOMING;
> +const IR = MozMobileConnection.CALL_BARRING_PROGRAM_INCOMING_ROAMING;
> +let incomingCBPs = [null, AI, IR];
> +
> +function setCBP(aProgram, aServiceClass, aEnabled, aPassword = "0000") {

We always use |serviceClass = 1| and |passsword = "0000"| in this file.  It's better to remove them from parameter list.
function setCBP(aProgram, aEnabled) {

@@ +15,5 @@
> +
> +function setCBP(aProgram, aServiceClass, aEnabled, aPassword = "0000") {
> +  return setCallBarringOption({
> +    program: aProgram,
> +    serviceClass: aServiceClass,

serviceClass: 1,

@@ +17,5 @@
> +  return setCallBarringOption({
> +    program: aProgram,
> +    serviceClass: aServiceClass,
> +    enabled: aEnabled,
> +    password: aPassword

password: "0000"

@@ +21,5 @@
> +    password: aPassword
> +  });
> +}
> +
> +function getCBP(aProgram, aServiceClass, aEnabled) {

remove aServiceClass

@@ +22,5 @@
> +  });
> +}
> +
> +function getCBP(aProgram, aServiceClass, aEnabled) {
> +  return getCallBarringOption({program: aProgram, serviceClass: aServiceClass})

serviceClass: 1

@@ +25,5 @@
> +function getCBP(aProgram, aServiceClass, aEnabled) {
> +  return getCallBarringOption({program: aProgram, serviceClass: aServiceClass})
> +    .then(result => {
> +      is( result.program, aProgram, "Program" );
> +      is( result.serviceClass, aServiceClass, "ServiceClass");

use 1 for serviceClass

@@ +33,5 @@
> +
> +function checkCBP(aOutgoingCBP, aIncomingCBP) {
> +  return Promise.resolve()
> +    // Check outgoing call barring programs
> +    .then(() => getCBP(AO, 1, aOutgoingCBP === AO ? true : false))

|aOutgoingCBP === AO|
It's already a boolean value. You don't need |? true : flase| this part
Fix this and all the followings

@@ +42,5 @@
> +    .then(() => getCBP(AI, 1, aIncomingCBP === AI ? true : false))
> +    .then(() => getCBP(IR, 1, aIncomingCBP === IR ? true : false));
> +}
> +
> +function resetCBP(aOutgoingCBP, aIncomingCBP) {

When you are doing resetCBP, you call to setCBP.  It's kind of recursive and weird.
I mean, you are going to verify your setCBP().  However, the test relies on resetCBP and setCBP which actually does not guarantee correct.

@@ +61,5 @@
> +function test(aOutgoingCBP, aIncomingCBP) {
> +  let promise = Promise.resolve();
> +
> +  // Test outoging
> +  for (let i = 1; i < outgoingCBPs.length; ++i) {

for..of

@@ +70,5 @@
> +      .then(() => checkCBP(CBP, aIncomingCBP));
> +  }
> +
> +  // Test incoming
> +  for (let i = 1; i < incomingCBPs.length; ++i) {

for..of

@@ +85,5 @@
> +// Start tests
> +startTestCommon(function() {
> +  let promise = Promise.resolve();
> +
> +  for (let i = 0; i < outgoingCBPs.length; ++i) {

use |for..of|
for (let outgoingCBP of outgoingCBPs)

@@ +86,5 @@
> +startTestCommon(function() {
> +  let promise = Promise.resolve();
> +
> +  for (let i = 0; i < outgoingCBPs.length; ++i) {
> +    for (let j = 0; j < outgoingCBPs.length; ++j) {

for..of

@@ +88,5 @@
> +
> +  for (let i = 0; i < outgoingCBPs.length; ++i) {
> +    for (let j = 0; j < outgoingCBPs.length; ++j) {
> +      let outgoingCBP = outgoingCBPs[i];
> +      let incomingCBP = incomingCBPs[j];

then you don't need these two lines.

@@ +96,5 @@
> +
> +  return promise
> +    .then(() => resetCBP(null, null))
> +    .catch((aError) => {
> +      return resetCBP(null, null).then(() => Promise.reject(aError));

It's the last one, I don't think that you need to reject it.
Let's reorder the code to avoid two reset CBP.

return promise
.catch(aError => {
 ok(false, ...)  // just let test faile
})
.then(() => resetCBP(null, null));
Attachment #8575162 - Flags: review?(szchen) → review-
Comment on attachment 8575162 [details] [diff] [review]
Part 3: Create a new testcase (V2)

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

::: dom/mobileconnection/tests/marionette/test_call_barring_basic_operations.js
@@ +15,5 @@
> +
> +function setCBP(aProgram, aServiceClass, aEnabled, aPassword = "0000") {
> +  return setCallBarringOption({
> +    program: aProgram,
> +    serviceClass: aServiceClass,

Have a descriptive constant for serviceClass and use it here.
ex:
const SERVICE_CLASS_VOICE = 1;
(Assignee)

Comment 8

3 years ago
Created attachment 8575783 [details] [review]
[external/qemu] pull request 134
Attachment #8575783 - Flags: review?(echen)
blocking-b2g: backlog → ---
tracking-b2g: --- → backlog
(Assignee)

Comment 9

3 years ago
Created attachment 8579290 [details] [diff] [review]
Part 1: Restore the service class (ril_worker.js)

Modify the commit message.
Attachment #8575133 - Attachment is obsolete: true
Attachment #8579290 - Flags: review+
(Assignee)

Comment 10

3 years ago
Created attachment 8579291 [details] [diff] [review]
Part 2: Update the existing testcases
Attachment #8575141 - Attachment is obsolete: true
Attachment #8579291 - Flags: review+
(Assignee)

Comment 11

3 years ago
Created attachment 8579292 [details] [diff] [review]
Part 3: Create a new testcase (V3)
Attachment #8575162 - Attachment is obsolete: true
(Assignee)

Comment 12

3 years ago
Comment on attachment 8579292 [details] [diff] [review]
Part 3: Create a new testcase (V3)

This testcase doesn't guarantee that setting outgoing and incoming call barring programs are independent. However, they don't and shouldn't affect each other in the real world, so I think it's okay not to test the independency here.
Attachment #8579292 - Flags: review?(szchen)
(Reporter)

Comment 13

3 years ago
Comment on attachment 8575783 [details] [review]
[external/qemu] pull request 134

I have left some comments on github, please address them and request review again. Thank you.
Attachment #8575783 - Flags: review?(echen)
Comment on attachment 8579292 [details] [diff] [review]
Part 3: Create a new testcase (V3)

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

I expect it could be better.  Let's have another round of review.
Also, I'm not a fan of acronym 'CBP'.  If you really want to use it, maybe adding a cooment in the beginning to explain it is better.

Thank you.

::: dom/mobileconnection/tests/marionette/test_call_barring_basic_operations.js
@@ +46,5 @@
> +      password: "0000" // The dafault call barring password of the emulator
> +    }))
> +
> +    // Check whether the result of this operation meets our expectation
> +    .then(() => _getCBP(aProgram, aEnabled));

Looks like the check is redundant.
In the end of setCBP, you always have a thorough test for all programs.

@@ +49,5 @@
> +    // Check whether the result of this operation meets our expectation
> +    .then(() => _getCBP(aProgram, aEnabled));
> +}
> +
> +function setCBP(aCBP, aCBPs) {

Naming is not clear enough. I expect something like..
setCBP(aActiveProgram, aAllPrograms)

Also, I don't like _getCBP, _setCBP, setCBP.
IMO, we'd better change it to getCBP, setCBP, and find a better name for original setCBP.

@@ +54,5 @@
> +  let promise = Promise.resolve();
> +
> +  if (aCBP !== null) {
> +    promise = promise
> +      .then(() => _setCBP(aCBP, true));

put it in the same line if width is allowed

@@ +60,5 @@
> +    // Deactive all barring programs in |aCBPs|.
> +    for(let _CBP of aCBPs) {
> +      let CBP = _CBP;
> +      promise = promise
> +        .then(() => _setCBP(CBP, false));

put it in the same line if width is allowed

@@ +61,5 @@
> +    for(let _CBP of aCBPs) {
> +      let CBP = _CBP;
> +      promise = promise
> +        .then(() => _setCBP(CBP, false));
> +    }

I think this should work.

promise = aCBPs.reduce(function(previousPromise, program) {
  return previousPromise.then(() => _setCBP(program, false));
}, promise);

It might take you some time to figure out the code if you're not familiar with functional programming.
However, once you see this kind of code, it will be very easy for you to understand the pattern next time.
Thus, I prefer the above usage. Moreover, it avoids creating another scope variable |CBP| and have both |CBP| and |_CBP| that lead to confusion.

@@ +69,5 @@
> +  for(let _CBP of aCBPs) {
> +    let CBP = _CBP;
> +    promise = promise
> +      .then(() => _getCBP(CBP, CBP === aCBP));
> +  }

You could apply the similar trick here.

@@ +74,5 @@
> +
> +  return promise;
> +}
> +
> +function testCBP(aCBPs) {

The purpose of this function is not so clear.
I'll suggest you break it into two functions:

// add comment here if the function name is not clear enough
function testSetCBPs(originalCBP, aAllCBPs)

// also, provide a comment here
function testXXX(aAllCBPs) {
  promise = Promise.resolve();

  promise = aAllCBPs.reduce(function(previousPromise, program) {
    return previousPromise.then(() => _testSetCBPs(program, aAllCBPs));
  }, promise);

 return promise;
}

@@ +98,5 @@
> +}
> +
> +// Start tests
> +startTestCommon(function() {
> +  let promise = Promise.resolve()

return Promise.resolve()

@@ +102,5 @@
> +  let promise = Promise.resolve()
> +    .then(() => setCBP(null, outgoingCBPs))
> +    .then(() => setCBP(null, incomingCBPs));
> +
> +  promise = promise

Just chain all |.then| together

@@ +108,5 @@
> +    .then(() => testCBP(outgoingCBPs))
> +    .then(() => log("=== Test incoming call barring programs ==="))
> +    .then(() => testCBP(incomingCBPs));
> +
> +  return promise

Just chain all |.then| together

@@ +109,5 @@
> +    .then(() => log("=== Test incoming call barring programs ==="))
> +    .then(() => testCBP(incomingCBPs));
> +
> +  return promise
> +    .catch((aError) => ok(false, "promise rejects during test: " + aError))

.catch(aError => ...)
() is not needed.
Attachment #8579292 - Flags: review?(szchen) → review-
(Assignee)

Comment 15

3 years ago
Created attachment 8581448 [details] [diff] [review]
Part 3: Create a new testcase (V4)
Attachment #8579292 - Attachment is obsolete: true
Attachment #8581448 - Flags: review?(szchen)
Comment on attachment 8581448 [details] [diff] [review]
Part 3: Create a new testcase (V4)

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

Good.
r=me with comment fixed or answered.

::: dom/mobileconnection/tests/marionette/test_call_barring_basic_operations.js
@@ +66,5 @@
> +
> +  return promise;
> +}
> +
> +function testSingleProgram (aOriginProgram, aOriginPrograms, aTargetPrograms) {

Second parameter looks like |aAllPrograms| that you want to verify.  Maybe put it at the third place is better.

@@ +67,5 @@
> +  return promise;
> +}
> +
> +function testSingleProgram (aOriginProgram, aOriginPrograms, aTargetPrograms) {
> +  let promise = setProgram(aOriginProgram, aOriginProgram);

What's the second parameter? should it be |true|?

@@ +83,5 @@
> +
> +  return aPrograms.reduce((previousPromise, program) => {
> +    return previousPromise
> +      .then(() => {
> +        targetPrograms.shift();

Why do we need a shift?
Why don't we test all kinds of targetPrograms?

@@ +84,5 @@
> +  return aPrograms.reduce((previousPromise, program) => {
> +    return previousPromise
> +      .then(() => {
> +        targetPrograms.shift();
> +        return testSingleProgram(program, aPrograms, targetPrograms);

In my mind, this like should be something like...
testSingleProgram(program, aPrograms, aPrograms.slice());
Attachment #8581448 - Flags: review?(szchen) → review+
(Assignee)

Comment 17

3 years ago
Comment on attachment 8581448 [details] [diff] [review]
Part 3: Create a new testcase (V4)

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

::: dom/mobileconnection/tests/marionette/test_call_barring_basic_operations.js
@@ +67,5 @@
> +  return promise;
> +}
> +
> +function testSingleProgram (aOriginProgram, aOriginPrograms, aTargetPrograms) {
> +  let promise = setProgram(aOriginProgram, aOriginProgram);

Thanks for pointing this out, it's certainly a mistake.

@@ +83,5 @@
> +
> +  return aPrograms.reduce((previousPromise, program) => {
> +    return previousPromise
> +      .then(() => {
> +        targetPrograms.shift();

Since in |testSingleProgram(...)|, we test all the program changes between |aOriginProgram| and every program in |aTargetPrograms|, so |aOriginProgram| can be removed from the program list after tested by |testSingleProgram(...)|. Otherwise, when we later invoke |testSingleProgram(...)| with another program as |aOriginProgram|, the program changes with the previous |aOriginProgram| will be tested again, which are unnecessary.
(Assignee)

Comment 18

3 years ago
Created attachment 8581532 [details] [diff] [review]
Part 3: Create a new testcase (V5)
Attachment #8581532 - Flags: review+
(Assignee)

Updated

3 years ago
Attachment #8581448 - Attachment is obsolete: true
(Assignee)

Comment 19

3 years ago
Comment on attachment 8575783 [details] [review]
[external/qemu] pull request 134

Hi, Edgar

I've update the pull request, do you mind reviewing it again?
Attachment #8575783 - Flags: review?(echen)
(Reporter)

Comment 20

3 years ago
Comment on attachment 8575783 [details] [review]
[external/qemu] pull request 134

Looks good, r=me with comments on github addressed. Thank you.
Attachment #8575783 - Flags: review?(echen) → review+
(Assignee)

Updated

3 years ago
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/1eb63f18209c
https://hg.mozilla.org/mozilla-central/rev/8f0c0886ac43
https://hg.mozilla.org/mozilla-central/rev/47fbd6a8b249
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox40: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → 2.2 S10 (17apr)
(Assignee)

Updated

3 years ago
Duplicate of this bug: 1100774
You need to log in before you can comment on or make changes to this bug.