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

try domains in a branch #513

Closed
tj opened this issue Jul 17, 2012 · 24 comments
Closed

try domains in a branch #513

tj opened this issue Jul 17, 2012 · 24 comments
Labels
type: feature enhancement proposal

Comments

@tj
Copy link
Contributor

tj commented Jul 17, 2012

this will fuck over client-side support nicely but yeah

@nemtsov
Copy link

nemtsov commented Jul 24, 2012

(+1) Just wrote a small connect middleware with a domain per request (as in the explicit binding section of the api) and in the process realized that I can't unit-test it. Even if the domain catches the error, the "uncaughtException" callback will still be called, and Runner#uncaught() will fail the test.

it('should not fail test', function (done) {
  var d = require('domain').create();
  d.on('error', function (er) {
    console.error('caught error:', er.message);
    done();
  });
  d.run(function () {
    process.nextTick(function () {
      throw new Error('test_err');
    });
  });
});

The example above, obviously, fails.

@christophsturm
Copy link

+1

4 similar comments
@fsvehla
Copy link

fsvehla commented Aug 31, 2012

+1

@telendt
Copy link

telendt commented Oct 8, 2012

+1

@jeffreyhorn
Copy link

+1

@vsagar
Copy link

vsagar commented Oct 17, 2012

+1

@evantahler
Copy link

+1

I've been trying to sort this one out as well. I've got a web framework that uses domains to catch request errors which I'd love to test. I've come up with a simple test case for how I would have expected Mocha to work here https://gist.github.com/3971280. I'm migrating from Vows to Mocha, and this this the only test I can't port.

I understand the desire to keep Mocha applicable for browsers as well, so is there a monkey-patch solution anyone has come up with for now?

@domenic
Copy link
Contributor

domenic commented Oct 30, 2012

Fwiw @dscape's specify was designed to work with domains. Never had the chance to use it myself though.

@CrabBot
Copy link
Contributor

CrabBot commented Dec 5, 2012

If you look at the domain module's uncaughtException handler, uncaughtHandler, you can see it sets err.domain. Wrapping Runner.prototype.uncaught in a process.nextTick and no-op'ing on err.domain != null seems to fix this issue.

In the meantime, it's a bit confusing as to what is happening if you're not familiar with how domains are implemented, b/c you're successfully catching the error, but the test still fails.

@dscape
Copy link

dscape commented Dec 5, 2012

fyi domains use uncaughtexception

@CrabBot
Copy link
Contributor

CrabBot commented Dec 5, 2012

Yup. I guess my point about it seeming confusing was in regards to domains not actually catching any errors, which has nothing to do with mocha. It belongs on the node issues, but domain.on('error') fails to prevent the error from still being 'uncaught', which is inconsistent with EventEmitter.emit('error') not throwing if there's a listener.

@dscape
Copy link

dscape commented Dec 5, 2012

Works in specify :P Remove your uncaught exceptions handlers :)

@evantahler
Copy link

@CrabBot cool solution, but I tired it out, and it doesn't look like it passes my test use case https://gist.github.com/3971280. In fact, it looks like the code isn't entering Runner.prototype.uncaught at all in this case (OSX, node 0.8.14)

@mcollina
Copy link

mcollina commented Jan 3, 2013

After digging in the code of both mocha and node, I implemented a simple workaround:
https://gist.github.com/4443963

Is it the way to go?

May it be possible to automate it in a function call like removeUncaughtExceptionHandler in the mocha API?

@evantahler
Copy link

A bit of a hack, but that works for me as well!

@CrabBot
Copy link
Contributor

CrabBot commented Jan 3, 2013

FWIW, going forward, domains will not use uncaughtException.

nodejs/node-v0.x-archive#4375

@CrabBot
Copy link
Contributor

CrabBot commented Jan 3, 2013

@evantahler If I recall correctly, there are two Runner.prototype.uncaughts in the code:

https://github.com/visionmedia/mocha/blob/master/lib/runner.js#L456-L473
https://github.com/visionmedia/mocha/blob/master/mocha.js#L4193-L4210

Again, if I recall correctly, adding it to the former is what worked for me. Did it not work for you?

@evantahler
Copy link

@CrabBot : @mcollina 's solution worked for me. You are correct that at the time of test invocation it does look like there are 2 functions bound to the global uncaughtException, and removing the last (newest) one was the trick.

If you are curious, here is the (now working!!!) test suite for actionHero which tests the notion of rendering a 500 internal error-type message to clients if something goes wrong: https://github.com/evantahler/actionHero/blob/master/test/core_exceptions.js

@sudsy
Copy link

sudsy commented Oct 24, 2013

Despite the fix in the node codebase from issue #4375 there still seems to be a problem testing while using node.js domains.

It seems that mocha wraps the test function in try catch blocks which means that any synchronous errors are caught by the try catch block and aren't ever caught by the domain.

I have detailed a test case in my answer to the following question.

http://stackoverflow.com/questions/19461234/domains-not-properly-catching-errors-while-testing-nodejs-in-mocha/19553909#19553909

While there is a workaround: Wrapping all tests in process.nextTick(function(){ ... });

It seem that a small change to the mocha codebase would make this unnecessary.

In my tests removal of the try catch blocks in runnable.js and runner.js alleviates the need for the workaround. If there is an unhandled synchronous error it is still caught by the uncaughtException handler by mocha and reported similarly to the case where the try catch block was in place.

I would make a pull request, but I am unsure of the impact to other (non nodejs) environments.

There are a number of possible approaches:

  • Remove try catch all together
  • Remove the try catch for node and leave it there for other environemnts
  • Replace the try catch blocks with a domain for node but try catch for other environments

Others?

What's the preferred way to detect the context we are running in for mocha and branch accordingly.

I am happy to make the changes if given guidelines.

@jonathanong
Copy link
Contributor

domains are deprecated :D

@evantahler
Copy link

Whoa! That's news to me. Google and http://blog.nodejs.org/ doesn't turn anything up... have a link?

@jonathanong
Copy link
Contributor

it was a conference. well he said, "i want to deprecate domains", so that's basically deprecating it

@mcollina
Copy link

mcollina commented Jul 8, 2014

I confirm.

@TimothyGu
Copy link
Contributor

@jonathanong said:

@evantahler said:

@jonathanong said:

domains are deprecated :D

Whoa! That's news to me. Google and http://blog.nodejs.org/ doesn't turn anything up... have a link?

it was a conference. well he said, "i want to deprecate domains", so that's basically deprecating it

OK, this is exceptionally lame.

  1. Domains are still not deprecated according to official docs.
    • I don't think oral deprecation is visible enough on the Internet or authoritative enough.
  2. People still use domains.
  3. People still use Mocha.js.
  4. People still throw exceptions synchronously.
  5. People still throw exceptions synchronously in domains in Mocha.js.
  6. This bug is closed.

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

No branches or pull requests