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 👍 / 👎.
| "$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 |
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.