sinon.logError should throw synchronously instead of after a timeout #172

Closed
airhorns opened this Issue Aug 20, 2012 · 7 comments

Comments

Projects
None yet
5 participants

In Sinon right now the following function gets executed when say a fake server encounters an error while responding to requests.

        logError: function (label, err) {
            var msg = label + " threw exception: "
            sinon.log(msg + "[" + err.name + "] " + err.message);
            if (err.stack) { sinon.log(err.stack); }

            setTimeout(function () {
                err.message = msg + err.message;
                throw err;
            }, 0);

The error is thrown after a timeout instead of synchronously. This means that if for example I want to test that my code errors if the server returns a 500, I can't wrap my test in a try catch because the error is caught and then thrown after the stack unwinds. I am sure there is a good reason why the error is through asynchronously right now but it would make it way easier to test code that throws on purpose in response to servers responding.

Yes please. This results weird and hard to trace errors when, for instance, an assertion fails inside of a fake server callback. With mocha syntax:

it('does xhr', function(done) {
  var server = sinon.fakeServer.create();
  $.get('http://example.com', function(result) {
    expect(result).to.equal('foo');
    done();
  });
  // server.respond() etc.
});

If the expectation, fails, the failure isn't even attributed to this example -- it gets attributed to some later test depending on timing artifacts.

Contributor

mroderick commented Aug 12, 2014

I don't think we should change the way that logError works by default, but I am considering putting something in place that would make it easier to debug situations like these.

We could do something like

sinon.logError.useImmediateExceptions = true;
// errors will now be thrown immediately and not asynchronously

I did something similar for PubSubJS as people were asking for help with debugging that as well. Perhaps this solution can also work here?

I have same issue. I'd like to throw an exception when there is an unexpected request instead of just returning 404.

var serverExpectations = {
    '/api/items': [200, { "Content-Type": "application/json" }, '{"some": "data"}']
};
this.server.respondWith(function(xhr) {
    var response = serverExpectations[xhr.url];
    if(xhr.method === 'GET' && response) {
        xhr.respond.apply(xhr, response);
    } else {
        throw new Error('Unexpected '+xhr.method+' '+xhr.url);
    }
});

But the following code doesn't work, because my exception are catched and I can't notice a possible error I my request logic.
Built-in Angular.JS fake server has this behaviour and I would like to see the same in Sinon.
So I vote up for the config, proposed by @mroderick

mroderick self-assigned this Feb 5, 2015

mroderick closed this in 288b363 Sep 15, 2015

@mroderick Thanks for adding the flag. Can you elaborate on why it's important to preserve the default behavior of using a timeout? From my perspective, it seems like the useImmediateExceptions = true behavior is more useful in the vast majority of cases, if not 100% of the time.

Contributor

mroderick commented Sep 15, 2015

Can you elaborate on why it's important to preserve the default behavior of using a timeout?

We're trying to be very careful about not breaking backwards compatibility, so that upgrading Sinon.JS in a project doesn't have to be a scary task. In the context of this ticket, it might seem most natural to change the default. However, there's likely to be many tests out there, that would fail in new and interesting ways if we change this behaviour.

I agree that we should probably change the default to be useImmediateExceptions = true, but I'd like to give the users some fair warning via some deprecation mechanism.

There has been some discussion on how best to get deprecation warnings to users. If you have good ideas on how to do this for a library like Sinon, then a new issue would be very welcome.

I've created cjohansen#841 to discuss any changes to defaults in the next major release.

Thanks, that makes sense! 👍

tombh commented Oct 14, 2015

Just wanted to say I got caught out with this and sadly lost a lot of frustrated hours tracking where my errors were being swallowed :(

But sinon.logError.useImmediateExceptions = true solved everything.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment