Swallowing errors

[mattheworiordan]

I have come across a similar problem to the EventEmitter swallowing discussion with another callback, see 93e3c4a for an example of the problem.

Here's the result of the Jasmine test being run:

$ grunt test:jasmine --spec spec/realtime/event_emitter.spec.js
Running "jasmine" task
Running Jasmine test suite against spec/realtime/event_emitter.spec.js
Started
Test App rzabrg has been set up
Realtime EventEmitter
  swallows exceptions, does not log the error, and leaves the suite hanging on failure waiting for a timeout ...
{ statusCode: 401,
  code: 40160,
  message: 'Channel denied access based on given capability; channelId = doesNotHavePermission' }
F Failed
    Error: Timeout - Async callback was not invoked within timeout specified by jasmine.DEFAULT_TIMEOUT_INTERVAL.
0 of 1 passed (0 skipped, 0 disabled).
Test suite behaviour
  stops immediately when an exception is raised in an async block ...

/Users/matthew/Projects/Ably/clients/ably-js/node_modules/chai/lib/chai/assertion.js:106
      throw new AssertionError(msg, {
            ^
AssertionError: failed
    at null._onTimeout (/Users/matthew/Projects/Ably/clients/ably-js/spec/realtime/event_emitter.spec.js:26:9)
    at Timer.listOnTimeout [as ontimeout] (timers.js:112:15)
>> Browser tests failed!

As you can see, the assert 93e3c4a#diff-611f6d447f261be4dc21af22d9c2348bR15 is simply swallowed by the Ably library, so no test can ever catch this error and fail the test for the right reason, instead it just times out. Equally no log message is shown.

If you look at 93e3c4a#diff-611f6d447f261be4dc21af22d9c2348bR26 however, that error is caught immediately and the test fails quickly.

I really don't like the idea that the client library is swallowing errors like this. I was actually trying to test the EventEmitter behaviour, and spent ages figuring out that the attach() block has the same behaviour. I believe we need a better solution.

[paddybyers]

Just because you throw an exception in a listener, that doesn't mean that that exception will ever propagate up to a catch block under the control of the test framework.

Using asserts in this way is IMO just broken.

[mattheworiordan]

I simply don't agree, and this is the idiomatic way for tests to run, the assert failures raise exceptions. This means that if anyone uses our Ably-JS library in their acceptance tests, and they have an error in a callback / closure on any one of our events, then they will never see the error and will never catch the error. That will apply to not just browsers but node as well. Given 99% of test suites that I am aware of catch exceptions, this to me is a big flaw in the design.