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

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[Robert Wood [:rwood]]

Develop a WebSMS test to verify the MozSmsManager.markMessageRead method.  This test will run on the b2g emulator using marionette.

Comment 1

[Robert Wood [:rwood]]

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.

Comment 2

[Sergey Tupchiy]

Hey Rob! Sorry seems I will not manage to finish it in near time, so assigning to you

Comment 3

[Robert Wood [: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 :)

Comment 4

[Robert Wood [:rwood]]

Attachment: Test for sms read/unread

Comment 5

[Dave Hunt [:davehunt] [he/him] ⌚BST]

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.

Comment 6

[Robert Wood [:rwood]]

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.

Comment 7

[Robert Wood [:rwood]]

Attachment: Updated patch for 794301

Comment 8

[Dave Hunt [:davehunt] [he/him] ⌚BST]

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.

Comment 9

[Robert Wood [:rwood]]

Thanks Dave! Marking for checkin-needed.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/00826d8e83d7

Comment 11

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/00826d8e83d7

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/a63f58624817
https://hg.mozilla.org/releases/mozilla-beta/rev/0ae2e1d5e215