806811 - WebSMS: add test case for multi-segment SMS and maximum segments supported

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

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

We should have test cases for maximum supported messages for both incoming and outgoing. For now, incoming messages can have 8-bit or 16-bit segment reference, and we're only using 8-bit segment reference for outgoing. The corresponding maximum text lengths are:

           | 7-bit alphabet                      | UCS2
 8-bit ref | 39525[1], 38760[2], 37740[3]        | 17340[7]
16-bit ref | 10092390[4], 9830250[5], 9371505[6] | 4390845[8]

[1]: (160 - Math.ceil((1 + 3 + 0) * 8 / 7)) * 255 = 39525
[2]: (160 - Math.ceil((1 + 3 + 3) * 8 / 7)) * 255 = 38760
[3]: (160 - Math.ceil((1 + 3 + 6) * 8 / 7)) * 255 = 37740
[4]: (160 - Math.ceil((1 + 4 + 0) * 8 / 7)) * 635535 = 10092390
[5]: (160 - Math.ceil((1 + 4 + 3) * 8 / 7)) * 635535 = 9830250
[6]: (160 - Math.ceil((1 + 4 + 6) * 8 / 7)) * 635535 = 9371505
[7]: (70 - Math.ceil((1 + 3) / 2)) * 255 = 17340
[8]: (70 - Math.ceil((1 + 4) / 2)) * 65535 = 4390845

Comment 1

[Beatriz Rodríguez [:brg]]

The operator requirement said that the device shall be capabale of receiving/sending up to 10 sms in a transparent manner(it means concatenated) 
This is: 10*140bytes= 1400 bytes

Not all those 1400 bytes will be characters because device needs to include some headers but this is the maximum required from a user point of view.

Does it help to know the maximum number required from user prespective?

Comment 2

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

Yes, it's much practical, thank you!

Comment 4

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

Change the summary for we don't have test cases for incoming multi-segment messages at all. Outgoing multi-segment test case is already committed in bug 786782.

Comment 6

[Robert Wood [:rwood]]

Attachment: WebAPI test for max incoming/outgoing SMS segments

Comment 7

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

Comment on attachment 690906 [details] [diff] [review]
WebAPI test for max incoming/outgoing SMS segments

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

Looks good and tests pass. Mostly just nits.

::: dom/sms/tests/marionette/test_incoming_max_segments.js
@@ +6,5 @@
> +SpecialPowers.setBoolPref("dom.sms.enabled", true);
> +SpecialPowers.addPermission("sms", true, document);
> +
> +let sms = window.navigator.mozSms;
> +let maxCharsPerSms = 160; //developer.mozilla.org/en-US/docs/DOM/SmsManager

nit: space after // and include protocol in URL

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

nit: trailing whitespace

@@ +22,5 @@
> +
> +  // Build the message text
> +  msgText = new Array((maxCharsPerSms * maxSegments) + 1).join('a');
> +  log("Simulating incoming multipart SMS (" + msgText.length
> +      + " chars total).");

nit: keep + on previous line

@@ +36,5 @@
> +    is(incomingSms.delivery, "received", "delivery");
> +    is(incomingSms.read, false, "read");
> +    is(incomingSms.receiver, null, "receiver");
> +    is(incomingSms.sender, fromNumber, "sender");
> +    ok(incomingSms.timestamp instanceof Date, "timestamp is istanceof date");

nit: typo istanceof/instance of

@@ +66,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: " + incomingSms.id + ") but should have.");
> +    ok(false,"SMS was not found");

nit: Add a space after the comma

