Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Errors in Async aren't caught correctly #2785

Closed
jonstorer opened this issue May 3, 2017 · 1 comment
Closed

Errors in Async aren't caught correctly #2785

jonstorer opened this issue May 3, 2017 · 1 comment

Comments

@jonstorer
Copy link

jonstorer commented May 3, 2017

This feels very similar to #1128, however, that solution was helped along with promises. I can't find a way to solve this with promises. My specific example is when I'm testing an Express.js controller:

// the Controller
let Promise = require('bluebird');

var controller = function (req, res, next) {
  let promise = new Promise(function(resolve) {
    setTimeout(resolve);
  });

  promise.then(function () {
    res.redirect('/otherPath');
  });
};

// the Test
let expect = require('chai').expect;

it('propagates errors correctly', function (done) {
  let res = {
    redirect: function(route) {
      expect(route).to.eql('/somePath');
      done();
    }
  };

  controller({}, res, function () {});
});

There is a promise involved which causes the mocha test runner to lose context, however, the promise happens in the controller, then resolves through a callback.

Here it is in JSFiddle... https://jsfiddle.net/ccwapndu/ (ProTip: open the console)

@ScottFreeCode
Copy link
Contributor

If your promise is swallowing the exceptions because it does not use catch and does not pass the promise out/back to anything that could add catch, it's at risk for hiding errors outside of test cases, so you'll want to change the promise usage to fix that in any case -- other than that ultimately you'll need to report the resulting error/rejection to done there's not much about Mocha one way or the other, there...

...Except that Mocha could do a better job of reporting the problem, for which we have issue #2640. (Closing this as a duplicate of that, since that's the solution -- or the closest thing we can get to one anyway -- on Mocha's end.)

For your specific case, however: I'm fairly sure* the third next parameter to Express handlers, besides being useful for middleware, is also useful for asynchronous error reporting... In fact, it functions pretty much identical to Mocha's done -- call it with no argument, it continues to the next handler, call it with an argument, it treats the argument as an error. (This is unfortunately, to my knowledge, only documented at the end of a seemingly unrelated section halfway down the page on writing error handlers, and not on the pages about writing regular handlers that would actually do the next(<some error>) calling. ...I meant to submit a PR to fix that months ago; I should double-check whether I had anything drafted for it...) So what you'd want is to change your controller to be like this:

var controller = function (req, res, next) {
  // NOTE: if multiple different `then`s are being called on the initial promise instead of on the returned promise from prior `then`s, the `catch` would need to be added to the returned promise from each of those branches. I'm not sure if that comes up or not, based on the intermediate variable that's only used once in the example.
  new Promise(function(resolve) {
    setTimeout(resolve);
  }).then(function () {
    res.redirect('/otherPath');
  }).catch(next);
};

...and then just change the Mocha test to give the controller done as its next function:

  controller({}, res, done);

*(Let me know if this doesn't actually work -- I'm sketching it out from memory as I haven't had to use explicit error handling in a non-error Express route handler recently.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants