fix: node coverage#2904
Conversation
|
|
![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: b0ab821194
ℹ️ 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".
| # the report produced here. | ||
| # ============================================================================== | ||
| if [ "${COVERAGE_DIR:-}" ] && [ "${COVERAGE_OUTPUT_FILE:-}" ]; then | ||
| js_coverage_report=$(find "$JS_BINARY__RUNFILES" -path '*/js/private/coverage/coverage.js' 2>/dev/null | head -1) |
There was a problem hiding this comment.
When a covered js_test has more than one runfile matching */js/private/coverage/coverage.js (for example, the application also has a file at that path in its data), this find | head -1 pipeline can fail even after finding a match: the launcher runs with set -o pipefail, so head closing the pipe may make find exit with SIGPIPE/141, and the assignment then exits the wrapper under set -e before it returns the test result or writes coverage. Use find ... -print -quit or a known merger runfile path instead.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Another robots suggestion for this:
The clean fix: js_binary.bzl already knows the coverage entry point label and merges _lcov_merger runfiles (js_binary.bzl:547-548). Plumb the runfiles path in as a template substitution / env var populated only when hasattr(ctx.attr, "_lcov_merger") — i.e., only for js_test. That's O(1), deterministic, and automatically excludes nested js_binary launchers since their scripts are expanded per-target at analysis time.
| "$JS_BINARY__NODE_BINARY" "$js_coverage_report" || | ||
| logf_error "coverage report generation failed" |
There was a problem hiding this comment.
If the in-test c8 conversion fails here (bad manifest/source path, missing tool, or a tool crash), the launcher only logs the error and still exits with the test's original status. In split coverage postprocessing, the later merger is exactly the action that lacks the instrumented sources, so this can turn a coverage-generation failure back into a successful bazel coverage run with empty or stale coverage instead of surfacing the failure as the old merger path did.
Useful? React with 👍 / 👎.
|
|
||
| # --- begin runfiles.bash initialization v3 --- | ||
| # Copy-pasted from the Bazel Bash runfiles library v3. | ||
| set -uo pipefail; set +e; f=bazel_tools/tools/bash/runfiles/runfiles.bash |
There was a problem hiding this comment.
the aspect format (shfmt) applied this automatically. It's purely cosmetic. We can revert , but it will resurface the next time someone touches the file.
| # ============================================================================== | ||
| # Generate the lcov coverage report here in the test action, where the V8 | ||
| # coverage data (NODE_V8_COVERAGE) and the instrumented sources both exist and | ||
| # their paths match. The _lcov_merger runs in a separate action that does not |
There was a problem hiding this comment.
robot says:
The fix only works because js_binary.bzl:538-548 merges _lcov_merger runfiles into the test's runfiles, and that block is annotated "TODO: Remove once bazel<8 support is dropped." After this PR, removing it would silently resurrect the 0% bug (the find misses → fallback → merger without sources). The comment needs updating so nobody executes that TODO.
Which makes me think this might not be the right approach for this...?
3 participants
Fixes #2901:
bazel coveragereported 0% (emptycoverage.dat) for JS tests whenever coverage postprocessing ran as its own action (--experimental_split_coverage_postprocessing, which is typical with remote execution).The V8 -> lcov conversion ran in the
_lcov_mergeraction, whose runfiles contain only the merger and node, not the instrumented sources, so c8 emittedLF:0for every file. The conversion now runs at the end of the test action, where the V8 data and sources coexist; the report is stashed in$COVERAGE_DIRand the merger simply publishes it, falling back to the old behavior when no stash exists. Adding the sources to the merger action was considered instead, but the V8 URLs still point at the test's sandbox paths and would need fragile rewriting.Changes are visible to end-users: yes
Test plan
e2e/coverage/test.shrunsbazel coveragein both the default and split-postprocessing modes and asserts the lcov report attributes hits and misses tolib.js. Fails against pre-fix sources (coverage.dat is missing or empty), passes with the fix.Disclaimer: This is my first contribution on this project. I've read the code of conduct and the contributing guidelines, but if I made a mistake in the creation of this PR, please let me know! I'm happy to change the target branch or anything else that is required.