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

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

Open
6 tasks done
DenizUgur opened this issue Jan 8, 2024 · 18 comments
Open
6 tasks done

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

DenizUgur opened this issue Jan 8, 2024 · 18 comments

Comments

@DenizUgur
Copy link

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
Copy link
Member

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

@DenizUgur
Copy link
Author

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

@sheremet-va
Copy link
Member

sheremet-va commented Jan 8, 2024

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
Copy link
Member

AriPerkkio commented Jan 8, 2024

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?

Copy link

github-actions bot commented Jan 8, 2024

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
Copy link
Author

DenizUgur commented Jan 8, 2024

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

@AriPerkkio
Copy link
Member

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
Copy link
Author

DenizUgur commented Jan 9, 2024

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

@AriPerkkio
Copy link
Member

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:

export function takeCoverage() {
// @ts-expect-error -- untyped global
const coverage = globalThis[COVERAGE_STORE_KEY]
// Reset coverage map to prevent duplicate results if this is called twice in row
// @ts-expect-error -- untyped global
globalThis[COVERAGE_STORE_KEY] = {}
return coverage
}

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
Copy link
Author

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
Copy link
Member

sheremet-va commented Jan 10, 2024

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
Copy link
Author

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

@AriPerkkio
Copy link
Member

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
Copy link
Author

Got it, thank you for the clarification.

@DenizUgur
Copy link
Author

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
Copy link
Member

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
Copy link
Author

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
Copy link
Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants