No vitest config detected on yarn + pnp + nx projects

[saulotoledo]

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

vscode/src/config.ts

Line 81 in 4283697

const cmd = getVitestCommand(workspace.uri.fsPath, vscodeConfig.commandLine)

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

vscode/src/config.ts

Line 84 in 4283697

? undefined

The proposed solutions:

• Check for yarn.lock and require a script "yarn vitest".
• Detect for .pnp files. If they exist, consider the command as "yarn vitest".

Additionally, there is an issue with the execution of chunksToLinesAsync(child.stdout) at

vscode/src/pure/utils.ts

Line 113 in 4283697

for await (const line of chunksToLinesAsync(child.stdout)) {

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

• Follow our Code of Conduct
• Read the Contributing Guidelines.
• Read the docs.
• Check that there isn't already an issue that reports the same bug to avoid creating a duplicate.
• Check that this is a concrete bug. For Q&A open a GitHub Discussion or join our Discord Chat Server.
• The provided reproduction is a minimal reproducible example of the bug.

[ffMathy]

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

[sheremet-va]

#253 doesn't support pnp currently.

[saulotoledo]

@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]

@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]

@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]

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

[saulotoledo]

@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]

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]

@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]

@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]

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:

vscode/src/worker/worker.ts

Line 63 in daf97d4

const vitestMode = await import(meta.vitestNodePath) as typeof import('vitest/node')

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?

• If yes, and they do not use yarn, some transitive dependencies might also not be explicitly declared there. So, is this a solution?
• If not, we must manually fall back to running yarn. An approach that works but does not use the vitest API at all. We have to restore a lot of code from version 4.x and support both implementations. I am really running from this.

[sheremet-va]

• 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]

@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]

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

[hi-ogawa]

Sorry about my comment #285 (comment). I guess that one is obsolete as I commented in your PR afterwords #316 (comment).

Let's continue discussion on your PR.