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

nyc 15 gives no coverage without the useSpawnWrap option when using pnpm #1308

Closed
leolcao opened this issue May 2, 2020 · 6 comments
Closed

Comments

@leolcao
Copy link

leolcao commented May 2, 2020

Since this issue #1303 (comment) has been closed, so I think it is better to up a new one.

@coreyfarrell I can reproduce this issue by using the @adjerbetian's git repo: https://github.com/adjerbetian/nyc-bug-demo-use-spawn

My desktop is Windows 10 10.0.19041.208, and the node.js environment is:

  • node.js: 12.16.2, installed by nvs
  • npm: 6.14.4

The reproduce steps:

internal/modules/cjs/loader.js:983
  throw err;
  ^

Error: Cannot find module 'node-preload.js'
Require stack:
- internal/preload
    at Function.Module._resolveFilename (internal/modules/cjs/loader.js:980:15)
    at Function.Module._load (internal/modules/cjs/loader.js:862:27)
    at Module.require (internal/modules/cjs/loader.js:1042:19)
    at Module._preloadModules (internal/modules/cjs/loader.js:1296:12)
    at loadPreloadModules (internal/bootstrap/pre_execution.js:444:5)
    at prepareMainThreadExecution (internal/bootstrap/pre_execution.js:68:3)
    at internal/main/run_main_module.js:7:1 {
  code: 'MODULE_NOT_FOUND',
  requireStack: [ 'internal/preload' ]
}
----------|---------|----------|---------|---------|-------------------
File      | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s
----------|---------|----------|---------|---------|-------------------
All files |       0 |        0 |       0 |       0 |
----------|---------|----------|---------|---------|-------------------
npm ERR! code ELIFECYCLE
npm ERR! errno 1
npm ERR! test-ts@1.0.0 nyc:wrong: `nyc mocha`
npm ERR! Exit status 1
npm ERR!
npm ERR! Failed at the test-ts@1.0.0 nyc:wrong script.
npm ERR! This is probably not a problem with npm. There is likely additional logging output above.
  • run npm run nyc:correct will ok to get good result:
> nyc --use-spawn-wrap mocha



  plus
    √ should add 2 numbers


  1 passing (11ms)

--------------|---------|----------|---------|---------|-------------------
File          | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s
--------------|---------|----------|---------|---------|-------------------
All files     |     100 |      100 |     100 |     100 |
 plus.js      |     100 |      100 |     100 |     100 |
 plus.spec.js |     100 |      100 |     100 |     100 |
--------------|---------|----------|---------|---------|-------------------

If I repeat above steps in the WSL2, they are all ok except not so good performance.

But, If i use npm install in both of Windows and WSL2, they are all good.

Then, I guess the package node-preload cannot handle links from pnpm installation on Windows.

Expected Behaviour:

  • Support pnpm install on Windows
@coreyfarrell
Copy link
Member

This is a bug in pnpm, a fix to pnpm/cmd-shim#3 should resolve the issue. The ability of node-preload to inject a path into NODE_PATH is mandatory, it appears pnpm via @zkochan/cmd-shim is overwriting NODE_PATH. Since @zkochan/cmd-shim appears to be overwriting NODE_PATH in a shell script this is not something node-preload can defend against.

@leolcao
Copy link
Author

leolcao commented May 3, 2020

Thanks for fast response. This situation make me struggling, the background is I am estimating the unit test frameworks. The choices are jest and mocha. Since jest@25 has performance issue although it supports code coverage directly, I decide to use mocha, and combined with nyc to get code coverage data.

So can you suggest something to me?

  • downgrade to nyc@14.1.1? I found it is no this problem to work with pnpm
  • continue to use nyc@15 with --use-spawn-wrap, but you mentioned the spawn-wrap is not reliable on windows and just it a temporary solution, right?
  • change pnpm to yarn, to support monorepo pattern

Or, something else?

@coreyfarrell
Copy link
Member

I would advise against downgrading to nyc 14. For what it's worth nyc 14 always uses spawn-wrap. NYC 15 has other big fixes so even with--use-spawn-wrap it is better than 14. I've seen situations where spawn-wrap simply doesn't work in Windows but usually it either always or never works for a given project.

Switching away from pnpm is an option though I don't want to throw shade at pnpm. If you have the ability to help them with a patch that would be great to see pnpm compatible with programs that need to append NODE_PATH.

@leolcao
Copy link
Author

leolcao commented May 3, 2020

Thanks, I choose nyc@15 with --use-spawn-wrap for short time solution. For workspaces stuff, I will estimate pnpm and yarn v2. Because yarn v2 is still in active development, so I can use pnpm
By the way, pnpm has this issue only on the windows 10.
About contributing a patch to pnpm, I think it is not easy to manage since I believe pnpm has its own design thinking, and this kind of patch will affect some essential part of pnpm.

@coreyfarrell
Copy link
Member

The conflict with pnpm and node-preload occurs anytime node-preload is installed in a place where the absolute path contains any space, double-quote or backslash. Therefore it always happens on Windows as all absolute paths contain backslashes. The likely conflict in Linux/OSX would be if node-preload gets installed to a location with a space in the path.

drazisil added a commit to rustymotors/server that referenced this issue Apr 9, 2021
Issule simal to istanbuljs/nyc#1308 (i'm using nvm)

Thanks to istanbuljs/nyc#1029 (comment), mainly
drazisil added a commit to rustymotors/server that referenced this issue Apr 10, 2021
* add service field to tcpmanager logs

* add correct type to logger

* corect service name for logger

* formatting

* corect coverage settings

Issule simal to istanbuljs/nyc#1308 (i'm using nvm)

Thanks to istanbuljs/nyc#1029 (comment), mainly

* remove circular require

* remove final typescript mentions

move typescript to a dev dependency

* exclude the app launchers

* test if deepcode accept this form of jsdoc

* improve jsdocs

* better type management

* remove taprc
@stale
Copy link

stale bot commented Jun 18, 2021

Is this still relevant? If so, what is blocking it? Is there anything you can do to help move it forward?

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

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

No branches or pull requests

2 participants