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

jsdom loading issue if threads false #835

Closed
6 tasks done
bkuster opened this issue Feb 22, 2022 · 11 comments · Fixed by #1263
Closed
6 tasks done

jsdom loading issue if threads false #835

bkuster opened this issue Feb 22, 2022 · 11 comments · Fixed by #1263

Comments

@bkuster
Copy link

bkuster commented Feb 22, 2022

Describe the bug

Issue

When starting vitest with threads: false tests fail with: Cannot read properties of undefined (reading 'child') The same test run while using threads: true.

Possible Causes

The test uses the @vue/test-utils package for component testing. These must be loaded after jsdom is loaded and the DOM is provided. (see this issue).

Failed workarounds

I tried loading jsdom via jsdom-global myself in a global setup hook, so far to no avail.

Reproduction

vite.config.js

import { defineConfig } from 'vite';
import { createVuePlugin } from 'vite-plugin-vue2';

const configMain = defineConfig({
  plugins: [
    createVuePlugin(),
  ],
  test: {
    threads: false,
    environment: 'jsdom',
  },
});

export default configMain;

Component.spec.js

import { describe, expect, it } from 'vitest';
import { mount, createLocalVue } from '@vue/test-utils';
import Component from './Component.vue';

describe('Compontent', () => {
  const localVue = createLocalVue();

  it('mounts', () => {
    const wrapper = mount(Component, { localVue });
    expect(wrapper).toMatchSnapshot();
  });
});

Component.vue

<template>
  <div>
    Foo Bar
  </div>
</template>

<script>
  export default {
    name: 'FooBar',
  };
</script>

setup

npm i vitest vite vue@^2.6.0 @vue/test-utils@1 jsdom

System Info

System:
    OS: Linux 5.4 Ubuntu 20.04.3 LTS (Focal Fossa)
    CPU: (6) x64 Intel(R) Core(TM) i7-8700 CPU @ 3.20GHz
    Memory: 643.27 MB / 31.36 GB
    Container: Yes
    Shell: 5.0.17 - /bin/bash
  Binaries:
    Node: 16.13.0 - ~/.nvm/versions/node/v16.13.0/bin/node
    npm: 8.1.0 - ~/.nvm/versions/node/v16.13.0/bin/npm
  Browsers:
    Chromium: 98.0.4758.102
    Firefox: 97.0
  npmPackages:
    vite: ^2.8.4 => 2.8.4 
    vitest: ^0.5.3 => 0.5.3

Used Package Manager

npm

Validations

@artemtam
Copy link

Having the same issues with jsdom and @testing-library/react.

cexbrayat added a commit to cexbrayat/vitest that referenced this issue Mar 31, 2022
When the teardown runs (when threads are disabled), it breaks JSDOM
It turns out that the teardown function is never called when running with threads enabled,
so this issue went unnoticed for most developers.

Fixes vitest-dev#835

The PR can be tested by running:

```
cd examples/vue
pnpm run test -- --no-threads
```

This fails on `main` but no longer fails with this commit.
@cexbrayat
Copy link
Contributor

I tried to dig a bit more into this issue, and it turns out that the teardown function of the jsdom environment is never called when running with threads

teardown(global) {
keys.forEach(key => delete global[key])
},

This is because the worker pool never finishes and the finally here is never called
const env = await environments[name].setup(globalThis, options)
try {
await fn()
}
finally {
await env.teardown(globalThis)
}

It is only called with threads disabled (when there is no pool), and that leads to inconsistencies between the two modes.
It is highly possible that the teardown function is not correct, but that the issue was never spotted because the CI tests of this repo run with threaded. I added a simple repro in #1060

The same probably happens with the other environments like happy-dom as well.
The repro I added also surfaces some mock tests failing, but I haven't found an obvious reason.

TL;DR:

  • the jsdom environment teardown function is only called when no threads are used, whereas it should be run even with threads
  • the jsdom environment teardown function probably needs to be fixed

@sheremet-va
Copy link
Member

sheremet-va commented Apr 6, 2022

I think one of the problems is tests using the same @vue/test-utils module, which tries to read inaccessible jsdom instances. I tried fixing it in #1102 - let me know if it at least partially fixes the issue?

Also, I agree, teardown function may need improvement.


