Closed Bug 883276 Opened 7 years ago Closed 7 years ago

Cover TelephonyHelper with unit tests

Categories

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

x86_64
Linux
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: etienne, Assigned: etienne)

References

Details

Attachments

(1 file, 4 obsolete files)

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

Let's cover the rest of the file.
Assignee: nobody → etienne
Attached patch Patch proposal (obsolete) — Splinter Review
Attachment #762805 - Flags: review?(felash)
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
Attachment #762805 - Flags: review?(felash)
(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 :)
Attached patch Patch v2 (obsolete) — Splinter Review
Well worth it.
Attachment #762805 - Attachment is obsolete: true
Attachment #766542 - Flags: review?(felash)
Attached patch Patch v2 rebased (obsolete) — Splinter Review
Just rebased this patch.
Would appreciate a review before it gets bit-rotten again :)
Attachment #766542 - Attachment is obsolete: true
Attachment #766542 - Flags: review?(felash)
Attachment #773204 - Flags: review?(felash)
Comment on attachment 773204 [details] [diff] [review]
Patch v2 rebased

I'm doing some sinon stuff today, therefore I'll review it today :)
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" ?
Attachment #773204 - Flags: review?(felash)
Depends on: 891927
Attached patch Patch v3 (obsolete) — Splinter Review
Attachment #773204 - Attachment is obsolete: true
Attachment #785837 - Flags: review?(felash)
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
Attachment #785837 - Flags: review?(felash)
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 :)
Attachment #785837 - Flags: review+
Attached patch Final PatchSplinter Review
Final patch for reference, carrying the r+
Attachment #785837 - Attachment is obsolete: true
Attachment #786332 - Flags: review+
(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
https://github.com/mozilla-b2g/gaia/commit/71ea532a899102c0bbc150ad6dc7518066976b34

\o/
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.