test_runner: improve code coverage cleanup

The test runner's code coverage leaves old coverage data in the
temp directory. This commit updates the cleanup logic to:

- Stop code collection. Otherwise V8 would write collection data
  again when the process exits.
- Remove the temp directory containing the coverage data.
- Attempt to clean up the coverage data even if parsing the
  data resulted in an error.

With this change, I no longer see any coverage data left behind
in the system temp directory.

Refs: https://github.com/nodejs/build/issues/3864
Refs: https://github.com/nodejs/build/issues/3887
PR-URL: https://github.com/nodejs/node/pull/54856
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Jake Yuesong Li <jake.yuesong@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: James M Snell <jasnell@gmail.com>

lib/internal/test_runner/coverage.js

@@ -23,6 +23,7 @@ const {
   mkdtempSync,
   opendirSync,
   readFileSync,
+  rmSync,
 } = require('fs');
 const { setupCoverageHooks } = require('internal/util');
 const { tmpdir } = require('os');
@@ -272,27 +273,34 @@ class TestCoverage {
   cleanup() {
     // Restore the original value of process.env.NODE_V8_COVERAGE. Then, copy
     // all of the created coverage files to the original coverage directory.
+    internalBinding('profiler').endCoverage();
+
     if (this.originalCoverageDirectory === undefined) {
       delete process.env.NODE_V8_COVERAGE;
-      return;
+    } else {
+      process.env.NODE_V8_COVERAGE = this.originalCoverageDirectory;
+      let dir;
+
+      try {
+        mkdirSync(this.originalCoverageDirectory, { __proto__: null, recursive: true });
+        dir = opendirSync(this.coverageDirectory);
+
+        for (let entry; (entry = dir.readSync()) !== null;) {
+          const src = join(this.coverageDirectory, entry.name);
+          const dst = join(this.originalCoverageDirectory, entry.name);
+          copyFileSync(src, dst);
+        }
+      } finally {
+        if (dir) {
+          dir.closeSync();
+        }
+      }
     }
 
-    process.env.NODE_V8_COVERAGE = this.originalCoverageDirectory;
-    let dir;
-
     try {
-      mkdirSync(this.originalCoverageDirectory, { __proto__: null, recursive: true });
-      dir = opendirSync(this.coverageDirectory);
-
-      for (let entry; (entry = dir.readSync()) !== null;) {
-        const src = join(this.coverageDirectory, entry.name);
-        const dst = join(this.originalCoverageDirectory, entry.name);
-        copyFileSync(src, dst);
-      }
-    } finally {
-      if (dir) {
-        dir.closeSync();
-      }
+      rmSync(this.coverageDirectory, { __proto__: null, recursive: true });
+    } catch {
+      // Ignore cleanup errors.
     }
   }
 

lib/internal/test_runner/harness.js

@@ -159,12 +159,15 @@ function collectCoverage(rootTest, coverage) {
 
   try {
     summary = coverage.summary();
+  } catch (err) {
+    rootTest.diagnostic(`Warning: Could not report code coverage. ${err}`);
+    process.exitCode = kGenericUserError;
+  }
+
+  try {
     coverage.cleanup();
   } catch (err) {
-    const op = summary ? 'clean up' : 'report';
-    const msg = `Warning: Could not ${op} code coverage. ${err}`;
-
-    rootTest.diagnostic(msg);
+    rootTest.diagnostic(`Warning: Could not clean up code coverage. ${err}`);
     process.exitCode = kGenericUserError;
   }
 

src/inspector_profiler.cc

@@ -556,6 +556,21 @@ static void StopCoverage(const FunctionCallbackInfo<Value>& args) {
   }
 }
 
+static void EndCoverage(const FunctionCallbackInfo<Value>& args) {
+  Environment* env = Environment::GetCurrent(args);
+  V8CoverageConnection* connection = env->coverage_connection();
+
+  Debug(env,
+        DebugCategory::INSPECTOR_PROFILER,
+        "EndCoverage, connection %s nullptr\n",
+        connection == nullptr ? "==" : "!=");
+
+  if (connection != nullptr) {
+    Debug(env, DebugCategory::INSPECTOR_PROFILER, "Ending coverage\n");
+    connection->End();
+  }
+}
+
 static void Initialize(Local<Object> target,
                        Local<Value> unused,
                        Local<Context> context,
@@ -565,6 +580,7 @@ static void Initialize(Local<Object> target,
       context, target, "setSourceMapCacheGetter", SetSourceMapCacheGetter);
   SetMethod(context, target, "takeCoverage", TakeCoverage);
   SetMethod(context, target, "stopCoverage", StopCoverage);
+  SetMethod(context, target, "endCoverage", EndCoverage);
 }
 
 void RegisterExternalReferences(ExternalReferenceRegistry* registry) {
@@ -572,6 +588,7 @@ void RegisterExternalReferences(ExternalReferenceRegistry* registry) {
   registry->Register(SetSourceMapCacheGetter);
   registry->Register(TakeCoverage);
   registry->Register(StopCoverage);
+  registry->Register(EndCoverage);
 }
 
 }  // namespace profiler