Closed Bug 794301 Opened 9 years ago Closed 8 years ago

[WebAPI] WebSMS: Develop a test to verify marking an SMS message read/unread

Categories

(Core :: DOM: Device Interfaces, defect)

All
Gonk (Firefox OS)
defect
Not set
normal

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 the MozSmsManager.markMessageRead method.  This test will run on the b2g emulator using marionette.
Ping! Hey Sergey, hope you're great. If you don't have time to work on this feel free to assign it to me if you like. Regards, Rob.
Hey Rob! Sorry seems I will not manage to finish it in near time, so assigning to you
Assignee: tupchii.sergii → rwood
That's no problem Sergey, thanks, I'll take care of it then. In the future if you happen to have some time and are interested, there will always be other tests, and you know where to find me :)
Attached patch Test for sms read/unread (obsolete) — Splinter Review
Attachment #684447 - Flags: review?(dave.hunt)
Comment on attachment 684447 [details] [diff] [review]
Test for sms read/unread

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

Tests look good, however I was under the impression that a failure would abort the current test. The various cleanup and next function calls after an ok(false) imply otherwise..?

::: dom/sms/tests/marionette/test_mark_msg_read.js
@@ +30,5 @@
> +}
> +
> +// Callback for incoming SMS
> +sms.onreceived = function onreceived(event) {
> +  ok(true, "Received 'onreceived' sms event");

Should this just be a log call?

@@ +40,5 @@
> +  // Add newly received message id to array of msgs
> +  smsList.push(incomingSms.id);
> +
> +  // Wait for emulator to catch up before continuing
> +  waitFor(sendSms,function() {

Nit: Space between arguments, space between function and opening parentheses

@@ +41,5 @@
> +  smsList.push(incomingSms.id);
> +
> +  // Wait for emulator to catch up before continuing
> +  waitFor(sendSms,function() {
> +    return(rcvdEmulatorCallback);

Is this the quirk you've mentioned where you need to wait for the send command to finish, even though the message may have already been received?

@@ +45,5 @@
> +    return(rcvdEmulatorCallback);
> +  });
> +};
> +
> +function sendSms() {  

Nit: Trailing whitespace

@@ +49,5 @@
> +function sendSms() {  
> +  let gotSmsSent = false;
> +  let gotRequestSuccess = false;
> +  let remoteNumber = "5557779999";
> +  let text = "Mo Mo Mo Zilla Zilla Zilla!";

:)

@@ +80,5 @@
> +        test1();
> +      }
> +    } else {
> +      log("smsrequest returned false for sms.send");
> +      ok(false,"SMS send failed");

Nit: Space between arguments

@@ +81,5 @@
> +      }
> +    } else {
> +      log("smsrequest returned false for sms.send");
> +      ok(false,"SMS send failed");
> +      deleteMsgs();

Will the test continue after a failure? If not, deleteMsgs would not be called here.

@@ +90,5 @@
> +    log("Received 'onerror' smsrequest event.");
> +    ok(event.target.error, "domerror obj");
> +    ok(false, "sms.send request returned unexpected error: "
> +        + event.target.error.name );
> +    deleteMsgs();

As above, I'm not sure this would be called.

@@ +121,5 @@
> +      if (foundSms.read == readBool) {
> +        ok(true, "marked sms " + text);
> +      } else {
> +        ok(false, "marking sms " + text + " didn't work");
> +        log("Expected SMS (id: " + foundSms.id + ") to be marked " + text

This log should be before ok(false) to ensure it gets called.

@@ +132,5 @@
> +      log("Received 'onerror' smsrequest event.");
> +      ok(event.target.error, "domerror obj");
> +      is(event.target.error.name, "NotFoundError", "error returned");
> +      log("Could not get SMS (id: " + outSmsId + ") but should have.");
> +      ok(false,"Could not get SMS");

Nit: Space between arguments

@@ +133,5 @@
> +      ok(event.target.error, "domerror obj");
> +      is(event.target.error.name, "NotFoundError", "error returned");
> +      log("Could not get SMS (id: " + outSmsId + ") but should have.");
> +      ok(false,"Could not get SMS");
> +      deleteMsgs();

As previous comments, will this be called?

@@ +142,5 @@
> +    log("Received 'onerror' smsrequest event.");
> +    ok(event.target.error, "domerror obj");
> +    ok(false, "sms.markMessageRead request returned unexpected error: "
> +        + event.target.error.name );
> +    nextFunction();

Again, continuing after a failure.

@@ +191,5 @@
> +  ok(request instanceof MozSmsRequest,
> +      "request is instanceof " + request.constructor);
> +
> +  request.onsuccess = function(event) {
> +    ok(true, "Received 'onsuccess' smsrequest event");

Should this be a log call?

@@ +201,5 @@
> +        cleanUp();
> +      }
> +    } else {
> +      log("SMS delete failed.");
> +      ok(false,"sms.delete request returned false");

Nit: Space between arguments

@@ +202,5 @@
> +      }
> +    } else {
> +      log("SMS delete failed.");
> +      ok(false,"sms.delete request returned false");
> +      cleanUp();

Will cleanUp be called after a failure?

@@ +211,5 @@
> +    log("Received 'onerror' smsrequest event.");
> +    ok(event.target.error, "domerror obj");
> +    ok(false, "sms.delete request returned unexpected error: "
> +        + event.target.error.name );
> +    cleanUp();

As above, would this be called?

::: dom/sms/tests/marionette/test_mark_msg_read_error.js
@@ +37,5 @@
> +  is(incomingSms.read, false, "incoming message read");
> +  smsId = incomingSms.id;
> +
> +  // Wait for emulator to catch up before continuing
> +  waitFor(test1,function() {

Nit: Space between arguments, space between function and opening parenthesis

@@ +49,5 @@
> +
> +  requestRet.onsuccess = function(event) {
> +    log("Received 'onsuccess' smsrequest event, but expected error.");
> +    ok(false, "Smsrequest should have returned error but did not");
> +    nextFunction();

Will this be called?

@@ +76,5 @@
> +      + "), expect error.");
> +  markMsgError(invalidId, false, deleteMsg);
> +}
> +
> +function deleteMsg() {

Same comments apply to the method here as they do in the other test file.
Attachment #684447 - Flags: review?(dave.hunt) → review-
Thanks Dave for the review, appreciated!

Regarding test failures, test execution does continue after a test failure - is() and ok() flag the test failures but then continue on with the next line of code.

> +  // Wait for emulator to catch up before continuing
> +  waitFor(sendSms,function() {
> +    return(rcvdEmulatorCallback);

> Is this the quirk you've mentioned where you need to wait for the send command > to finish, even though the message may have already been received?

Yes that is correct. Sometimes the emulator is slow and after an emulator command is issued and executed, the resulting/next expected WebAPI event is fired even before the emulator callback itself has fired.

Attaching an updated patch.
Attachment #684447 - Attachment is obsolete: true
Attachment #685296 - Flags: review?(dave.hunt)
Comment on attachment 685296 [details] [diff] [review]
Updated patch for 794301

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

Looks good, and the tests pass for me locally.
Attachment #685296 - Flags: review?(dave.hunt) → review+
Thanks Dave! Marking for checkin-needed.
Keywords: checkin-needed
Whiteboard: [automation-needed-in-aurora][automation-needed-in-beta]
https://hg.mozilla.org/mozilla-central/rev/00826d8e83d7
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
You need to log in before you can comment on or make changes to this bug.