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

Modules appear to always be re-evaluated between test files #4413

Closed
zbjornson opened this issue Sep 3, 2017 · 9 comments
Closed

Modules appear to always be re-evaluated between test files #4413

zbjornson opened this issue Sep 3, 2017 · 9 comments

Comments

@zbjornson
Copy link

Do you want to request a feature or report a bug?
Possible bug.

What is the current behavior?
require'ed modules appear to always be re-evaluated between test files.
https://repl.it/KeOa/1
Notice that "module being evaluated" is printed twice. This isn't affected by --runInBand; it seems like the single node.js process is actually re-evaluating the module.

What is the expected behavior?
Modules should only be evaluated once (cached) unless resetModules: true is set or jest.resetModules() is called, I think? The docs say "between tests", and this issue is happening between test files, so maybe this is working-as-intended. However, see my note below about the problem this seems to create.

Please provide your exact Jest configuration and mention your Jest, node, yarn/npm version and operating system.
Jest 20.0.4, node 6.11.1 and 7.4.0, npm 3.10.10


Why I actually care: If I'm right about what's going on... This breaks the premise that node.js modules are singletons, which means it's easy to create leaks. In this case, I'm testing a module that installs a listener (process.on(...)) when it's loaded that lasts the lifetime of the node.js process (i.e. there is no "cleanup" function). Because jest is re-evaluating the module, multiple listeners are being installed. Other examples of this sort of leak include if a module opens file descriptors, opens sockets or sets timeouts/intervals.

@SimenB
Copy link
Member

SimenB commented Sep 3, 2017

I think this is by design, as a what you do with the imported module in one test (file) (e.g. stub out some behavior) shouldn't affect other modules

@SimenB
Copy link
Member

SimenB commented Sep 3, 2017

We might consider injecting a process per test-file, though? @zbjornson might be interesting to test if you get the same leak if you do require('process') instead of relying on the global process object?

@cpojer
Copy link
Member

cpojer commented Sep 3, 2017

Yes, that is the expectation. If you have expensive setup that you'd like to share, please build your own custom environment, based off of jest-environment-node for example.

@cpojer cpojer closed this as completed Sep 3, 2017
@cpojer
Copy link
Member

cpojer commented Sep 3, 2017

Oh, also, if this is causing leaks then that means that you are not removing all those listeners correctly. You should always have a way to free up memory when tearing down things.

@zbjornson
Copy link
Author

The principle makes sense, but I'm surprised this doesn't cause widespread problems for node.js packages given that it's very common to do things like open a database connection and let it die when the process ends, or to listen to node's process events. (The vast majority of published npm packages using jest are client-side though, it appears.)

If you have expensive setup that you'd like to share ... You should always have a way to free up memory when tearing down things.

The setup is cheap and freeing resources is easy. This is a matter of jest changing node.js's normal module lifecycle (singletons, with a few exceptions). That is, knowing when to free resources is the hard part. Looks like maybe extending NodeEnvironment with a custom dispose method to do the application-specific cleanup would be the (moderately painful) way to go?

@cpojer
Copy link
Member

cpojer commented Sep 3, 2017

I'm not sure if node makes an explicit guarantee around modules and Jest doesn't violate it if such a guarantee were to exist because you can consider every single test file to be completely isolated from each other. Note that you may end up with many multiple copies of a single module in node if your package manager (npm, Yarn) decides to install a third-party dependency multiple times within direct dependencies rather than hoisting the module specific module to the top. You'll get "singleton instances" as you call it, but you may get multiple instances of modules with the same source code and there is no guarantee in the node module resolution algorithm to prevent that.

Resources should be freed in afterAll or afterEach within a test file.

@zbjornson
Copy link
Author

I'm not sure if node makes an explicit guarantee around modules

They do: every call to require('foo') will get exactly the same object returned, if it would resolve to the same file. (https://nodejs.org/api/modules.html#modules_caching) There are caveats around filename resolution as described there and in your comment, but the issue I'm looking at is around files within an npm module (not dependencies).

Jest doesn't violate it if such a guarantee were to exist because you can consider every single test file to be completely isolated from each other

I don't quite understand what you mean here. I get the principle that each test file is supposed to be independent, but in actuality resources like file descriptors and event listeners on node's core modules (which are not re-evaluated) persist between v8 contexts. That is, the way jest works right now is not the same as a brand new process starting between every test file, which appears to be the behavioral goal(?).

@cpojer
Copy link
Member

cpojer commented Sep 3, 2017

Yup, you are right, Jest spawns N processes (where N is normally the number of cores) and M contexts (where M number of tests). Each process runs a variable amount of tests, which can be approximated by M/N. So yeah, requiring the same module in every test will result in M/N instances of that module per process.

Ideally we'd spawn one process per test, but the overhead of spawning a process is high.

Regardless, we won't be making any changes here, so I recommend using afterEach and afterAll to tear down resources in every test.

gluxon added a commit to gluxon/jest that referenced this issue Jul 5, 2018

Verified

This commit was signed with the committer’s verified signature.
addaleax Anna Henningsen
The documentation at the moment describes the behavior of Jest when
`resetModules` is set to `true`, but leaves the default/false case
unclarified. I had a misconception that the module registry would not be
reset at all in the false case, where it's still reset between different
test files. This misconception was clarified by SimienB in issue jestjs#6007.

The reporter of issue jestjs#4413 also had this question.

This commit updates the documentation of `resetModules` to explain the
default case more explicitly.
gluxon added a commit to gluxon/jest that referenced this issue Jul 5, 2018

Verified

This commit was signed with the committer’s verified signature.
addaleax Anna Henningsen
The documentation at the moment describes the behavior of Jest when
`resetModules` is set to `true`, but leaves the default/false case
unclarified. I had a misconception that the module registry would not be
reset at all in the false case, where it's still reset between different
test files. This misconception was clarified by SimienB in issue jestjs#6007.

The reporter of issue jestjs#4413 also had this question.

This commit updates the documentation of `resetModules` to explain the
default case more explicitly.
gluxon added a commit to gluxon/jest that referenced this issue Jul 5, 2018
The documentation at the moment describes the behavior of Jest when
`resetModules` is set to `true`, but leaves the default/false case
unclarified. I had a misconception that the module registry would not be
reset at all in the false case, where it's still reset between different
test files. This misconception was clarified by SimienB in issue jestjs#6007.

The reporter of issue jestjs#4413 also had this question.

This commit updates the documentation of `resetModules` to explain the
default case more explicitly.
gluxon added a commit to gluxon/jest that referenced this issue Jul 5, 2018
The documentation at the moment describes the behavior of Jest when
`resetModules` is set to `true`, but leaves the default/false case
unclarified. I had a misconception that the module registry would not be
reset at all in the false case, where it's still reset between different
test files. This misconception was clarified by SimienB in issue jestjs#6007.

The reporter of issue jestjs#4413 also had this question.

This commit updates the documentation of `resetModules` to explain the
default case more explicitly.
rickhanlonii pushed a commit that referenced this issue Jul 6, 2018
The documentation at the moment describes the behavior of Jest when
`resetModules` is set to `true`, but leaves the default/false case
unclarified. I had a misconception that the module registry would not be
reset at all in the false case, where it's still reset between different
test files. This misconception was clarified by SimienB in issue #6007.

The reporter of issue #4413 also had this question.

This commit updates the documentation of `resetModules` to explain the
default case more explicitly.
@github-actions
Copy link

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.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 13, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants