883276 - Cover TelephonyHelper with unit tests

Status: RESOLVED FIXED

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

Description

[Etienne Segonzac (:etienne)]

We currently have 1, that was the hardest part :)

Let's cover the rest of the file.

Comment 1

[Etienne Segonzac (:etienne)]

Attachment: Patch proposal

Comment 2

[Julien Wajsberg [:julienw]]

Comment on attachment 762805 [details] [diff] [review]
Patch proposal

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

With the test-agent, you can access a sinon sandbox in tests using this.sinon. The test-agent will automatically restore the mocks/stubs/spies at teardown. So you can just keep empty functions in the mock and stub/spy/mock them in the tests.

If you want to use sinon in mocks, which could make the test more readable (because they would only contains the expectations and configurations) this is not possible to use that sandbox but you can create a sandbox in mSetup, make all functions a mock (so that tests can use either stub/spy or mock functionality on them) using that sandbox and destroy the sandbox in mTeardown. see http://sinonjs.org/docs/#sinon-sandbox

There is no absolute best solution yet, I've not experimented a lot with using sinon with mocks yet, so let's experiment now !

::: apps/communications/dialer/test/unit/mock_confirm_dialog.js
@@ +1,1 @@
>  var MockConfirmDialog = {

could you please add a "use strict" statement here ?

@@ +6,5 @@
> +  show: function(title, body) {
> +    this.mShown = true;
> +    this.mTitle = title;
> +    this.mBody = body;
> +  },

I'm sure you'd like to experiment with sinonjs instead ;)

