Szu-Yu Chen [:aknow] (inactive) Reporter
	Description • 11 years ago

As Bug 887690 Comment 28, file another bug for test cases.

	Szu-Yu Chen [:aknow] (inactive) Reporter
	Comment 3 • 11 years ago

In #2,
Merge two test cases (1) exit by normal call, (2) exit by user request into 1 file.
Add a new test for sending the enter event twice.

In #3,
Correct the test manifest.

	Hsin-Yi Tsai (she/her) [:hsinyi]
	Comment 4 • 11 years ago

Comment on attachment 800030 [details] [diff] [review]
#3 Test emergency callback mode (marionette)

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

::: dom/network/tests/marionette/manifest.ini
@@ +16,5 @@
>  [test_mobile_roaming_preference.js]
>  [test_call_barring_get_option.js]
>  [test_call_barring_set_error.js]
>  [test_call_barring_change_password.js]
> +[test_mobile_cdma_ecm.js]

Due to Bug 891707, let's disable the cdma marionette tests for now.

::: dom/network/tests/marionette/test_mobile_cdma_ecm.js
@@ +22,5 @@
> +let iccManager = navigator.mozIccManager;
> +let connection = navigator.mozMobileConnection;
> +let telephony = navigator.mozTelephony;
> +ok(connection instanceof MozMobileConnection,
> +   "connection is instanceof " + connection.constructor);

Why checking only mobileconnection?

@@ +25,5 @@
> +ok(connection instanceof MozMobileConnection,
> +   "connection is instanceof " + connection.constructor);
> +
> +let outgoing;
> +let eventChecked;

let eventChecked = true;

