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

runInBand + dynamic imports break in Jest 27.0.0-next.11 #11438

Closed
jakobrosenberg opened this issue May 23, 2021 · 32 comments
Closed

runInBand + dynamic imports break in Jest 27.0.0-next.11 #11438

jakobrosenberg opened this issue May 23, 2021 · 32 comments

Comments

@jakobrosenberg
Copy link

jakobrosenberg commented May 23, 2021

🐛 Bug Report

Running jest in parallel works fine, but with runInBand enabled, all tests with dynamic imports fail with the error Test environment has been torn down

Often, if I run a single test, the test will work the first time and then error on subsequent tests. Changing the imported file will fix the test on the next run, but then fail on the subsequent ones.

To reproduce

//foo.js
export default 'hello'

//faulty.test.js
test('test1', async () => {
    const module = await import('./foo.js')
    expect(module).toBeDefined()
})

Run the above test with --runInBand --watch. Once finished, click enter to trigger another run.

Expected behavior

Test should pass on subsequent runs, but instead only the first test passes and subsequent runs break.

envinfo

Verified on:
Windows, Mac, Ubuntu (Github action), WSL
Node: 14 & 15
NPM: 6 & 7

  System:
    OS: Windows 10 10.0.19041
    CPU: (16) x64 AMD Ryzen 7 1700 Eight-Core Processor
  Binaries:
    Node: 14.17.0 - ~\AppData\Local\Volta\tools\image\node\14.17.0\node.EXE
    Yarn: 1.21.1 - C:\Program Files\Volta\yarn.EXE
    npm: 6.14.13 - ~\AppData\Local\Volta\tools\image\npm\6.14.13\bin\npm.CMD
  npmPackages:
    jest: ^27.0.0-next.11 => 27.0.0-next.11
@ahnpnl
Copy link
Contributor

ahnpnl commented May 24, 2021

I tried this code on my Mac

test('should work', async () => {
  const packageJson = await import('../../package.json');

  expect(packageJson).toBeDefined();
});

but i don't see the issue. How do you run into it?

@jakobrosenberg
Copy link
Author

jakobrosenberg commented May 24, 2021

After some further debugging this seems to be what causes the flaky behavior:

const module = await import(path).then(r => r.default)

Omitting either the await or the then fixes the issue.

EDIT: I may have been too hasty in this conclusion. Doing some further testing.

@jakobrosenberg
Copy link
Author

jakobrosenberg commented May 24, 2021

@ahnpnl could you try this snippet in watch mode. If it doesn't fail on first test, try hit enter to trigger another test.

test('test1', async () => {
    const res= await import('some/file.js') //not package.json, but a .js file
    expect(res).toBeTruthy()
})

@ahnpnl
Copy link
Contributor

ahnpnl commented May 25, 2021

hmm i still don't see the issue, what if you clear Jest cache and try again?

@SimenB
Copy link
Member

SimenB commented May 25, 2021

Could you put together a minimal repository showing the error?

@fwh1990
Copy link

fwh1990 commented May 26, 2021

@fwh1990
Copy link

fwh1990 commented May 26, 2021

It's not about async/await, it will fail to test once we use import() syntax with jest --runInBand

@fwh1990
Copy link

fwh1990 commented May 26, 2021

image
This picture shows there are two files executed, index2.test.ts succeed and another failed. However, they are just the same file except filename.

@azban
Copy link

azban commented Jun 2, 2021

seems related to #10944

i'm running into the same issue with ESM and --runInBand and think i'm running into a similar issue as seen in #10944 (comment)

Same issue here, is there some work around that you have used?
What i get here is that in jest-runtime index.js line 532,
const context = this._environment.getVmContext();
this function is returning null, for second test file.

@damianobarbati
Copy link

@azban is there a workaround for this? I have it either with --runInBand or without.

@azban
Copy link

azban commented Jun 11, 2021

