Closed Bug 844900 Opened 7 years ago Closed 7 years ago

Replace SMS "Desktop Mock" with mozSms mock

Categories

(Firefox OS Graveyard :: Gaia::SMS, defect)

x86_64
Linux
defect
Not set

Tracking

(b2g18 fixed)

RESOLVED FIXED
Tracking Status
b2g18 --- fixed

People

(Reporter: jugglinmike, Assigned: jugglinmike)

Details

Attachments

(1 file, 4 obsolete files)

This application implements a "desktop mock" to facilitate testing in environments that do not implement `navigator.mozSms`. The mock overrides SMS application logic with scripted behavior.

This mock is too high-level for two reasons:

1. The assurances it provides are very limited. The SMS application
   will execute entirely separate code paths depending on the
   environment. This means that application behavior can diverge
   significantly between the desktop and B2G.
2. It adversely effects maintainability. Changes to the application
   implementation require changes to the mock. As was the case with
   Bug 843780, it is easy for the desktop mock to "fall behind"
   implementation changes made for B2G.

I propose that we re-implement the mock to instead implement the behavior of the `navigator.mozSms` API in environments where it is not defined. This will solve both problems listed above--the same application logic will be used across environments, and changes in the application will not require changes in the mock.

This approach is especially appropriate considering that the underlying API is still being defined:

https://wiki.mozilla.org/WebAPI/WebSMS

There is risk in in developing applications around an unfinished specification. A good way to manage that risk is to organize the application's expectations in an interface. This re-implementation would be that interface and would serve as an explicit contract describing how the SMS application expects the WebSMS API to behave.
Flags: needinfo?(felash)
Assignee: nobody → mike
We'll need a mock for unit testing anyway so this work will be needed. You can have a look to how we implemented some mocks (sometimes not very completely  but that's fine) in the system app unit tests, so that we keep a consistent approach. We're still in the process of refining how we do them, but a bottom-line is to keep them as dumb as possible, ie we try very hard to not throw any logic into them.

