fix: add better error checking and message #5911 #5311

[jleach]

Summary

I ran into the bug described in #5911 #5311 (et al) and found it tough to figure out what was causing the problem.

Test plan

The command:
NODE_ENV=test npx jest -I __tests__

Now triggers the error case:

(node:1915) UnhandledPromiseRejectionWarning: Error: Caught error after test environment was torn down

[facebook-github-bot]

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need the corporate CLA signed.

If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks!

[facebook-github-bot]

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

[facebook-github-bot]

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

[SimenB]

Thanks for sending a PR!

Could you run yarn eslint packages/jest-jasmine2/src/jasmine/Env.js --fix to remove the unrelated whitespace changes?

This also needs a test and a changelog entry 🙂

[SimenB]

can move this above checkIsError as that's not necessary if we throw here.

[SimenB]

@aaronabramov does circus behave correctly here already?

[aaronabramov]

i think so! but i'm not 100% sure :) i remember we did some handling there while we were in LON

[jleach]

@SimenB Will do.

[jleach]

Hey. I'm going to cancel this. I've tried quite a bit to get the test running yarn run test-ci-partial or yarn test with no luck (too many missing packages etc). Also I have no idea where to add a test.

[SimenB]

@jleach sorry about the slow response. What do you mean missing packages? Have you followed https://github.com/facebook/jest/blob/master/CONTRIBUTING.md#workflow-and-pull-requests?

[SimenB]

Best way to test it is probably to add an e2e test if you have a good reproduction. A quick reproduction I threw together:

test('a failing test', done => {
  setTimeout(() => done('fail async'), 5);
  done();
});

You can look at other tests on the e2e directory for examples of other integration tests.

With jasmine it gives

Circus:

With the changes in this PR, I get the following error from jasmine:

@aaronabramov not sure if I think Circus does this correctly. Should we try to throw an error if stuff is done after the test is complete?

[SimenB]

@jleach wanna pick this up again? 🙂

[nathany]

@SimenB You may already be familiar with this, but I've found "apply mail" (with hub) is very helpful when a PR is mostly there but just needs a CHANGELOG and so on.
https://www.spreedly.com/2014/06/24/merge-pull-request-considered-harmful/

[SimenB]

@nathany that article is a bit dated - maintainers can push to PRs (so I can add the changelog on github), and we squash merge for the clean history.

However, the issue with this PR is mostly that it does not have a test. We also would have to make sure circus behaves correctly 🙂

[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.