995425 - upgrade sinon to 1.9.1

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia, defect)

Description

[Zibi Braniecki [:zbraniecki][:gandalf]]

Currently used version of sinon does not support add|removeEventListeners on XHR.

The latest stable version: 1.9.1 does.

Comment 1

[Zibi Braniecki [:zbraniecki][:gandalf]]

It may also help with bug 982862

Comment 2

[Zibi Braniecki [:zbraniecki][:gandalf]]

It seems that there are 5 tests failing with the new sinon.

Two in system and three in communications.

All happen on this.sinon.stub().throws()

Example: https://github.com/mozilla-b2g/gaia/blob/master/apps/communications/dialer/test/unit/calls_handler_test.js#L1359

  5) [communications-dialer/test/unit/calls_handler_test.js] calls handler > headphone and bluetooth support > connecting to bluetooth headset should show the speaker button when connected if no bluetoothreceiver is available:
     Error: 
    at throwsException (http:
  /communications.gaiamobile.org:8080/common/vendor/sinon/sinon.js:1895:13)
      at (anonymous) (http://communications.gaiamobile.org:8080/common/vendor/sinon/sinon.js:2324:25)
      at (anonymous) (http://communications.gaiamobile.org:8080/dialer/test/unit/calls_handler_test.js:1359:1)

I'm learning sinon and trying to fix that.

Comment 3

[Zibi Braniecki [:zbraniecki][:gandalf]]

Attachment: patch to get sinon 1.9.1

Comment 4

[Zibi Braniecki [:zbraniecki][:gandalf]]

Attachment: patch to remove throws from tests

This patch removes the .throws from the tests and fixes the regressions caused by upgrade to sinon 1.9.1.

I'm not sure what the .throws() calls were for and if they should be preserved so I'll ask for r= from people who introduced them.

Comment 5

[Zibi Braniecki [:zbraniecki][:gandalf]]

The changeset in simon.js repo that introduces the bug is https://github.com/cjohansen/Sinon.JS/commit/7aa72b6589fbcf59a4be1dccd03ca63d25d81fd1

Comment 6

[Zibi Braniecki [:zbraniecki][:gandalf]]

Rexboy: you added the test in bug 907155. Can you help me identify what should happen with the .throws() call in the test in apps/system/test/unit/battery_manager_test.js:121 ?

Comment 7

[Zibi Braniecki [:zbraniecki][:gandalf]]

Borja: you added the test in bug 929388. Can you help me identify what should happen with the .throws() call in the test in apps/system/test/unit/fxa_manager_test.js:249 ?

Comment 8

[Zibi Braniecki [:zbraniecki][:gandalf]]

Actually, the three tests that use .throws in communications were also added by :rexboy. That should help with identifying proper fixes :)

Comment 9

[KM Lee [:rexboy] (inactivate after Sep. 22)]

Attachment: Quick fix for batteryManager Test

Comment 10

[KM Lee [:rexboy] (inactivate after Sep. 22)]

Attachment: Quick fix for batteryManager Test

(In reply to Zbigniew Braniecki [:gandalf] from comment #6)
> Rexboy: you added the test in bug 907155. Can you help me identify what
> should happen with the .throws() call in the test in
> apps/system/test/unit/battery_manager_test.js:121 ?

Here my intention is: define window.dispatchEvent to throw exceptions when
its first parameter is not 'batteryshutdown'.

The code I wrote is a little bit confusing.. We can get rid of it.
See my quick fix. And below are some descriptions to the old code:

I inserted two stubs in line:121 and line:122.
If parameter is 'batteryshutdown', the stub in line 122 is triggered;
else stub in line 121 is triggered and throw exception.

Seems the new version of Sinon may think the first stub should be triggered
even after second stub is triggered. Anyway we don't need to use exception
to finish the test here. You may confirm if other occurrences are in the
same case.

Comment 11

[Borja Salguero [:borjasalguero]]

(In reply to Zbigniew Braniecki [:gandalf] from comment #7)
> Can you help me identify what should happen with the .throws() call in the test in
> apps/system/test/unit/fxa_manager_test.js:249 ?

Im gonna forward this to Fernando, who was in charge of adding this test.

Comment 12

[Zibi Braniecki [:zbraniecki][:gandalf]]

(In reply to KM Lee [:rexboy][:蠟蠟蠟蠟] from comment #10)
> Seems the new version of Sinon may think the first stub should be triggered
> even after second stub is triggered. Anyway we don't need to use exception
> to finish the test here. You may confirm if other occurrences are in the
> same case.

If I understand the API correctly the way they recommend using throw is to chain it after a stub call. For example:

stub().withArgs(true).returns('yay!');
stub().withArgs(false).throws('oh no! nonononono!');

Comment 13

[Zibi Braniecki [:zbraniecki][:gandalf]]

Attachment: pull request

Ok, I followed your patch proposal with all use cases of .throws and passed travis tests.

Comment 14

[KM Lee [:rexboy] (inactivate after Sep. 22)]

Comment on attachment 8406501 [details] [review]
pull request

Thanks for the patch!
Some minor change needed, see Github comment.

Comment 15

[Zibi Braniecki [:zbraniecki][:gandalf]]

Comment on attachment 8406501 [details] [review]
pull request

Thanks for the comments! I updated the pull request. Can you take another look?

Comment 16

[KM Lee [:rexboy] (inactivate after Sep. 22)]

Comment on attachment 8406501 [details] [review]
pull request

r=me withe one change needed.

Thanks a lot!

Comment 17

[Zibi Braniecki [:zbraniecki][:gandalf]]

Cool! I applied your comment, travis is green. Waiting for the remaining r= and sr=

Comment 18

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Comment on attachment 8406501 [details] [review]
pull request

I really have no idea about which version of sinon we should use. Maybe Julien knows more.

Comment 19

[Julien Wajsberg [:julienw]]

Ok, I investigated a little to see what changed between the sinon we use (version 1.6.0) and newer sinon versions (I tested 1.8.1 and 1.9.1 which behaves the same for this issue).

In all versions, when you do:

  var stub = sinon.stub();
  stub.throws('string');

=> calling |stub()| will throw an error, with an empty message, and the name is 'string'. That's why the output in the previous errors is only "Error:", because there is an empty message. Moreover, since the Error object is intanciated in sinon at the moment "throws" is called (as opposed to when the stub is called), the line information won't be very useful, and even misleading.
As take away, if you use |throws('string')|, you should always specify a second parameter to set the message as well.

Now, the difference in behavior:

Let's take this code, 

  var stub = sinon.stub();
  stub.throws('this is an error');
  stub.withArgs('foo');

  stub('foo');

This won't throw in 1.6.0 but will throw in 1.9.1.

However:

  var stub = sinon.stub();
  stub.throws('this is an error');
  stub.withArgs('foo').returns(false);

  stub('foo');

This won't throw in version 1.6.0 and version 1.9.1.

It seems that using |withArgs| without a behavior is a noop in recent versions of sinon, which IMO is completely logical and expected.


Now, I added some comments on github, because I think the faulty assertions could be a lot better written using sinon's assertions (sinon.assert.calledWith and sinon.assert.calledWithMatch).

Hope this helps !

Comment 20

[Julien Wajsberg [:julienw]]

Comment on attachment 8406501 [details] [review]
pull request

I'm perfectly fine with updating sinon to the latest version, but please rewrite the faulty assertions using sinon's assertions.

Thanks !

Comment 21

[Fernando Jiménez Moreno [:ferjm]]

Comment on attachment 8406501 [details] [review]
pull request

It seems that the review is already in good hands :)

Comment 22

[Ryan VanderMeulen [:RyanVM]]

Master: https://github.com/mozilla-b2g/gaia/commit/9869eaa311e54248140731955496d44d1f578943

Comment 23

[Ryan VanderMeulen [:RyanVM]]

This was hitting Gaia unit test failures on TBPL. Reverted.
https://github.com/mozilla-b2g/gaia/commit/c528db5651d8ce7fc5715039abbd99163bd5eb3d

https://tbpl.mozilla.org/php/getParsedLog.php?id=37972796&tree=B2g-Inbound

gaia-unit-tests TEST-UNEXPECTED-FAIL | costcontrol/test/unit/widget_startup_test.js | Widget Startup Modes Test Suite > SIM is detected after an AirplaneMode message on a previous startup | TypeError: thisCall is null (http://costcontrol.gaiamobile.org:8080/common/vendor/sinon/sinon.js?time=1397701515843:1599)
gaia-unit-tests TEST-UNEXPECTED-FAIL | costcontrol/test/unit/widget_startup_test.js | "after each" hook | this.sinon is null

Comment 24

[Zibi Braniecki [:zbraniecki][:gandalf]]

Thanks Ryan!

I'm not sure why costcontrol is not fired with all gaia-unit-tests, but hey!

So, the bug is in this test: https://github.com/mozilla-b2g/gaia/blob/master/apps/costcontrol/test/unit/widget_startup_test.js#L224-L271

My understanding of what is going on:

It seems to me that the test is prone to race condition because it fires showSimErrorSpy.reset(); in line 242 but the callback fired earlier fires asynchronous function in line 110.

In result we reset the spy, but then the asynchronous function fires code on it. The result error stack looks like this:

invoke@http://costcontrol.gaiamobile.org:8080/common/vendor/sinon/sinon.js?time=1397715524935:1598:1
proxy@http://costcontrol.gaiamobile.org:8080/common/vendor/sinon/sinon.js?time=1397715524935 line 1519 > eval:1:26
_errorNoSim/<@http://costcontrol.gaiamobile.org:8080/js/widget.js?time=1397715525163:493:9
MockAirplaneModeHelper.ready@http://costcontrol.gaiamobile.org:8080/test/unit/mock_airplane_mode_helper.js?time=1397715525132:14:7
_errorNoSim@http://costcontrol.gaiamobile.org:8080/js/widget.js?time=1397715525163:488:7
failingLoadDataSIMIccId/<@http://costcontrol.gaiamobile.org:8080/test/unit/widget_startup_test.js?time=1397715525032:110:8

In result the spy.invoke in sinon.js:1598 is trying to access thisCall which is null after reset.

Julien, I'm sorry for bothering you so much, but can you help me identify how it should be fixed? I'm still not sure if I fully understand sinon, and this test is very confusing for me.

Comment 25

[Zibi Braniecki [:zbraniecki][:gandalf]]

Also, maybe Marina will be able to help here.

Marine: One of the tests you just landed in bug 993949 does not fly with sinon 1.9.1. Could you help me identify the fix?

Comment 26

[Julien Wajsberg [:julienw]]

(In reply to Zbigniew Braniecki [:gandalf] from comment #24)
> Thanks Ryan!
> 
> I'm not sure why costcontrol is not fired with all gaia-unit-tests, but hey!

Yeah I don't know why this did not show up in Travis. Maybe because it's racy :)

> 
> So, the bug is in this test:
> https://github.com/mozilla-b2g/gaia/blob/master/apps/costcontrol/test/unit/
> widget_startup_test.js#L224-L271
> 
> My understanding of what is going on:
> 
> It seems to me that the test is prone to race condition because it fires
> showSimErrorSpy.reset(); in line 242 but the callback fired earlier fires
> asynchronous function in line 110.
> 
> In result we reset the spy, but then the asynchronous function fires code on
> it. The result error stack looks like this:
> 
> invoke@http://costcontrol.gaiamobile.org:8080/common/vendor/sinon/sinon.
> js?time=1397715524935:1598:1
> proxy@http://costcontrol.gaiamobile.org:8080/common/vendor/sinon/sinon.
> js?time=1397715524935 line 1519 > eval:1:26
> _errorNoSim/<@http://costcontrol.gaiamobile.org:8080/js/widget.
> js?time=1397715525163:493:9
> MockAirplaneModeHelper.ready@http://costcontrol.gaiamobile.org:8080/test/
> unit/mock_airplane_mode_helper.js?time=1397715525132:14:7
> _errorNoSim@http://costcontrol.gaiamobile.org:8080/js/widget.
> js?time=1397715525163:488:7
> failingLoadDataSIMIccId/<@http://costcontrol.gaiamobile.org:8080/test/unit/
> widget_startup_test.js?time=1397715525032:110:8
> 
> In result the spy.invoke in sinon.js:1598 is trying to access thisCall which
> is null after reset.
> 
> Julien, I'm sorry for bothering you so much, but can you help me identify
> how it should be fixed? I'm still not sure if I fully understand sinon, and
> this test is very confusing for me.

I don't like the style of the text either; restoring original stubs in the middle of the test sounds bad, not to mention the use of assertions in stub callbacks, and the use of a setTimeout. I think your analysis is correct.

Instead, we should really look at the "yield" function of sinon (not "yields") or "callArg" (not "callsArg") to control synchronously when a callback is called, instead of relying on setTimeout and stub functions.

Maybe the quick fix is simply to convert "failingLoadDataSIMIccId" to a stub and call "callsArg" "manually" after Widget.init().

I can help to rewrite the test if necessary.

This is kind of funny that the sinon change exposed this more...

Comment 27

[Zibi Braniecki [:zbraniecki][:gandalf]]

Attachment: pull request

updated pull request with r+ carried over from rexboy and the costcontrol test fix r=julienw

Comment 28

[Zibi Braniecki [:zbraniecki][:gandalf]]

travis is green

Comment 29

[Ryan VanderMeulen [:RyanVM]]

Master: https://github.com/mozilla-b2g/gaia/commit/a338645d27c509f064ac3365ca2acc50a08fceb5

Comment 30

[Julien Wajsberg [:julienw]]

a=tests
uplifted to v1.4 to fix bug 1016852: 83cae7b7510fc837e27ecd0e298aa462e61f7761

Comment 31

[Julien Wajsberg [:julienw]]

tentative to do the same on v1.3t for the same reason: https://github.com/mozilla-b2g/gaia/pull/20351

Comment 32

[Julien Wajsberg [:julienw]]

v1.3t: 7eb215880944cae583bd7aa77a463c9176613715