Closed Bug 806811 Opened 7 years ago Closed 7 years ago

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

Categories

(Core :: DOM: Device Interfaces, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla21

People

(Reporter: vicamo, Assigned: rwood)

References

Details

Attachments

(1 file, 6 obsolete files)

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
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?
Yes, it's much practical, thank you!
Duplicate of this bug: 808195
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.
Summary: WebSMS: add test case for maximum segments supported → WebSMS: add test case for multi-segment SMS and maximum segments supported
Duplicate of this bug: 810083
Depends on: 814761
Attachment #690906 - Flags: review?(dave.hunt)
Assignee: nobody → rwood
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 )
Attachment #690906 - Flags: review?(dave.hunt) → review-
Attached patch Updated tests (obsolete) — Splinter Review
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.
Attachment #697491 - Flags: review?(dave.hunt)
Attachment #690906 - Attachment is obsolete: true
(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.
Ahh right, makes sense, thanks. I'll submit another patch with that correction.
Attachment #697491 - Attachment is obsolete: true
Attachment #697491 - Flags: review?(dave.hunt)
Attached patch Latest test fix (obsolete) — Splinter Review
Thanks Dave
Attachment #697515 - Flags: review?(dave.hunt)
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.
Attachment #697515 - Flags: review?(dave.hunt) → review+
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!
Attachment #697515 - Attachment is obsolete: true
Attachment #698815 - Flags: review?(dave.hunt)
Pushed latest patch to try:
https://tbpl.mozilla.org/?tree=Try&rev=98abeff275b8
Pushed to try, this time actually running the WebAPI tests ;)
https://tbpl.mozilla.org/?tree=Try&rev=18b8bb1753d8
Carrying forward the r+ as just updating the patch with latest manifest.ini as it was bitrotted again.
Attachment #698815 - Attachment is obsolete: true
Attachment #702887 - Flags: review+
Pushed latest patch to try:
https://tbpl.mozilla.org/?tree=Try&rev=986d2a807876
Patch was bitrotted again right before I pushed it. Updated patch with latest manifest, carrying forward the r+.
Attachment #702887 - Attachment is obsolete: true
Attachment #703479 - Flags: review+
Pushing latest patch to try...
https://tbpl.mozilla.org/?tree=Try&rev=260ac7866e6f
Orange bug I believe it was unrelated, but pushing same patch to try again...
https://tbpl.mozilla.org/?tree=Try&rev=0f77e62797c9
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.
Attachment #703479 - Attachment is obsolete: true
Attachment #704911 - Flags: review+
Pushed this latest patch to try:
https://tbpl.mozilla.org/?tree=Try&rev=f1b0dbe48c7f
https://hg.mozilla.org/integration/mozilla-inbound/rev/1aeb49c8f4d9
Whiteboard: [automation-needed-in-b2g18]
https://hg.mozilla.org/mozilla-central/rev/1aeb49c8f4d9
Status: NEW → RESOLVED
Closed: 7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
Whiteboard: [automation-needed-in-b2g18]
You need to log in before you can comment on or make changes to this bug.