I found that it already has vue instance loaded before running test for some reason. Will try to fix it

@sheremet-va
Copy link
Member

sheremet-va commented Apr 6, 2022

Also, I want to point out that Vitest treats workers differently than Jest. Jest provides isolation with vm module, but we cannot do that due to its poor ESM support, so for isolation we are heavily relying on isolated workers. Disabling workers you are disabling Vitest's isolation and have to do it yourself.

Please, don't expect --no-threads to act the same as --runInBand.

@cexbrayat
Copy link
Contributor

@sheremet-va Thanks for your feedback.

Yeah, I was trying to fix CI issues that occur since vitest 0.7+, and thought that --no-threads could save the day (as --runInBand was sometimes helpful on CI machines for Jest), but I now understand the concepts are not exactly the same. That could be worth documenting, as I suppose I'm not the only one who we'll make a similar assumption.

If I understand correctly #1102, we would need to call vi.resetModules() before each test? If so, would it be possible to have a config option as we do for clearMocks for example?

@sheremet-va
Copy link
Member

sheremet-va commented Apr 6, 2022

If I understand correctly #1102, we would need to call vi.resetModules() before each test? If so, would it be possible to have a config option as we do for clearMocks for example?

It is called automatically. I've tried calling it in your repro, and came to this conclusion:

I found that vue uses cached runtime-dom.cjs.js, removing it from cache helped:

Object.keys(require.cache)
  .filter(k => k.includes('vue'))
  .forEach(k => delete require.cache[k])
  
import { mount } from '@vue/test-utils'

I guess what happens is vue plugin imports some instance of vue, which gets cached and is returned every time. It's fine if it was required, but we can't clear cache if it was imported (since Node doesn't expose import cache), but we haven't encountered problems with it yet.

@sheremet-va
Copy link
Member

sheremet-va commented Apr 6, 2022

So, to paint a full picture:

  1. Running vitest --no-threads
  2. Vitest starts a Vite server
  3. Vite plugins are initialized
    3.5. Setup jsdom/happy-dom
  4. Vitest starts tests in the SAME thread (which means it shares a state with Vite server and all it's caches)
  5. Test imports @vue/test-utils
  6. @vue/test-utils imports cached on require.cache instance of vue, which was initialized on step 3 BEFORE jsdom is setup
  7. Fail

This is all tested on examples/vitesse. I assume similar happens to other frameworks.

@sheremet-va
Copy link
Member

sheremet-va commented Apr 6, 2022

I have tried clearing the cache entirely and/or depending on what was required in plugins, but this causes other problems.

So, as a patch solution, I propose deps.clear config options: an array of regexps to clear from cache before each test. This should help, if a package depends on a global variable, defined when requiring (like Vue does).

But I guess it will only be usefull with --no-threads, since isolated workes don't share the cache.

WDYT, @antfu @patak-dev?

@drewlyton
Copy link

Any news on this @antfu @patak-dev @sheremet-va? I'm experiencing a similar problem using --no-threads and @testing-library/react. First run passes, but watched second run fails.

Let me know if I can provide any additional information 👍

@sheremet-va
Copy link
Member

sheremet-va commented May 7, 2022

So, with --no-threads we should define environment only one time. If we remove teardown and don't run new JSDOM, when window is already defined, all tests are passing.

This is usually ok, if you don't use @vitest-environment, because it shutters this logic a bit. So, I propose we define window only once and throw an error, if someone uses different @vitest-environment for now. To give people time to test this approach. After that we can try to figure out other usecases.

For example, we can group files by different environments.


We can also clear caches of dependencies that rely on having window and other global variables defined. This is the real problem - this dependencies remember objects, defined in the first test, but we redefine them for other test, and everything fails. For example, instanceof Element checks fill fail, because global Element is now referencing another object.

@sheremet-va
Copy link
Member

I also want to point out that I don't recommend using --no-threads with Vue and React projects, because you loos isolation, and have to provided it yourself.

I guess usually, if you don't rely on some global variable, and cleanup/unmount after each test, it should be ok. But disable --threads on your own risk!

chaii3 pushed a commit to chaii3/vitest that referenced this issue May 13, 2022
@github-actions github-actions bot locked and limited conversation to collaborators Jun 18, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants