vitest-dev / vitest

Next generation testing framework powered by Vite.
https://vitest.dev
MIT License
12.37k stars 1.1k forks source link

`@vitest/web-worker` error when using browser environment #4899

Open DenizUgur opened 6 months ago

DenizUgur commented 6 months ago

Describe the bug

I'm trying to get test coverage output with istanbul but I've found out vitest can't see codes executed in web workers (per #2911). So I figured I could wrap my worker with @vitest/web-worker and execute it in main thread. However, since that worker executes WebAssembly I want to keep using browser environment.

Now, in theory I don't see any problem with this approach but I'm getting a dynamic import error.

TypeError: Failed to fetch dynamically imported module: http://localhost:5173/home/foo/workspace/bar/node_modules/@vitest/web-worker/dist/index.js?v=1704701752071

This could be just a configuration error on my side, maybe there is a way to bundle this worker wrapper in browser environment but that didn't shine any ideas to my mind.

Reproduction

If the problem is not obvious I'll create a reproduction project

System Info

System:
    OS: Linux 6.2 Ubuntu 22.04.3 LTS 22.04.3 LTS (Jammy Jellyfish)
    CPU: (32) x64 Intel(R) Core(TM) i9-14900K
    Memory: 54.16 GB / 62.57 GB
    Container: Yes
    Shell: 5.8.1 - /usr/bin/zsh
  Binaries:
    Node: 20.10.0 - ~/.nvm/versions/node/v20.10.0/bin/node
    npm: 10.2.3 - ~/.nvm/versions/node/v20.10.0/bin/npm
  Browsers:
    Chrome: 120.0.6099.129
  npmPackages:
    @vitejs/plugin-react: ^4.2.1 => 4.2.1 
    @vitest/browser: ^1.1.3 => 1.1.3 
    @vitest/coverage-istanbul: ^1.1.3 => 1.1.3 
    @vitest/coverage-v8: ~1.1.3 => 1.1.3 
    @vitest/ui: ~1.1.3 => 1.1.3 
    @vitest/web-worker: ^1.1.3 => 1.1.3 
    vite: ^5.0.11 => 5.0.11 
    vitest: ~1.1.3 => 1.1.3

Used Package Manager

npm

Validations

sheremet-va commented 6 months ago

This package is exclusively for running tests in Node.js environment.

DenizUgur commented 6 months ago

Do I have any alternative for getting coverage report for workers then?

sheremet-va commented 6 months ago

Do I have any alternative for getting coverage report for workers then?

From what I understand, it should just work in the browser because worker import is processed on the server. cc @AriPerkkio

However I guess it can't send the data about coverage to the main thread.

AriPerkkio commented 6 months ago

I'm not actually sure how this works right now. My guess is that runner in browser doesn't see worker's scope and cannot collect the coverage from window.__VITEST_COVERAGE__ object. @DenizUgur could you create a minimal reproduction for this?

github-actions[bot] commented 6 months ago

Hello @DenizUgur. Please provide a minimal reproduction using a GitHub repository or StackBlitz. Issues marked with need reproduction will be closed if they have no activity within 3 days.

DenizUgur commented 6 months ago

Here you go @AriPerkkio: ~https://github.com/DenizUgur/vitest-worker-coverage~

AriPerkkio commented 6 months ago

I can reproduce the issue now. However when debugging anything browser-mode related with locally linked Vitest, whole test run freezes completely. Need to figure out that one first. This kind of issues come up quite often - browser mode is experimental.

DenizUgur commented 6 months ago

That's true. Let me know if I can be of assistance in any way.

AriPerkkio commented 6 months ago

Coverage collection for browser's Workers is not supported. I don't think this has been looked into before as there as been no feature requests for it.

Istanbul instrumented files collect the coverage data into global scope. In Vitest's case that's in globalThis.__VITEST_COVERAGE__ object. After running all tests of a file, we collect that object back to main process. In this case we don't see Worker's coverage as it's in its own context. We would need something like:

// Inject this at the end of all workers that are loaded through Vite
const original = self.onmessage;
self.onmessage = (event) => {
  if(event.data === "__VITEST_COLLECT_COVERAGE__") {
    const coverage = globalThis.__VITEST_COVERAGE__;
    return self.postMessage({ coverage }) // Not sure if JSON.stringify would be needed here
  }
  original?.(event);
}

And when ever new Worker() is invoked, we would need to store those in the scope:

const worker = new Worker(new URL(TestWorker, import.meta.url).href, {type: 'module'})

// Inject this by Vitest - let's hope this new reference doesn't introduce memory leaks
+ globalThis.__VITEST_INTERCEPTED_WORKERS__ = globalThis.__VITEST_INTERCEPTED_WORKERS__ || []
+ globalThis.__VITEST_INTERCEPTED_WORKERS__.push(worker);

And then in here we would also collect coverage from each worker:

https://github.com/vitest-dev/vitest/blob/9ec3f743edfd0a5996a4c6eced35bbf95cf1604e/packages/coverage-istanbul/src/index.ts#L10-L19

let coverage = globalThis[COVERAGE_STORE_KEY]

if (globalThis.__VITEST_INTERCEPTED_WORKERS__?.length) {
  // Demonstrating just a single worker now, intentional [0] index access
  const worker = globalThis.__VITEST_INTERCEPTED_WORKERS__[0]

  const promise = new Promise((resolve) => {
    worker.onmessage = (event) => {
      if (event.data.coverage) {
        resolve(event.data.coverage)
      }
    }
  })

    worker.postMessage('__VITEST_GET_COVERAGE__')
    const workerCoverage = await promise

    coverage = {
      ...coverage, // This holds coverage of the file that loaded Worker, main.ts in repro's case
      ...workerCoverage, // Coverage of worker.ts
    };
  }

I did some quick hacky testing and run into issues with Istanbul. For some reason writing into globalThis inside Worker did not work. Vitest might need to rewrite some parts of Istanbul instrumented code as well: https://github.com/istanbuljs/istanbuljs/blob/5584b50305a6a17d3573aea25c84e254d4a08b65/packages/istanbul-lib-instrument/src/visitor.js#L688-L691.

So to summarize: It should be possible to collect coverage from Workers. If something like this would be implemented I think we should also look into hooking into node:child_process and node:worker_threads.

DenizUgur commented 6 months ago

This is fantastic progress, thank you. Although this seems like the proper way to approach this problem, wouldn't be easier if we force worker to run in main thread? Like what @vitest/web-worker is trying to do?

What exactly blocks us from doing that? We get a syntax error when we try to use it but if that gets resolved, it should just work right?

See this new branch on my demo repo

sheremet-va commented 6 months ago

Like what @vitest/web-worker is trying to do?

This package is not trying to run web workers in the same thread, it's a side effect of trying to bring Web Worker API to Node.js. Since web workers are already supported in the browser, this package is not needed there.

The proper way would be to communicate with the worker, although I am not sure it's a good idea to send messages to user workers which would trigger user land event listeners. Maybe something like BroadcastChannel API is better suited. We can inject it by checking for worker query in the transform hook.

DenizUgur commented 5 months ago

Will this be implemented or is it in the backlog? Just asking to set my expectations. Thank you

AriPerkkio commented 5 months ago

At the moment this is not being worked on.

I cannot give any time estimates right now but at some point we should try to implement this. This won't be easy task so I would imagine some kind of proof-of-concept work to be needed first. Supporting browser's Workers would be a good first step before looking into node:child_process and node:worker_threads support.

DenizUgur commented 5 months ago

Got it, thank you for the clarification.

DenizUgur commented 2 months ago

Hi @AriPerkkio, I'm really invested in this idea and would like to help bring Worker coverage support for both browser and Node. If nobody is working on this atm I can try to propose a PR.

For that though, I might need some pointers. Are you open to discussing this feature?

AriPerkkio commented 2 months ago

I'm not yet sure how exactly the implementation would go. I would need to do some proof-of-concept work before to see the bigger picture.

But basically the idea would be:

  1. Browser mode requests my-worker.js?worker
  2. Vite plugin detects this request from ?worker query and injects code for receiving and sending messages from/to main thread.
  3. Tests run and worker's coverage is collected as usual. This part should already be implemented.
  4. Tests end and main thread calls Coverage Provider module's takeCoverage(). This method should be modified to also read coverage from the intercepted workers.

I think main thread should also know how many Workers it should be waiting for in takeCoverage().

DenizUgur commented 2 months ago

I would need to do some proof-of-concept work before to see the bigger picture.

Let me know if I can be any help. I'll have some time soon, I can work on it.

DenizUgur commented 2 months ago

Also a quick note, it's not related to this issue but if we were to attach BroadcastChannel or similar solution to get coverage reports we could use the same solution to relay console.logs