Handle async errors

[dignifiedquire]

The goal is to resolve the issues in #2059.

General ideas are

1. Attach uncaughtException and unhandledRejection handlers around the test suite execution

• if something is caught
  • during a spec run: fail the spec
  • outside a spec, inside a suite: fail the suite

2. Add support for the signature

test('my test', done => {
  done(new Error('fail'));
});

I have gotten 2. working, but ran into an issue with 1. I am able to catch the exceptions and set the errors from Env.js, but there is currently no way to cancel a spec run. To enable this I would need to change queue_runner.js such that it allows for cancellation. Before I go on I would like to get some feedback on this.

PS: This is the same approach that mocha currently takes to handle these: https://github.com/mochajs/mocha/blob/master/lib/runner.js#L681

[dignifiedquire]

I have added some basic cancelation handling into the queue_runner which makes the test in promise_it.test.js fail as expected now.

[daviddias]

Hi there! Really looking for this feature in jest. Any hopes of getting this merged soon? Thank you!

[dignifiedquire]

This should be ready, I am not sure why the CI is failing yarn test passes fine locally for me

[aaronabramov]

hey @wtgtybhertgeghgtwtg! do you mind giving this a look? :)

[wtgtybhertgeghgtwtg]

It looks like the issue is with resolving jest-runner. I can't seem to find any other issues like this, although jest-runner is only like six days old.

[dignifiedquire]

@wtgtybhertgeghgtwtg any pointers on how I can help debug this?

[wtgtybhertgeghgtwtg]

Nothing particularly helpful.

• Find module jest-runner.
• Use a package to make promises cancelable instead of rewriting the solution to reduce area.

[dignifiedquire]

• updated to use p-cancelable
• rebased onto master
• appveyor is failing with the same issue as on master: https://ci.appveyor.com/project/Daniel15/jest/branch/master

[dignifiedquire]

@wtgtybhertgeghgtwtg circle is happy now :) appveyor fail seems unrelated

[dignifiedquire]

@aaronabramov @wtgtybhertgeghgtwtg any more feedback? I would really like to see this merged soon as it is currently blocking me from migrating a bunch of projects over to jest.

[cpojer]

You cannot remove this, this is not related to tests.

[cpojer]

We also need to keep this one. The workers don't exclusively run Jasmine, and uncaught errors outside of Jasmine (like when no test is run) need to keep working. Is there a concept of "bubbling" in node's exception handling? I'm fine if this handler is disabled while the other exception handler is active, but at least one of the two needs to always be active.

[dignifiedquire]

I tried making it work with these handlers in place, but the issue was that as soon as they are here I am unable to capture the exceptions in Jasmine. (Same for the one in coverage_worker`). Is there an easy way to detect what is currently being run, so I can switch between those?

[cpojer]

I'm not sure, I would prefer some sort of nesting, where this handle here is the parent and the other handler you install is the child. Not sure how to make it work elegantly right now :(

[dignifiedquire]

I have pushed a version that removes and restores the listeners from jasmine, which seems to work well.

[cpojer]

"result"

[cpojer]

I was quite heavily against this approach, but I like the solution we ended up with because it captures all uncaught exceptions without overwriting setTimeout etc. for every single test, and something we should do regardless. Good work.

I'll spend some time testing this soon and giving it a thorough review, but we'll try to get this into Jest 21.

[cpojer]

This looks solid, so I'm went ahead and merged it, but I'm somewhat scared that this is going to mess everything up in some subtle way and we'll find out in a month or two ;) It's a scary change, but thank you @dignifiedquire for proposing it and making a PR.

If you'd like to catch errors thrown in timer functions, we do have FakeTimers which defines the timers. They are off by default, but what you could do, if you'd like to send another PR, is to change it so that setTimeout and others in a vm context (Every test file is run in isolation) is wrapped with a try-catch block that will catch errors and fail the test. What do you think?

[dignifiedquire]

🎉 great to see this merged, I hope the subtle breakages are not too bad ;)

Regarding the timers, thanks for the pointer I'll take a look in the next days and see if I can figure it out.

[cpojer]

Published jest@test (Jest 21 beta) so you can give this a try. Also enjoy a substantially faster Jest startup.

[dignifiedquire]

@cpojer not sure that publish (or my install) works as expected. I am not getting any jest binary anymore after installing. If I run ls node_modules/jest I get this:

ls node_modules/jest
browser.spec.js fixtures        test.spec.js
config          lint.spec.js    utils.spec.js

not even listing a package.json

[cpojer]

@dignifiedquire not sure what you did, but you definitely didn't install Jest. yarn add jest@test works just fine for me:

[dignifiedquire]

I think I hit a bug/feature in yarn, running yarn add jest@test with a folder called test just added that folder as a module Oo

[cpojer]

ohhh that's possible…

[jamesreggio]

Perhaps this is the wrong place to comment, but I'm fairly concerned about the change in semantics around the asynchronous done function. In my opinion, it's a major breaking change.

If the pre-21 done function ignores arguments, it's simple to supply a bare done reference as the recipient of any callback without concern about the arguments passed to the callback. As soon as it begins interpreting arguments as an error condition, all kinds of existing tests are going to break.

For example, the event argument received by the onPress callback will change this test from passing to failing.

it('fires the onPress callback', done => {
  const wrapper = shallow(<TouchableOpacity onPress={done} />);
  wrapper.find('TouchableOpacity').simulate('press');
});

I realize that interpreting arguments to done makes Jest more consistent with other JS testing frameworks, but I worry that the time for this convergence has sailed, especially since Jest offers done.fail as the preferred method for explicitly failing a test.

Sorry about the drive-by FUD. Just thought I'd mention it because this has become a subject of discussion internally.

[github-actions]

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.