Jest seems to share JSdom environments accross tests

[julienw]

🐛 Bug Report

From the documentation in https://jestjs.io/docs/en/configuration#testenvironment-string, the test environment should be sandboxed:

Note: TestEnvironment is sandboxed. Each test suite will trigger setup/teardown in their own TestEnvironment.

But in our tests we're mocking some prototype methods and it looks like that they're shared, and this produces intermittents.

To Reproduce

Steps to reproduce the behavior:

1. Run these commands:

git clone https://github.com/julienw/testcase-jest1
cd testcase-jest1
yarn
yarn test

This repository contains several times the same tests:

• testcaseX.test.js mocks HTMLElement.prototype.getBoundingClientRect
• defaultX.test.js doesn't mock this method
• all files testcaseX.test.js have the same content, so they should have the same result
• all files defaultX.test.js have the same content, so they should have the same result

Also, the configuration contains:

    "restoreMocks": true,
    "resetMocks": true,

But this doesn't seem to make a difference in the result. What does make a difference is these changes: https://github.com/julienw/testcase-jest1/compare/reset-mocks. So maybe the issue is the implementation for resetMocks and not the test environment?

For me this fails nearly 100% of the time, but because this is likely a frequent intermittent you might need to repeat the test several times.

Here is the error I get:

 FAIL  ./default1.test.js
  ✕ returns the default bounding box (6ms)

  ● returns the default bounding box

    expect(received).toEqual(expected)

    Expected value to equal:
      {"bottom": 0, "height": 0, "left": 0, "right": 0, "top": 0, "width": 0}
    Received:
      {"bottom": 300, "height": 300, "left": 0, "right": 300, "top": 0, "width": 300, "x": 0, "y": 0}

    Difference:

    - Expected
    + Received

      Object {
    -   "bottom": 0,
    -   "height": 0,
    +   "bottom": 300,
    +   "height": 300,
        "left": 0,
    -   "right": 0,
    +   "right": 300,
        "top": 0,
    -   "width": 0,
    +   "width": 300,
    +   "x": 0,
    +   "y": 0,
      }

      3 |   document.body.appendChild(element);
      4 | 
    > 5 |   expect(element.getBoundingClientRect()).toEqual({
        |                                           ^
      6 |     bottom: 0,
      7 |     height: 0,
      8 |     left: 0,

      at Object.toEqual (default1.test.js:5:43)

Link to repl or repo (highly encouraged)

https://github.com/julienw/testcase-jest1

Run npx envinfo --preset jest

Paste the results here:

npx: 1 installé(s) en 1.166s

  System:
    OS: Linux 4.18 Debian GNU/Linux 9 (stretch) 9 (stretch)
    CPU: (4) x64 Intel(R) Core(TM) i7-6600U CPU @ 2.60GHz
  Binaries:
    Node: 9.11.1 - ~/.nvm/versions/node/v9.11.1/bin/node
    Yarn: 1.12.3 - ~/.yarn/bin/yarn
    npm: 5.6.0 - ~/.nvm/versions/node/v9.11.1/bin/npm
  npmPackages:
    jest: ^23.6.0 => 23.6.0 

[julienw]

My guess is that Jest properly creates a new jsdom environment for each tests, but jsdom reuses its prototype objects. Which I think should be expected ? But if the configuration option restoreMocks and manually calling the function jest.restoreAllMocks have different bevaviors, then there's a problem.

[SimenB]

My guess is that Jest properly creates a new jsdom environment for each tests, but jsdom reuses its prototype objects.

That seems reasonable. Not sure if it's expected or not, you're supposed to be running inside of a sandbox. They might be open to a PR?

What does make a difference is these changes: https://github.com/julienw/testcase-jest1/compare/reset-mocks.

All restoreMocks in config does is add jest.restoreAllMocks() in a beforeEach, see https://github.com/facebook/jest/blob/3e3d4058f0178de1807d98d8ddf266f1178e318a/packages/jest-jasmine2/src/index.js#L111-L113

[julienw]

@SimenB why is this happening in a beforeEach rather than a afterEach call? Could that be our bug?

[SimenB]

Shouldn't really matter, but might be! Not sure what the correct semantics are here. @thymikee?

[julienw]

My concern is that if it's in a beforeEach then we won't restore it after the last test. This shouldn't be an issue if we were in a real sandbox, but well :)

[julienw]

Wondering if this could be the root cause for my other issue #7573 as well... 🤔

[julienw]

We're moving forward in our investigation on our project: even when restoring the mocks at the right moment, we still get intermittents because of the shared prototypes.

Restoring mocks in afterEach definitely makes the issue less present though.

[andrebautista]

@julienw I'm currently tackling a similar issue. Let me know if you find a solution!

[julienw]

regarding beforeEach vs afterEach: if a test fails, afterEach isn't called, which means the mocks aren't restored if we do this in afterEach, and this is bad because this makes other tests fail. Maybe that's the reason why it's done in beforeEach? (which has its downsides too as said above). Ideally I'd like a afterEach that would run even if the test fails.

Maybe we should do it in both in the current state of things.

[jmtoung]

We're moving forward in our investigation on our project: even when restoring the mocks at the right moment, we still get intermittents because of the shared prototypes.

Restoring mocks in afterEach definitely makes the issue less present though.

If I understand correctly, are you saying that even restoring mocks in afterEach, you do get intermittents? Not sure what you mean by "less present", as that doesn't sound reassuring.

[SimenB]

JSDOM v16 has started migrating off of shred prototypes, btw: https://github.com/jsdom/jsdom/releases/tag/16.0.0. So we could close this issue and point to upstream

[julienw]

If I understand correctly, are you saying that even restoring mocks in afterEach, you do get intermittents? Not sure what you mean by "less present", as that doesn't sound reassuring.

yes.. it also highly depends on the order tests are run. For example working on some file would make the problem appear suddenly, because babel compiling the changed files changes the timing of running.

JSDOM v16 has started migrating off of shred prototypes, btw: https://github.com/jsdom/jsdom/releases/tag/16.0.0. So we could close this issue and point to upstream

That's a great news! This would effectively fix the problem with jsdom!

I still wonder why restoring mocks happen in beforeEach, possibly as I said before because afterEach isn't run in case a test fails. I don't know if that's what users expect, at least the documentation doesn't say anything about that.
Should I file a separate issue to discuss this specific topic?

[SimenB]

Should I file a separate issue to discuss this specific topic?

Sure! It's on purpose, although I do agree it's not necessarily 100% expected

https://github.com/facebook/jest/blob/faa52673f4d02ee1d273103ada9382a0805b54a4/packages/jest-circus/src/legacy-code-todo-rewrite/jestAdapter.ts#L56-L77
https://github.com/facebook/jest/blob/faa52673f4d02ee1d273103ada9382a0805b54a4/packages/jest-jasmine2/src/index.ts#L100-L120