i haven't found a workaround, but also am not seeing this issue with --runInBand, so you may be seeing something else.

@LinqLover
Copy link

Could this be related to #11601? Short summary is in LinqLover/downstream-repository-mining#65 (comment) or here:

In the last weeks, I have noticed a number of failing builds in my repository which actually turned out to be flaky tests. Whenever the build fails, I see something like this:

$ yarn test --verbose=true --silent=false test/references.test.ts

yarn run v1.22.10
$ cross-env NODE_OPTIONS=--experimental-vm-modules jest --verbose=true --silent=false test/references.test.ts
(node:42760) ExperimentalWarning: VM Modules is an experimental feature. This feature could change at any time
(Use `node --trace-warnings ...` to show where the warning was created)

 RUNS  test/references.test.ts
error Command failed with exit code 1.

(The jest arguments and the specific test file were added for breaking the bug down, but it also occurs when running all tests together). I assume that somehow jest is occasionally dropping the promises generated when a dynamic import is called. This is really a hard flaky test, it only occurs in about 1 of 40 cases (so debugging this was very hard) ... I did git bisect and identified the cause of this issue in LinqLover/downstream-repository-mining@5f3a609.

That is, I don't get actual error messages from the dynamic imports, but by adding them to my project I create a small likeliness that jest will fail without executing all the tests.

@feugy
Copy link

feugy commented Sep 1, 2021

Hi there. I'm also experiencing this issue, and made a stable reproduction repository here

As you can see, I've stripped down all my code to the minimum: a module (index.js) dynamically importing another one (internals.js).

As long as I had a single test file importing index.js, everything was fine.
As soon as I've added another test file on the same guy, the issue happens.

I've lost considerable time digging it, without finding any workaround nor root cause, but my gut feeling is that the jest-node-environment is tear down, then reused. Its VMcontext is gone, preventing further dynamic imports to work.

@feugy
Copy link

feugy commented Sep 10, 2021

Going one step further: by placing console logs within jest-runtime loadEsmModule() function and jest-environment-node constructor and teardown(), I found out a mismatch in environment handling.

Basically, each test file has its own Node environment: it is configured before running tests within the file, and tore down at the end.
What happens is

  1. env # 1 is created for the test file
  2. tests are run, dynamically importing files within the env
  3. env # 1 is tore down
  4. env # 2 is created for the second test file
  5. tests are run, but they are importing files within env # 1, triggering the error
  6. env # 2 is tore down

I've tried to import all my modules dynamically, to use isolateModule() and resetModules(), but I couldn't find a way to force the second file using its own environment.

@jakobrosenberg
Copy link
Author

@SimenB would you be able to take a look at @feugy 's reproductible, #11438 (comment).

@SimenB
Copy link
Member

SimenB commented Sep 21, 2021

This is almost certainly nodejs/node#36351 or nodejs/node#33439 (either way it's further upstream: https://bugs.chromium.org/p/v8/issues/detail?id=10284)

DarkPark added a commit to DarkPark/api that referenced this issue Sep 21, 2021
@DarkPark
Copy link

I have the same problem.
But without flag --runInBand I don't get the error on my PC, tests are completed in parallel successfully.
When I add --runInBand flag I have the ReferenceError: You are trying to import a file after the Jest environment has been torn down..
The same tests always fail in Github Actions runner (either with or without --runInBand). So I had to skip all files but one for now.
My nodejs version is v14.17.6
My tests can be seen here https://github.com/DarkPark/api/tree/dc50c7fc75db321daf65b0bebd87cb9727de3a7e

@SimenB
Copy link
Member

SimenB commented Sep 22, 2021

Again, this is a bug in V8, there is nothing we can do except wait for Google to fix and release it, then Node to update their version of V8 and backport it to v12, v14 and v16

@DarkPark
Copy link

@SimenB but it works without --runInBand flag.

@SimenB
Copy link
Member

SimenB commented Sep 22, 2021

