792495 - [WebAPI] WebSMS: Develop test to delete an SMS

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[Robert Wood [:rwood]]

Create a new WebSMS test to verify deleting an SMS (using both the SMS id as well as an SmsMessage object).

Comment 1

[Robert Wood [:rwood]]

Attachment: bug792495.patch

Comment 2

[Jonathan Griffin (:jgriffin)]

Comment on attachment 663728 [details] [diff] [review]
bug792495.patch

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

Looks good, and nice to see some new SMS tests.  Just a couple of minor revisions needed.

::: dom/sms/tests/marionette/test_incoming_delete.js
@@ +33,5 @@
> +    is(incomingSms.sender, fromNumber, "sender");
> +    ok(incomingSms.timestamp instanceof Date, "timestamp is istanceof date");
> +    let timeDiff = Math.abs((Math.floor(incomingSms.timestamp.getTime() / 1000)) 
> +        - (Math.floor(testStartTime / 1000)));
> +    ok(timeDiff <= 1, "diff between starttesttime and sms timestamp <= 1");

This is a little dangerous, since we can't always be sure that messages will be received within 1s...the emulator can be very slow as you know, and is typically slower on continuous-integration machines than it is on developer machines.  In general, we want to avoid hardcoding durations in tests as much as possible.  I think just verifying that the timestamp of the message is greater than testStartTime (like test_incoming.js does) is better.

@@ +77,5 @@
> +    if(event.target.result){
> +      verifySmsDeleted(smsMsgObj.id);
> +    } else {
> +      log("smsrequest returned false for sms.delete");
> +      ok(false,"SMS delete failed");

There should be a call to cleanUp() here.

::: dom/sms/tests/marionette/test_outgoing_delete.js
@@ +38,5 @@
> +    is(sentSms.sender, null, "sender");
> +    ok(sentSms.timestamp instanceof Date, "timestamp is istanceof date");
> +    let timeDiff = Math.abs((Math.floor(sentSms.timestamp.getTime() / 1000)) 
> +        - (Math.floor(testStartTime / 1000)));
> +    ok(timeDiff <= 1, "diff between starttesttime and sms timestamp <= 1");    

See comment for test_incoming_delete.js.

@@ +103,5 @@
> +    if(event.target.result){
> +      verifySmsDeleted(smsId);
> +    } else {
> +      log("smsrequest returned false for sms.delete");
> +      ok(false,"SMS delete failed");

There should be a call to cleanUp() here.

Comment 3

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

(In reply to Jonathan Griffin (:jgriffin) from comment #2)
> ::: dom/sms/tests/marionette/test_incoming_delete.js
> @@ +33,5 @@
> > +    is(incomingSms.sender, fromNumber, "sender");
> > +    ok(incomingSms.timestamp instanceof Date, "timestamp is istanceof date");
> > +    let timeDiff = Math.abs((Math.floor(incomingSms.timestamp.getTime() / 1000)) 
> > +        - (Math.floor(testStartTime / 1000)));
> > +    ok(timeDiff <= 1, "diff between starttesttime and sms timestamp <= 1");
> 
> This is a little dangerous, since we can't always be sure that messages will
> be received within 1s...the emulator can be very slow as you know, and is
> typically slower on continuous-integration machines than it is on developer
> machines.  In general, we want to avoid hardcoding durations in tests as
> much as possible.  I think just verifying that the timestamp of the message
> is greater than testStartTime (like test_incoming.js does) is better.

The timestamp got here is SMSC UTC time. This timestamp is inserted by host side of the emulator and could be probably much faster than you think. On my desktop, 8 core i7(qemu-arm is single threaded though), the time differences is about ~50ms or so. I do agree we should avoid hardcoding durations in tests, however, MARIONETTE_TIMEOUT itself is already a mandatory hardcoded duration, isn't it? To avoid the problem, there is still one more precise way -- use raw pdu. We can verify the timestamp with an exact value and no more guess is needed.

IMHO, there is too much unrelated tests in this test script. Maybe we can focus on whether the message had been deleted or not in this issue, and leave timestamp matching problem in test_incoming.js.

Comment 4

[Robert Wood [:rwood]]

Attachment: bug792495update.patch

Thanks for the feedback guys, updated patch attached.

Comment 5

[Jonathan Griffin (:jgriffin)]

Comment on attachment 667996 [details] [diff] [review]
bug792495update.patch

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

lgtm

Comment 6

[Jonathan Griffin (:jgriffin)]

https://hg.mozilla.org/mozilla-central/rev/65e1eb91cff9.

Requesting basecamp-blocker status to uplift to aurora.  This change contains tests only, no product impact.

Comment 7

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Not a blocker, but approved for aurora landing.

Please ask for aurora approval instead of blocking? for things that you don't think are blockers but want to land on aurora. Like tests.

Comment 8

[Jonathan Griffin (:jgriffin)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/3d56940e1859

Comment 9

[(PTO June 19-July 7) Ryan VanderMeulen]

Target milestone tracks when a fix lands on m-c. Use the status flags for branch landings.