@@ +51,5 @@
> +
> +function setRadioEnabled(enabled, callback) {
> +  changeSetting("ril.radio.disabled", !enabled, function() {
> +    iccManager.addEventListener("cardstatechange", function handler() {
> +      // Wait card state change to "ready" when turned on radio

Wait until card state changes to "ready" after turning on radio.

@@ +52,5 @@
> +function setRadioEnabled(enabled, callback) {
> +  changeSetting("ril.radio.disabled", !enabled, function() {
> +    iccManager.addEventListener("cardstatechange", function handler() {
> +      // Wait card state change to "ready" when turned on radio
> +      // Wait card state chagne to "not-ready" when turned off radio

ditto.

@@ +68,5 @@
> +    let resultArray = result[0].split(' ');
> +    let tech = resultArray[0];
> +    let prefer = resultArray[1];
> +
> +    log("Current Technology is " + tech);

nit:s/Technology/technology

@@ +82,5 @@
> +      callback();
> +      return;
> +    }
> +
> +    // turn off radio

nit: s/turn/Turn & place a period at the end of the line, here and the other places.

@@ +88,5 @@
> +      // Switch technology via console command
> +      runEmulatorCmd("modem tech " + technology, function(result) {
> +        log("Switch Technology to " + technology + ": " + result);
> +        is(result[0], "OK");
> +        // turn on radio

nit: s/turn/Turn

@@ +95,5 @@
> +    });
> +  });
> +}
> +
> +function makeSureNoCalls(callback) {

Use the console command 'gsm clear.' You don't need this function then.

@@ +226,5 @@
> +
> +function setNextEmergencyCbModeCheck(active, timeoutMs) {
> +  eventChecked = false;
> +  connection.onemergencycbmodechange = function(event) {
> +    log("[ECM] Received emergencycbmodechange: " + event.active);

I think what we want here is to make sure we've received |connection.onemergencycbmodechange| before nextCase/nextTest. However, the patch doesn't handle that carefully enough. 

Imagine that the execution is moved from test_enter_twice.case1 to test_enter_twice.case2 in hangUp() even we don't receive 'emergencycbmodechange' event.

@@ +246,5 @@
> +    nextStep();
> +  };
> +  request.onerror = function() {
> +    ok(false, "request exit result");
> +    cleanUp();

Shouldn't we call finalize() to switch back to 'gsm' instead?

@@ +303,5 @@
> + */
> +function test_enter_twice(step) {
> +  switch (step) {
> +    case 1:
> +      waitEventCheckedThenDial("911", true);

According to your comment, shouldn't the name be 'dialThenWaitEventChecked' ?

@@ +313,5 @@
> +      waitEventChecked(nextTest);
> +      break;
> +    default:
> +      ok(false, "should not enter here");
> +      cleanUp();

ditto.

@@ +336,5 @@
> +      waitEventChecked(nextTest);
> +      break;
> +    default:
> +      ok(false, "should not enter here");
> +      cleanUp();

ditto.

@@ +340,5 @@
> +      cleanUp();
> +  }
> +}
> +
> +/* *

Remove the space between *

@@ +362,5 @@
> +      waitEventChecked(nextTest);
> +      break;
> +    default:
> +      ok(false, "should not enter here");
> +      cleanUp();

ditto.

@@ +367,5 @@
> +  }
> +}
> +
> +let currentTest = -1;
> +let currentStep = 1;

It's a nit, but can't we have the same initial values for these two?

@@ +373,5 @@
> +let tests = [
> +  test_enter_twice,
> +  test_exit_by_normal_call,
> +  test_exit_by_request
> +];

Please move these 'let' to the beginning.

	Hsin-Yi Tsai (she/her) [:hsinyi]
	Comment 5 • 11 years ago

Comment on attachment 800030 [details] [diff] [review]
#3 Test emergency callback mode (marionette)

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

::: dom/network/tests/marionette/test_mobile_cdma_ecm.js
@@ +85,5 @@
> +
> +    // turn off radio
> +    setRadioEnabled(false, function() {
> +      // Switch technology via console command
> +      runEmulatorCmd("modem tech " + technology, function(result) {

We need to be more careful of emulatorCmd.

<Borrowing Vicamo's comment>
Please have a utility function for this.  See test_filter_mixed.js[1].  Or this case may, not very often, result in an intermittent failure like bug 903058.

[1]: https://mxr.mozilla.org/mozilla-central/source/dom/mobilemessage/tests/marionette/test_filter_mixed.js#16

	Szu-Yu Chen [:aknow] (inactive) Reporter
	Comment 6 • 11 years ago

Comment on attachment 800030 [details] [diff] [review]
#3 Test emergency callback mode (marionette)

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

Please click [review] to see my responses. Thx.

::: dom/network/tests/marionette/test_mobile_cdma_ecm.js
@@ +25,5 @@
> +ok(connection instanceof MozMobileConnection,
> +   "connection is instanceof " + connection.constructor);
> +
> +let outgoing;
> +let eventChecked;

The flag is set to true in nextTest() because it should be initiated for all of the three cases.

@@ +226,5 @@
> +
> +function setNextEmergencyCbModeCheck(active, timeoutMs) {
> +  eventChecked = false;
> +  connection.onemergencycbmodechange = function(event) {
> +    log("[ECM] Received emergencycbmodechange: " + event.active);

|eventChecked| is set to false in the beginning and set to true when we enter the callback function. It is used to check whether we have indeed received the event after we called |setNextEmergencyCbModeCheck()|.
So for this scenario, test case could move from test_enter_twice.step1 to test_enter_twice.step2 in hangUp(). However, in step2, it will wait until |eventChecked == true|. If not, test case fails by time out.

@@ +303,5 @@
> + */
> +function test_enter_twice(step) {
> +  switch (step) {
> +    case 1:
> +      waitEventCheckedThenDial("911", true);

I will correct the comment above.
The function's behaviour is indeed waitEventChecked then dial.

@@ +367,5 @@
> +  }
> +}
> +
> +let currentTest = -1;
> +let currentStep = 1;

The initial value for currentStep could be removed here.

	Szu-Yu Chen [:aknow] (inactive) Reporter
	Comment 7 • 11 years ago

1. Add depended bug 891707
2. keep manifest unchanged in this version, two proposals
   (a) set it as disable case, then land the patch (should remember to enable it in the future)
   (b) keep the patch on bugzilla. then wait for the depended bug 891707
3. I have rewrite the flow for test case control

	Hsin-Yi Tsai (she/her) [:hsinyi]
	Comment 8 • 11 years ago

(In reply to Szu-Yu Chen [:aknow] from comment #6)
> Comment on attachment 800030 [details] [diff] [review]
> #3 Test emergency callback mode (marionette)
> 
> Review of attachment 800030 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Please click [review] to see my responses. Thx.
> 
> ::: dom/network/tests/marionette/test_mobile_cdma_ecm.js
> @@ +25,5 @@
> > +ok(connection instanceof MozMobileConnection,
> > +   "connection is instanceof " + connection.constructor);
> > +
> > +let outgoing;
> > +let eventChecked;
> 
> The flag is set to true in nextTest() because it should be initiated for all
> of the three cases.
> 
> @@ +226,5 @@
> > +
> > +function setNextEmergencyCbModeCheck(active, timeoutMs) {
> > +  eventChecked = false;
> > +  connection.onemergencycbmodechange = function(event) {
> > +    log("[ECM] Received emergencycbmodechange: " + event.active);
> 
> |eventChecked| is set to false in the beginning and set to true when we
> enter the callback function. It is used to check whether we have indeed
> received the event after we called |setNextEmergencyCbModeCheck()|.
> So for this scenario, test case could move from test_enter_twice.step1 to
> test_enter_twice.step2 in hangUp(). However, in step2, it will wait until
> |eventChecked == true|. If not, test case fails by time out.
> 

Yup, I understood the test will fail because of time out as you said. But why do we move to the next test if the previous one doesn't end? I would like to see every test has clear steps and borderline. Please revise it, thanks.

> @@ +303,5 @@
> > + */
> > +function test_enter_twice(step) {
> > +  switch (step) {
> > +    case 1:
> > +      waitEventCheckedThenDial("911", true);
> 
> I will correct the comment above.
> The function's behaviour is indeed waitEventChecked then dial.
> 
> @@ +367,5 @@
> > +  }
> > +}
> > +
> > +let currentTest = -1;
> > +let currentStep = 1;
> 
> The initial value for currentStep could be removed here.

	Szu-Yu Chen [:aknow] (inactive) Reporter
	Comment 9 • 11 years ago

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #8)

> > > +
> > > +function setNextEmergencyCbModeCheck(active, timeoutMs) {
> > > +  eventChecked = false;
> > > +  connection.onemergencycbmodechange = function(event) {
> > > +    log("[ECM] Received emergencycbmodechange: " + event.active);
> > 
> > |eventChecked| is set to false in the beginning and set to true when we
> > enter the callback function. It is used to check whether we have indeed
> > received the event after we called |setNextEmergencyCbModeCheck()|.
> > So for this scenario, test case could move from test_enter_twice.step1 to
> > test_enter_twice.step2 in hangUp(). However, in step2, it will wait until
> > |eventChecked == true|. If not, test case fails by time out.
> > 
> 
> Yup, I understood the test will fail because of time out as you said. But
> why do we move to the next test if the previous one doesn't end? I would
> like to see every test has clear steps and borderline. Please revise it,
> thanks.

There are three sub test cases in this test file. Each test case consists of several steps.
I have designed each step as following:
  (1) execute a sequence of actions.
  (2) expect to receive one 'emergencycbmodechange' event after above actions

Before really execute the next step, it waits until the expected event from previous step is indeed triggered.
So, in the scenario you mentioned, the test flow will enter to the next step. But it halts there to waiting for the event.

The result is that whole test case is failed by time out. Moreover, it will not enter next sub case.
ex:

After running test case 1, step 3
Enter test case 1, step 4. before running this step, make sure the desired event from step 3 is received
  if not received, wait here

Test case 2, test case 3 will not execute.
From log, it shows that the current test is test case 1. Borderline between test cases is clear.

	Szu-Yu Chen [:aknow] (inactive) Reporter
	Comment 10 • 11 years ago

(In reply to Szu-Yu Chen [:aknow] from comment #9)
> (In reply to Hsin-Yi Tsai [:hsinyi] from comment #8)
> 
> > > > +
> > > > +function setNextEmergencyCbModeCheck(active, timeoutMs) {
> > > > +  eventChecked = false;
> > > > +  connection.onemergencycbmodechange = function(event) {
> > > > +    log("[ECM] Received emergencycbmodechange: " + event.active);
> > > 
> > > |eventChecked| is set to false in the beginning and set to true when we
> > > enter the callback function. It is used to check whether we have indeed
> > > received the event after we called |setNextEmergencyCbModeCheck()|.
> > > So for this scenario, test case could move from test_enter_twice.step1 to
> > > test_enter_twice.step2 in hangUp(). However, in step2, it will wait until
> > > |eventChecked == true|. If not, test case fails by time out.
> > > 
> > 
> > Yup, I understood the test will fail because of time out as you said. But
> > why do we move to the next test if the previous one doesn't end? I would
> > like to see every test has clear steps and borderline. Please revise it,
> > thanks.
> 
> There are three sub test cases in this test file. Each test case consists of
> several steps.
> I have designed each step as following:
>   (1) execute a sequence of actions.
>   (2) expect to receive one 'emergencycbmodechange' event after above actions
> 
> Before really execute the next step, it waits until the expected event from
> previous step is indeed triggered.
> So, in the scenario you mentioned, the test flow will enter to the next
> step. But it halts there to waiting for the event.
> 
> The result is that whole test case is failed by time out. Moreover, it will
> not enter next sub case.
> ex:
> 
> After running test case 1, step 3
> Enter test case 1, step 4. before running this step, make sure the desired
> event from step 3 is received
>   if not received, wait here
> 
> Test case 2, test case 3 will not execute.
> From log, it shows that the current test is test case 1. Borderline between
> test cases is clear.

Well...
I could make it. Please wait my patch.

	Hsin-Yi Tsai (she/her) [:hsinyi]
	Comment 12 • 11 years ago

Comment on attachment 801431 [details] [diff] [review]
#5 Test emergency callback mode (marionette)

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

::: dom/network/tests/marionette/manifest.ini
@@ +16,5 @@
>  [test_mobile_roaming_preference.js]
>  [test_call_barring_get_option.js]
>  [test_call_barring_set_error.js]
>  [test_call_barring_change_password.js]
> +[test_mobile_cdma_ecm.js]

Seems you prefer landing the test after the blocker is fixed because you didn't add "disabled" keyword? I am fine with either proposal. We just need to make sure the patch won't break others.

::: dom/network/tests/marionette/test_mobile_cdma_ecm.js
@@ +88,5 @@
> +    }
> +
> +    log("[TEST #" + currentTestId + "]");
> +    currentTest = allTests[currentTestId];
> +    eventChecked = true;

The |eventChecked| should be changed to true in the only one place, i.e. onemergencycbmodechange, not here. Instead, here we would need to check |ok(eventChecked)| in nextTest() to see if every single test is starting with the right precondition. So we would need |let eventChecked = true;| in the very beginning of the whole test.

@@ +111,5 @@
> +}());
> +
> +function sendToEmulator(cmd, callback) {
> +   pendingEmulatorCmdCount++;
> +   runEmulatorCmd(cmd, function (result) {

nit: no space between function and (

@@ +132,5 @@
> +    callback();
> +  });
> +  setReq.addEventListener("error", function onSetError() {
> +    ok(false, "cannot set '" + key + "'");
> +    cleanUp();

finalize() here?

@@ +296,5 @@
> + */
> +function setEventChecking(active, action) {
> +  eventChecked = false;
> +  connection.onemergencycbmodechange = function(event) {
> +    log("[ECM] Received emergencycbmodechange: " + event.active);

Not that big deal but could you please use the same style for the log messages all over the test script? I mean if you want to have the [ECM] keyword, maybe it should appear in other log messages all over the file? Or don't have it at all?

@@ +317,5 @@
> +  switchTechnology("gsm", cleanUp);
> +}
> +
> +function cleanUp() {
> +  is(pendingEmulatorCmdCount, 0, "has pending emulator command");

We don't really want to 'test' the pendingEmulatorCmdCount. What we want is make sure we've received all the pending responses before leaving a marionette test, so that the next test could be started with a clean emulator state.

That said we should do the following here:

if (pendingEmulatorCmdCount) {
  window.setTimeout(cleanUp, 100);
  return;
}

@@ +329,5 @@
> +
> +setPreferredNetworkType("wcdma/gsm/cdma/evdo", function() {
> +  switchTechnology("cdma", testManager.nextTest);
> +});
> +

nit: no new line

	Szu-Yu Chen [:aknow] (inactive) Reporter
	Comment 13 • 11 years ago

Comment on attachment 801431 [details] [diff] [review]
#5 Test emergency callback mode (marionette)

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

::: dom/network/tests/marionette/test_mobile_cdma_ecm.js
@@ +88,5 @@
> +    }
> +
> +    log("[TEST #" + currentTestId + "]");
> +    currentTest = allTests[currentTestId];
> +    eventChecked = true;

I found that I don't have to set the |eventChecked = true| here or in the beginning. Also it is no need to check |ok(eventChecked)| in nextTest(). |eventCheck| is guaranteed before entering next test step. I hava an infinite wait in 
  (1) hangup
  (2) userExitEmergencyCbMode
It controls whether we could enter the next step.

@@ +132,5 @@
> +    callback();
> +  });
> +  setReq.addEventListener("error", function onSetError() {
> +    ok(false, "cannot set '" + key + "'");
> +    cleanUp();

I am not sure how to write this part so it could work correctly.
In finalize(), we call switchTechnology(), and switchTechnology() calls the changeSetting() here. Then, if error occurs, calling finalize() again is not appropriate. Actually, I don't know how to recover everything in this case.

	Szu-Yu Chen [:aknow] (inactive) Reporter
	Comment 15 • 11 years ago

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #12)

> We don't really want to 'test' the pendingEmulatorCmdCount. What we want is
> make sure we've received all the pending responses before leaving a
> marionette test, so that the next test could be started with a clean
> emulator state.
> 
> That said we should do the following here:
> 
> if (pendingEmulatorCmdCount) {
>   window.setTimeout(cleanUp, 100);
>   return;
> }

How about writing the code as follows.

function cleanUp() {                                                    
  SpecialPowers.removePermission("telephony", document);                
  waitFor(finish, function() { return pendingEmulatorCmdCount === 0; });
}                                                                       

Use marionette's built-in function 'waitFor'.
I prefer this one rather than 'setTimeout'. The risk is that I remove the permission first. If not doing so, I need to write all the cleaning work in a function and put it in the place of 'finish'. The code looks uglier than current version.

	Szu-Yu Chen [:aknow] (inactive) Reporter
	Comment 16 • 11 years ago

I think the test is not blocked by Bug 891707. So, it's OK to add this one into manifest.

Bug 891707 said:
> There is no way to know the radio switching process is finished. If we start test before it is ready, we may got some unexpected timing issue. My idea is using voice registration event [1], when radio technology changes, qemu should report voice registration change with the current radio technology [2]. But this mechanism seems not work properly now.

In this test case, I turn off the radio first and the do the switching. Then turn on the radio and wait until card state ready. This kind of mechanism should guarantee the switching process is correct and finished. It is not really depends on the voice registration behaviour we would like to fix in that bug.

Need info Edgar to confirm it. Will the above switching process has some risk? Or it is better to land this one after bug 891707 resolved.

	Szu-Yu Chen [:aknow] (inactive) Reporter
	Comment 17 • 11 years ago

Comment on attachment 802787 [details] [diff] [review]
#6 Test emergency callback mode (marionette)

Cancel the review.
I will rewrite the test case by using Promise for the async call flow, which enhance the readability.

	Edgar Chen [:edgar]
	Comment 18 • 11 years ago

(In reply to Szu-Yu Chen [:aknow] from comment #16)
> I think the test is not blocked by Bug 891707. So, it's OK to add this one
> into manifest.
> 
> Bug 891707 said:
>> There is no way to know the radio switching process is finished. If we start test before it is ready, we may got some unexpected timing issue. My idea is using voice registration event [1], when radio technology changes, qemu should report voice registration change with the current radio technology [2]. But this mechanism seems not work properly now.
> 
> In this test case, I turn off the radio first and the do the switching. Then
> turn on the radio and wait until card state ready. This kind of mechanism
> should guarantee the switching process is correct and finished. It is not
Yes, I think this can guarantee the process is finished.

> really depends on the voice registration behaviour we would like to fix in
> that bug.
> 
> Need info Edgar to confirm it. Will the above switching process has some
> risk? Or it is better to land this one after bug 891707 resolved.
If you don't need to monitor radio technology dynamically, bug 891707 is not necessary.

Actually there is one thought in my mind. If Multi-SIM is the key feature of v1.3, maybe we could consider write CDMA related tests based on Multi-SIM. We could enable two services in emulator, one service runs as GSM, the other runs as CDMA, then tests them separately. In this way, we don't need to consider these switching things. Does this sound a good idea? :p

	BMO Automation
	Comment 19 • 7 years ago

Firefox OS is not being worked on