1083688 - [B2G][RIL] add marionette test cases for USSD features

Status: RESOLVED WONTFIX

(Firefox OS Graveyard :: RIL, defect)

Comment 1

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

Writing down the use cases on my mind...

1) Filtering redundant unsol null messages.
2) When there's no existent session, use telephony.dial() to trigger a new ussd session, then use USSDSession.send() and .cancel() APIs to react. After bug 1199141, only the API caller will receive the response.
3) When there's an existent session, using telephony.dial() to trigger a new ussd session should still work.
4) A system message "ussd-received" is sent when there comes a network initiated ussd message.
5) Testing timeout - Promise of .send() and .cancel() should be resolved/rejected when time expired

Comment 2

[pelloux]

Attachment: 0001-Bug-1083688-add-marionette-test-cases-for-USSD-featu.patch

Here's a test file with several unit tests.
Hopefully it can serve as a starting base for more ussd unit tests.

What do you think?

Comment 3

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

Comment on attachment 8695465 [details] [diff] [review]
0001-Bug-1083688-add-marionette-test-cases-for-USSD-featu.patch

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

This is a good start, thank you!

A high-level comments: 
a) let's split it into several separate files. Our past experience is that it's easy to run into Timeout error on Try server if a single file covers many cases. Remember to add new file names into manifest.ini.
b) According to USSDSession.webidl, we will get promise rejected when send()/cancel() fails. However, I always get promise resolved with MMICall... Could you please investigate more in case we miss something wrong in the implementation.
c) Promise is a double-edged knife, we must be very careful of the timing and execution ordering. Find more details in-line.
d) Add a case for Comment 1 item 3. Then I believe the coverage is good enough. Thanks for the hard work.

I have to say that it's not easy to write these tests correctly for the first time, complicated... I did the review at late night, hopefully I didn't make stupid suggestions ;) Let me know if you have more questions.

b.t.w. Mozilla is going to have a big event next week, so my response might be slow.

::: dom/telephony/test/marionette/test_ussd_received.js
@@ +3,5 @@
> +
> +MARIONETTE_TIMEOUT = 90000;
> +MARIONETTE_HEAD_JS = 'head.js';
> +
> +function testUssdReceivedViaSystemMessage() {

My comment 1 item 4 is covered, great!

@@ +16,5 @@
> +      is(message.message, msg, "ussd message: " + msg); // message.message is what you specify for "msg."
> +    });
> +
> +  let p2 = emulator.runCmd(cmd);
> +  return Promise.all([p1, p2]);

Let's extend it a bit so that we will cover more test data with "type" being 0 ~ 5.

