coverage 0% with yarn v2 P'n'P

[thealjey]

https://github.com/thealjey/coverage-problem

git checkout master
yarn
yarn test

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

total: 1
passing: 1

duration: 2.1s

✨ Done in 2.78s.

---

git checkout yarnv2
yarn
yarn test

ERROR: Coverage for lines (0%) does not meet global threshold (100%)
ERROR: Coverage for functions (0%) does not meet global threshold (100%)
ERROR: Coverage for statements (0%) does not meet global threshold (100%)

----------|----------|----------|----------|----------|-------------------|
File      |  % Stmts | % Branch |  % Funcs |  % Lines | Uncovered Line #s |
----------|----------|----------|----------|----------|-------------------|
All files |        0 |      100 |        0 |        0 |                   |
 index.js |        0 |      100 |        0 |        0 |                 1 |
----------|----------|----------|----------|----------|-------------------|

total: 1
passing: 1

duration: 1.7s

---

The only difference between the branches is yarn version.

I was only forced for switch to v2 because Husky does not support v1 P'n'P (plug and play).

Also, yarn itself recommends using the v2 version of P'n'P, because it is "more stable".

And I find the prospect of "Zero-Installs" very appealing.

The only thing I was not able to make work thus far is nyc.

And I do not know how to even begin debugging this problem myself, as there are no errors thrown or anything obvious like that.

[coreyfarrell]

I've seen reports of yarn apparently interfering with nyc, specifically it seems to break spawn-wrap. Could you test with [email protected]? This next version bypasses spawn-wrap by default.

[thealjey]

now it does throw an error

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

Error: Cannot find module '/Users/al/Projects/coverage-problem/.yarn/cache/node-preload-npm-0.1.3-55fd0dfa1d.zip/node_modules/node-preload/node-preload.js'
Require stack:

• internal/preload
  at Function.Module._resolveFilename (internal/modules/cjs/loader.js:780:15)
  at Function.Module._load (internal/modules/cjs/loader.js:685:27)
  at Module.require (internal/modules/cjs/loader.js:838:19)
  at Module._preloadModules (internal/modules/cjs/loader.js:1114:12)
  at loadPreloadModules (internal/bootstrap/pre_execution.js:429:5)
  at prepareMainThreadExecution (internal/bootstrap/pre_execution.js:53:3)
  at internal/main/run_main_module.js:7:1 {
  code: 'MODULE_NOT_FOUND',
  requireStack: [ 'internal/preload' ]
  }
  ERROR: Coverage for lines (0%) does not meet global threshold (100%)
  ERROR: Coverage for functions (0%) does not meet global threshold (100%)
  ERROR: Coverage for statements (0%) does not meet global threshold (100%)
  (node:69196) UnhandledPromiseRejectionWarning: Error: A package is trying to access another package without the second one being listed as a dependency of the first one

Required package: lcov (via "lcov")
Required by: istanbul-reports@npm:3.0.0-alpha.2 (via /Users/al/Projects/coverage-problem/.yarn/cache/istanbul-reports-npm-3.0.0-alpha.2-97da37ec3d.zip/node_modules/istanbul-reports/index.js)

at Object.makeError (/Users/al/Projects/coverage-problem/.pnp.js:3819:26)
at resolveToUnqualified (/Users/al/Projects/coverage-problem/.pnp.js:6226:43)
at resolveRequest (/Users/al/Projects/coverage-problem/.pnp.js:6272:31)
at Object.resolveRequest (/Users/al/Projects/coverage-problem/.pnp.js:6311:32)
at Function.module_1.default._resolveFilename (/Users/al/Projects/coverage-problem/.pnp.js:5742:37)
at Function.module_1.default._load (/Users/al/Projects/coverage-problem/.pnp.js:5663:45)
at Module.require (internal/modules/cjs/loader.js:838:19)
at require (internal/modules/cjs/helpers.js:74:18)
at Object.create (/Users/al/Projects/coverage-problem/.yarn/cache/istanbul-reports-npm-3.0.0-alpha.2-97da37ec3d.zip/node_modules/istanbul-reports/index.js:15:20)
at /Users/al/Projects/coverage-problem/.yarn/cache/nyc-npm-15.0.0-beta.0-1d1aca574b.zip/node_modules/nyc/index.js:428:15

(node:69196) UnhandledPromiseRejectionWarning: Unhandled promise rejection. This error originated either by throwing inside of an async function without a catch block, or by rejecting a promise which was not handled with .catch(). (rejection id: 1)
(node:69196) [DEP0018] DeprecationWarning: Unhandled promise rejections are deprecated. In the future, promise rejections that are not handled will terminate the Node.js process with a non-zero exit code.

[coreyfarrell]

This looks like yarn pnp is creating a broken install or otherwise changing the node.js runtime environment in a way nyc cannot deal with. The error at node_modules/istanbul-reports/index.js:15:20 shows a failure being reported in a fallback path. This means that two lines earlier require(path.join(__dirname, 'lib', name)) failed to load ./lib/lcov' - this should have successfully loaded ./lib/lcov/index.js`.

[thealjey]

@coreyfarrell
so, what do I do? 😄
do I report the bug to Yarn and what would be the specifics of that?
so far, everything I've tried to use works perfectly, except for nyc

[coreyfarrell]

Failure to load lcov is definitely a yarn bug. when node_modules/istanbul-reports/index.js runs with name === 'lcov' this is expected to load node_modules/istanbul-reports/lib/lcov/index.js as it does when running under npm or using node directly.

As far as the other error where it fails to load node-preload, nyc has a direct dependency on node-preload and it is added by absolute path to NODE_OPTIONS as a --require option for child processes. I'm not sure how this has managed to get broken, the only thing I can guess is that node-preload is loaded in the main process with a __filename that sub-processes cannot load. I suspect this is also a bug in yarn pnp as this works great when packages are installed to the expected locations. Possibly some kind of bootstrapping race between node-preload and yarn pnp. I'd be open to discussing this with the yarn folks (feel free to CC me in a yarn bug and link to this bug).

so far, everything I've tried to use works perfectly, except for nyc

yarn has created a system which operates under slightly different (at least somewhat incompatible) rules. Many projects are very simple and can deal with changes to edge cases. Simple is not a valid description of nyc.

[arcanis]

The load fails in the 3.0.0-alpha.2 (but not in latest) because istanbul-reports silences the error:

https://github.com/istanbuljs/istanbuljs/blob/a8b355732367f7c4a740677553b9785f5e9eac61/packages/istanbul-reports/index.js#L12-L16

I'd suggest to add a check that e.code === 'MODULE_NOT_FOUND' before using the fallback, but I'm not sure exactly what that entails so maybe that's not possible 😅 Anyway, this is the real error:

Error: A package is trying to access another package without the second one being listed as a dependency of the first one

Required package: istanbul-lib-report (via "istanbul-lib-report")
Required by: istanbul-reports@npm:3.0.0-alpha.2 (via /private/tmp/nyctesdt/.yarn/cache/istanbul-reports-npm-3.0.0-alpha.2-97da37ec3d.zip/node_modules/istanbul-reports/lib/lcov/index.js)

    at Object.makeError (/private/tmp/nyctesdt/.pnp.js:2094:26)
    at resolveToUnqualified (/private/tmp/nyctesdt/.pnp.js:4501:43)
    at resolveRequest (/private/tmp/nyctesdt/.pnp.js:4547:31)
    at Object.resolveRequest (/private/tmp/nyctesdt/.pnp.js:4586:32)
    at Function.module_1.default._resolveFilename (/private/tmp/nyctesdt/.pnp.js:4017:37)
    at Function.module_1.default._load (/private/tmp/nyctesdt/.pnp.js:3938:45)
    at Module.require (internal/modules/cjs/loader.js:681:19)
    at require (internal/modules/cjs/helpers.js:16:16)
    at Object.<anonymous> (/private/tmp/nyctesdt/.yarn/cache/istanbul-reports-npm-3.0.0-alpha.2-97da37ec3d.zip/node_modules/istanbul-reports/lib/lcov/index.js:6:24)
    at Module._compile (internal/modules/cjs/loader.js:774:30)

Which is actually correct: istanbul-lib-report is used here but isn't in the package dependencies (it's in the devDependencies, but devDependencies are never taken into consideration by package managers for transitive dependencies).

[coreyfarrell]

@arcanis thanks for looking at this and commenting, I'm moving forward with both of your suggestions in istanbul-reports.

Any thoughts on the node-preload issue? The fact that node.js attempted to require /Users/al/Projects/coverage-problem/.yarn/cache/node-preload-npm-0.1.3-55fd0dfa1d.zip/node_modules/node-preload/node-preload.js shows that the main nyc process successfully loaded node-preload, but then the child process failed to find it. node-preload is meant to be the solution to the fact that spawn-wrap has proven unreliable (it often fails in Windows and seems to have issues under yarn).

[arcanis]

Hm ... Cannot find module is a message that's only emitted when operating outside of a PnP context (otherwise we would say the reason why the module cannot be found, like in my previous post).

I suspect that node-preload is injected in front of NODE_OPTIONS (or maybe completely overrides it?), causing it to be loaded before Node is able to require the files from within the cache zip archives. Do you know where the injection happens?

[coreyfarrell]

node-preload definitely inserts itself to the start of NODE_OPTIONS. The reason for this is to support coverage when code intended to be loaded with the --require option. So if a test contains:

const result = require('child_process').execFileSync(
  process.execPath,
  [require.resolve('./fixtures/spawn.js')],
  {
    env: {NODE_OPTIONS: `--require "${require.resolve('../register.js')}"`}
  }
);

The child process will collect coverage for ../register.js.

Edit:
The injection happens inside the child_process functions which spawns a new process. This is done by patching process.binding('spawn_sync').spawn and require('child_process').ChildProcess.prototype.spawn. In both paths the internal spawn options.env is altered at https://github.com/cfware/node-preload/blob/master/node-preload-singleton.js#L84-L107 to ensure nyc gets loaded into the child process.

[arcanis]

I see - in this case, I'm afraid NYC must have a special code to support PnP. It shouldn't be too long, the following would probably work:

if (process.versions.pnp)
  nodeOptionRequireSelf = `--require ${require.resolve('pnpapi')} ${nodeOptionRequireSelf}`;

This way you'd bootstrap PnP before loading NYC. Does that sound right?

[coreyfarrell]

Will require.resolve('pnpapi') work from node-preload even though it won't be a dependency? What versions of node.js are supported by pnpapi? It's actually not as simple as just --require ${require.resolve('pnpapi')}.

If pnpapi only works in Node.js 12 then the path passed to require can be quoted with double-quotes and backslashes escaped. For node.js 8-11 it's more difficult as it's impossible to pass a path with a space to --require. This means NODE_PATH has to be setup to include the pnpapi directory and NODE_OPTIONS would include --require ${path.basename(require.resolve('pnpapi'))}. Configuring NODE_PATH like this is concerning if path.dirname(require.resolve('pnpapi')) contains lots of arbitrary files. The problem is that the user might already be using NODE_PATH to add a certain file to the path but another file of the same name might also be in the pnpapi directory.

This difference before/after node.js 12 is a big part of why node-preload is a stand-alone module rather than being part of nyc. Keeping it separate allowed for better testing. I can do some refactoring of the way that node-preload calculates NODE_PATH and NODE_OPTIONS allowing it to incorporate pnpapi. I'd like some more information about the risk of this leaking too much stuff to NODE_PATH though.

[arcanis]

The pnpapi module is a special builtin that's always available in PnP environments (regardless of the Node version). In practice require.resolve('pnpapi') points to the actual ./.pnp.js file that Yarn generates, which exposes the PnP API and associated hook (so adding it to NODE_PATH would be similar to adding the project root).

For node.js 8-11 it's more difficult as it's impossible to pass a path with a space

Yep I know, it's actually my PR which implemented the space support 😄 I think a reasonable approach might be to simply throw an exception on Node < 12 if the path to pnpapi contains a space. It's less intrusive that modifying NODE_PATH, at least.

Fwiw Yarn currently injects the hook using the path, regardless of whether it contains spaces or not. I'll fix that to follow my own suggestion and throw instead on Node < 12, but since noone reported a problem so far I'd expect this use case to be somewhat fringe.

[coreyfarrell]

Another option to deal with node < 12 could be to put your file into a subdirectory. If pnpapi were in ./.yarn/pnp/.yarnpnp.js adding that otherwise empty directory to NODE_PATH would have very little risk of conflicting (who else is going to name a file .yarnpnp.js).

If I understand correctly spawning a subprocess with a cleared NODE_OPTIONS env will currently prevent the subprocess from loading pnpapi. Once I make the necessary changes node-preload will actually prevent removal of pnpapi in this way. What I'm planning is to delay the check of process.versions.pnp until the options.env rewrite during spawn of a new process. At that time if process.versions.pnp is truthy loading of the current pnpapi will be forced into the child process even if the env has been cleared.