I do agree that the current way of doing (with the desktopTesting.js file) is not optimal (and to be fair I haven't looked so much into it yet).

As of having a mock where the platform lacks this object, I really believe this should be done in the platform. I have no clue on how this could be done though, I'm redirecting the needinfo request to Fabrice who may help you here. This may need a thread on the mailing list too.
Flags: needinfo?(felash) → needinfo?(fabrice)
I don't think we want to expose testing mocks to general content when we don't support an API on desktop. So at least they need to be only enabled behind some pref.
Flags: needinfo?(fabrice)
The current solution (along with the replacement I'm building) only defines the mock if `navigator.mozSms` is not available. Is this an acceptable solution to your concern, Fabrice?
(In reply to Mike Pennisi [:jugglinmike] from comment #3)
> The current solution (along with the replacement I'm building) only defines
> the mock if `navigator.mozSms` is not available. Is this an acceptable
> solution to your concern, Fabrice?

Are you working on a gecko patch or on a gaia one?
I'm working on a patch for Gaia
Comment on attachment 718518 [details] [diff] [review]
Define a stub version of mozSMS when it is not available

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

That looks sane overall. Appart from the issues below, something that looks incorrect in all the returned request objects is that they don't mock properly a SMSRequest object.
A SMSRequest (https://mxr.mozilla.org/mozilla-central/source/dom/sms/interfaces/nsIDOMSmsRequest.idl) is in fact a DOMRequest (see https://mxr.mozilla.org/mozilla-central/source/dom/base/nsIDOMDOMRequest.idl#15), and these objects have no "errorCode" property. They have an error.name though.

::: apps/sms/js/sms_mock.js
@@ +149,5 @@
> +    if (!handlers) {
> +      handlers = allHandlers[eventName] = [];
> +    }
> +    handlers.push(handler);
> +  };

Nit: add blank line

@@ +169,5 @@
> +      }
> +    };
> +
> +    messagesDb.messages.push(sendInfo.message);
> +    trigger('sending', sendInfo);

Here you're sending this pseudo-event in the same message loop. That should not be.

@@ +219,5 @@
> +  //  - onerror: Function that may be set by the suer. If set, will be invoked
> +  //    in the event of a failure
> +  mozSms.getThreadList = function() {
> +    // For now, never simulate a failure
> +    // TODO: Implement a global debugging switch to control this behavior

Yes, please.

@@ +221,5 @@
> +  mozSms.getThreadList = function() {
> +    // For now, never simulate a failure
> +    // TODO: Implement a global debugging switch to control this behavior
> +    var simulateFail = false;
> +    var simulateDelay = 500 * Math.random();

Why is it a different randomness than for send() ?
Attachment #718518 - Flags: review?(fabrice) → feedback+
Thanks for the review, Fabrice. I'm implementing your feedback, but I have one remaining question:

> That looks sane overall. Appart from the issues below, something that looks
> incorrect in all the returned request objects is that they don't mock
> properly a SMSRequest object.
> A SMSRequest
> (https://mxr.mozilla.org/mozilla-central/source/dom/sms/interfaces/
> nsIDOMSmsRequest.idl) is in fact a DOMRequest (see
> https://mxr.mozilla.org/mozilla-central/source/dom/base/nsIDOMDOMRequest.
> idl#15), and these objects have no "errorCode" property. They have an
> error.name though.

There are 5 references to the "errorCode" attribute of the request objects in the application logic*. None of these are critical--they are all used to generate error messages. Should I update the application to reference the "name" property instead?

* For example: https://github.com/mozilla-b2g/gaia/blob/aca113a37ed6e94b7576ea8bcf0c8281d3cb4d2e/apps/sms/js/message_manager.js#L290
I would fix these, yes.
This incorporates feedback from :fabrice's review.
Attachment #718518 - Attachment is obsolete: true
Attachment #719203 - Flags: review?(fabrice)
Comment on attachment 719203 [details] [diff] [review]
Define a stub version of mozSMS when it is not available

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

Please address comments and attach a new patch to commit. Thanks!

::: apps/sms/js/contacts.js
@@ +25,5 @@
>        callback(result);
>      };
>  
>      req.onerror = function onerror() {
> +      var msg = 'Contact finding error. Error: ' + req.name;

This should be req.error.name

@@ +49,5 @@
>        callback(req.result);
>      };
>  
>      req.onerror = function onerror() {
> +      var msg = 'Contact finding error. Error: ' + req.name;

Ditto

::: apps/sms/js/message_manager.js
@@ +254,5 @@
>        }
>      };
>  
>      request.onerror = function onerror() {
> +      var msg = 'Reading the database. Error: ' + request.name;

request.error.name

@@ +284,5 @@
>          }
>        }
>      };
>      request.onerror = function onerror() {
> +      var msg = 'Reading the database. Error: ' + request.name;

Ditto

@@ +306,5 @@
>        callback && callback(req.result);
>      };
>  
>      req.onerror = function onerror() {
> +      var msg = 'Deleting in the database. Error: ' + req.name;

Ditto

::: apps/sms/js/sms_mock.js
@@ +139,5 @@
> +    }
> +
> +    for (idx = 0, len = handlers.length; idx < len; ++idx) {
> +      handlers[idx].call(null, eventData);
> +    }

Why don't you use handlers.forEach there?

@@ +156,5 @@
> +      return window.SMSDebugDelay;
> +    } else {
> +      return 0;
> +    }
> +  };

Nit: add blank line