@@ +74,5 @@
> +
> +function deleteSms(smsMsgObj){
> +  log("Deleting SMS (id: " + smsMsgObj.id + ") using smsmsg obj parameter.");
> +  let requestRet = sms.delete(smsMsgObj);
> +  ok(requestRet,"smsrequest obj returned");

nit: Add a space after the comma

@@ +78,5 @@
> +  ok(requestRet,"smsrequest obj returned");
> +
> +  requestRet.onsuccess = function(event) {
> +    log("Received 'onsuccess' smsrequest event.");
> +    if(event.target.result){

nit: Add a space after if and also before the {

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

nit: Add a space after the comma

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

nit: keep + on previous line and indent the continuation so it's inline with "sms.delete... also remove space before closing )

::: dom/sms/tests/marionette/test_outgoing_max_segments.js
@@ +6,5 @@
> +SpecialPowers.setBoolPref("dom.sms.enabled", true);
> +SpecialPowers.addPermission("sms", true, document);
> +
> +let sms = window.navigator.mozSms;
> +let maxCharsPerSms = 160; //developer.mozilla.org/en-US/docs/DOM/SmsManager

nit: space after // and include protocol in URL

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

nit: trailing whitespace

@@ +18,5 @@
> +
> +function sendSms() {
> +  let destNumber = "5551234567";
> +  let msgText = "";
> +  let gotReqOnsuccess = false;

nit: capitalise 'Success'

@@ +19,5 @@
> +function sendSms() {
> +  let destNumber = "5551234567";
> +  let msgText = "";
> +  let gotReqOnsuccess = false;
> +  let gotSmsOnsent = false;

nit: capitalise 'Sent'

@@ +25,5 @@
> +
> +  // Build the message text
> +  msgText = new Array((maxCharsPerSms * maxSegments) + 1).join('a');
> +  log("Sending multipart SMS (" + msgText.length
> +      + " chars total).");

nit: keep + on previous line

@@ +38,5 @@
> +    is(sentSms.body.length, msgText.length, "text length");
> +    is(sentSms.body, msgText, "msg body");
> +    is(sentSms.delivery, "sent", "delivery");
> +
> +    if (gotSmsOnsent && gotReqOnsuccess) { verifySmsExists(sentSms); }

gotSmsOnsent will be true because we've just set that, so you should be able to just check the state of gotReqOnsuccess here.

@@ +48,5 @@
> +  requestRet.onsuccess = function(event) {
> +    log("Received 'onsuccess' smsrequest event.");
> +    gotReqOnsuccess = true;
> +    if (event.target.result) {
> +      if (gotSmsOnsent && gotReqOnsuccess) { verifySmsExists(sentSms); }

As above, we shouldn't need to include gotReqOnsuccess here, because that will always be true.

@@ +51,5 @@
> +    if (event.target.result) {
> +      if (gotSmsOnsent && gotReqOnsuccess) { verifySmsExists(sentSms); }
> +    } else {
> +      log("smsrequest returned false for sms.send");
> +      ok(false,"SMS send failed");

nit: space after comma

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

nit: keep + on previous line and indent the continuation so it's inline with "sms.send... also remove space before closing )

@@ +86,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: " + sentSms.id + ") but should have.");
> +    ok(false,"SMS was not found");

nit: space after comma

@@ +91,5 @@
> +    cleanUp();
> +  };
> +}
> +
> +function deleteSms(smsMsgObj){

nit: space before {

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

nit: space after comma

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

nit: keep + on previous line and indent the continuation so it's inline with "sms.delete... also remove space before closing )

Comment 8

[Robert Wood [:rwood]]

Attachment: Updated tests

Thanks Dave!

Regarding:

> +    if (gotSmsOnsent && gotReqOnsuccess) { verifySmsExists(sentSms); }

> gotSmsOnsent will be true because we've just set that, so you should be able > to just check the state of gotReqOnsuccess here.

This is done this way because it isn't guaranteed that the sms.onsent and sms send request.onsuccess events will be received in the same order each time. This way no matter which event is received first, the code will only continue after both events have been received and verified.

New patch attached.

Comment 9

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

(In reply to Rob Wood [:rwood] from comment #8)
> Created attachment 697491 [details] [diff] [review]
> Updated tests
> 
> Thanks Dave!
> 
> Regarding:
> 
> > +    if (gotSmsOnsent && gotReqOnsuccess) { verifySmsExists(sentSms); }
> 
> > gotSmsOnsent will be true because we've just set that, so you should be able > to just check the state of gotReqOnsuccess here.
> 
> This is done this way because it isn't guaranteed that the sms.onsent and
> sms send request.onsuccess events will be received in the same order each
> time. This way no matter which event is received first, the code will only
> continue after both events have been received and verified.

I understood this, however unless I'm mistaken you will only need to check that the /other/ event has been received, because we're inside the listener for the initial event.

Comment 10

[Robert Wood [:rwood]]

Ahh right, makes sense, thanks. I'll submit another patch with that correction.

Comment 11

[Robert Wood [:rwood]]

Attachment: Latest test fix

Thanks Dave

Comment 12

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

Comment on attachment 697515 [details] [diff] [review]
Latest test fix

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

Looks good, and the tests are passing for me.

Comment 13

[Robert Wood [:rwood]]

Attachment: Patch with tests and latest manifest

Dave, can you please give me an r+. My previous approved patch was bitrotted, the manifest.ini had been updated multi times by others, so created a new patch with same test code but latest updated manifest.ini.  Thanks!

Comment 14

[Robert Wood [:rwood]]

Pushed latest patch to try:
https://tbpl.mozilla.org/?tree=Try&rev=98abeff275b8

Comment 15

[Robert Wood [:rwood]]

Pushed to try, this time actually running the WebAPI tests ;)
https://tbpl.mozilla.org/?tree=Try&rev=18b8bb1753d8

Comment 16

[Robert Wood [:rwood]]

Attachment: Max segments WebSMS test with updated manifest

Carrying forward the r+ as just updating the patch with latest manifest.ini as it was bitrotted again.

Comment 17

[Robert Wood [:rwood]]

Pushed latest patch to try:
https://tbpl.mozilla.org/?tree=Try&rev=986d2a807876

Comment 18

[Robert Wood [:rwood]]

Attachment: Max segment tests with newest manifest

Patch was bitrotted again right before I pushed it. Updated patch with latest manifest, carrying forward the r+.

Comment 19

[Robert Wood [:rwood]]

Pushing latest patch to try...
https://tbpl.mozilla.org/?tree=Try&rev=260ac7866e6f

Comment 20

[Robert Wood [:rwood]]

Orange bug I believe it was unrelated, but pushing same patch to try again...
https://tbpl.mozilla.org/?tree=Try&rev=0f77e62797c9

Comment 21

[Robert Wood [:rwood]]

Attachment: Fixed WebSMS tests for max segments and updated manifest

Found bug in test_outgoing_max_segments.js. Wasn't setting the sms.onsent handler to null. Unsure if this is related to the unit test failure on try but will see. Carrying the r+ forward as only a one line change and will push to try again.

Comment 22

[Robert Wood [:rwood]]

Pushed this latest patch to try:
https://tbpl.mozilla.org/?tree=Try&rev=f1b0dbe48c7f

Comment 23

[Robert Wood [:rwood]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1aeb49c8f4d9

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1aeb49c8f4d9