Currently used version of sinon does not support add|removeEventListeners on XHR. The latest stable version: 1.9.1 does.

It may also help with bug 982862

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.

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.

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

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 ?

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 ?

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

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

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

(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!');

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

Comment on attachment 8406501 [details] pull request Thanks for the patch! Some minor change needed, see Github comment.

Comment on attachment 8406501 [details] pull request Thanks for the comments! I updated the pull request. Can you take another look?

Comment on attachment 8406501 [details] pull request r=me withe one change needed. Thanks a lot!

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

Comment on attachment 8406501 [details] pull request I really have no idea about which version of sinon we should use. Maybe Julien knows more.

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 on attachment 8406501 [details] 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 on attachment 8406501 [details] pull request It seems that the review is already in good hands :)

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

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

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.

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?

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

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

travis is green

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

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

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

v1.3t: 7eb215880944cae583bd7aa77a463c9176613715