refactor: collect v8 coverage from node, not the launcher - #2993
Merged
Conversation
acozzette
force-pushed
the
node-v8-coverage
branch
5 times, most recently
from
August 29, 2026 18:44
2800fb4 to
efa49f4
Compare
The bash launcher currently exports the `NODE_V8_COVERAGE` environment variable and points it at `$COVERAGE_DIR`. When we switch over to hermetic_launcher we will no longer be able to do this before Node starts, so this change works around that problem by enabling coverage from within Node, without the need to set that environment variable in advance. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
acozzette
force-pushed
the
node-v8-coverage
branch
from
August 31, 2026 21:00
efa49f4 to
90bda74
Compare
startCollection registers its exit listener before reportOnExit does, so the profile is already written by the time the report hook runs. Only the branch that asks node for its profile is live; the return value is now just a flag saying which of us collects. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The preload now runs in every js_binary under `bazel coverage`, so a throw while opening the session fails the target before its own code starts. Report empty coverage instead: v8.takeCoverage() is a no-op when nothing is collecting, so the report hook needs no extra state. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A coverage failure should mark the target failed, but a program that already exited non-zero has chosen its own code: expected_exit_code saw 1 where the program asked for 42. node keeps process.exitCode current as it exits and leaves it unset only on a clean exit, so a falsy check is enough. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The header still described the file as only a report generator, and claimed bootstrap.cjs requires it only for JS_BINARY__COVERAGE_REPORT. Both stopped being true when collection moved in here, and the second is the gate the runfiles change in js_binary.bzl hangs on. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Not worth a public escape hatch just so a test can see that collection started; child.js asserts NODE_V8_COVERAGE instead, which coverage.cjs sets only once the session is running. flush is now reachable only as the exit listener, so its idempotency guard goes too. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
bootstrap.cjs requires it whenever COVERAGE_DIR is set, but only _js_binary_impl declared it, so rules that call create_launcher and assemble their own runfiles shipped a bootstrap that could not find it. Under `bazel coverage`, //js/private/test/create_launcher:test fails with `Cannot find module './coverage.cjs'` once module resolution stops escaping the runfiles tree through the source symlink. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
session.post() reports a command failure through its callback and drops it entirely without one, so a failed Profiler.enable left us claiming to collect with no profiler running: children got a NODE_V8_COVERAGE nobody writes, and the report skipped its v8.takeCoverage() fallback. The callback is synchronous, so routing both commands through it reaches the degrade path. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
acozzette
marked this pull request as ready for review
September 2, 2026 16:00
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0453bfe0e3
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
node records a source map for every module it compiles with a //# sourceMappingURL and writes them alongside its own profile under "source-map-cache"; the reporter needs them to map V8 offsets in generated code back to the original source. The reporter re-reads that comment from disk on its own, so this makes no difference to code compiled ahead of time, where the generated file and its map are both present. It matters for a runtime transpiler such as ts-node, which compiles from memory: nothing on disk holds the generated code or its map, and without the cache the reporter attributes execution to the wrong lines rather than reporting none. The cache is already being populated. node gates it on NODE_V8_COVERAGE, read as each module compiles rather than at startup, and startCollection sets that before any of the program is compiled. Nothing here enables source-mapped stack traces. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Nothing covered the source-map cache in the coverage profile: every other target here runs code that is on disk as it was compiled, where the reporter finds the map itself and the cache changes nothing. transpiled.js stands in for a runtime transpiler such as ts-node. It compiles transpiled_lib.js from memory with a banner line prepended, so the generated code sits one line below the file on disk and the only record of the offset is the one node keeps for us. Dropping the cache from the profile does not empty the report, it shifts it, so the merger asserts specific lines rather than that coverage is non-empty: FN:1 and DA:5,0 and DA:8,1 all move without the map. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
No behavior change; cleanup from a review pass. - js_binary.bzl: _coverage_bootstrap is an unconditional member of the shared attrs, and _node_patches beside it is already dereferenced unguarded, so the hasattr could never be false. It made a false parallel with the one in _generates_coverage_report, which guards an attr that really is optional. - bootstrap.cjs: the gate restated startCollection's precondition in the file that does not own it. One term per capability now, and coverage.cjs is the only place that knows what "already collecting" means; a child that inherits NODE_V8_COVERAGE just requires a module that no-ops. Hence the image goldens, which record this file's size. - coverage.cjs: flush() re-derived post()'s synchronous-callback trick inline rather than calling it, which cost a second copy of the explanation and four levels of nesting. post() now returns the result and flush() is flat. - test/coverage: one copy of the enable_runfiles select rather than six, and drop the module setup in transpiled.js that _compile(content, filename) makes unnecessary. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
jbedard
reviewed
Sep 2, 2026
reportOnExit was called once, from a top-level if, with a parameter that was already an in-scope constant. The function bought a name and a forward reference and nothing else; the module now reads straight down, collect then report. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
jbedard
approved these changes
Sep 2, 2026
acozzette
added a commit
that referenced
this pull request
Sep 3, 2026
Three fixes to the hermetic launcher added in the previous commit: - Drop the `NODE_V8_COVERAGE` export. #2993 removed these lines from `js_binary.sh.tpl` so that coverage is started from within node; the JS launcher was ported from a pre-#2993 base and still carried them. Output is byte-identical either way (node collects natively when the variable is preset), but the launcher sets it *after* chdir'ing to `BAZEL_BINDIR`, where `coverage.cjs` resolves `COVERAGE_DIR` against the execroot, so a relative `COVERAGE_DIR` would have landed profiles in the wrong place. - Normalize `RUNFILES_MANIFEST_FILE` before testing its suffix, as `bash.bzl` does. On Windows Bazel hands out a backslash-separated path, which would not match `/MANIFEST` and would take the fatal branch. - Accept a drive-letter or UNC prefix when deciding whether a `node_toolchain`'s `target_tool_path` is absolute. `startswith("/")` misses `C:\...`, which would then be embedded in the stub as the rlocation `_main/C:\...` and never resolve. The launcher itself already classified it correctly via `path.isAbsolute`. The latter two are Windows-only paths, which this repo does not exercise. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The bash launcher currently exports the
NODE_V8_COVERAGEenvironment variable and points it at$COVERAGE_DIR. When we switch over to hermetic_launcher we will no longer be able to do this before Node starts, so this change works around that problem by enabling coverage from within Node, without the need to set that environment variable in advance.Changes are visible to end-users: no
Test plan