Skip to content

Uncaught exception helper #771

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

Closed
jamestalmage opened this issue Apr 19, 2016 · 17 comments
Closed

Uncaught exception helper #771

jamestalmage opened this issue Apr 19, 2016 · 17 comments
Labels
enhancement new functionality question

Comments

@jamestalmage
Copy link
Contributor

jamestalmage commented Apr 19, 2016

As an alternative (or possibly in addition) to using domains to try and track the source of an uncaught exception, what if we used a babel plugin to wrap function declarations?

test.cb(t => {
  setTimeout(() => {
     assert(something); // a failing assertion thrown here doesn't get grouped with the correct test.
     t.end();
  });
});

Becomes:

test(t => {
  setTimeout(t.__wrapFn(() => {
    assert(something);
    t.end();
  });
});

t.__wrapFn would just mark the error with the associated test before letting it continue to bubble up:

t.prototype.__wrapFn = function (fn) {
  var self = this;
  return function () {
    try {
      return fn.apply(this, arguments);
    } catch (e) {
      e.__associatedAvaTest = self;
      throw e;
    }
  }
}

Our uncaughtException handler could then detect the special property and fail the specific associated test instead of killing the whole process.

@sotojuan
Copy link
Contributor

Is there a babel plugin out there to do this?

@jamestalmage
Copy link
Contributor Author

No, it would mean developing our own - similar to babel-plugin-ava-throws-helper

@sindresorhus
Copy link
Member

I like the idea. Any potential drawbacks?

And we're probably never going to use domains anyways.

@novemberborn
Copy link
Member

Any potential drawbacks?

It wouldn't extend to library code but that can't be helped really.

I like it!

@novemberborn novemberborn added enhancement new functionality question labels Apr 20, 2016
@sotojuan
Copy link
Contributor

Looking into doing this since I'm interested in learning Babel stuff—just need more info. Are we just turning all anonymous callbacks inside tests to arguments to t.__wrapFn?

@jamestalmage
Copy link
Contributor Author

Hey @sotojuan - I've got a start on this already. I'm happy to give you a crack at it though. I'll push up what I've got and you can roll from there.

@sotojuan
Copy link
Contributor

@jamestalmage It's fine! It was either this or the tape -> AVA codemods. I'll work on those instead.

@jamestalmage
Copy link
Contributor Author

👍 jscodeshift is a much more beginner friendly API, though neither it or Babel are well documented.

You should familiarize yourself with ast-types (look at ast-types/def/core.js or the estree project to learn the basic AST types). jscodeshift has it's own API that wraps ast-types, but inside a jscodeshift forEach you are using the ast-types API.

If you find it overwhelming initially, then you're in the same boat as everybody else. Feel free to open issues on those projects or ping me on gitter. I watch all of them, contributing occasionally. I would like to contribute some docs to jscodeshift anyways.

@jamestalmage
Copy link
Contributor Author

Oh also make sure you start with astexplorer. It's a great resource to get started.

@ORESoftware
Copy link

this will work for one level of code - good idea - but instead of letting the error go to global scope I would suggest just using the value for self/this since you already have it in hand.

@ORESoftware
Copy link

ORESoftware commented May 13, 2016

is Babel capable of doing this for all types of asynchronous callbacks?

for example, Babel might recognize this:

test('foo', t => {
   setTimeout(function(){

   }, 100);
});

and convert it to

test('foo', t => {
   setTimeout(t.__wrapFn(function(){

   }), 100);
});

but what about something like this:

test('foo', t => {
   client.get('redis_key', function(err, reply){

   });
});

that would need to become:

test('foo', t => {
   client.get('redis_key', t.__wrapFn(function(err, reply){

   }));
});

I guess Babel can do that without any problems

@jamestalmage
Copy link
Contributor Author

jamestalmage commented May 13, 2016

is Babel capable of doing this for all types of asynchronous callbacks?

It can do it for the examples you listed, however there would be scenarios where it wouldn't work:

function commonCallback () {
    throw new Exception('this is uncaught');
}

test(t => {
  client.get('redis_key', commonCallback);
});

@ORESoftware
Copy link

Ok that's interesting, so yeah maybe on top of the Babel plugin you should make users aware of the potential need to manually call t.__wrap and in that case perhaps alias it with t.wrap = t.__wrap or whatever.

@jamestalmage
Copy link
Contributor Author

I keep wondering if it wouldn't just be easier to do domains. Domains would neatly handle everything this proposal would, plus some stuff it wouldn't.

@ORESoftware
Copy link

ORESoftware commented May 13, 2016

domains don't work with ES6 promises yet, which is a big problem, supposedly someone special is going to patch this at some point soon.

@jamestalmage
Copy link
Contributor Author

domains don't work with ES6 promises yet, which is a big problem

Yeah, that's not great. They seem to work with bluebird and core-js promises though, so that would cover a significant portion of users.

@novemberborn
Copy link
Member

Wrapping declarations is tricky given generators and async functions. The wrapped code suddenly runs in a non-generator/async function context, and will crash.

We've recently improved detection of hanging tests; throwing an uncaught exception before t.end() is called would leave that test hanging.

Aside from #583 I think we've done as much as we can to handle this scenario.

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

No branches or pull requests

5 participants