A specific example I can think of is if someone installed nyc 15 globally and ran nyc yarn test from a repository with pnp enabled. nyc would load node-preload which would wrap the yarn process. So when yarn uses the child_process function to spawn the repository test script process.versions.pnp needs to be set correctly and require.resolve('pnpapi') would need to resolve to .pnp.js in the target repository.

Another edge case to consider, what happens if yarn test of one pnp repository executes yarn test of another repository (pnp or not)? Do you take steps to prevent the .pnp.js of the first repository from loading the test script of the new repository?

I've posted cfware/node-preload#3 for the yarn pnp changes. FWIW I testing locally using yarn v2 with a devDependency for nyc to install from git://github.com/coreyfarrell/nyc#yarnpnp (this includes the istanbul-reports fix and install from git of node-preload). This consistently caused yarn v2 to lock up upon trying to install nyc from that git url. It was some kind of infinite loop, the yarn process started using 100% CPU and didn't stop until I hit Ctrl-C. Neither yarn v1 nor npm had this issue. Removing .yarn/cache and trying again not help. I don't want to do a new release of node-preload nor nyc until I can test with yarn v2 pnp, so your advice here is appreciated.

[coreyfarrell]

Additional context, my local test using PnP with git://github.com/coreyfarrell/nyc#yarnpnp did work using yarn v1.

[arcanis]

Another option to deal with node < 12 could be to put your file into a subdirectory. If pnpapi were in ./.yarn/pnp/.yarnpnp.js adding that otherwise empty directory to NODE_PATH would have very little risk of conflicting (who else is going to name a file .yarnpnp.js).

I would prefer not to move the file. Some tools might already assume PnP environments if this specific file exists, and better keep this assumption working (we could add an indirection, but I'm not sure it's worth it - it causes problems when watching the file etc).

Another edge case to consider, what happens if yarn test of one pnp repository executes yarn test of another repository (pnp or not)? Do you take steps to prevent the .pnp.js of the first repository from loading the test script of the new repository?

It already kinda happens - when you run yarn install with a git repository, Yarn will clone the repo and runs yarn pack there, which in turn call yarn prepack and in turn may call the binaries and scripts from the second project.

While I haven't wrote explicit code to handle that, it definitely seems to work - I think it's because the two hooks are loaded, but only the last one "wins".

This consistently caused yarn v2 to lock up upon trying to install nyc from that git url. It was some kind of infinite loop, the yarn process started using 100% CPU and didn't stop until I hit Ctrl-C.

Can you run:

yarn set version from sources --branch 539
yarn install --inline-builds

And wait for it to run for at least one minute? What happens is that Yarn is cloning the repository and running yarn install && yarn pack there. It's likely why you experience this slowdown, and since we don't print anything it might seem like it's stucked in a loop. If it runs for more than a minute something is probably wrong, however (it succeeded for me, fwiw).

Note that I'm going in 🌴🍹 for a week tomorrow. I'll make sure to check back as soon as I come back!

[coreyfarrell]

I tried installing your branch:

$ yarn set version from sources --branch 539
➤ YN0000: Cloning the remote repository

Cloning into '/tmp/yarnpkg-sources'...
error: pathspec 'origin/539' did not match any file(s) known to git
➤ YN0001: Error: Child "git" exited with exit code 1

I checked in /tmp/yarnpkg-sources, the remote URL for origin is https://github.com/yarnpkg/berry.git. Any suggestion?

[arcanis]

Oh right, cloning from PRs is pretty recent. Try this:

yarn set version from sources # First update to master
yarn set version from sources --branch 539 # Then fetch the PR

It should do the trick.

[coreyfarrell]

Branch 539 allows it to install, unfortunately the test fails. I'm going to post my test repository in a minute.

$ yarn test
internal/modules/cjs/loader.js:783
    throw err;
    ^

