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

Error is not thrown when running inside Mocha test #266

Closed
xixixao opened this issue Jul 18, 2014 · 9 comments
Closed

Error is not thrown when running inside Mocha test #266

xixixao opened this issue Jul 18, 2014 · 9 comments

Comments

@xixixao
Copy link
Contributor

xixixao commented Jul 18, 2014

Ok, this is a really strange bug and quite likely might belong to Mocha. Given that I likely won't get help there, if anyone has any useful input, I will be grateful.

Promise = require 'bluebird'

describe 'test', ->
  it 'should throw an error', (done) ->
    Promise.delay 10
    .done ->
      throw new Error "bla"

This test times out. Note that all of the below simply thrown an error:

describe 'test', ->
  it 'does throw an error', (done) ->
    setTimeout ->
      throw new Error "bla"
    , 10
describe 'test', ->
  it 'does throw an error', (done) ->
    throw new Error "bla"

(this has nothing to do with time, I am just using it for an example). Thanks!

@petkaantonov
Copy link
Owner

You are not returning a promise from the test and you are not calling the done argument passed to your test so mocha treats it as synchronous test but errors thrown in done are thrown in the process asynchronously

@jamiebuilds
Copy link

@petkaantonov Coffeescript automatically adds the return. So that code example expands to:

var Promise;

Promise = require('bluebird');

describe('test', function() {
  return it('should throw an error', function(done) {
    return Promise.delay(10).done(function() {
      throw new Error("bla");
    });
  });
});

I've also tested Async errors without calling done() which works fine:

it('should throw Error', function(done) {
  throw new Error('Exit');
});

it('should throw Error inside setTimeout', function(done) {
  setTimeout(function() {
    throw new Error('Exit');
  }, 200);
});

The errors are reproducible without the done argument:

it('should throw Error inside promise', function() {
  return Promise.delay(10).done(function() {
    throw new Error('Exit');
  });
});
// >> No Error is thrown to Mocha

Although it appears to be a problem with Bluebirds done method, finally works:

it('should throw Error inside promise', function() {
  return Promise.delay(10).finally(function() {
    throw new Error('Exit');
  });
});
// >> Error is thrown

@tgriesser
Copy link
Contributor

.done doesn't return a promise, it returns undefined... so as @petkaantonov said, because you're not returning a promise, Mocha assumes it's sync - but it's not, you're throwing async.

Also worth noting, there's typically no need for using .done with bluebird as it's smart enough to log unhandled exceptions.

@domenic
Copy link

domenic commented Jul 18, 2014

Also promise-returning tests should use () not (done) in the header

@jamiebuilds
Copy link

@tgriesser I did not know .done didn't return a Promise. @xixixao This works fine:

it('should throw Error', function(done) {
  return Promise.delay(10).done(function(text) {
    throw new Error(text);
    done();
  });
});
// >> throws Error

@domenic
Copy link

domenic commented Jul 18, 2014

@thejameskyle Right because you are not using Mocha's promise-based async testing, but instead its done(). The two are incompatible.

@xixixao
Copy link
Contributor Author

xixixao commented Jul 18, 2014

@petkaantonov I don't think Mocha treats this as a synchronous test. If it did, the test wouldn't time out. Mocha only looks at the it callback, sees it takes an argument and hence waits for it to be called. See the setTimeout example.

@thejameskyle That code is equivalent to mine and indeed runs, but I am on a different machine atm. I'll have to retest on mine again tonight. Closing for now until I can reproduce it again.

@xixixao xixixao closed this as completed Jul 18, 2014
@domenic
Copy link

domenic commented Jul 18, 2014

Now that I am off my iPad and on something with a real keyboard: this is the best version IMO.

it('should throw Error', function() { // NOTE: no done
  return Promise.delay(10).then(function(text) { // NOTE: then, not done
    throw new Error(text);
  });
});

@benjamingr
Copy link
Collaborator

Not to be a drag, but although it's different (error created at different time) I'd do:

it('should throw Error', function() { // NOTE: no done
  return Promise.delay(10).throw(new Error(text));
});

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

6 participants