Closed Bug 794557 Opened 7 years ago Closed 7 years ago

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

Categories

(Core :: DOM: Device Interfaces, defect)

All
Gonk (Firefox OS)
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla20
Tracking Status
firefox18 --- fixed
firefox19 --- fixed
firefox20 --- fixed

People

(Reporter: rwood, Assigned: rwood)

Details

Attachments

(1 file, 1 obsolete file)

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.
Attached patch Patch for 794557 (obsolete) — Splinter Review
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.
Attachment #676620 - Flags: feedback?(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.
Attachment #676620 - Flags: feedback?(vyang) → feedback-
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).
Attachment #676620 - Attachment is obsolete: true
Attachment #677831 - Flags: feedback?(vyang)
Comment on attachment 677831 [details] [diff] [review]
Updated patch for 794557

Moving to review stage.
Attachment #677831 - Flags: feedback?(vyang) → review?(jgriffin)
Attachment #677831 - Flags: review?(jgriffin) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/b7ee5158465d
Whiteboard: [automation-needed-in-aurora][automation-needed-in-beta]
https://hg.mozilla.org/mozilla-central/rev/b7ee5158465d
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
You need to log in before you can comment on or make changes to this bug.