Repository: istanbuljs/nyc
Pull request #1169 (2019-09-06 23:28:13)

feat: Add `--use-spawn-wrap=true` option
Completely bypass spawn-wrap unless overridden with this new option.

coveralls: 
[![Coverage Status](https://coveralls.io/builds/25579942/badge)](https://coveralls.io/builds/25579942)

Coverage increased (+0.01%) to 98.514% when pulling **6b78a7ba5b999dc106f5b600a3db80d143891cac on coreyfarrell:set-node-options** into **5c1b7a9c43771f3439af44a1104e5426519e1123 on istanbuljs:master**.

isaacs: This is cool.  When did the support for `env.NODE_OPTIONS` land?
coreyfarrell: > This is cool. When did the support for `env.NODE_OPTIONS` land?

node.js 8.0.0 added the option: https://nodejs.org/dist/latest-v8.x/docs/api/cli.html#cli_node_options_options

node.js < 12 does not support quoting the path given to `--require` so it is not possible to directly do:
```js
process.env.NODE_OPTIONS = `--require ${pathWithSpaces}`
```
isaacs: I was thinking about the case where someone clears the environment variable before calling spawn, like `spawn(process.execPath, ['foo.js'], { env: { NODE_OPTIONS: 'different' } })`.

We could update nyc's `wrap.js` to munge ChildProcess.spawn kind of like how spawn-wrap does, but without writing a shim file, such that it _always_ puts the environment variable back in.

Then the only hazard would be something like `spawn('bash', ['escape.sh'])` where `escape.sh` does `NODE_OPTIONS= node foo.js`, but that's enough of an explicit step that we should probably respect it anyway.
coreyfarrell: > I was thinking about the case where someone clears the environment variable before calling spawn, like `spawn(process.execPath, ['foo.js'], { env: { NODE_OPTIONS: 'different' } })`.
> 
> We could update nyc's `wrap.js` to munge ChildProcess.spawn kind of like how spawn-wrap does, but without writing a shim file, such that it _always_ puts the environment variable back in.

I thought of this, I've created https://github.com/cfware/node-preload as a potential alternative to spawn-wrap.  The functionality has enough platform/version edge cases that it probably deserves it's own testing.  I'm actually looking to get it to a state where it might be possible to get the functionality into the node.js core.  My module is not published to npm yet, I want to get a little feedback first.

> Then the only hazard would be something like `spawn('bash', ['escape.sh'])` where `escape.sh` does `NODE_OPTIONS= node foo.js`, but that's enough of an explicit step that we should probably respect it anyway.

Agreed.  Also we can't control everything.

Patch:
diff --git a/bin/nyc.js b/bin/nyc.js
index 6dd78bf..da9f456 100755
--- a/bin/nyc.js
+++ b/bin/nyc.js
@@ -3,11 +3,9 @@
 
 const configUtil = require('../lib/config-util')
 const foreground = require('foreground-child')
+const resolveFrom = require('resolve-from')
 const NYC = require('../index.js')
 
-const sw = require('spawn-wrap')
-const wrapper = require.resolve('./wrap.js')
-
 // parse configuration and command-line arguments;
 // we keep these values in a few different forms,
 // used in the various execution contexts of nyc:
@@ -44,10 +42,7 @@ async function main () {
     await nyc.addAllFiles()
   }
 
-  var env = {
-    // Support running nyc as a user without HOME (e.g. linux 'nobody'),
-    // https://github.com/istanbuljs/nyc/issues/951
-    SPAWN_WRAP_SHIM_ROOT: process.env.SPAWN_WRAP_SHIM_ROOT || process.env.XDG_CACHE_HOME || require('os').homedir(),
+  const env = {
     NYC_CONFIG: JSON.stringify(argv),
     NYC_CWD: process.cwd()
   }
@@ -58,7 +53,29 @@ async function main () {
     env.BABEL_DISABLE_CACHE = process.env.BABEL_DISABLE_CACHE = '1'
   }
 
-  sw([wrapper], env)
+  if (!argv.useSpawnWrap) {
+    const { preloadAppend, propagateEnv } = require('node-preload')
+
+    nyc.require.forEach(requireModule => {
+      const mod = resolveFrom.silent(nyc.cwd, requireModule) || requireModule
+      preloadAppend(mod)
+      require(mod)
+    })
+    preloadAppend(require.resolve('../lib/wrap.js'))
+    Object.assign(propagateEnv, env)
+  }
+
+  if (argv.all) nyc.addAllFiles()
+
+  if (argv.useSpawnWrap) {
+    const wrapper = require.resolve('./wrap.js')
+    // Support running nyc as a user without HOME (e.g. linux 'nobody'),
+    // https://github.com/istanbuljs/nyc/issues/951
+    env.SPAWN_WRAP_SHIM_ROOT = process.env.SPAWN_WRAP_SHIM_ROOT || process.env.XDG_CACHE_HOME || require('os').homedir()
+    const sw = require('spawn-wrap')
+
+    sw([wrapper], env)
+  }
 
   // Both running the test script invocation and the check-coverage run may
   // set process.exitCode. Keep track so that both children are run, but
diff --git a/bin/wrap.js b/bin/wrap.js
index 3aa2a9d..63d8205 100644
--- a/bin/wrap.js
+++ b/bin/wrap.js
@@ -1,29 +1,2 @@
-var sw = require('spawn-wrap')
-var NYC = require('../index.js')
-
-var config = {}
-if (process.env.NYC_CONFIG) config = JSON.parse(process.env.NYC_CONFIG)
-config.isChildProcess = true
-
-config._processInfo = {
-  pid: process.pid,
-  ppid: process.ppid,
-  parent: process.env.NYC_PROCESS_ID || null
-}
-if (process.env.NYC_PROCESSINFO_EXTERNAL_ID) {
-  config._processInfo.externalId = process.env.NYC_PROCESSINFO_EXTERNAL_ID
-  delete process.env.NYC_PROCESSINFO_EXTERNAL_ID
-}
-
-if (process.env.NYC_CONFIG_OVERRIDE) {
-  const override = JSON.parse(process.env.NYC_CONFIG_OVERRIDE)
-  config = {
-    ...config,
-    ...override
-  }
-  process.env.NYC_CONFIG = JSON.stringify(config)
-}
-
-;(new NYC(config)).wrap()
-
-sw.runMain()
+require('../lib/wrap')
+require('spawn-wrap').runMain()
diff --git a/index.js b/index.js
index 8b45915..c51bf75 100755
--- a/index.js
+++ b/index.js
@@ -129,6 +129,10 @@ class NYC {
   }
 
   _loadAdditionalModules () {
+    if (!this.config.useSpawnWrap) {
+      return
+    }
+
     this.require.forEach(requireModule => {
       // Attempt to require the module relative to the directory being instrumented.
       // Then try other locations, e.g. the nyc node_modules folder.
@@ -347,6 +351,12 @@ class NYC {
 
   wrap (bin) {
     process.env.NYC_PROCESS_ID = this.processInfo.uuid
+    // This is a bug with the spawn-wrap method where
+    // we cannot force propagation of NYC_PROCESS_ID.
+    if (!this.config.useSpawnWrap) {
+      const { propagateEnv } = require('node-preload')
+      propagateEnv.NYC_PROCESS_ID = this.processInfo.uuid
+    }
     this._addRequireHooks()
     this._wrapExit()
     this._loadAdditionalModules()
diff --git a/lib/config-util.js b/lib/config-util.js
index 3acf94a..28d61e0 100644
--- a/lib/config-util.js
+++ b/lib/config-util.js
@@ -201,6 +201,12 @@ async function processConfig (cwd) {
       type: 'boolean',
       global: false
     })
+    .option('use-spawn-wrap', {
+      describe: 'use spawn-wrap instead of setting process.env.NODE_OPTIONS',
+      default: false,
+      type: 'boolean',
+      global: false
+    })
     .option('clean', {
       describe: 'should the .nyc_output folder be cleaned before executing tests',
       default: true,
diff --git a/lib/wrap.js b/lib/wrap.js
new file mode 100644
index 0000000..cb32133
--- /dev/null
+++ b/lib/wrap.js
@@ -0,0 +1,29 @@
+const NYC = require('../index.js')
+
+let config = {}
+if (process.env.NYC_CONFIG) {
+  config = JSON.parse(process.env.NYC_CONFIG)
+}
+config.isChildProcess = true
+
+config._processInfo = {
+  pid: process.pid,
+  ppid: process.ppid,
+  parent: process.env.NYC_PROCESS_ID || null
+}
+
+if (process.env.NYC_PROCESSINFO_EXTERNAL_ID) {
+  config._processInfo.externalId = process.env.NYC_PROCESSINFO_EXTERNAL_ID
+  delete process.env.NYC_PROCESSINFO_EXTERNAL_ID
+}
+
+if (process.env.NYC_CONFIG_OVERRIDE) {
+  const override = JSON.parse(process.env.NYC_CONFIG_OVERRIDE)
+  config = {
+    ...config,
+    ...override
+  }
+  process.env.NYC_CONFIG = JSON.stringify(config)
+}
+
+;(new NYC(config)).wrap()
diff --git a/package-lock.json b/package-lock.json
index df614af..38b3f93 100644
--- a/package-lock.json
+++ b/package-lock.json
@@ -3313,6 +3313,11 @@
       "integrity": "sha1-jZ2+KJZKSsVxLpExZCEHxx6Q7EA=",
       "dev": true
     },
+    "node-preload": {
+      "version": "0.1.2",
+      "resolved": "https://registry.npmjs.org/node-preload/-/node-preload-0.1.2.tgz",
+      "integrity": "sha512-OIVpCWTqvPClyXxt9HtKbHDKqQQzqzQIcv6FJgXGcgrQQiYPXNL38vDp1RbRwsF7HVDGWc7yQa5DNu2Zbs9MVw=="
+    },
     "normalize-package-data": {
       "version": "2.5.0",
       "resolved": "https://registry.npmjs.org/normalize-package-data/-/normalize-package-data-2.5.0.tgz",
diff --git a/package.json b/package.json
index 7e5de9d..ff2f0a1 100644
--- a/package.json
+++ b/package.json
@@ -87,6 +87,7 @@
     "make-dir": "^3.0.0",
     "merge-source-map": "^1.1.0",
     "p-map": "^3.0.0",
+    "node-preload": "^0.1.2",
     "resolve-from": "^5.0.0",
     "rimraf": "^3.0.0",
     "signal-exit": "^3.0.2",


Test patch:
diff --git a/tap-snapshots/test-nyc-integration.js-TAP.test.js b/tap-snapshots/test-nyc-integration.js-TAP.test.js
index 83d4586..b026b05 100644
--- a/tap-snapshots/test-nyc-integration.js-TAP.test.js
+++ b/tap-snapshots/test-nyc-integration.js-TAP.test.js
@@ -113,7 +113,7 @@ All files                        |       0 |        0 |       0 |       0 |
   external-instrumenter.js       |       0 |        0 |       0 |       0 | 1                       
   gc.js                          |       0 |      100 |     100 |       0 | 2,3                     
   half-covered-failing.js        |       0 |        0 |     100 |       0 | 1,3,5,6,7,8             
-  selfspawn-fibonacci.js         |       0 |        0 |       0 |       0 | ...19,21,24,25,26,27,28 
+  selfspawn-fibonacci.js         |       0 |        0 |       0 |       0 | ...21,23,26,27,28,29,30 
   skip-full.js                   |       0 |      100 |     100 |       0 | 1,2                     
   test.js                        |       0 |        0 |       0 |       0 |                         
  cli/fakebin                     |       0 |      100 |     100 |       0 |                         
@@ -144,8 +144,8 @@ exports[`test/nyc-integration.js TAP --ignore-class-method skips methods that ma
 ---------------------------------|---------|----------|---------|---------|-------------------------
 File                             | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s       
 ---------------------------------|---------|----------|---------|---------|-------------------------
-All files                        |    1.45 |        0 |       5 |    1.89 |                         
- cli                             |    2.08 |        0 |    5.56 |    3.13 |                         
+All files                        |    1.44 |        0 |       5 |    1.87 |                         
+ cli                             |    2.06 |        0 |    5.56 |    3.08 |                         
   args.js                        |       0 |      100 |     100 |       0 | 1                       
   by-arg2.js                     |       0 |        0 |     100 |       0 | 1,2,3,4,5,7             
   classes.js                     |   66.67 |      100 |      50 |   66.67 | 6                       
@@ -158,7 +158,7 @@ All files                        |    1.45 |        0 |       5 |    1.89 |
   gc.js                          |       0 |      100 |     100 |       0 | 2,3                     
   half-covered-failing.js        |       0 |        0 |     100 |       0 | 1,3,5,6,7,8             
   half-covered.js                |       0 |        0 |     100 |       0 | 1,3,5,6,7,8             
-  selfspawn-fibonacci.js         |       0 |        0 |       0 |       0 | ...19,21,24,25,26,27,28 
+  selfspawn-fibonacci.js         |       0 |        0 |       0 |       0 | ...21,23,26,27,28,29,30 
   skip-full.js                   |       0 |      100 |     100 |       0 | 1,2                     
  cli/fakebin                     |       0 |      100 |     100 |       0 |                         
   npm-template.js                |       0 |      100 |     100 |       0 | 2,3,4,7,9               
@@ -202,8 +202,8 @@ exports[`test/nyc-integration.js TAP --show-process-tree displays a tree of spaw
 ------------------------|---------|----------|---------|---------|-------------------
 File                    | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s 
 ------------------------|---------|----------|---------|---------|-------------------
-All files               |   90.91 |       70 |     100 |     100 |                   
- selfspawn-fibonacci.js |   90.91 |       70 |     100 |     100 | 4,25,27           
+All files               |    91.3 |       70 |     100 |     100 |                   
+ selfspawn-fibonacci.js |    91.3 |       70 |     100 |     100 | 6,27,29           
 ------------------------|---------|----------|---------|---------|-------------------
 nyc
 │   100 % Lines
@@ -214,19 +214,41 @@ nyc
   │ ├─┬ node ./selfspawn-fibonacci.js 3
   │ │ │   100 % Lines
   │ │ ├── node ./selfspawn-fibonacci.js 2
-  │ │ │     31.58 % Lines
+  │ │ │     35 % Lines
   │ │ └── node ./selfspawn-fibonacci.js 1
-  │ │       26.32 % Lines
+  │ │       30 % Lines
   │ └── node ./selfspawn-fibonacci.js 2
-  │       31.58 % Lines
+  │       35 % Lines
   └─┬ node ./selfspawn-fibonacci.js 3
     │   100 % Lines
     ├── node ./selfspawn-fibonacci.js 2
-    │     31.58 % Lines
+    │     35 % Lines
     └── node ./selfspawn-fibonacci.js 1
-          26.32 % Lines
+          30 % Lines
 
 
+`
+
+exports[`test/nyc-integration.js TAP --use-spawn-wrap=false is functional > stdout 1`] = `
+3
+------------------------|---------|----------|---------|---------|-------------------
+File                    | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s 
+------------------------|---------|----------|---------|---------|-------------------
+All files               |    91.3 |       70 |     100 |     100 |                   
+ selfspawn-fibonacci.js |    91.3 |       70 |     100 |     100 | 6,27,29           
+------------------------|---------|----------|---------|---------|-------------------
+
+`
+
+exports[`test/nyc-integration.js TAP --use-spawn-wrap=true is functional > stdout 1`] = `
+3
+------------------------|---------|----------|---------|---------|-------------------
+File                    | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s 
+------------------------|---------|----------|---------|---------|-------------------
+All files               |    91.3 |       70 |     100 |     100 |                   
+ selfspawn-fibonacci.js |    91.3 |       70 |     100 |     100 | 6,27,29           
+------------------------|---------|----------|---------|---------|-------------------
+
 `
 
 exports[`test/nyc-integration.js TAP allows .nycrc configuration to be overridden with command line args > stdout 1`] = `
@@ -853,30 +875,6 @@ exports[`test/nyc-integration.js TAP passes configuration via environment variab
 ]
 `
 
-exports[`test/nyc-integration.js TAP produce-source-map enabled > stdout 1`] = `
-Error: Blarrh
-    at blah (./stack-trace.js:3:1)
-----------------|---------|----------|---------|---------|-------------------
-File            | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s 
-----------------|---------|----------|---------|---------|-------------------
-All files       |     100 |      100 |     100 |     100 |                   
- stack-trace.js |     100 |      100 |     100 |     100 |                   
-----------------|---------|----------|---------|---------|-------------------
-
-`
-
-exports[`test/nyc-integration.js TAP produce-source-map not enabled > stdout 1`] = `
-Error: Blarrh
-    at blah (./stack-trace.js:1:1037)
-----------------|---------|----------|---------|---------|-------------------
-File            | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s 
-----------------|---------|----------|---------|---------|-------------------
-All files       |     100 |      100 |     100 |     100 |                   
- stack-trace.js |     100 |      100 |     100 |     100 |                   
-----------------|---------|----------|---------|---------|-------------------
-
-`
-
 exports[`test/nyc-integration.js TAP recursive run does not throw > stdout 1`] = `
 
 `
diff --git a/test/add-all-files.js b/test/add-all-files.js
index 38750dc..6369473 100644
--- a/test/add-all-files.js
+++ b/test/add-all-files.js
@@ -60,7 +60,11 @@ t.test('transpiles .js files added via addAllFiles', async t => {
     'utf-8'
   )
 
-  const nyc = new NYC(await parseArgv(fixtures, ['--require', transpileHook]))
+  const nyc = new NYC(await parseArgv(fixtures, [
+    '--use-spawn-wrap=true',
+    '--require',
+    transpileHook
+  ]))
   await nyc.reset()
   await nyc.addAllFiles()
 
@@ -83,6 +87,7 @@ t.test('does not attempt to transpile files when they are excluded', async t =>
   )
 
   const nyc = new NYC(await parseArgv(fixtures, [
+    '--use-spawn-wrap=true',
     `--require=${transpileHook}`,
     '--extension=.do-not-transpile',
     '--include=needs-transpile.do-not-transpile'
@@ -103,6 +108,7 @@ t.test('transpiles non-.js files added via addAllFiles', async t => {
   )
 
   const nyc = new NYC(await parseArgv(fixtures, [
+    '--use-spawn-wrap=true',
     `--require=${transpileHook}`,
     '--extension=.whatever'
   ]))
diff --git a/test/fixtures/cli/selfspawn-fibonacci.js b/test/fixtures/cli/selfspawn-fibonacci.js
index d176415..8d983a6 100644
--- a/test/fixtures/cli/selfspawn-fibonacci.js
+++ b/test/fixtures/cli/selfspawn-fibonacci.js
@@ -1,6 +1,8 @@
 'use strict';
 var cp = require('child_process');
 
+process.env = {};
+
 var index = +process.argv[2] || 0
 if (index <= 1) {
   console.log(0)
diff --git a/test/nyc-integration.js b/test/nyc-integration.js
index ba68d22..278d35a 100644
--- a/test/nyc-integration.js
+++ b/test/nyc-integration.js
@@ -186,6 +186,14 @@ t.test('--show-process-tree displays a tree of spawned processes', t => testSucc
   args: ['--show-process-tree', process.execPath, 'selfspawn-fibonacci.js', '5']
 }))
 
+t.test('--use-spawn-wrap=true is functional', t => testSuccess(t, {
+  args: ['--use-spawn-wrap=true', process.execPath, 'selfspawn-fibonacci.js', '5']
+}))
+
+t.test('--use-spawn-wrap=false is functional', t => testSuccess(t, {
+  args: ['--use-spawn-wrap=false', process.execPath, 'selfspawn-fibonacci.js', '5']
+}))
+
 t.test('can run "npm test" which directly invokes a test file', t => testSuccess(t, {
   args: ['npm', 'test'],
   cwd: path.resolve(fixturesCLI, 'run-npm-test')