jest icon indicating copy to clipboard operation
jest copied to clipboard

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

Open jakobrosenberg opened this issue 3 years ago • 26 comments

🐛 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

jakobrosenberg avatar May 23 '21 19:05 jakobrosenberg

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?

ahnpnl avatar May 24 '21 09:05 ahnpnl

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 avatar May 24 '21 09:05 jakobrosenberg

@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()
})

jakobrosenberg avatar May 24 '21 09:05 jakobrosenberg

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

ahnpnl avatar May 25 '21 07:05 ahnpnl

Could you put together a minimal repository showing the error?

SimenB avatar May 25 '21 07:05 SimenB

https://github.com/easy-demo/ts-jest-issue/tree/await-import Look at this branch.

fwh1990 avatar May 26 '21 04:05 fwh1990

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

fwh1990 avatar May 26 '21 05:05 fwh1990

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.

fwh1990 avatar May 26 '21 05:05 fwh1990

seems related to https://github.com/facebook/jest/issues/10944

i'm running into the same issue with ESM and --runInBand and think i'm running into a similar issue as seen in https://github.com/facebook/jest/issues/10944#issuecomment-781523091

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.

azban avatar Jun 02 '21 02:06 azban

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

damianobarbati avatar Jun 10 '21 12:06 damianobarbati

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

azban avatar Jun 11 '21 02:06 azban

Could this be related to #11601? Short summary is in https://github.com/LinqLover/downstream-repository-mining/issues/65#issuecomment-866255883 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 https://github.com/LinqLover/downstream-repository-mining/pull/41/commits/5f3a60985f0d04d6f070f936e4af806c007c2f21.

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.

LinqLover avatar Jun 22 '21 19:06 LinqLover

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 avatar Sep 01 '21 07:09 feugy

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.

feugy avatar Sep 10 '21 08:09 feugy

@SimenB would you be able to take a look at @feugy 's reproductible, https://github.com/facebook/jest/issues/11438#issuecomment-910031145.

jakobrosenberg avatar Sep 20 '21 14:09 jakobrosenberg

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

SimenB avatar Sep 21 '21 10:09 SimenB

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

DarkPark avatar Sep 22 '21 06:09 DarkPark

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

SimenB avatar Sep 22 '21 07:09 SimenB

@SimenB but it works without --runInBand flag.

DarkPark avatar Sep 22 '21 08:09 DarkPark

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

SimenB avatar Sep 22 '21 08:09 SimenB

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.

DarkPark avatar Sep 22 '21 08:09 DarkPark

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

jakobrosenberg avatar Sep 24 '21 15:09 jakobrosenberg

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

AlonMiz avatar Sep 25 '21 10:09 AlonMiz

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

Pomax avatar Oct 01 '21 17:10 Pomax

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:

  • commit: https://github.com/L2jLiga/fastify-decorators/pull/457/commits/379e280384e5beefc8ecf095c7af5323847ab87c
  • GitHub Actions run: https://github.com/L2jLiga/fastify-decorators/runs/4039225530?check_suite_focus=true

@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

L2jLiga avatar Oct 28 '21 19:10 L2jLiga

this also happens without --runInBand.

skeddles avatar Sep 12 '22 20:09 skeddles