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

RESOLVED FIXED

Status

Firefox OS
Gaia::SMS
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: julienw, Assigned: julienw)

Tracking

unspecified
x86_64
Linux

Firefox Tracking Flags

(b2g18 fixed, b2g-v1.1hd fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

5 years ago
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.
(Assignee)

Comment 1

5 years ago
Created attachment 773214 [details] [diff] [review]
patch v1

PR is at https://github.com/mozilla-b2g/gaia/pull/10891
Attachment #773214 - Flags: review?(gnarf37)
(Assignee)

Comment 2

5 years ago
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

5 years ago
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?

Updated

5 years ago
Attachment #773214 - Flags: review?(gnarf37) → review+
(Assignee)

Updated

5 years ago
Depends on: 891927
(Assignee)

Comment 4

5 years ago
Created attachment 773386 [details] [diff] [review]
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.
Attachment #773214 - Attachment is obsolete: true
Attachment #773386 - Flags: review+
(Assignee)

Comment 5

5 years ago
travis is happy
master: da54242450d84f68ad10ffe73e14c4bfdcec3d3e
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
(Assignee)

Comment 6

5 years ago
John, would you uplift this with a=tests please ?
Flags: needinfo?(jhford)
[v1-train 6644c36]
status-b2g18: --- → fixed
Flags: needinfo?(jhford)
v1.1.0hd: 6644c367ebf26ad17c0f1860aeec84a1557247e8
status-b2g-v1.1hd: --- → fixed
You need to log in before you can comment on or make changes to this bug.