::: apps/communications/dialer/test/unit/mock_mozMobileConnection.js
@@ +168,5 @@
>                                                           sessionEnded) {
>      MmiManager.handleMMIReceived(message, sessionEnded);
>    },
>  
>    teardown: function mmmc_teardown() {

should this be mTeardown ?

@@ +171,5 @@
>  
>    teardown: function mmmc_teardown() {
> +    this.voice = {
> +      network: 'Fake voice network'
> +    };

I'd rather see this in mSetup ?

::: apps/communications/dialer/test/unit/mock_moztelephony.js
@@ +1,1 @@
>  var MockMozTelephony = {

could you please add a "use strict" statement here ?

@@ +6,5 @@
> +
> +  mTeardown: function() {
> +    this.active = null;
> +    this.dial = function() {};
> +    this.dialEmergency = function() {};

looks strange to set the functions...

::: apps/communications/dialer/test/unit/telephony_helper_test.js
@@ +4,1 @@
>  requireApp('communications/dialer/test/unit/mock_l10n.js');

could you please add a "use strict" statement here ?

@@ +12,3 @@
>  if (!this.LazyL10n) {
>    this.LazyL10n = null;
>  }

why no mocks helper ?

have a look to the mocks helper that is in sms, it has a function to setup all the phases in one call.

might make sense to finally move it to /shared/test/unit/mocks/ by the way

@@ +23,5 @@
>  
> +  function checkDialog(title, body) {
> +    assert.isTrue(MockConfirmDialog.mShown);
> +    assert.equal(MockConfirmDialog.mTitle, title);
> +    assert.equal(MockConfirmDialog.mBody, body);

should use spys to do this.

@@ +68,5 @@
> +  function(done) {
> +    navigator.mozMobileConnection.voice.emergencyCallsOnly = true;
> +    var dialNumber = '112';
> +    navigator.mozTelephony.dialEmergency = function(number) {
> +      assert.equal(number, dialNumber);

looks like you badly want sinon.js mocks. see http://sinonjs.org/docs/#mocks

Maybe stubs will be necessary too, see http://sinonjs.org/docs/#stubs

@@ +91,4 @@
>  
> +  test('should display an error if there is no network', function() {
> +    var dialNumber = '01 45 34 55 20';
> +    navigator.mozMobileConnection.voice = null;

I'd rather set the property using the mock name, so that we clearly read it's in the mock.

@@ +107,5 @@
> +    setup(function() {
> +      mockCall = {};
> +      navigator.mozTelephony.dial = function(number) {
> +        return mockCall;
> +      };

you want a sinon stub here

@@ +112,5 @@
> +    });
> +    test('should trigger oncall as soon as we get a call object',
> +    function(done) {
> +      subject.call('123', function() {
> +        assert.isTrue(true);

do we need this ? Looks useless to me. If we come here, we'll reach done, and if we don't come here, the test will timeout.

@@ +119,5 @@
> +    });
> +
> +    test('should bind the onconnected callback', function(done) {
> +      subject.call('123', null, function() {
> +        assert.isTrue(true);

ditto

@@ +121,5 @@
> +    test('should bind the onconnected callback', function(done) {
> +      subject.call('123', null, function() {
> +        assert.isTrue(true);
> +        done();
> +      });

btw do we really need to call done here ? or you want to check that calling onconnected actually triggers calling the callback (which is not in the test name) ?

(same in the following tests :) )

@@ +122,5 @@
> +      subject.call('123', null, function() {
> +        assert.isTrue(true);
> +        done();
> +      });
> +      mockCall.onconnected();

you should |assert.isFunction(mockCall.onconnected)| before calling it

@@ +127,5 @@
> +    });
> +
> +    test('should bind the ondisconnected callback', function(done) {
> +      subject.call('123', null, null, function() {
> +        assert.isTrue(true);

ditto

@@ +130,5 @@
> +      subject.call('123', null, null, function() {
> +        assert.isTrue(true);
> +        done();
> +      });
> +      mockCall.ondisconnected();

you should |assert.isFunction(mockCall.ondisconnected)| before calling it

@@ +135,5 @@
> +    });
> +
> +    test('should bind the onerror callback', function(done) {
> +      subject.call('123', null, null, null, function() {
> +        assert.isTrue(true);

ditto

@@ +138,5 @@
> +      subject.call('123', null, null, null, function() {
> +        assert.isTrue(true);
> +        done();
> +      });
> +      mockCall.onerror({call: {error: 'mock'}});

you should |assert.isFunction(mockCall.onerror)| before calling it

@@ +147,5 @@
> +    var mockCall;
> +    setup(function() {
> +      mockCall = {};
> +      navigator.mozTelephony.dial = function(number) {
> +        return mockCall;

you want a stub

@@ +150,5 @@
> +      navigator.mozTelephony.dial = function(number) {
> +        return mockCall;
> +      };
> +      navigator.mozTelephony.dialEmergency = function(number) {
> +        return mockCall;

you want a stub too

@@ +157,5 @@
> +
> +    suite('BadNumberError handle', function() {
> +      var error;
> +      setup(function() {
> +        error = {call: {error: {name: 'BadNumberError' }}};

You use that error only twice, I'd rather define it in each test.

But please create a function to generate that object, so that we can do :

  var error = error('BadNumberError');

@@ +193,5 @@
> +  });
> +
> +  test('should display a message if we didn\'t get a call back', function() {
> +    navigator.mozTelephony.dial = function(number) {
> +      return null;

you want a stub

Comment 3

[Etienne Segonzac (:etienne)]

(In reply to Julien Wajsberg [:julienw] from comment #2)
> Comment on attachment 762805 [details] [diff] [review]
> Patch proposal
> 
> Review of attachment 762805 [details] [diff] [review]:
> ----------------------------------------------------------------- 
> >    teardown: function mmmc_teardown() {
> > +    this.voice = {
> > +      network: 'Fake voice network'
> > +    };
> 
> I'd rather see this in mSetup ?

I'm putting back the mock in it's initial state, reverting changes made by the test, so I'd like to keep this in a (now renamed) mTeardown().
(added a comment to make it clear)

That said, a new patch is incoming with every single other comment addressed :)

Comment 4

[Etienne Segonzac (:etienne)]

Attachment: Patch v2

Well worth it.

Comment 5

[Etienne Segonzac (:etienne)]

Attachment: Patch v2 rebased

Just rebased this patch.
Would appreciate a review before it gets bit-rotten again :)

Comment 6

[Julien Wajsberg [:julienw]]

Comment on attachment 773204 [details] [diff] [review]
Patch v2 rebased

I'm doing some sinon stuff today, therefore I'll review it today :)

Comment 7

[Julien Wajsberg [:julienw]]

Comment on attachment 773204 [details] [diff] [review]
Patch v2 rebased

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

looks very good !

some comments still :)

::: apps/communications/dialer/test/unit/mmi_test.js
@@ +1,1 @@
>  requireApp('communications/dialer/js/mmi.js');

nit: this file misses a 'use strict' declaration

@@ +1,3 @@
>  requireApp('communications/dialer/js/mmi.js');
>  
> +requireApp('communications/shared/test/unit/mocks/mocks_helper.js');

could be a good idea to put this in communications' setup.js instead ?

btw there is already one there, so better replace it with the one in shared. this should be backward compatible so you shouldn't need to change the other communications tests.

(same remark for the other files of course)

@@ +16,3 @@
>  
>  suite('dialer/mmi', function() {
> +  var mocksHelper = mocksHelperForMMI;

just call |mocksHelperForMMI.attachTestHelpers()| and you're done !

(same remark for the files where I forgot to say it ;) )

::: apps/communications/dialer/test/unit/mock_confirm_dialog.js
@@ +3,3 @@
>  var MockConfirmDialog = {
> +  show: function(title, body) {
> +  },

why this change ? just to make it clear that this function is awaiting 2 parameters ?

::: apps/communications/dialer/test/unit/mock_mozMobileConnection.js
@@ +197,5 @@
>      MmiManager.handleMMIReceived(message, sessionEnded);
>    },
>  
> +  mTeardown: function mmmc_mTeardown() {
> +    // Back to the initial state

I wonder if using a mSetup instead would not be more logical. But that's a matter of style, do what you think is better.

::: apps/communications/dialer/test/unit/mock_moztelephony.js
@@ +8,5 @@
> +
> +  mTeardown: function() {
> +    this.active = null;
> +    this.dial = function() {};
> +    this.dialEmergency = function() {};

is it really necessary to redefine the functions in mTeardown ? When using sinon they should be restored automatically

::: apps/communications/dialer/test/unit/suggestion_bar_test.js
@@ +21,3 @@
>  
>  suite('suggestion Bar', function() {
> +  var mocksHelper = mocksHelperForSuggestionBar;

you just need to call `mocksHelperForSuggestionBar.attachTestHelpers()` and it should attach setup/teardown/suiteSetup/suiteTeardown

::: apps/communications/dialer/test/unit/telephony_helper_test.js
@@ +49,5 @@
> +  setup(function() {
> +    mocksHelper.setup();
> +
> +    spyConfirmShow = sinon.spy(ConfirmDialog, 'show');
> +    mockTelephony = sinon.mock(MockMozTelephony);

if you use this.sinon instead of sinon, they should be automatically restored.

(although there could be a bug for the top-level suites that I also hit in bug 891811 and that I resolved using my own sandbox, see [1], could be cleaner. Is that because of that bug that you do your restore yourself ?).

[1] https://github.com/mozilla-b2g/gaia/pull/10891/files#L4R131

@@ +61,5 @@
> +    mockTelephony.restore();
> +    MockMozTelephony.mTeardown();
> +  });
> +
> +  function callError(name) {

createCallError ?

@@ +113,5 @@
> +    var mockCall;
> +
> +    setup(function() {
> +      mockCall = {};
> +      sinon.stub(MockMozTelephony, 'dial').returns(mockCall);

same remark here: if you use this.sinon instead of sinon, this will be restored automatically at the end of the test. In an inner suite I found no bug so this should work !

::: shared/test/unit/mocks/mocks_helper.js
@@ +22,5 @@
> +    });
> +    return this;
> +  },
> +
> +  attachTestHelpers: function mh_attachTestHelpers() {

can you add a comment "these functions are already bound to this in the constructor" ?

Comment 8

[Etienne Segonzac (:etienne)]

Attachment: Patch v3

Comment 9

[Julien Wajsberg [:julienw]]

Comment on attachment 785837 [details] [diff] [review]
Patch v3

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

mostly nits

I don't r+ with nits because I want to understand why they're badly failing in my setup, but they're failing also on master so we may file another bug for this. But let's see if I can find the problem quickly enough.

::: apps/communications/dialer/test/unit/handled_call_test.js
@@ +21,5 @@
> +  'Utils',
> +  'LazyL10n'
> +]);
> +
> +mocksHelperForHandledCall.init();

nit: in the most recent mocks_helper, you can do something like:

  var mocksHelperForHandledcall = new MocksHelper([
  ...
  }).init();

ie the "init" function returns "this". Just to make it a little more compact.

::: apps/communications/dialer/test/unit/mock_moztelephony.js
@@ +6,5 @@
>    active: null,
> +  calls: null,
> +
> +  mTeardown: function() {
> +    this.active = null;

why don't you reset "calls" as well ?

::: apps/communications/dialer/test/unit/telephony_helper_test.js
@@ +142,5 @@
> +        return mockCall;
> +      };
> +      navigator.mozTelephony.dialEmergency = function(number) {
> +        return mockCall;
> +      };

you can use this.sinon.stub here instead (like you do in other places)

@@ +182,5 @@
> +  test('should display a message if we didn\'t get a call back', function() {
> +    this.sinon.stub(MockMozTelephony, 'dial').returns(null);
> +    subject.call('123');
> +    spyConfirmShow.calledWith('unableToCallTitle', 'unableToCallMessage');
> +    MockMozTelephony.dial.restore();

nit: you don't need to restore

Comment 10

[Julien Wajsberg [:julienw]]

Comment on attachment 785837 [details] [diff] [review]
Patch v3

r=me with nits

The failures I see come from bug 898512 so it's unrelated. Since I can't run the tests I'll trust you on running the tests on Travis :)

Comment 11

[Etienne Segonzac (:etienne)]

Attachment: Final Patch

Final patch for reference, carrying the r+

Comment 12

[Etienne Segonzac (:etienne)]

(In reply to Julien Wajsberg [:julienw] from comment #10)
> I'll trust you on running the tests on Travis :)

Let's wait for the green before merging, here is the PR:
https://github.com/mozilla-b2g/gaia/pull/11381

Comment 13

[Etienne Segonzac (:etienne)]

https://github.com/mozilla-b2g/gaia/commit/71ea532a899102c0bbc150ad6dc7518066976b34

\o/