See bug 867649 comment 3: Julien volunteered to write a unit test for this bug.

Bug 840075 will likely modify activity_handler.js to make it more easily testable, so I'll wait for this bug.

* Moved the wake lock and notification helper mocks to /shared * added lots of mocks, some very basic * added tests for the actibity handler --- apps/sms/test/unit/activity_handler_test.js | 125 ++++++++++++++++++-- apps/sms/test/unit/mock_black_list.js | 13 ++ apps/sms/test/unit/mock_contacts.js | 35 ++++++ apps/sms/test/unit/mock_messages.js | 53 +++++++++ apps/sms/test/unit/mock_navigator_moz_apps.js | 31 +++++ .../unit/mock_navigator_moz_set_message_handler.js | 35 ++++++ apps/sms/test/unit/mock_threads.js | 9 ++ apps/system/test/unit/app_install_manager_test.js | 2 +- apps/system/test/unit/mock_navigator_wake_lock.js | 41 ------- apps/system/test/unit/mock_notification_helper.js | 22 ---- apps/system/test/unit/update_manager_test.js | 2 +- shared/test/unit/mocks/mock_navigator_wake_lock.js | 41 +++++++ shared/test/unit/mocks/mock_notification_helper.js | 32 +++++ 13 files changed, 369 insertions(+), 72 deletions(-) create mode 100644 apps/sms/test/unit/mock_black_list.js create mode 100644 apps/sms/test/unit/mock_contacts.js create mode 100644 apps/sms/test/unit/mock_messages.js create mode 100644 apps/sms/test/unit/mock_navigator_moz_apps.js create mode 100644 apps/sms/test/unit/mock_navigator_moz_set_message_handler.js create mode 100644 apps/sms/test/unit/mock_threads.js delete mode 100644 apps/system/test/unit/mock_navigator_wake_lock.js delete mode 100644 apps/system/test/unit/mock_notification_helper.js create mode 100644 shared/test/unit/mocks/mock_navigator_wake_lock.js create mode 100644 shared/test/unit/mocks/mock_notification_helper.js see also PR https://github.com/mozilla-b2g/gaia/pull/10040 I tried to use sinon.js to do some mocks (especially the contacts mock) but they are too specific here.

Comment on attachment 754812 [details] [diff] [review] patch v1 Review of attachment 754812 [details] [diff] [review]: ----------------------------------------------------------------- Couple of random comments, none of it blocking though. Also, I assume you can tell me why we are removing mocks from apps/system? So long as the whole suite still passes: r=me ::: apps/sms/test/unit/activity_handler_test.js @@ +42,5 @@ > + realWakeLock = navigator.requestWakeLock; > + navigator.requestWakeLock = MockNavigatorWakeLock.requestWakeLock; > + > + realMozApps = navigator.mozApps; > + navigator.mozApps = MockNavigatormozApps; Seeing us add three of these in one test, might it be time to make MocksHelper able to do navigator. namespace replaces as well? @@ +80,5 @@ > > setup(function() { > this.prevHash = window.location.hash; > + > + shareActivity = { Why use a var instead of this. ? @@ +135,5 @@ > + }); > + > + suite('after getSelf', function() { > + setup(function(done) { > + setTimeout(function() { done(); }); Could the response to getSelf be stubbed and then just called here instead of going async with the setup? ::: apps/sms/test/unit/mock_messages.js @@ +30,5 @@ > + id: 1, > + threadId: 1, > + sender: 'sender', > + receivers: ['receiver'], > + body: 'body', mms don't have a body @@ +37,5 @@ > + timestamp: Date.now(), > + read: true, > + subject: '', > + smil: null, > + attachments: [], [new Blob(['body'], {type: 'text/plain'})] ::: apps/sms/test/unit/mock_navigator_moz_apps.js @@ +3,5 @@ > +var MockNavigatormozApps = { > + getSelf: function mnma_getSelf() { > + var request = {}; > + > + setTimeout(function nextTick() { Could we make this a method we can call from the test setup rather than force an async here?

(In reply to Corey Frang [:gnarf] [:gnarf37] from comment #4) > Comment on attachment 754812 [details] [diff] [review] > patch v1 > > Review of attachment 754812 [details] [diff] [review]: > ----------------------------------------------------------------- > > Couple of random comments, none of it blocking though. Also, I assume you > can tell me why we are removing mocks from apps/system? So long as the > whole suite still passes: r=me not removing, moving to /shared ;) > ::: apps/sms/test/unit/activity_handler_test.js > @@ +42,5 @@ > > + realWakeLock = navigator.requestWakeLock; > > + navigator.requestWakeLock = MockNavigatorWakeLock.requestWakeLock; > > + > > + realMozApps = navigator.mozApps; > > + navigator.mozApps = MockNavigatormozApps; > > Seeing us add three of these in one test, might it be time to make > MocksHelper able to do navigator. namespace replaces as well? I already tried this numerous time but I can't find a nice enough API. So for now I'm not doing it. > > @@ +80,5 @@ > > > > setup(function() { > > this.prevHash = window.location.hash; > > + > > + shareActivity = { > > Why use a var instead of this. ? Just a matter of habit, that's how I do for other tests, and I feel like this can lead to less unknown side-effects. I didn't want to change what was existing though. I can change this if you want though. You're the reviewer, you're the master ;) > > @@ +135,5 @@ > > + }); > > + > > + suite('after getSelf', function() { > > + setup(function(done) { > > + setTimeout(function() { done(); }); > > Could the response to getSelf be stubbed and then just called here instead > of going async with the setup? > ::: apps/sms/test/unit/mock_navigator_moz_apps.js > @@ +3,5 @@ > > +var MockNavigatormozApps = { > > + getSelf: function mnma_getSelf() { > > + var request = {}; > > + > > + setTimeout(function nextTick() { > > Could we make this a method we can call from the test setup rather than > force an async here? Yep, I can try this. You're right that calling a method or calling setTimeout from the test feels quite the same, and we would have the benefit of a more readable test. Thanks !

(In reply to Julien Wajsberg [:julienw] from comment #5) > > Why use a var instead of this. ? > > Just a matter of habit, that's how I do for other tests, and I feel like > this can lead to less unknown side-effects. I didn't want to change what was > existing though. > > I can change this if you want though. You're the reviewer, you're the master > ;) I'm not sure either is better, but we should make up our minds (soon) and use one. I think using the `this.` scope is more protected, each test is it's own namespace that way, you can't have a leaky `var`. Do whichever, both work.

carrying r=gnarf The mozApps mock feels really better now, therefore I promoted it to /shared, along with the setMessageHandler mock. Pushed to the PR https://github.com/mozilla-b2g/gaia/pull/10040, waiting for travis.

unrelated failures on Travis. master: 933fe64315ff7445aa619b16bf08832f9b5faaf0

a=npotb v1-train: b09378e2ec19aabaff18bf9582b7230f6a28c49c

(In reply to Corey Frang [:gnarf] [:gnarf37] from comment #6) > I'm not sure either is better, but we should make up our minds (soon) and > use one. I think using the `this.` scope is more protected, each test is > it's own namespace that way, you can't have a leaky `var`. > > Do whichever, both work. I read mocha's code and it looks like the context is shared between all suites/tests. If the this context is only shared to the current suite/test, then I'd be ok with using it, but I'd like that we investigate more before that.