search

vitest-dev / vscode

VS Code extension for Vitest
https://vitest.dev/vscode
MIT License
740 stars 83 forks source link

No vitest config detected on yarn + pnp + nx projects #285

Closed saulotoledo closed 3 weeks ago

saulotoledo commented 6 months ago

Describe the bug

No tests are detected on Yarn + PnP projects. The logs show the following lines

Vitest Workspace [workspace]: Vitest version = undefined
ERR [Extension Host] Cannot run tests: no Vitest config found.

Reproduction

I cannot share the project code I used to verify it, but I believe the investigation below might be enough to highlight the problem.

The function https://github.com/vitest-dev/vscode/blob/main/src/pure/utils.ts#L38 is triggered at https://github.com/vitest-dev/vscode/blob/4283697a5d638105dfabf83ec417f55c5f0669cd/src/config.ts#L81 and only checks for the script at node_modules folder. Yarn PnP projects do not have such folder and, therefore,the resulting command is null. As a consequence, the vitest version is undefined at https://github.com/vitest-dev/vscode/blob/4283697a5d638105dfabf83ec417f55c5f0669cd/src/config.ts#L84

The proposed solutions:

Additionally, there is an issue with the execution of chunksToLinesAsync(child.stdout) at https://github.com/vitest-dev/vscode/blob/4283697a5d638105dfabf83ec417f55c5f0669cd/src/pure/utils.ts#L113 which empty when the command is correct on the system informed in this bug report. This will result in a failure when loading the version through yarn.

System Info

GNU/Linux, VSCode.

Used Package Manager

yarn

Validations

ffMathy commented 6 months ago

Will probably be fixed by #253, and it should also increase the stability I believe.

sheremet-va commented 6 months ago

253 doesn't support pnp currently.

saulotoledo commented 5 months ago

@sheremet-va I took a look into it and made some progress. However, when running vitest through the .pnp.cjs files, we need to have all vitest dependencies in the package.json. This should not be necessary. I need to dig deeper to understand what can be done. For a minute, I thought it would be easier to fallback to yarn for the PnP case. I will try to find some time to work on it, but I might need some help. Would it be ok if I open a draft PR to discuss solutions if I do not make progress here?

sheremet-va commented 5 months ago

@sheremet-va I took a look into it and made some progress. However, when running vitest through the .pnp.cjs files, we need to have all vitest dependencies in the package.json. This should not be necessary. I need to dig deeper to understand what can be done. For a minute, I thought it would be easier to fallback to yarn for the PnP case. I will try to find some time to work on it, but I might need some help. Would it be ok if I open a draft PR to discuss solutions if I do not make progress here?

Sure, give it a shot. The current approach was me trying to make it work, but I wasn't able to crack it before the release.

saulotoledo commented 5 months ago

@sheremet-va I pushed the PR and added comments. It fails because yarn requires dependencies, but I have not yet understood the differences between running the process through yarn and running it in child_process.fork(), which has led to the failures. We could fall back to yarn, but I would like to understand why that approach is not working.

Also, do you have a link for the findNode API you mentioned in the comments? I first thought it was from the Yarn APIs, but I did not find it.

sheremet-va commented 5 months ago

Maybe we can use yarn exec for pnp projects? https://yarnpkg.com/cli/exec

saulotoledo commented 5 months ago

@sheremet-va Ok, I will try that. I was trying to skip running yarn directly, but I had no success. I will find some time today and/or tomorrow to push a solution with yarn exec.

hi-ogawa commented 5 months ago

Also how about yarn node https://yarnpkg.com/cli/node ? I'm not really a yarn pnp user, but I've used this when I needed to investigate some yarn pnp specific issues.

Something like fork(workerPath, { execPath: "(yarn bin path)", execArgv: ["node", ...] })?

saulotoledo commented 5 months ago

@sheremet-va, @hi-ogawa, here's a bit of the status on the task: I tried both, and they sort of work, but I am having trouble running them through the process.fork. I have to switch to process.spawn, but then I have to reimplement everything through it just for yarn. We will have to support two implementations. I tried creating an adapter, but it is still too much code. I still need to understand how much of the code works in the extension. It would be much easier if I could stick with process.fork, but piping stdin and stdout are not working with yarn in the middle. If you have any more ideas or examples, please let me know. I am working on new stuff for me here, so maybe I just need to read a bit about IPC. I will continue and will update the status for you.

hi-ogawa commented 5 months ago

@saulotoledo Thanks for investigating this further. Yeah, I see now yarn exec and yarn node (which is actually just yarn exec node ...) creates a wrapper process for our worker process, so I would assume NodeJS's automatic fork IPC setup doesn't work.

One idea I'm thinking is maybe we can sniff necessary env vars by spawning extra process like yarn node script-to-dump-env-var.cjs and then reuse it in fork(workerPath, { env: { ...} })?

For example, I'm seeing this when running yarn exec env:

$ yarn exec env | grep -P 'NODE|npm_'
npm_execpath=/tmp/xfs-84d65527/yarn
npm_node_execpath=/tmp/xfs-84d65527/node
npm_package_name=root-workspace-0b6124
npm_package_version=
npm_package_json=/home/hiroshi/code/personal/reproductions/yarn-process-fork/package.json
npm_config_user_agent=yarn/4.1.1 npm/? node/v20.11.0 linux x64
NODE_OPTIONS=--require /home/hiroshi/code/personal/reproductions/yarn-process-fork/.pnp.cjs
saulotoledo commented 5 months ago

Hi @hi-ogawa. Thanks for your suggestions. I will check them carefully. I might also need to approach the problem from a different perspective. Let me write that down as well below. The code written by @sheremet-va uses the vitest API. The API is imported directly from the node_modules folder below:

https://github.com/vitest-dev/vscode/blob/daf97d4d349bc22937aea7cd65c6c4fc67d68ee2/src/worker/worker.ts#L63

However, when using the PnP loader, the direct import complains about the non-declared dependencies from vitest in the project's package.json file. The requirement for explicit dependencies is a feature on Yarn+PnP, and there is no way to deactivate that behavior. That said, even if we can capture the input/output from running yarn node instead of just node as today (which is my current problem), we will face the same problem. So, maybe I should not invest any more time in this approach.

The issue with the dependencies in yarn happens because we import vitest directly using the project's package.json as a reference. Can we use vitest's package.json instead?

sheremet-va commented 5 months ago
  • So, is this a solution?

Can't we just import vitest/node and run exec with the root cwd? So, instead of resolving paths first for pnp in the main thread, they do it themselves in yarn exec.

saulotoledo commented 5 months ago

@sheremet-va Do you mean not setting the cwd to dirname(pnp) when forking the process? If by root you mean the project root, I believe the problem remains: just importing vitest/node through .pnp.cjs will throw errors for every vitest dependency (which will not be in the project). Only yarn has those requirements, which explains why it works for all other cases. And I think using yarn exec will not change that behavior: the import of vitest/node will trigger the dependency management on yarn again through the .pnp.* files, leading to the same error. Please let me know if I understood your proposal correctly. I will be online on Discord if you find some time to discuss ideas there as well.

saulotoledo commented 5 months ago

I just saw @hi-ogawa's review. I will have time to work on it in a few minutes.

hi-ogawa commented 5 months ago

Sorry about my comment https://github.com/vitest-dev/vscode/issues/285#issuecomment-2017287845. I guess that one is obsolete as I commented in your PR afterwords https://github.com/vitest-dev/vscode/pull/316#issuecomment-2017334778.

Let's continue discussion on your PR.

sheremet-va commented 3 weeks ago

Should be fixed in https://github.com/vitest-dev/vscode/pull/456