Closed
Bug 1039643
Opened 10 years ago
Closed 10 years ago
Write some unit tests for shared/js/dialer/contacts.js
Categories
(Firefox OS Graveyard :: Gaia::Dialer, defect)
Firefox OS Graveyard
Gaia::Dialer
Tracking
(Not tracked)
RESOLVED
FIXED
2.1 S2 (15aug)
People
(Reporter: rik, Assigned: thills)
Details
(Whiteboard: [planned-sprint c=][in-sprint=v2.1-S1])
Attachments
(1 file, 7 obsolete files)
It is unacceptable that this library still has no unit tests. We should write some so that later modifications have a foundation to work from.
Updated•10 years ago
|
Assignee: nobody → thills
Whiteboard: [planned-sprint] → [planned-sprint c=4]
Assignee | ||
Updated•10 years ago
|
Status: NEW → ASSIGNED
Assignee | ||
Comment 1•10 years ago
|
||
Hi Anthony, here's where I'm currently heading with these tests. Looking for feedback. Thanks! -tamara
Attachment #8463195 -
Flags: feedback?(anthony)
Reporter | ||
Updated•10 years ago
|
Attachment #8463195 -
Attachment is patch: true
Attachment #8463195 -
Attachment mime type: text/x-patch → text/plain
Reporter | ||
Comment 2•10 years ago
|
||
Comment on attachment 8463195 [details] [diff] [review] feedback tests Review of attachment 8463195 [details] [diff] [review]: ----------------------------------------------------------------- Because we are not looking for a perfect coverage here, I'm not going to be picky about the tests that are written. Except one :) I'd like to see something testing the data that those two methods return in the general case. Don't hesitate to ask me for live IRC help or even via video. ::: apps/communications/dialer/test/unit/contacts_test.js @@ +1,1 @@ > +/*global mocha, MocksHelper, MockAttachment, MockL10n, loadBodyHTML, ThreadUI, This file should live in apps/sharedtest/test/unit/dialer since it is shared with callscreen. @@ +17,5 @@ > +mocha.setup({ globals: ['alert'] }); > + > +// For Desktop testing > +if (!navigator.mozContacts) { > + require('/js/desktop_contact_mock.js'); It seems this file is not included. But it shouldn't be necessary. @@ +20,5 @@ > +if (!navigator.mozContacts) { > + require('/js/desktop_contact_mock.js'); > +} > + > +require('/js/utils.js'); This should be a mock. We have one :) @@ +50,5 @@ > +suite('dialer/contacts', function() { > + var realMozContacts; > + var realSimplePhoneMatcher; > + var realFb; > + var realAsyncStorage; realSimplePhoneMatcher, realFb and realAsyncStorage should not be needed since MocksHelper takes care of this for us. If MocksHelper is not helping, then we should remove them from the mocksHelperForContacts array. @@ +81,5 @@ > + SimplePhoneMatcher = new MockSimplePhoneMatcherObj(); > + fb = new MockfbObj(aFacebookTestContact); > + asyncStorage = MockasyncStorage; > + > + mocksHelper.suiteSetup(); Rather than initialising this here, you can do mocksHelperForContacts.attachTestHelpers() in the suite(). It will install suiteSetup, suiteTeardown, suite and teardown for each helper. @@ +120,5 @@ > + > + test('loads Facebook (FB)', function(done) { > + Contacts.findByNumber('333', function(contact, matchingTel) { > + }); > + sinon.assert.calledWith(MockLazyLoader.load, fbFiles); No need to test the lazy loading. This is an implementation detail. @@ +134,5 @@ > + > + Contacts.findByNumber('333', function(contact, matchingTel) { > + sinon.assert.calledWith(navigator.mozContacts.find, options); > + }); > + done(); You can remove the done() call if you remove the done parameter. It is only useful when dealing with asynchronous code. @@ +137,5 @@ > + }); > + done(); > + }); > + > + test('should create variants for number >= 7 digits', function(done) { This is not testing that it creates variants. A better name for the previous test and this one would be: "should find the exact number for <7 digits" "should find numbers matching for >=7 digits" @@ +178,5 @@ > + test('should return null if no FB or MozContact found', function(done) { > + navigator.mozContacts.setEmptyContacts(); > + Mockfb.setEmptyContacts(); > + Contacts.findByNumber('3334445555', function(contact, matchingTel) { > + sinon.match(!contact); You can do assert.isNull(contact) here. @@ +211,5 @@ > + this.sinon.spy(MockasyncStorage, 'getItem'); > + this.sinon.spy(navigator.mozContacts, 'find'); > + }); > + > + test('should call asynchstorage to getItem', function(done) { This test is not needed since the two other tests are verifying that the sort order depends on the value of asyncStorage. So that means we queried asyncStorage. ::: apps/communications/dialer/test/unit/mock_async_storage.js @@ +1,1 @@ > +/*exported MockasyncStorage */ Don't create a new mock, we already have one in shared/test/unit/mocks/mock_async_storage.js. ::: apps/communications/dialer/test/unit/mock_fb.js @@ +1,1 @@ > +'use strict'; We already have one in apps/communications/contacts/test/unit/mock_fb.js ::: apps/communications/dialer/test/unit/mock_simple_phone_matcher.js @@ +1,1 @@ > +'use strict'; We already have one in apps/callscreen/test/unit/mock_simple_phone_matcher.js. We should move it to shared/test/unit/mocks and extend it. ::: shared/test/unit/mocks/mock_navigator_moz_contacts.js @@ +1,1 @@ > +'use strict'; We already have one mock in apps/communications/contacts/test/unit/mock_mozContacts.js
Attachment #8463195 -
Flags: feedback?(anthony)
Assignee | ||
Comment 3•10 years ago
|
||
Attachment #8463195 -
Attachment is obsolete: true
Attachment #8464667 -
Flags: feedback?(anthony)
Assignee | ||
Comment 4•10 years ago
|
||
Attachment #8464667 -
Attachment is obsolete: true
Attachment #8464667 -
Flags: feedback?(anthony)
Attachment #8464681 -
Flags: feedback?(anthony)
Assignee | ||
Comment 5•10 years ago
|
||
Hi Anthony, This has a "workaround" for the Mockfb.Contact() issue. I tried moving everything out, but I think that will be a big refactor. Also, added some validation on some of the outputs.
Attachment #8464681 -
Attachment is obsolete: true
Attachment #8464681 -
Flags: feedback?(anthony)
Attachment #8465426 -
Flags: feedback?(anthony)
Assignee | ||
Comment 6•10 years ago
|
||
Updated
Attachment #8465426 -
Attachment is obsolete: true
Attachment #8465426 -
Flags: feedback?(anthony)
Attachment #8465501 -
Flags: feedback?(anthony)
Reporter | ||
Comment 7•10 years ago
|
||
Comment on attachment 8465501 [details] [diff] [review] 1039643v5.diff Review of attachment 8465501 [details] [diff] [review]: ----------------------------------------------------------------- Overall this is pretty good! I still have some comments but the core of it is good. ::: apps/callscreen/test/unit/calls_handler_test.js @@ +10,4 @@ > > require('/js/audio_competing_helper.js'); > require('/test/unit/mock_call_screen.js'); > +require('/shared/test/unit/mocks/mock_simple_phone_matcher.js'); Well done finding all the call sites. ::: apps/callscreen/test/unit/mock_simple_phone_matcher.js @@ -1,1 @@ > -var MockSimplePhoneMatcher = { We need to remove this file from build/jshint/xfail.list ::: apps/communications/contacts/test/unit/mock_mozContacts.js @@ +3,4 @@ > var MockMozContacts; > > function MockMozContactsObj(contacts) { > + this.contacts = new Array(contacts); Why are you doing this change? The contacts parameter is already an array. @@ +83,5 @@ > + return "eventstring"; > + }, > + > + setContacts: function(contacts) { > + this.contacts = new Array(contacts); Same comment, contacts should already be an array. ::: apps/sharedtest/test/unit/dialer/contacts_test.js @@ +42,5 @@ > + } > + ] > + }; > + > + var aFacebookTestContact = { This is not used, can remove. @@ +67,5 @@ > + > + suite('> Contacts Revision', function() { > + test('> Should return revision', function() { > + Contacts.getRevision(function(status) { > + assert(status); A better test would be assert.equal(status, "eventstring") @@ +77,5 @@ > + setup(function() { > + this.sinon.spy(navigator.mozContacts, 'find'); > + this.sinon.spy(Mockfb, 'isFbContact'); > + this.sinon.spy(Mockfb, 'getContactByNumber'); > + this.sinon.spy(Mockfb, 'getData'); I think some spies are not used anymore, so you can remove them. Also, if some spy is used in only one test, move the installation of that spy inside that test. ::: shared/test/unit/mocks/mock_async_storage.js @@ +3,5 @@ > 'use strict'; > > var MockasyncStorage = { > + orderLastName: true, > + getItem: function(key, callback) { Don't modify this mock, use asyncStorageGetStub.yield() to send the value you want.
Attachment #8465501 -
Flags: feedback?(anthony) → feedback+
Assignee | ||
Comment 8•10 years ago
|
||
Attached a link to the pull request. I addressed the issues in the feedback you last gave (Thank you!).
Attachment #8465501 -
Attachment is obsolete: true
Attachment #8465766 -
Flags: review?(anthony)
Reporter | ||
Comment 9•10 years ago
|
||
Comment on attachment 8465766 [details] [review] Link to pull request I've left all my comments in the pull request.
Attachment #8465766 -
Flags: review?(anthony) → feedback+
Assignee | ||
Comment 10•10 years ago
|
||
Hi Anthony, Thanks for the upgrades. I made the changes you suggested and it is reflected in the existing pull request: https://github.com/mozilla-b2g/gaia/pull/22377/. Thanks.
Attachment #8465766 -
Attachment is obsolete: true
Attachment #8466326 -
Flags: review?(anthony)
Reporter | ||
Comment 11•10 years ago
|
||
Comment on attachment 8466326 [details] [review] Updated commits on existing pull request after initial review Still some details to iron on the pull request.
Attachment #8466326 -
Flags: review?(anthony)
Updated•10 years ago
|
Whiteboard: [planned-sprint c=4] → [planned-sprint c=][in-sprint=v2.1-S1]
Target Milestone: 2.1 S1 (1aug) → 2.1 S2 (15aug)
Assignee | ||
Comment 12•10 years ago
|
||
Changes to 1) remove the additional requires 2) do a more thorough deepEquals of each object returned to validate the structure. Sorry, I realized I should have given you a separate diff so you can see the changes from last time *before* squashing all this. The only file that changed is contacts_test.js.
Attachment #8466326 -
Attachment is obsolete: true
Attachment #8468443 -
Flags: review?(anthony)
Reporter | ||
Comment 13•10 years ago
|
||
Comment on attachment 8468443 [details] [review] Updated pull request after review comments Yeah \o/
Attachment #8468443 -
Flags: review?(anthony) → review+
Reporter | ||
Comment 14•10 years ago
|
||
https://github.com/mozilla-b2g/gaia/commit/60271a311f4a55e22eb3bb51a7fbdb4c7ad9b78c
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•