891811 - [sms] tests: properly use sinon so that we have no side effects between suites

Status: RESOLVED FIXED

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

Description

[Julien Wajsberg [:julienw]]

In my setup, one test was failing because another test was modifying one object without restoring it afterwards. This failure depends on the environment because it really depends on which test file is picked before, therefore we don't see it on travis, but we might see it elsewhere (eg: tbpl).

I took this as an opportunity to properly use the provided sinon sandbox so that the restore calls are implicit.

Comment 1

[Julien Wajsberg [:julienw]]

Attachment: patch v1

PR is at https://github.com/mozilla-b2g/gaia/pull/10891

Comment 2

[Julien Wajsberg [:julienw]]

Also, I've seen at lease one place where restore was not called, and another place where a spy was used whereas a stub was wanted (and the test was still working by the way)

Comment 3

[Corey Frang [:gnarf]]

Comment on attachment 773214 [details] [diff] [review]
patch v1

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

Couple of spots that we will have some merge dirt if my bug lands first, otherwise r=me (if you can answer the "for some reason" even better, but not blocking on it)

::: apps/sms/test/unit/compose_test.js
@@ +496,5 @@
>        this.blob = new Blob(['test'], {type: 'image/png'});
>        this.attachment = mockAttachment();
>        Compose.append(this.attachment);
> +      this.sinon.stub(AttachmentMenu, 'open');
> +      this.sinon.stub(AttachmentMenu, 'close');

My patch for 889735 is already doing this one

@@ +508,5 @@
>      });
>      suite('clicking on buttons', function() {
>        suite('view', function() {
>          setup(function() {
> +          this.sinon.stub(this.attachment, 'view');

same

::: apps/sms/test/unit/sms_test.js
@@ +123,5 @@
>      window.removeEventListener('hashchange', boundOnHashChange);
>    });
>  
> +  setup(function() {
> +    // for some reason, the main sinon sandbox provided by the test agent is not

I hate seing "for some reason" - Do we know why, can we find out why?

Comment 4

[Julien Wajsberg [:julienw]]

Attachment: patch v2

carrying r=gnarf

PR is still at https://github.com/mozilla-b2g/gaia/pull/10891

removed the sandbox hack now that bug 891927 is fixed.

Comment 5

[Julien Wajsberg [:julienw]]

travis is happy
master: da54242450d84f68ad10ffe73e14c4bfdcec3d3e

Comment 6

[Julien Wajsberg [:julienw]]

John, would you uplift this with a=tests please ?

Comment 7

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/018116ac84f8

Comment 8

[John Ford [:jhford] CET/CEST Berlin Time]

[v1-train 6644c36]

Comment 9

[John Ford [:jhford] CET/CEST Berlin Time]

v1.1.0hd: 6644c367ebf26ad17c0f1860aeec84a1557247e8