cache is probably only shared within a worker/process, so using multiple workers doesn't have the same issue

@DarkPark
Copy link

It's rather strange that I have different results in my local environment and Github Actions runner. The same OS (Ubuntu), nodejs version and run command.

@jakobrosenberg
Copy link
Author

Seeing as we're probably going to be stuck with this bug for a while, does anyone have a workaround?

@AlonMiz
Copy link

AlonMiz commented Sep 25, 2021

@jakobrosenberg, i think this is related to the issue I have with esbuild compiling jest.
aelbore/esbuild-jest#48
try adding --experimental-vm-modules to the jest node bin.

@Pomax
Copy link

Pomax commented Oct 1, 2021

A workaround I'm using is to run each test file separately, rather than telling Jest to run everything in a dir tree. It's kludgy, but until V8 fixes this, so that Node can fix it, so that Jest can work with it, not a lot of choice if you don't want to run into caching issues:

  "scripts": {
    "test": "run-s test:jest:*",
    "test:jest": "cross-env NODE_OPTIONS=--experimental-vm-modules jest --verbose=false",
    "test:jest:models": "npm run test:jest -- test/models/model.test.js",
    "test:jest:migration": "npm run test:jest -- test/migration/migration.test.js",
    "test:jest:...": "...",
    "...": "...",
  },
  "devDependencies": {
    "cross-env": "^7.0.3",
    "jest": "^27.2.2",
    "npm-run-all": "^4.1.5",
    "...": "...",
  }

With testing code that relies on dynamic imports:

import { Models } from "../../index.js";
import { User } from "./user.model.js";

describe(`Testing User model`, () => {

  beforeAll(async () => {
    await Models.useDefaultStore(`./data-store`); // this falls through to a dynamic import
    Models.register(User);
  });

  ...
});

with the operative dynamic import in Models:

static async useDefaultStore(path) {
  const { FileSystemStore } = await import("./store/filesystem-store.js");
  Models.setStore(new FileSystemStore(path));
}

But even then, this workaround probably still has pitfalls that I've simply not run into (yet).

@L2jLiga
Copy link

L2jLiga commented Oct 28, 2021

I'm also facing the same issue with --runInBand and it seems like that node.js have fixed it in version 16.11.0, see my GitHub Actions run.

Workaround is to disable runInBand in your project and set maxWorkers and maxConcurrency to value greater or equal test amount, for example in my repository I have maximum 7 tests per project that uses dynamic import, so I've set this values to 8 (and probably that's why tests are pass on my local machine, I have 8 cores on laptop and 16 on desktop)
As well I've increased timeout and magic happened! My CI is now green, links:

@SimenB it seems like that for ESM we have to instantiate new worker per each test file to workaround this issue at least for thoose who use Node.js < 16.11.0

Dennor added a commit to graphql-editor/stucco-js that referenced this issue Nov 22, 2021
petermetz pushed a commit to Leeyoungone/cactus that referenced this issue May 9, 2022
Migrated test from Tap to Jest.

File Path:
packages/cactus-test-cmd-api-server/src/test/typescript/integration/runtime-plugin-imports.test.ts

=======================================================================

This is a flaky test case that can't be reliably reproduce. I've ran the
test individually 40 times and all 40 times it has passed successfully.

This error, however, seems to pop up only when the `yarn jest` command
is run (when I ran the test individually).

