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

Introducing mocha.throwError for better async error handling #985

Merged
merged 1 commit into from
Dec 7, 2013

Conversation

jpbochi
Copy link
Contributor

@jpbochi jpbochi commented Sep 27, 2013

This mocha.throwError is supposed to be called by assertion libraries.

It will allows awesome diffs (like this) to be shown even when running async tests is a browser like phantomjs.

It's an alternative to #278 and #942

I plan to send a PR to chai, with a mirroring change.

@refack
Copy link

refack commented Oct 14, 2013

👍

@jpbochi
Copy link
Contributor Author

jpbochi commented Oct 15, 2013

@refack thanks for reviewing this.

travisjeffery added a commit that referenced this pull request Dec 7, 2013
Introducing mocha.throwError for better async error handling

* jpbochi/async-error-handling:
  introduced mocha.throwError for better async error handling
@travisjeffery travisjeffery merged commit 9e652eb into mochajs:master Dec 7, 2013
@jpbochi
Copy link
Contributor Author

jpbochi commented Dec 7, 2013

@travisjeffery Nice! I'll start working on an PR for chai right away.

if ('uncaughtException' == e) {
global.onerror = function() {};

var indexOfFn = uncaughtExceptionHandlers.indexOf(fn);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

way too verbose haha var i = would do :p

travisjeffery added a commit that referenced this pull request Dec 8, 2013
if ('uncaughtException' == e) {
global.onerror = function() {};

var indexOfFn = uncaughtExceptionHandlers.indexOf(fn);
if (indexOfFn != -1) { uncaughtExceptionHandlers.splice(indexOfFn, 1); }
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rename indexOfFn here, too? (I know this file is actually generated from the other)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nah, this is the one i fixed. the other one in mocha.js is the generated one. don't bother with that one.

@jpbochi
Copy link
Contributor Author

jpbochi commented Dec 8, 2013

@travisjeffery @visionmedia I just create this PR (chaijs/chai#222) for chai to use mocha.throwError whenever available. Please review. :neckbeard:

@christophercliff
Copy link

Has this made it into should.js yet?

@jpbochi
Copy link
Contributor Author

jpbochi commented Jan 18, 2014

@christophercliff nope. I made a PR to chai. I should probably have created one for should.js first, sinc it's maintained by the same folks.

I'll see if I create one this weekend.

@tj
Copy link
Contributor

tj commented Jan 18, 2014

actually now that im reading this again it doesn't quite make sense, it's only supported in the browser, and they're not uncaught, they're just assertion errors so those var names are a little wonky

@jpbochi
Copy link
Contributor Author

jpbochi commented Jan 20, 2014

@christophercliff There's one complication to implement that on should.js. Currently, there's no easy way to run its tests on a browser.

@visionmedia I'm figuring out a way to run all (or at least some) should.js's tests on a browser. Is it ok if I introduce karma there?

@jpbochi
Copy link
Contributor Author

jpbochi commented Jan 20, 2014

should.js relies on try+catch of AssertionError's for negative assertions. Unfortunately, this doesn't play out nice with mocha.throwError because the error will be reported to mocha even if you catch it later.

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

Successfully merging this pull request may close these issues.

None yet

5 participants