Node: NODE_V8_COVERAGE broken on OSX (perhaps other platforms)

[bcoe]

https://github.com/nodejs/node/pull/25127 seems to break coverage on OSX:

NODE_V8_COVERAGE=.coverage ./node ./test/parallel/test-v8-coverage.js 

TypeError [ERR_INVALID_ARG_TYPE]: The "path" argument must be one of type string, Buffer, or URL. Received type undefined
    at mkdirSync (fs.js:766:3)
    at process.writeCoverage (internal/process/coverage.js:16:5)
    at process.emit (events.js:193:15)

My guess is that coverage might be initialized either before the inspector is running, or before the process.env object has been populated.

Not sure why this wouldn't have been caught in the linux test suite.

I believe this is the root cause of https://github.com/nodejs/node-v8/issues/97

CC: @nodejs/build

[cjihrig]

Initializing coverage, relies on path.resolve(), which uses process.cwd(), which hasn't been setup yet. The following patch seems to fix the problem on my machine, which is macOS:

diff --git a/lib/internal/process/coverage.js b/lib/internal/process/coverage.js
index 5c5d0c2b61..e3cf74f395 100644
--- a/lib/internal/process/coverage.js
+++ b/lib/internal/process/coverage.js
@@ -76,9 +76,10 @@ function setup() {
   }));

   try {
+    const { cwd } = internalBinding('process_methods');
     const { resolve } = require('path');
     coverageDirectory = process.env.NODE_V8_COVERAGE =
-      resolve(process.env.NODE_V8_COVERAGE);
+      resolve(cwd(), process.env.NODE_V8_COVERAGE);
   } catch (err) {
     console.error(err);
   }

It might just be simpler to move the coverage setup until after the process object is setup in lib/internal/bootstrap/node.js though.

EDIT: It's also worth noting that the existing code without this patch does throw an exception, but console.error() is not set up yet either, which might be a stronger case for moving this to lib/internal/bootstrap/node.js.

[addaleax]

Thanks for investigating! Joyee probably has an easier time digging in as she’s on OSX as well (I think).

before the process.env object has been populated.

This happens very early during bootstrap, so I assume it’s not that.

[cjihrig]

Here is an alternative approach that moves the coverage setup to a later point in the bootstrapping process. It sacrifices a bit of core internals coverage, but the process is in a much more usable state:

diff --git a/lib/internal/bootstrap/loaders.js b/lib/internal/bootstrap/loaders.js
index 93fb186574..d8d0827e2d 100644
--- a/lib/internal/bootstrap/loaders.js
+++ b/lib/internal/bootstrap/loaders.js
@@ -353,12 +353,6 @@ NativeModule.prototype.cache = function() {
   NativeModule._cache[this.id] = this;
 };

-// Coverage must be turned on early, so that we can collect
-// it for Node.js' own internal libraries.
-if (process.env.NODE_V8_COVERAGE) {
-  NativeModule.require('internal/process/coverage').setup();
-}
-
 function internalBindingWhitelistHas(name) {
   if (!internalBindingWhitelistSet) {
     const { SafeSet } = NativeModule.require('internal/safe_globals');
diff --git a/lib/internal/bootstrap/node.js b/lib/internal/bootstrap/node.js
index 3c14d4851a..87fd2076f3 100644
--- a/lib/internal/bootstrap/node.js
+++ b/lib/internal/bootstrap/node.js
@@ -141,7 +141,11 @@ function startup() {
     NativeModule.require('internal/process/write-coverage').setup();

   if (process.env.NODE_V8_COVERAGE) {
-    NativeModule.require('internal/process/coverage').setupExitHooks();
+    const { setup, setupExitHooks } =
+      NativeModule.require('internal/process/coverage');
+
+    setup();
+    setupExitHooks();
   }

   if (process.config.variables.v8_enable_inspector) {

[bcoe]

@cjihrig my preference would be to keep the bootstrapping is early as possible, assuming we can actually raise a build failure if we have a regression like this. If we move initialization too late, it makes it difficult to use NODE_V8_COVERAGE for Node's own test coverage, defeating the motivation for a lot of this work -- perhaps we just need to do without the console.error message.

[devsnek]

i'm in favor of the first patch.

@bcoe i think you can use process._rawDebug instead of console.error

[joyeecheung]

For now, using methods that are available earlier during bootstrap sounds good to me (thanks @cjihrig !) though I wonder whether we should postpone the setup to some point later, or move setup() to C++. It's hard to imagine how to keep this working if we are going to pursue the snapshot - ideally none of per_context.js, loaders.js nor the early part of node.js should have execution flows that depend on CLI flags or environment variables in order to have a point to create a stable snapshot that can be loaded consistently later. Also when snapshots do happen I believe the coverage can not cover the JS code run before the snapshot capture anyways.

[joyeecheung]

Another way to fix the particular issue at hand is to postpone the coverageDirectory initialization to a later point - IIUC, the only thing that needs to happen as early as possible is the Profiler.startPreciseCoverage dispatch. The initialization of coverageDirectory can at least be be postpone to any point prior to user code execution, and that's the only part that has transitive dependency on many other modules.

[bcoe]

@joyeecheung you are correct that the Profiler.startPreciseCoverage is what needs to have run as early as possible.

Unrelated, shouldn't this have broken tests? we should make sure that our regression tests actually work before we land a fix.

[cjihrig]

Unrelated, shouldn't this have broken tests? we should make sure that our regression tests actually work before we land a fix.

I updated the coverage test in #25289, so this should be caught moving forward.