@@ +19,5 @@
> +  let p2 = emulator.runCmd(cmd);
> +  return Promise.all([p1, p2]);
> +}
> +
> +function testUssdReceivedViaPromise() {

Comment 1 item 2 is covered, great!

@@ +27,5 @@
> +  let cmd = "ussd unsol 0 " + aMessage;
> +
> +  let p1 = gSendMMI("*#1234#")
> +    .then( message => {
> +      is(message.statusMessage, aMessage, "Check statusMessage");

We might also want to check if "message.additionalInformation" is null for type 0.

And add one case to check if "message.additionalInformation" is instance of USSDSession for type 1.

@@ +30,5 @@
> +    .then( message => {
> +      is(message.statusMessage, aMessage, "Check statusMessage");
> +    });
> +
> +  let p2 = emulator.runCmd(cmd);

We have to be very careful of the usage of Promise.all. 

If I get your idea, you want to create an unsol ussd in response to telephony.dial(ussdstring) request. However, there's no ordering b/w p1 and p2. That means, it could happen that p2 executes before telephony.dial(ussdstring) request is made that is not what we expect.

So what do we do?

function testUssdReceivedViaPromise() {
  log("testUssdReceivedViaPromise");
  let promises = [];
  let aMessage = "testmessage";
  let cmd = "ussd unsol 0 " + aMessage;

  return telephony.dial("#1234#")
    .then(mmiCall => {
      ok(mmiCall instanceof MMICall, "mmiCall is instance of MMICall");
      ok(mmiCall.result instanceof Promise, "result is Promise");
      let p1 = mmiCall.result
        .then( message => {
          is(message.statusMessage, aMessage, "Check statusMessage");
        });

      let p2 = emulator.runCmd(cmd);

      return Promise.all([p1, p2]);
    });
}

@@ +35,5 @@
> +
> +  return Promise.all([p1, p2]);
> +}
> +
> +function testUssdSessionCancel() {

Comment 1 item 2 is covered, great!

However, I see two major issues in this part. One is the Promise.all ordering flaw that I've mentioned above, the other is the emulator.runCmd usage. Due to the restriction of Eumlator, it cannot well handle more than 1 AT commands at the same time. So, be sure whenever you want to .runCmd, you need to make sure the result of the previous one is returned. In your case, I sometimes hit a situation that cmd2 is sent before cmd1 returns that makes response of cmd1 overwritten and lost.

My suggestion is as below:

function testUssdSessionCancel() {
  log("testUssdSessionCancel");
  let aMessage1 = "testmessage1";
  let aMessage2 = "testmessage2";
  // First unsol message should be routed via promise
  let cmd1 = "ussd unsol 1 " + aMessage1;
  // 2nd message should be delivered via system message
  let cmd2 = "ussd unsol 2 " + aMessage2;

  let p1 = gWaitForSystemMessage("ussd-received")
    .then(message => {
      is(message.message, aMessage2, "Check 2nd statusMessage");
    });

  let p2 =
    telephony.dial("#1234#")
    .then(mmiCall => {
      ok(mmiCall instanceof MMICall, "mmiCall is instance of MMICall");
      ok(mmiCall.result instanceof Promise, "result is Promise");

      let pp1 = mmiCall.result
        .then( message => {
          is(message.statusMessage, aMessage1, "Check 1st statusMessage");
        });
      let pp2 = emulator.runCmd(cmd1);

      return Promise.all([pp1, pp2]);
    })
    .then(() => {
      emulator.runCmd(cmd2);
    });

    return Promise.all([p1, p2]);
}

@@ +53,5 @@
> +  let p2 = gSendMMI("*#1234#")
> +    .then( message => {
> +      is(message.statusMessage, aMessage1, "Check 1st statusMessage");
> +      let session = message.additionalInformation;
> +      session.cancel().then (

I assume that this part is going to test two routing paths, so I don't get why we want to have session.cancel() here.

@@ +64,5 @@
> +
> +  return Promise.all([p1, p2, p3]);
> +}
> +
> +function testFilteringNullMessages() {

Seems you aim at covering comment 1 item 1, though I don't quite understand what you are trying to test here. Could you explain a bit?

@@ +81,5 @@
> +
> +  return Promise.all([p1, p2, p3]);
> +}
> +
> +function testUssdTimeout() {

Comment 1 item 5 is covered :)

@@ +85,5 @@
> +function testUssdTimeout() {
> +  log("testUssdTimeout");
> +  return gSendMMI("*#1234#").then( (message) => {
> +    log("great, got exepcted timeout error");
> +    is (message.statusMessage, "TimeoutError");

nit: no space between "is" and "("

@@ +88,5 @@
> +    log("great, got exepcted timeout error");
> +    is (message.statusMessage, "TimeoutError");
> +  });
> +}
> +

nit: remove one empty line

@@ +90,5 @@
> +  });
> +}
> +
> +
> +function testUssdTimeoutOnReply() {

Comment 1 item 5 is covered. :)

@@ +100,5 @@
> +    .then( message => {
> +      log("got answer sending reply: " + JSON.stringify(message));
> +      is(message.statusMessage, "dummy");
> +
> +      message.additionalInformation.send("test").then ( request => {

This promise chain won't be called as expected.

@@ +101,5 @@
> +      log("got answer sending reply: " + JSON.stringify(message));
> +      is(message.statusMessage, "dummy");
> +
> +      message.additionalInformation.send("test").then ( request => {
> +        request.then( () => {

This is wrong - look at USSDSession.webidl again, there's no |request.then|.

@@ +109,5 @@
> +        })
> +      })
> +    });
> +
> +  let p2 = emulator.runCmd(cmd);

We also have the promise.all ordering issue here.

And considering other issues above, my suggestion for revision would be:
function testUssdTimeoutOnReply() {
  log("testUssdTimeoutOnReply");

  let cmd = "ussd unsol 1 dummy";
  
  return  telephony.dial("#1234#")
    .then(mmiCall => {
      ok(mmiCall instanceof MMICall, "mmiCall is instance of MMICall");
      ok(mmiCall.result instanceof Promise, "result is Promise");

      let p1 = mmiCall.result
        .then(message => {
          message.additionalInformation.send("xxxxxtest")
        })
        .then (
          request => {
            ok(false, "Expected a timeout"); /* NOTE: I always run into this line which conflicts my expectation. Please investigate. */
          },
          error => {
            is(error, "TimeoutError");
        });

      let p2 = emulator.runCmd(cmd);

      return Promise.all([p1, p2]);
    });
}

Comment 4

[pelloux]

Attachment: v2

Thanks for your in-depth review.
Here's another patch.
All the tests works if you have the patches from 1007535, 1231324 in your tree.

I've removed the test testing for null messages for now but I'm still working on it.

Comment 5

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

Comment on attachment 8696953 [details] [diff] [review]
v2

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

The shape looks much better! We need one more iteration then we should be ready to go. 

Remember to remove "test_mmi_ussd.js" entirely, since it fails without modification after your gecko work. However, we don't really need to modify it as the case has been covered by other newly added ones.

::: dom/telephony/test/marionette/test_ussd_received_promise.js
@@ +38,5 @@
> +}
> +
> +startTest(function() {
> +  return testUssdReceivedViaPromise(0)
> +    .then(() => testUssdReceivedViaPromise(1))

We'd make sure that every time when we leave a test case, we'd recover the system state to prevent unexpected behavior in any proceeding tests.. In this case, we need to make the "ussd session state" back to "none." If you run all the telephony marionette-webapi tests, you might easily run into a situation that the one right after this case fails randomly.

In sum, please add the following lines:

// Restore the ussd session state
.then(() => restoreUssdSessionState())

where a new function, restoreUssdSessionState, is defined as

function restoreUssdSessionState() {
  log("Restore ussd session state to done.");
  let p1 = gWaitForSystemMessage("ussd-received")
    .then(message => {
      ok(!message.session, "Ussd session should be ended.");
    });
  let p2 = emulator.runCmd("ussd unsol 0 testRestore");

  return Promise.all([p1, p2]);
}

::: dom/telephony/test/marionette/test_ussd_received_system_message.js
@@ +25,5 @@
> +  }, {
> +    type: 3,
> +    message: "Other_local_client_has_responded",
> +    result: {
> +      sessionEnded: false

Per [1], sessionEnded is "false" only when type is 1. Please correct the value for type 3 ~ 5.

[1] https://dxr.mozilla.org/mozilla-central/source/dom/system/gonk/ril_worker.js?from=ril_worker.js#5127

@@ +47,5 @@
> +
> +  let p1 = gWaitForSystemMessage("ussd-received")
> +    .then(message => {
> +      is(message.message, scenario.message, "check message");
> +      // fail?? is(message.sessionEnded, scenario.result.sessionEnded, "check sessionEnded");

Referring to USSDReceivedEvent.webidl [1], there should be "message.session" rather than "session.sessionEnded". (Sorry that we do have several layers of interfaces that leads confusion to new contributors...)

The correction would be
is(!message.session, scenario.result.sessionEnded, "check session existence");

[1] https://dxr.mozilla.org/mozilla-central/source/dom/webidl/USSDReceivedEvent.webidl?from=USSDReceivedEvent.webidl#15

@@ +59,5 @@
> +  return testUssdReceivedViaSystemMessage(TEST_DATA[0])
> +    .then(() => testUssdReceivedViaSystemMessage(TEST_DATA[1]))
> +    .then(() => testUssdReceivedViaSystemMessage(TEST_DATA[2]))
> +    .then(() => testUssdReceivedViaSystemMessage(TEST_DATA[3]))
> +    .then(() => testUssdReceivedViaSystemMessage(TEST_DATA[4]))

TEST_DATA[5] is missing. Please add it.

::: dom/telephony/test/marionette/test_ussd_session_cancel.js
@@ +3,5 @@
> +
> +MARIONETTE_TIMEOUT = 90000;
> +MARIONETTE_HEAD_JS = 'head.js';
> +
> +function testUssdSessionCancel() {

The function name (and the file name) doesn't reflect the case content. How about we name it testUssdSessionRoutes?

@@ +23,5 @@
> +    .then(mmiCall => {
> +      ok(mmiCall instanceof MMICall, "mmiCall is instance of MMICall");
> +      ok(mmiCall.result instanceof Promise, "result is Promise");
> +
> +      let pp1 = mmiCall.result

I was so bad at naming... pp1 and pp2 weren't meaningful at all. Do you have better ideas?

::: dom/telephony/test/marionette/test_ussd_start_new_session.js
@@ +10,5 @@
> +  return telephony.dial("#1234#")
> +    .then(mmiCall => {
> +      let p1 = mmiCall.result
> +        .then( message => {
> +          is(message.statusMessage, aMessage, "Check session statusMessage");

This is the most tough and tricky case. That is, prior to creating the 2nd session, the gecko implementation will cancel the 1st one. So, we will have 3 execution commands: a) cancel ussd, b) send ussd, and c) emu unsol ussd. Now, here is the problem... as we don't have a determined way to control the sequence among these 3, it's likely the sequence c) - b) - a) happens. We will never wait for what we want but get "TimeoutError" :(

The solutions in my mind are:
a) drop this case at all, or
b) let's make it more tolerable. In this way, the line #14 will be replaced with 

if (message.statusMessage == aMessage ||
    message.statusMessage == "TimeoutError") {
  ok(true, "session statusMessage: " + message.statusMessage);
} else {
  ok(false, "unexpected statusMessage: " + message.statusMessage);
}
 
If we do so, we at least could check if the 2nd session is requested successfully or not. So, I prefer #b. If you have better ideas, let me know, please~

@@ +29,5 @@
> +  // start first session
> +  return newSession(aSession1Message)
> +    .then(() => {
> +      // start second session
> +      newSession(aSession2Message);

Always be careful of the returning object in promise chain, otherwise you might find that finish() is called before the 2nd newSession is resolved, which is not the expected behavior but hardly caught.

We don't need |testUssdStartNewSession| but let's modify startTest():

startTest(function() {
   let aSession1Message = "dummy";
   let aSession2Message = "foobar";

   return newSession(aSession1Message)
      .then(() => newSession(aSession2Message))
      .then(() => restoreUssdSessionState()) // Add "restoreUssdSessionState" function simply as what we do for test_ussd_received_promise.js

      .catch(error => ok(false, "Promise reject: " + error))
      .then(finish);
 });

Comment 6

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

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #5)
> 
> ::: dom/telephony/test/marionette/test_ussd_start_new_session.js
> @@ +10,5 @@
> > +  return telephony.dial("#1234#")
> > +    .then(mmiCall => {
> > +      let p1 = mmiCall.result
> > +        .then( message => {
> > +          is(message.statusMessage, aMessage, "Check session statusMessage");
> 
> This is the most tough and tricky case. That is, prior to creating the 2nd
> session, the gecko implementation will cancel the 1st one. So, we will have
> 3 execution commands: a) cancel ussd, b) send ussd, and c) emu unsol ussd.
> Now, here is the problem... as we don't have a determined way to control the
> sequence among these 3, it's likely the sequence c) - b) - a) happens. We
> will never wait for what we want but get "TimeoutError" :(
> 
> The solutions in my mind are:
> a) drop this case at all, or
> b) let's make it more tolerable. In this way, the line #14 will be replaced
> with 
> 
> if (message.statusMessage == aMessage ||
>     message.statusMessage == "TimeoutError") {
>   ok(true, "session statusMessage: " + message.statusMessage);
> } else {
>   ok(false, "unexpected statusMessage: " + message.statusMessage);
> }
>  
> If we do so, we at least could check if the 2nd session is requested
> successfully or not. So, I prefer #b. If you have better ideas, let me know,
> please~
> 

Revisiting comment 5 - 
My comment 5 doesn't work. To verify the start new session scenario, we need to be able to send two emu unsol commands - one for the internal "cancelUssd" and the other for the creation of the 2nd session. However, we have no way to control the timing exactly that we fail at verifying what we want. So, let's just skip this scenario entirely. 

> @@ +29,5 @@
> > +  // start first session
> > +  return newSession(aSession1Message)
> > +    .then(() => {
> > +      // start second session
> > +      newSession(aSession2Message);
> 
> Always be careful of the returning object in promise chain, otherwise you
> might find that finish() is called before the 2nd newSession is resolved,
> which is not the expected behavior but hardly caught.
> 
> We don't need |testUssdStartNewSession| but let's modify startTest():
> 
> startTest(function() {
>    let aSession1Message = "dummy";
>    let aSession2Message = "foobar";
> 
>    return newSession(aSession1Message)
>       .then(() => newSession(aSession2Message))
>       .then(() => restoreUssdSessionState()) // Add
> "restoreUssdSessionState" function simply as what we do for
> test_ussd_received_promise.js
> 
>       .catch(error => ok(false, "Promise reject: " + error))
>       .then(finish);
>  });

Comment 7

[BMO Automation]

Firefox OS is not being worked on