794557 - [WebAPI] WebSMS: Develop a test to verify getMessages (multiple messages)

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[Robert Wood [:rwood]]

Develop a WebSMS test to verify MozSmsManager.getMessages (getting multiple messages).  To run on the b2g device emulator via marionette.  Note: This test is intended for getMessages, using only a basic Sms filter.  Separate tests will be developed to verify Sms filters themselves.

Comment 1

[Robert Wood [:rwood]]

Attachment: Patch for 794557

If you could please review and provide some feedback Vicamo that would be great. This is just a basic test for sms.getMessages, with a default / no filter. Other tests will be developed to test sms filters.

Comment 2

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

Comment on attachment 676620 [details] [diff] [review]
Patch for 794557

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

Thanks for your works, but please simplify your test case as possible. Make each step clear and independent to others. Communicate to next step by function arguements instead of global variables. Reorder the functions by the time of first invoke unless it's a widely used utility function.

::: dom/sms/tests/marionette/test_getmessages.js
@@ +10,5 @@
> +// the marionette_timeout accordingly if this number is increased
> +let numMsgsToRcv = 10;
> +
> +let sms = window.navigator.mozSms;
> +let smsFilter = new MozSmsFilter;

I don't really know the purpose of this issue. It seems to duplicate existing

@@ +21,5 @@
> +let msgList = new Array();
> +let foundSmsCount = 0;
> +let smsDelCount = 0;
> +let nextSmsToDel;
> +let cursorRet;

There are so many global variables! What do they mean and when will they be used? Were it possible to define some more clear, independent steps?

@@ +23,5 @@
> +let smsDelCount = 0;
> +let nextSmsToDel;
> +let cursorRet;
> +
> +function deleteExistingMsgs() {

I don't know who calls this function. I have to trace all this file to find out where is it first invoked. You can turn it into one of the steps and layout the code in execution order. Actually, I don't think you have to delete all existing messages first here. You're just testing getMessages(). Isn't it more preferable if there are already messages to get?

@@ +29,5 @@
> +  // at test start. We are testing getMessages with no filter which will find
> +  // all messages. Want to verify exact message count and content returned.
> +  log("Checking for pre-existing SMS messages.");
> +
> +  let requestRet = sms.getMessages(smsFilter, false);

What does Ret mean? Can't it be just "request"? I can't find another request object in this function.

@@ +30,5 @@
> +  // all messages. Want to verify exact message count and content returned.
> +  log("Checking for pre-existing SMS messages.");
> +
> +  let requestRet = sms.getMessages(smsFilter, false);
> +  ok(requestRet, "smsrequest obj returned");

ok(request instanceof MozSmsRequest,
   "request is instanceof " + request.constructor);

@@ +35,5 @@
> +  let existingMsgsDel = 0;
> +
> +  requestRet.onsuccess = function(event) {
> +    ok(event.target.result, "smsrequest event.target.result");
> +    cursorRet = event.target.result;

I just found you don't have to make cursorRet a global variable not only in this function but also in others. Don't use global variables unless it's a must!

@@ +42,5 @@
> +      // Add the message id to pre-existing sms list
> +      msgList.push(cursorRet.message);
> +      // Now get next message in the list
> +      cursorRet.continue();
> +    } else {

Bail-out early please. Use:

  if (...) {
    ...
    return;
  }

  // something else

@@ +98,5 @@
> +
> +function verifyInitialState() {
> +  log("Verifying initial state.");
> +  ok(sms, "mozSms");
> +  deleteExistingMsgs();  

tailing white spaces

@@ +192,5 @@
> +      // Compare found message with corresponding one in msgList array
> +      is(cursorRet.message.id, msgList[(numMsgsToRcv - foundSmsCount)].id, "message id");
> +      is(cursorRet.message.body, msgList[(numMsgsToRcv - foundSmsCount)].body, "message body");
> +      is(cursorRet.message.delivery, msgList[(numMsgsToRcv - foundSmsCount)].delivery, "delivery");
> +      is(cursorRet.message.read, msgList[(numMsgsToRcv - foundSmsCount)].read, "read flag");

Could you please extract these duplicated code into another function?

@@ +194,5 @@
> +      is(cursorRet.message.body, msgList[(numMsgsToRcv - foundSmsCount)].body, "message body");
> +      is(cursorRet.message.delivery, msgList[(numMsgsToRcv - foundSmsCount)].delivery, "delivery");
> +      is(cursorRet.message.read, msgList[(numMsgsToRcv - foundSmsCount)].read, "read flag");
> +
> +      // Bug XXXXXX: When sms received ('onreceived' event) receiver field is

What's the bug number actually?

@@ +208,5 @@
> +      cursorRet.continue();
> +    } else {
> +      // No more messages
> +      if(foundSmsCount == numMsgsToRcv){
> +        log("All " + foundSmsCount + " sms messages were returned by getMessages.");     

ditto

@@ +213,5 @@
> +      } else {
> +        log("Expected getMessages to return more messages.");
> +        ok(false, "Not all sms messages were found by sms.getMessages");
> +      }
> +      deleteSms(false);      

ditto

@@ +277,5 @@
> +}
> +
> +function cleanUp() {
> +  sms.onreceived = null;
> +  SpecialPowers.removePermission("sms", document);

use SpecialPowers.clearUserPref instead.

Comment 3

[Robert Wood [:rwood]]

Attachment: Updated patch for 794557

Thanks for the feedback Vicamo.  Here is a much improved patch.

Btw, the reason any existing SMS messages must be deleted at test start is because to have a valid test case, it must be known exactly how many SMS messages (and content) that is expected to be found by getMessages.  Odds are there will not be any existing SMS messages at the time of test start as tests clean-up after themselves, however must check and make sure (in case a previous test failed to clean-up).

Comment 4

[Robert Wood [:rwood]]

Comment on attachment 677831 [details] [diff] [review]
Updated patch for 794557

Moving to review stage.

Comment 5

[Jonathan Griffin (:jgriffin)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b7ee5158465d

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/b7ee5158465d

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/b6e816f97d8e
https://hg.mozilla.org/releases/mozilla-beta/rev/a8a5d21dfba3