Skip to content

Commit

Permalink
test_runner: improve code coverage cleanup
Browse files Browse the repository at this point in the history
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: nodejs/build#3864
Refs: nodejs/build#3887
PR-URL: #54856
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Jake Yuesong Li <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: James M Snell <[email protected]>
  • Loading branch information
cjihrig authored and aduh95 committed Sep 13, 2024
1 parent 94fb7ab commit de73d1e
Show file tree
Hide file tree
Showing 3 changed files with 48 additions and 20 deletions.
40 changes: 24 additions & 16 deletions lib/internal/test_runner/coverage.js
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ const {
mkdtempSync,
opendirSync,
readFileSync,
rmSync,
} = require('fs');
const { setupCoverageHooks } = require('internal/util');
const { tmpdir } = require('os');
Expand Down Expand Up @@ -272,28 +273,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() {
Expand Down
11 changes: 7 additions & 4 deletions lib/internal/test_runner/harness.js
Original file line number Diff line number Diff line change
Expand Up @@ -159,12 +159,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;
}

Expand Down
17 changes: 17 additions & 0 deletions src/inspector_profiler.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -565,13 +580,15 @@ 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) {
registry->Register(SetCoverageDirectory);
registry->Register(SetSourceMapCacheGetter);
registry->Register(TakeCoverage);
registry->Register(StopCoverage);
registry->Register(EndCoverage);
}

} // namespace profiler
Expand Down

0 comments on commit de73d1e

Please sign in to comment.