Error: Cannot find module 'pnpapi'
Require stack:
- internal/preload
    at Function.Module._resolveFilename (internal/modules/cjs/loader.js:780:15)
    at Function.Module._load (internal/modules/cjs/loader.js:685:27)
    at Module.require (internal/modules/cjs/loader.js:838:19)
    at Module._preloadModules (internal/modules/cjs/loader.js:1114:12)
    at loadPreloadModules (internal/bootstrap/pre_execution.js:429:5)
    at prepareMainThreadExecution (internal/bootstrap/pre_execution.js:53: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 |                   
----------|---------|----------|---------|---------|-------------------

[coreyfarrell]

I've posted a minimum demonstration of the issue at https://github.com/coreyfarrell/yarn-pnp.

[arcanis]

Perfect! I've just opened yarnpkg/berry#542 to fix the problem you've witnessed, it will be merged soonish. Running yarn test after updating to --branch 542 yields the following 🎉

I think we'll also add an E2E test when I get some time, to track potential regressions 🙂

[coreyfarrell]

When I get a chance I'll add .pnp.js to the default nyc exclude list.

I do still have concerns about node-preload being problematic in cases where multiple yarn pnp's are wrapped. Lets say you have:

.pnp.js
packages/pkg1/.pnp.js

Assume yarn test in the repository root runs nyc node run-tests.js and run-tests.js executes yarn test from each sub directory. nyc and node-preload will resolved from within the root repository but will still wrap processes that run in the sub directory. Both .pnp.js files would be in NODE_OPTIONS but node-preload would always re-insert the current pnpapi to the start of NODE_OPTIONS. I'm not sure if this is just my misunderstand of how yarn would be used, maybe workspaces could solve this problem and style of monorepo just wouldn't be viable?

BTW you mentioned that you were planning on having node.js < 12 just throw if process.cwd() has a space. I just wish to point out that this is likely to create trouble in Windows, nyc has seen a number failures caused by spaces in different paths (I believe due to the scripts that spawn-wrap created so my hope is that nyc 15 will eliminate this problem).

[arcanis]

maybe workspaces could solve this problem and style of monorepo just wouldn't be viable?

Yep, that's how I see things. When using workspaces, you'd only have a single .pnp.js at the root of your repository, which would contain the dependency tree for all your packages.

That said the scenario you describe might cause problems when using a globally-installed nyc, as it would run the local script with the PnP hook listing nyc's dependencies, not the project ones ... or maybe it would just be a matter of calling nyc yarn node ./run-tests.js, which would ensure the right PnP file ends up loaded 🤔

To be fair, the NODE_OPTIONS preload is admittedly a bit clunky. There are some discussions in the Node Modules WG to instead be able to define loaders through the package.json - that would be much better. In the meantime NODE_OPTIONS is a reasonable tradeoff!

[coreyfarrell]

Would globally installed nyc get PnP? I have no plans to add any yarn specific options to nyc's package.json. I would expect a globally installed nyc to have a 'normal' node_modules directory with all the dependencies installed.

[arcanis]

PnP is the default linking algorithm we're switching to in the v2, correct (although we have some plans to eventually support legacy node_modules installs, maybe even in time for the stable release).

It's not so much about what features the individual projects use, but rather about the guarantees that the package manager can provide to the user. Given that PnP has the same runtime interface characteristics as traditional node_modules installs but with better speed and safety characteristics, that's definitely where we'll focus our efforts in the future.

[coreyfarrell]

I think nyc globally installed via PnP would require that yarn implement "merging" functionality in .pnp.js. Consider running nyc yarn test. This will result in nyc adding it's node-preload dependency to NODE_OPTIONS and spawning yarn test. If yarn then replaces the nyc pnpapi with a project specific one I expect all children of yarn test to fail when NODE_OPTIONS tells it to --require node-preload (and node-preload performs require on nyc by absolute filename). So the repository .pnp.js needs to consider any already loaded pnpapi, actually merge the list of available modules.

[thonatos]

@coreyfarrell

I have run my tests with [email protected], it does solve the problem. when will the official version be released ?

[coreyfarrell]

@thonatos The official version will be released when it is ready. When updates are available for the new release they will be posted to #1104.

[thealjey]

@coreyfarrell

I have run my tests with [email protected], it does solve the problem. when will the official version be released ?

it might be fixing your tests, but it does not fix the tests linked to in this issue

[coreyfarrell]

@thealjey Pointing your package.json nyc version to git://github.com/istanbuljs/nyc#8078a79 fixes the issue in my test. A new beta will be released soon. If you are still having problems after updating it likely indicates an issue with a cache or lockfile.

[thealjey]

@coreyfarrell I had to remove and reinstall everything, including Yarn v2 itself
and then it worked! 🥇

[thealjey]

I think this issue can be closed, in fact