Better error messages when the jest environment is used after teardown by async code

[BernieSumption]

🚀 Feature Proposal

Provide better errors when async code uses the environment after the test suite finished executing. Something like "This method can't be called because the test suite has finished running. This can happen if you forget to return a promise from a test method that performs asynchronous operations."

Motivation

I just spent a while debugging the following issue:

Cannot read property 'runAllTimers' of null
TypeError: Cannot read property 'runAllTimers' of null
    at Object.runAllTimers (/Users/bernie/Documents/GitHub/amara-checkout/node_modules/jest-runtime/build/index.js:969:56)

Caused by a jest.runAllTimers(); in my code. Turns out, this method calls this._environment.fakeTimers.runAllTimers(), and _environment.fakeTimers was null because the environment had been torn down, because the test suite had finished executing.

The root cause was that I'd omitted an await before an async method in a test, and the test method itself was not async.

It would be great to be able to offer an error message that pointed the user to the source of the issue.

Example

n/a

Pitch

Why does this feature belong in the Jest core platform?

It's a tweak to help people use existing functionality correctly.

[SimenB]

Yeah, that makes sense! We've added similar errors for require after teardown (#5888). Wanna send a PR for it? The code lives in jest-runtime

[BernieSumption]

I'm not likely to have time for a PR in the next month, but I may return to it if nobody else has picked it up by then. Since I've been looking through the source I'll leave this thought here for me or someone else: In terms of implementation, I'd think that on teardown we could iterate over the properties of the `jestObject` and replace all functions with a function that throws an error. That seems more reasonable than adding conditions in all the places where the code could fail as a result of the environment being torn down.

…

On Thu, 4 Oct 2018 at 09:25, Simen Bekkhus ***@***.***> wrote: Yeah, that makes sense! We've added similar errors for require after teardown (#5888 <#5888>). Wanna send a PR for it? The code lives in jest-runtime — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#7099 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAzNSA3oxkPP1yW_l1mJCdl8qiD5K9vCks5uhcXpgaJpZM4XHjcn> .

[ritanorinho]

Hello, me and another student are trying to fix this issue for a course in college. However, it's our first time working with jest so we are not so sure which files to run to reproduce this issue... Can you please replecate it in repl.it?

[SimenB]

That's awesome, @ritanorinho! A quick repl.it showing the error: https://repl.it/repls/HurtfulSuddenGraphics

Feel free to ask questions if you're stuck or our contributing docs are lacking 🙂

[SimenB]

I can give a quick start, though.

The relevant code lives here: https://github.com/facebook/jest/blob/b502c07e4b5f24f491f6bf840bdf298a979ec0e7/packages/jest-runtime/src/index.js#L922-L963

WHat you need to do is add a guard around every access on this._environment that throws an explicit error that the environment has already been torn down. You can look at how I solved it in #5888 to get an idea how the error message could look like.

Should probably have a _getTestEnvironemt function that does e.g.

if (this._environment) {
  return this._environment;
}

throw new PrettyError();

[ritanorinho]

Hello! Thank you so much for the quick reply. We just have one question,we are trying to reproduce the error locally, but we are not sure in which folder we should add the test files... Once again, thank you for the help!!

[SimenB]

What I would do in this case is create an integration test - the setup and teardown of a unit test at this level is quite involved and brittle IMO. Again, I'd like to point you to #5888 (although, not that integrations-tests directory has been renamed to just e2e since that).

File under test: https://github.com/facebook/jest/blob/053b74128d7dbff8ad423e85ff45ea167c23f192/e2e/require-after-teardown/__tests__/late-require.test.js
Test: https://github.com/facebook/jest/blob/053b74128d7dbff8ad423e85ff45ea167c23f192/e2e/__tests__/require_after_teardown.test.js

Instead of doing const double = require('../'); you can do e.g. jest.runAllTimers(); (see the repl.it link I provided above). Jest should then throw the PrettyError you create.

Our contributing guide has some more information on how to write and run integrations tests: https://github.com/facebook/jest/blob/053b74128d7dbff8ad423e85ff45ea167c23f192/CONTRIBUTING.md#integration-tests

Does that answer your question?

[ritanorinho]

After creating a test folder and trying to reproduce locally the error we got this error. Do you know what's happening?


Btw, we also tried with the link in the repl but there is no build folder in jest-runtime.
Thank you for the help!!

[JacobBlomgren]

@ritanorinho Don't know if you're still stuck, but that happens because node is run on a non-transpiled file. Try to run yarn build in the jest root and you should get a build folder in jest-runtime.

[github-actions]

This issue 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.