867885 - Write a test for bug 867649: Test that SMS app acquires CPU wake lock after receiving system message and then releases it after displaying a notification to the user

Status: RESOLVED FIXED

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

Description

[Justin Lebar (not reading bugmail)]

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

Comment 1

[Julien Wajsberg [:julienw]]

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

Comment 2

[Julien Wajsberg [:julienw]]

Attachment: WIP patch v1

Comment 3

[Julien Wajsberg [:julienw]]

Attachment: patch v1

* 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 4

[Corey Frang [:gnarf]]

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?

Comment 5

[Julien Wajsberg [:julienw]]

(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 !

Comment 6

[Corey Frang [:gnarf]]

(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.

Comment 7

[Julien Wajsberg [:julienw]]

Attachment: patch v2

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.

Comment 8

[Julien Wajsberg [:julienw]]

unrelated failures on Travis.

master: 933fe64315ff7445aa619b16bf08832f9b5faaf0

Comment 9

[Julien Wajsberg [:julienw]]

a=npotb
v1-train: b09378e2ec19aabaff18bf9582b7230f6a28c49c

Comment 10

[Julien Wajsberg [:julienw]]

(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.