@@ +246,5 @@
> +  //  - onerror: Function that may be set by the suer. If set, will be invoked
> +  //    in the event of a failure
> +  mozSms.getThreadList = function() {
> +    var request = {
> +      name: 'mock getThreadList'

There's no such property on DOMRequest.

@@ +285,5 @@
> +  //  - onerror: Function that may be set by the suer. If set, will be invoked
> +  //    in the event of a failure
> +  mozSms.getMessages = function(filter, reverse) {
> +    var request = {
> +      name: 'mock getMessages'

Ditto

@@ +352,5 @@
> +  //  - onerror: Function that may be set by the suer. If set, will be invoked
> +  //    in the event of a failure
> +  mozSms.delete = function(id) {
> +    var request = {
> +      name: 'mock delete'

ditto
Attachment #719203 - Flags: review?(fabrice) → review+
Incorporate second round of feedback from :fabrice. This includes:

1) Correct generation of mock DOMRequest objects' `error` property
2) Correct invocation of DOMRequest callbacks (i.e. in the context of the request object)
3) Code style modifications

DOMRequest reference:

- https://developer.mozilla.org/en-US/docs/DOM/DOMRequest
- https://bugzilla.mozilla.org/show_bug.cgi?id=722626
Attachment #719203 - Attachment is obsolete: true
Attachment #719450 - Flags: review?(fabrice)
Attachment #719450 - Flags: review?(fabrice) → review+
Thanks for the review, Fabrice! I cannot land this myself; would you mind merging it on my behalf?
Can you rebase your patch onto the current master?  I am seeing changes to the apps/sms/index.html file which is in a section of the file that no longer exists, specifically:

      <script defer src="js/fixed_header.js"></script>
-     <script defer src="js/desktop_testing.js"></script>
  
    </head>

and

      <!-- Specific code -->
+     <script defer src="js/sms_mock.js"></script>
      <script defer src="js/blacklist.js"></script>
Rebased over latest master, as requested by :jhford.
Attachment #719450 - Attachment is obsolete: true
Attachment #722207 - Flags: review?(jhford)
All set!
master: 3d9aa37379f5c1f7e61807da1fe11058bc78da21
Status: NEW → RESOLVED
Closed: 7 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Comment on attachment 722207 [details] [diff] [review]
Define a stub version of mozSMS when it is not available

I can't r+ this patch, but it was a rebasing of an r+ patch, so we should be fine to keep the previous r+.
Attachment #722207 - Flags: review?(jhford)
Reverted in 20beb8d627a6f2fde1680956b70c01ecaf5093bd
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
I don't know why you changed all occurrences of navigator.mozSMS to mozSMS but this is wrong. Please revert these changes and upload a new patch.
This is necessary because `navigator.mozSms` is a read-only property. We need to alias it to a new global variable (`mozSms`) in order to stub the functionality in environments that do not define the mozSms API.

If you have suggestions for other approaches, I'd be happy to comply.
* call your mock MockNavigatormozSms
(the small "m" at "moz" is intentional)
* in MessageManager (the only place where we use mozSms if I'm not wrong) : this._mozSms = navigator.mozSms || MockNavigatormozSms.
* use this._mozSms when you want to use the API

Does that sounds good ?
I introduced the bug in the final rebase of the patch. I will submit an updated patch momentarily.

Julien: I will also incorporate the method for mocking the API that you have recommended.

Apologies all around for the confusion I've caused.
Another solution is to have your mock_sms.js set MessageManager._mozSms itself.
Attachment #722207 - Attachment is obsolete: true
Attachment #722813 - Flags: review?(felash)
Comment on attachment 722813 [details] [diff] [review]
Define a stub version of mozSMS when it is not available (v5)

r=me, go ahead !
Attachment #722813 - Flags: review?(felash) → review+
master: ae2a435a39e7cb35a63f73edcfa4b4cbcf0ab1dd
Status: REOPENED → RESOLVED
Closed: 7 years ago7 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Comment on attachment 722813 [details] [diff] [review]
Define a stub version of mozSMS when it is not available (v5)

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): -
User impact if declined: blocks automatic uplifting of leo+ work
Testing completed: yes, it's in master for a very long time
Risk to taking this patch (and alternatives if risky): low
String or UUID changes made by this patch: none

I'd like to uplift this as this generates conflicts on all other uplifts that are using the mozSms API.

As a matter of fact we already uplifted part of it through other uplifts, so this one will need to be uplifted manually for sure, but this will make other uplifts easier.
Attachment #722813 - Flags: approval-gaia-v1?(21)
Comment on attachment 722813 [details] [diff] [review]
Define a stub version of mozSMS when it is not available (v5)

pre-approval from me for MMS workweek expediency - seems fine given the nod from Fabrice and Julien. Vivien, feel free to comment or un-approve if you think this should be reconsidered.
Attachment #722813 - Flags: approval-gaia-v1?(21) → approval-gaia-v1+
I'll uplift myself as this won't probably uplift automatically.
Flags: needinfo?(felash)
I was not able to uplift this bug to v1-train.  If this bug has dependencies which are not marked in this bug, please comment on this bug.  If this bug depends on patches that aren't approved for v1-train, we need to re-evaluate the approval.  Otherwise, if this is just a merge conflict, you might be able to resolve it with:

  git checkout v1-train
  git cherry-pick -x  ae2a435a39e7cb35a63f73edcfa4b4cbcf0ab1dd
  <RESOLVE MERGE CONFLICTS>
  git commit
v1-train: 17a6be36e2d961182db680786c43fd2c6697d2e1
Flags: needinfo?(felash)
You need to log in before you can comment on or make changes to this bug.