diff --git a/doc/api/cli.md b/doc/api/cli.md index 514af04c02cd..a23c6214eb6c 100644 --- a/doc/api/cli.md +++ b/doc/api/cli.md @@ -4146,6 +4146,32 @@ If `value` equals `'child'`, test reporter options will be overridden and test output will be sent to stdout in the TAP format. If any other value is provided, Node.js makes no guarantees about the reporter format used or its stability. +### `NODE_TEST_COVERAGE_EXCLUDE_NODE_MODULES=1` + + + +When the coverage filter requested by [`NODE_TEST_COVERAGE_FILTER_DIR`][] is +active, scripts located inside a `node_modules` directory are also omitted +from the written coverage data. + +### `NODE_TEST_COVERAGE_FILTER_DIR=dir` + + + +Set by the test runner, along with [`NODE_V8_COVERAGE`][], while it is +collecting code coverage. If the directory that a process writes its code +coverage data to matches `dir` exactly, coverage data for scripts that cannot +appear in the test runner's coverage report, such as Node.js internals, is +omitted from the written files. Coverage written to any other directory is +not affected. + +This environment variable is an implementation detail of the test runner and +is not meant to be set manually. + ### `NODE_TLS_REJECT_UNAUTHORIZED=value` If `value` equals `'0'`, certificate validation is disabled for TLS connections. @@ -4534,7 +4560,9 @@ node --stack-trace-limit=12 -p -e "Error.stackTraceLimit" # prints 12 [`ERR_INVALID_TYPESCRIPT_SYNTAX`]: errors.md#err_invalid_typescript_syntax [`ERR_UNSUPPORTED_TYPESCRIPT_SYNTAX`]: errors.md#err_unsupported_typescript_syntax [`NODE_OPTIONS`]: #node_optionsoptions +[`NODE_TEST_COVERAGE_FILTER_DIR`]: #node_test_coverage_filter_dirdir [`NODE_USE_ENV_PROXY=1`]: #node_use_env_proxy1 +[`NODE_V8_COVERAGE`]: #node_v8_coveragedir [`NO_COLOR`]: https://no-color.org [`Web Storage`]: https://developer.mozilla.org/en-US/docs/Web/API/Web_Storage_API [`YoungGenerationSizeFromSemiSpaceSize`]: https://chromium.googlesource.com/v8/v8.git/+/refs/tags/10.3.129/src/heap/heap.cc#328 diff --git a/doc/node.1 b/doc/node.1 index dd541051de28..9dc898f1d544 100644 --- a/doc/node.1 +++ b/doc/node.1 @@ -2347,6 +2347,19 @@ If \fBvalue\fR equals \fB'child'\fR, test reporter options will be overridden an output will be sent to stdout in the TAP format. If any other value is provided, Node.js makes no guarantees about the reporter format used or its stability. . +.It Ev NODE_TEST_COVERAGE_EXCLUDE_NODE_MODULES Ar 1 +When the coverage filter requested by \fBNODE_TEST_COVERAGE_FILTER_DIR\fR is +active, scripts located inside a node_modules directory are also omitted from +the written coverage data. +. +.It Ev NODE_TEST_COVERAGE_FILTER_DIR Ar dir +Set by the test runner, along with \fBNODE_V8_COVERAGE\fR, while it is collecting +code coverage. If the directory that a process writes its code coverage data +to matches \fBdir\fR exactly, coverage data for scripts that cannot appear in the +test runner's coverage report, such as Node.js internals, is omitted from the +written files. This environment variable is an implementation detail of the +test runner and is not meant to be set manually. +. .It Ev NODE_TLS_REJECT_UNAUTHORIZED Ar value If \fBvalue\fR equals \fB'0'\fR, certificate validation is disabled for TLS connections. This makes TLS, and HTTPS by extension, insecure. The use of this environment diff --git a/lib/internal/test_runner/coverage.js b/lib/internal/test_runner/coverage.js index 55151d64563f..bab8e92e9691 100644 --- a/lib/internal/test_runner/coverage.js +++ b/lib/internal/test_runner/coverage.js @@ -362,6 +362,8 @@ class TestCoverage { // 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(); + delete process.env.NODE_TEST_COVERAGE_FILTER_DIR; + delete process.env.NODE_TEST_COVERAGE_EXCLUDE_NODE_MODULES; if (this.originalCoverageDirectory === undefined) { delete process.env.NODE_V8_COVERAGE; @@ -685,6 +687,14 @@ function setupCoverage(options) { // child processes. process.env.NODE_V8_COVERAGE = coverageDirectory; + if (originalCoverageDirectory === undefined) { + process.env.NODE_TEST_COVERAGE_FILTER_DIR = enabled; + + if ((options.coverageIncludeGlobs?.length ?? 0) === 0) { + process.env.NODE_TEST_COVERAGE_EXCLUDE_NODE_MODULES = '1'; + } + } + return new TestCoverage( coverageDirectory, originalCoverageDirectory, diff --git a/src/inspector_profiler.cc b/src/inspector_profiler.cc index 559b4fd27d56..e49541e6dc33 100644 --- a/src/inspector_profiler.cc +++ b/src/inspector_profiler.cc @@ -20,13 +20,13 @@ namespace node { namespace profiler { using errors::TryCatchScope; +using v8::Array; using v8::Context; using v8::Function; using v8::FunctionCallbackInfo; using v8::HandleScope; using v8::Isolate; using v8::Local; -using v8::NewStringType; using v8::Object; using v8::String; using v8::Value; @@ -254,6 +254,105 @@ void V8ProfilerConnection::WriteProfile(simdjson::ondemand::object* result) { WriteResult(env_, path.c_str(), profile); } +// Whether a coverage entry for this script URL can appear in the test +// runner's coverage report at all. The report only considers file: URLs, and +// skips node_modules unless include globs (which only the test runner process +// knows about) could add them back. +static bool ShouldKeepScriptUrl(std::string_view url, + bool exclude_node_modules) { + if (!url.starts_with("file:")) { + return false; + } + if (exclude_node_modules && + url.find("/node_modules/") != std::string_view::npos) { + return false; + } + return true; +} + +// Rebuild `profile` (a serialized Profiler.takePreciseCoverage result) with +// the scripts that can never appear in the coverage report removed, so that +// they are neither written to disk nor read and parsed again by the report +// generator. The raw JSON of the kept entries is copied verbatim; only the +// "result" array is rewritten. `profile` must be backed by a buffer with at +// least simdjson::SIMDJSON_PADDING readable bytes past its end. +// Returns false if the profile could not be processed; `out` must be +// discarded in that case. +static bool FilterCoverageProfile(std::string_view profile, + bool exclude_node_modules, + std::string* out) { + simdjson::ondemand::parser parser; + simdjson::ondemand::parser script_parser; + simdjson::ondemand::document doc; + simdjson::ondemand::object top; + if (parser + .iterate(profile.data(), + profile.size(), + profile.size() + simdjson::SIMDJSON_PADDING) + .get(doc) || + doc.get_object().get(top)) { + return false; + } + + out->reserve(profile.size() / 4); + *out += '{'; + bool first_field = true; + for (auto field_result : top) { + simdjson::ondemand::field field; + if (std::move(field_result).get(field)) { + return false; + } + std::string_view key = field.escaped_key(); + if (!first_field) *out += ','; + first_field = false; + *out += '"'; + *out += key; + *out += "\":"; + if (key == "result") { + simdjson::ondemand::array scripts; + if (field.value().get_array().get(scripts)) { + return false; + } + *out += '['; + bool first_script = true; + for (auto script : scripts) { + std::string_view raw; + if (script.raw_json().get(raw)) { + return false; + } + // Peek at the script's URL by re-parsing the raw slice; the slice + // points into the padded message buffer, so over-reading + // SIMDJSON_PADDING bytes is safe. + simdjson::ondemand::document script_doc; + std::string_view url; + if (script_parser + .iterate(raw.data(), + raw.size(), + raw.size() + simdjson::SIMDJSON_PADDING) + .get(script_doc) || + script_doc["url"].get_string().get(url)) { + return false; + } + if (!ShouldKeepScriptUrl(url, exclude_node_modules)) { + continue; + } + if (!first_script) *out += ','; + first_script = false; + *out += raw; + } + *out += ']'; + } else { + std::string_view raw; + if (field.value().raw_json().get(raw)) { + return false; + } + *out += raw; + } + } + *out += '}'; + return true; +} + void V8CoverageConnection::WriteProfile(simdjson::ondemand::object* result) { Isolate* isolate = env_->isolate(); HandleScope handle_scope(isolate); @@ -271,14 +370,49 @@ void V8CoverageConnection::WriteProfile(simdjson::ondemand::object* result) { Local context = env_->context(); Context::Scope context_scope(context); - // Generate the profile output from the subclass. - auto profile_opt = GetProfile(result); - if (!profile_opt.has_value()) { + std::string_view profile; + if (result->raw_json().get(profile)) { + fprintf(stderr, "Cannot get raw string of the %s profile\n", type()); return; } - std::string_view profile = profile_opt.value(); - // append source-map cache information to coverage object: + // When the profile is written into a directory owned by the test runner + // (advertised through this env var handshake so that it also reaches the + // test child processes writing into the same directory), drop the scripts + // its report is guaranteed to discard - node:* internals, which usually + // dominate the profile, and node_modules unless include globs may add them + // back - before serialization, instead of writing, re-reading and + // re-parsing them. User-provided NODE_V8_COVERAGE directories always + // receive the full profile. + bool filter = false; + bool exclude_node_modules = false; + { + std::optional filter_dir = + env_->env_vars()->Get("NODE_TEST_COVERAGE_FILTER_DIR"); + if (filter_dir.has_value() && !filter_dir->empty() && + *filter_dir == env_->coverage_directory()) { + filter = true; + exclude_node_modules = + env_->env_vars() + ->Get("NODE_TEST_COVERAGE_EXCLUDE_NODE_MODULES") + .has_value(); + } + } + + std::string filtered_profile; + if (filter) { + if (FilterCoverageProfile(profile, exclude_node_modules, + &filtered_profile)) { + profile = filtered_profile; + } else { + // Fall back to the full profile; the report filters it again anyway. + fprintf(stderr, + "Failed to filter %s profile, writing it in full\n", + type()); + } + } + + // Gather source-map cache information to append to the coverage object. Local source_map_cache_v; { TryCatchScope try_catch(env()); @@ -309,58 +443,59 @@ void V8CoverageConnection::WriteProfile(simdjson::ondemand::object* result) { // Only insert source map cache when there's source map data at all. if (!source_map_cache_v->IsUndefined()) { - // It would be more performant to just find the last } and insert the source - // map cache in front of it, but source map cache is still experimental - // anyway so just re-parse it with V8 for now. - Local profile_str; - if (!v8::String::NewFromUtf8(isolate, - profile.data(), - v8::NewStringType::kNormal, - profile.length()) - .ToLocal(&profile_str)) { - fprintf(stderr, "Failed to re-parse %s profile as UTF8\n", type()); - return; - } - Local profile_value; - if (!v8::JSON::Parse(context, profile_str).ToLocal(&profile_value) || - !profile_value->IsObject()) { - fprintf(stderr, "Failed to re-parse %s profile from JSON\n", type()); - return; + // Apply the same filter to the source map cache: entries for scripts + // that were dropped above can never be looked up by the report. The + // getter builds a fresh object on every call, so it is safe to prune. + if (filter && source_map_cache_v->IsObject()) { + Local cache_obj = source_map_cache_v.As(); + Local names; + if (cache_obj->GetOwnPropertyNames(context).ToLocal(&names)) { + for (uint32_t i = 0; i < names->Length(); i++) { + Local cache_key; + if (!names->Get(context, i).ToLocal(&cache_key)) { + break; + } + Utf8Value key_utf8(isolate, cache_key); + if (!ShouldKeepScriptUrl(key_utf8.ToStringView(), + exclude_node_modules)) { + if (cache_obj->Delete(context, cache_key).IsNothing()) { + break; + } + } + } + } } - if (profile_value.As() - ->Set(context, - FIXED_ONE_BYTE_STRING(isolate, "source-map-cache"), - source_map_cache_v) - .IsNothing()) { - fprintf(stderr, - "Failed to insert source map cache into %s profile\n", - type()); + + // Serialize only the source map cache and splice it in front of the + // profile's closing brace, instead of round-tripping the entire profile + // through V8's JSON parser and serializer. + Local cache_json; + if (!v8::JSON::Stringify(context, source_map_cache_v) + .ToLocal(&cache_json)) { + fprintf(stderr, "Failed to stringify %s source map cache\n", type()); return; } - Local result_s; - if (!v8::JSON::Stringify(context, profile_value).ToLocal(&result_s)) { - fprintf(stderr, "Failed to stringify %s profile result\n", type()); + Utf8Value cache_utf8(isolate, cache_json); + size_t end = profile.find_last_of('}'); + if (end == std::string_view::npos) { + fprintf(stderr, "Malformed %s profile result\n", type()); return; } - Utf8Value result_utf8(isolate, result_s); - WriteResult(env_, path.c_str(), result_utf8.ToStringView()); + std::string_view head = profile.substr(0, end); + std::string output; + output.reserve(head.length() + cache_utf8.length() + 32); + output += head; + // `head` is the profile object minus its closing brace; it always + // contains at least the "result" field, so a separating comma is needed. + output += ",\"source-map-cache\":"; + output += cache_utf8.ToStringView(); + output += '}'; + WriteResult(env_, path.c_str(), output); } else { WriteResult(env_, path.c_str(), profile); } } -std::optional V8CoverageConnection::GetProfile( - simdjson::ondemand::object* result) { - std::string_view profile_raw; - if (result->raw_json().get(profile_raw)) { - fprintf(stderr, - "Cannot get raw string of the 'profile' field from %s profile\n", - type()); - return std::nullopt; - } - return profile_raw; -} - std::string V8CoverageConnection::GetDirectory() const { return env()->coverage_directory(); } diff --git a/src/inspector_profiler.h b/src/inspector_profiler.h index d07334c818d9..7fb743ee652b 100644 --- a/src/inspector_profiler.h +++ b/src/inspector_profiler.h @@ -92,10 +92,7 @@ class V8CoverageConnection : public V8ProfilerConnection { std::string GetDirectory() const override; std::string GetFilename() const override; - std::optional GetProfile( - simdjson::ondemand::object* result) override; void WriteProfile(simdjson::ondemand::object* result) override; - void WriteSourceMapCache(); void TakeCoverage(); void StopCoverage(); diff --git a/test/parallel/test-v8-coverage-test-runner-filter.js b/test/parallel/test-v8-coverage-test-runner-filter.js new file mode 100644 index 000000000000..17fdf0eb93a5 --- /dev/null +++ b/test/parallel/test-v8-coverage-test-runner-filter.js @@ -0,0 +1,117 @@ +'use strict'; + +const common = require('../common'); +const assert = require('assert'); +const fs = require('fs'); +const path = require('path'); +const { pathToFileURL } = require('url'); +const { spawnSync } = require('child_process'); + +common.skipIfInspectorDisabled(); + +const tmpdir = require('../common/tmpdir'); +tmpdir.refresh(); + +const sourceMap = Buffer.from(JSON.stringify({ + version: 3, + sources: ['original.js'], + mappings: '', +})).toString('base64'); +const mapComment = `//# sourceMappingURL=data:application/json;base64,${sourceMap}`; + +const appPath = tmpdir.resolve('app.js'); +const depPath = tmpdir.resolve(path.join('node_modules', 'dep', 'index.js')); +fs.mkdirSync(path.dirname(depPath), { recursive: true }); +// Source maps of files inside node_modules are only cached when node_modules +// source map support is enabled explicitly. +fs.writeFileSync(appPath, 'require(\'module\')' + + '.setSourceMapsSupport(true, { nodeModules: true });\n' + + `require('dep');\n${mapComment}\n`); +fs.writeFileSync(depPath, `module.exports = 42;\n${mapComment}\n`); + +const appUrl = pathToFileURL(appPath).href; +const depUrl = pathToFileURL(depPath).href; + +let dirc = 0; +// `makeEnv` receives the coverage directory and returns extra env vars. +function runWithCoverage(makeEnv) { + const coverageDirectory = tmpdir.resolve(`cov_${++dirc}`); + const output = spawnSync(process.execPath, [appPath], { + cwd: tmpdir.path, + env: { + ...process.env, + NODE_V8_COVERAGE: coverageDirectory, + ...makeEnv(coverageDirectory), + }, + }); + if (output.status !== 0) { + console.log(output.stderr.toString()); + } + assert.strictEqual(output.status, 0); + assert.strictEqual(output.stderr.toString(), ''); + + const urls = []; + const sourceMapCacheKeys = []; + for (const coverageFile of fs.readdirSync(coverageDirectory)) { + const coverage = + JSON.parse(fs.readFileSync(path.join(coverageDirectory, coverageFile))); + for (const script of coverage.result) { + urls.push(script.url); + } + sourceMapCacheKeys.push(...Object.keys(coverage['source-map-cache'] ?? {})); + } + return { coverageDirectory, urls, sourceMapCacheKeys }; +} + +// Filter directory and node_modules exclusion set: only file: URLs outside of +// node_modules remain, in the coverage results as well as in the source map +// cache. +{ + const { urls, sourceMapCacheKeys } = runWithCoverage((dir) => ({ + NODE_TEST_COVERAGE_FILTER_DIR: dir, + NODE_TEST_COVERAGE_EXCLUDE_NODE_MODULES: '1', + })); + assert.ok(urls.length > 0); + assert.ok(urls.every((url) => url.startsWith('file:')), urls.join(',')); + assert.ok(urls.includes(appUrl)); + assert.ok(!urls.includes(depUrl)); + assert.ok(urls.every((url) => !url.includes('/node_modules/'))); + assert.deepStrictEqual(sourceMapCacheKeys, [appUrl]); +} + +// Filter directory alone: node:* internals are dropped, node_modules are +// kept. +{ + const { urls, sourceMapCacheKeys } = runWithCoverage((dir) => ({ + NODE_TEST_COVERAGE_FILTER_DIR: dir, + })); + assert.ok(urls.every((url) => url.startsWith('file:')), urls.join(',')); + assert.ok(urls.includes(appUrl)); + assert.ok(urls.includes(depUrl)); + assert.ok(sourceMapCacheKeys.includes(appUrl)); + assert.ok(sourceMapCacheKeys.includes(depUrl)); +} + +// A filter directory that does not match the coverage directory must have no +// effect: this is what keeps user-facing NODE_V8_COVERAGE output complete +// when the env var leaks into a process that redirects NODE_V8_COVERAGE. +{ + const { urls } = runWithCoverage(() => ({ + NODE_TEST_COVERAGE_FILTER_DIR: tmpdir.resolve('some_other_dir'), + NODE_TEST_COVERAGE_EXCLUDE_NODE_MODULES: '1', + })); + assert.ok(urls.some((url) => url.startsWith('node:'))); + assert.ok(urls.includes(appUrl)); + assert.ok(urls.includes(depUrl)); +} + +// Plain NODE_V8_COVERAGE keeps the full profile, including node:* internals, +// with the source map cache appended. +{ + const { urls, sourceMapCacheKeys } = runWithCoverage(() => ({})); + assert.ok(urls.some((url) => url.startsWith('node:'))); + assert.ok(urls.includes(appUrl)); + assert.ok(urls.includes(depUrl)); + assert.ok(sourceMapCacheKeys.includes(appUrl)); + assert.ok(sourceMapCacheKeys.includes(depUrl)); +}