From 5c4f2d2685ca042ef481c1e14142f97b14e782eb Mon Sep 17 00:00:00 2001
From: Colin Ihrig <cjihrig@gmail.com>
Date: Tue, 10 Sep 2024 22:50:28 -0400
Subject: [PATCH] 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 | 40 +++++++++++++++++-----------
 lib/internal/test_runner/harness.js  | 11 +++++---
 src/inspector_profiler.cc            | 17 ++++++++++++
 3 files changed, 48 insertions(+), 20 deletions(-)

diff --git a/lib/internal/test_runner/coverage.js b/lib/internal/test_runner/coverage.js
index 81a06182929fa8..385a52fb34f2d5 100644
--- a/lib/internal/test_runner/coverage.js
+++ b/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');
@@ -270,28 +271,35 @@ 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;
-    }
-
-    process.env.NODE_V8_COVERAGE = this.originalCoverageDirectory;
-    let dir;
+    } else {
+      process.env.NODE_V8_COVERAGE = this.originalCoverageDirectory;
+      let dir;
 
-    try {
-      mkdirSync(this.originalCoverageDirectory, { __proto__: null, recursive: true });
-      dir = opendirSync(this.coverageDirectory);
+      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();
+        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();
+        }
       }
     }
+
+    try {
+      rmSync(this.coverageDirectory, { __proto__: null, recursive: true });
+    } catch {
+      // Ignore cleanup errors.
+    }
   }
 
   getCoverageFromDirectory() {
diff --git a/lib/internal/test_runner/harness.js b/lib/internal/test_runner/harness.js
index c7d951d6b485a5..d67bcf7cd2beab 100644
--- a/lib/internal/test_runner/harness.js
+++ b/lib/internal/test_runner/harness.js
@@ -113,12 +113,15 @@ function collectCoverage(rootTest, coverage) {
 
   try {
     summary = coverage.summary();
-    coverage.cleanup();
   } catch (err) {
-    const op = summary ? 'clean up' : 'report';
-    const msg = `Warning: Could not ${op} code coverage. ${err}`;
+    rootTest.diagnostic(`Warning: Could not report code coverage. ${err}`);
+    process.exitCode = kGenericUserError;
+  }
 
-    rootTest.diagnostic(msg);
+  try {
+    coverage.cleanup();
+  } catch (err) {
+    rootTest.diagnostic(`Warning: Could not clean up code coverage. ${err}`);
     process.exitCode = kGenericUserError;
   }
 
diff --git a/src/inspector_profiler.cc b/src/inspector_profiler.cc
index aeebc1f18a026d..0ebd2ea61adef5 100644
--- a/src/inspector_profiler.cc
+++ b/src/inspector_profiler.cc
@@ -503,6 +503,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,
@@ -512,6 +527,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) {
@@ -519,6 +535,7 @@ void RegisterExternalReferences(ExternalReferenceRegistry* registry) {
   registry->Register(SetSourceMapCacheGetter);
   registry->Register(TakeCoverage);
   registry->Register(StopCoverage);
+  registry->Register(EndCoverage);
 }
 
 }  // namespace profiler