Based on the linked
["hint"](https://stackoverflow.com/a/50793993)
from the issue, it seems like using `jest.useFakeTimers()` a good
solution. However, using the `jest.useFakeTimers()` with`async/await`
was not recommended.

However, I found a [different source]
(https://gist.github.com/apieceofbart/e6dea8d884d29cf88cdb54ef14ddbcc4#file-test-js-L58)
that showed examples
of how to use the `jest.useFakeTimers()`.
Our original problem, however, is probably because the after the test
environment is torn down, the second test file is trying to import
files from the first environment, triggering the error
([explained here](jestjs/jest#11438 (comment))).

So I've come to the solution of installing another node package:
[node-cleanup](https://www.npmjs.com/package/node-cleanup) and running
that within the afterAll at the very end of the test.

My laptop can't take running the `yarn jest` script too much so I
haven't seen it reproduce the error yet. (WOT ruhrow)

_______________________edit below _________________________

So it turns out that this flaky test due to libraries trying to third
libraries trying to run after the test runs is a simple fix that was
documented [here](https://testing-library.com/docs/using-fake-timers/)
where we just add the `useRealTimer` in the `afterEach`. Lol so sorry!!

=======================================================================

This is a PARTIAL resolution to issue hyperledger-cacti#238

Fixes hyperledger-cacti#1667

Signed-off-by: awadhana <[email protected]>
Signed-off-by: Youngone Lee <[email protected]>
Signed-off-by: Peter Somogyvari <[email protected]>
petermetz pushed a commit to hyperledger-cacti/cacti that referenced this issue May 10, 2022
Migrated test from Tap to Jest.

File Path:
packages/cactus-test-cmd-api-server/src/test/typescript/integration/runtime-plugin-imports.test.ts

=======================================================================

This is a flaky test case that can't be reliably reproduce. I've ran the
test individually 40 times and all 40 times it has passed successfully.

This error, however, seems to pop up only when the `yarn jest` command
is run (when I ran the test individually).

Based on the linked
["hint"](https://stackoverflow.com/a/50793993)
from the issue, it seems like using `jest.useFakeTimers()` a good
solution. However, using the `jest.useFakeTimers()` with`async/await`
was not recommended.

However, I found a [different source]
(https://gist.github.com/apieceofbart/e6dea8d884d29cf88cdb54ef14ddbcc4#file-test-js-L58)
that showed examples
of how to use the `jest.useFakeTimers()`.
Our original problem, however, is probably because the after the test
environment is torn down, the second test file is trying to import
files from the first environment, triggering the error
([explained here](jestjs/jest#11438 (comment))).

So I've come to the solution of installing another node package:
[node-cleanup](https://www.npmjs.com/package/node-cleanup) and running
that within the afterAll at the very end of the test.

My laptop can't take running the `yarn jest` script too much so I
haven't seen it reproduce the error yet. (WOT ruhrow)

_______________________edit below _________________________

So it turns out that this flaky test due to libraries trying to third
libraries trying to run after the test runs is a simple fix that was
documented [here](https://testing-library.com/docs/using-fake-timers/)
where we just add the `useRealTimer` in the `afterEach`. Lol so sorry!!

=======================================================================

This is a PARTIAL resolution to issue #238

Fixes #1667

Signed-off-by: awadhana <[email protected]>
Signed-off-by: Youngone Lee <[email protected]>
Signed-off-by: Peter Somogyvari <[email protected]>
zondervancalvez pushed a commit to zondervancalvez/cactus that referenced this issue May 18, 2022
Migrated test from Tap to Jest.

File Path:
packages/cactus-test-cmd-api-server/src/test/typescript/integration/runtime-plugin-imports.test.ts

=======================================================================

This is a flaky test case that can't be reliably reproduce. I've ran the
test individually 40 times and all 40 times it has passed successfully.

This error, however, seems to pop up only when the `yarn jest` command
is run (when I ran the test individually).

Based on the linked
["hint"](https://stackoverflow.com/a/50793993)
from the issue, it seems like using `jest.useFakeTimers()` a good
solution. However, using the `jest.useFakeTimers()` with`async/await`
was not recommended.

However, I found a [different source]
(https://gist.github.com/apieceofbart/e6dea8d884d29cf88cdb54ef14ddbcc4#file-test-js-L58)
that showed examples
of how to use the `jest.useFakeTimers()`.
Our original problem, however, is probably because the after the test
environment is torn down, the second test file is trying to import
files from the first environment, triggering the error
([explained here](jestjs/jest#11438 (comment))).

So I've come to the solution of installing another node package:
[node-cleanup](https://www.npmjs.com/package/node-cleanup) and running
that within the afterAll at the very end of the test.

My laptop can't take running the `yarn jest` script too much so I
haven't seen it reproduce the error yet. (WOT ruhrow)

_______________________edit below _________________________

So it turns out that this flaky test due to libraries trying to third
libraries trying to run after the test runs is a simple fix that was
documented [here](https://testing-library.com/docs/using-fake-timers/)
where we just add the `useRealTimer` in the `afterEach`. Lol so sorry!!

=======================================================================

This is a PARTIAL resolution to issue hyperledger-cacti#238

Fixes hyperledger-cacti#1667

Signed-off-by: awadhana <[email protected]>
Signed-off-by: Youngone Lee <[email protected]>
Signed-off-by: Peter Somogyvari <[email protected]>
@skeddles
Copy link

this also happens without --runInBand.

@erayhanoglu
Copy link

This problem still exists.

@aazbeltran
Copy link

aazbeltran commented Feb 10, 2023

On my tests, this issue is fixed on Node >=16.11
issue.zip <- Credits to @fwh1990 , I just simplified the example removing ts and other unrequired code

@erayhanoglu
Copy link

On my tests, this issue is fixed on Node >=16.11 issue.zip <- Credits to @fwh1990 , I just simplified the example removing ts and other unrequired code

I don't think it has been fixed yet. I got this error with Node 17, 18 and 19.

@MondoGao
Copy link

For jest users:

Try running jest --clearCache before running your actual tests (equals to disable jest's transform cache).

This may help in some situations.

If this works for you, you can turn off jest's cache ability in jest.config with cache: false.

nodejs/node#35889 (comment)

@SimenB
Copy link
Member

SimenB commented Sep 16, 2023

I cannot reproduce this from any of the provided reproductions.

If anybody still encounters this issue, please try with a current Node nightly (https://nodejs.org/download/nightly/) or with Node 21 when it comes out in October.

@SimenB SimenB closed this as completed Sep 16, 2023
ryanwilsonperkin added a commit to ryanwilsonperkin/unimported that referenced this issue Sep 22, 2023
Consistently running into a segfault coming from Node as a result of
cosmiconfig attempting to run a dynamic import on the listed .js config
file caused by:

nodejs/node#35889
jestjs/jest#11438

Using cosmiconfigSync to workaround this which will fall back to using a
synchronous require() call.
smeijer added a commit to smeijer/unimported that referenced this issue Sep 22, 2023
Fixes #118 

Introduces `cosmiconfig` as requested in #118 in order to support
loading the config from alternative file formats like:
```
.unimportedrc.js
.unimportedrc.yml
package.json > "unimported" key
```

I'm using `cosmiconfigSync` utilities instead of the async version,
despite them being available since the `getConfig` is an async method.
When we use the async equivalent, we hit a segfault in Node as a result
of `cosmiconfig` trying to call a dynamic import on the file within a
Jest context that doesn't allow it to. Patched in a recent version of
Node, and once the test infra can require the latest version it should
be fine to switch to the async version.

See nodejs/node#35889 and
jestjs/jest#11438

I haven't made any efforts to change the `update` function to write
updates to the loaded files, as this would be difficult/impossible to
update something like a .js or .yml (if using features like anchors) in
a meaningful way.

A few notes on the other changes included:
1. `cosmiconfig` pulls in a newer version of TypeScript which was
incompatible with the version of `@types/node` we used, updated it
2. One of the tests produced invalid JSON to a config file, which failed
silently before and passed the test, but now fails loudly when
`cosmiconfig` tries to read and parse the JSON. Updated it to be valid
to fulfill the spirit of the test.

---------

Co-authored-by: Stephan Meijer <[email protected]>
@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 Oct 17, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests