ci(coverage): add PR coverage summary comments#203
Conversation
There was a problem hiding this comment.
Pull request overview
This PR enhances the coverage workflow to provide visibility on pull requests by enabling the workflow to run on PR events and automatically posting coverage summaries as PR comments.
Changes:
- Enable coverage workflow to trigger on
pull_requestevents with the same path filters as push events - Add a coverage summary step that formats and outputs coverage data for PR comments
- Implement PR comment posting for coverage summaries with fork repository checks to prevent workflow failures
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
📊 solx Tester Report ➡️ Download |
|
📊 Hardhat Projects Report ➡️ Download |
|
📊 Foundry Projects Report ➡️ Download |
hedgar2017
force-pushed
the
ci/coverage-pr-comments
branch
2 times, most recently
from
04de9f9 to
f6831df
Compare
Coverage Summary
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 7 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
hedgar2017
force-pushed
the
ci/coverage-pr-comments
branch
from
87c63f1 to
d9713a5
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 6 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 6 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
hedgar2017
force-pushed
the
ci/coverage-pr-comments
branch
from
7693ca5 to
6799b09
Compare
- Enable coverage workflow on pull_request events - Add coverage summary step using llvm-cov report - Post coverage summary as PR comment using mshick/add-pr-comment - Add fork checks to skip secrets-dependent steps on fork PRs - Add pull-requests: write permission for commenting Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Coverage: trigger only with `ci:coverage` label - Sanitizer: trigger only with `ci:sanitizer` label - Remove push-to-main, schedule, and workflow_dispatch triggers - Rename sanitizers.yaml to sanitizer.yaml (singular) - Add fork checks to skip jobs for fork PRs - Move sanitizer params to job-level env vars Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
The `llvm-cov export` command writes to `./llvm/codecov.lcov` but the directory did not exist, causing the step to fail with ENOENT. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Ignore .claude/, aider files, and logs/ directory. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The coverage summary included thousands of LLVM/solc/cargo library files, making the output too large for Docker's argument limit when posting the PR comment. Filter with --ignore-filename-regex to keep only solx project sources, and truncate the PR comment to header + totals for a compact summary. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Add COVERAGE_IGNORE_REGEX job-level env var to centralize filename filtering across llvm-cov show/export/report commands - Add --ignore-filename-regex to HTML report generation and LCOV export to exclude solx-llvm, target-llvm, solx-solidity, and cargo/registry - Rewrite coverage summary step to produce per-crate markdown table with emoji indicators (green/yellow/red) instead of raw llvm-cov text - PR comment and job summary now render as a formatted table - Use COVERAGE_EOF delimiter for robustness Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…tions - Move sanitizer job from standalone sanitizer.yaml into test.yaml as a label-gated job (ci:sanitizer), preserving same config - Add sanitizer to pr-checks needs with allowed-skips so it gates the PR when present but doesn't block when absent - Delete standalone sanitizer.yaml - Remove redundant step-level conditions in coverage.yaml (workflow only triggers on pull_request, job already checks label and fork) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…workspace code - Add Codecov Report, HTML artifact, and Workflow Run links to PR comment - Enable Codecov PR comments via codecov.yml comment section - Skip non-workspace Rust files from coverage table (removes "other" row) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…cro failures Global RUSTFLAGS with -Zsanitizer=address breaks proc-macro crate builds (e.g. data-encoding-macro) because proc-macros are dynamic libraries. Use CARGO_TARGET_<triple>_RUSTFLAGS to scope sanitizer flags to the target only, keeping host proc-macro compilation unaffected. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…rectly Without --target, CARGO_TARGET_<triple>_RUSTFLAGS is not applied to `cargo build`, so -Zsanitizer=address is missing and ASan-instrumented LLVM libs cause undefined __asan_* symbols at link time. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The stable Rust toolchain doesn't ship the ASan runtime library. When LLVM is built with --sanitizer Address, its .a files contain __asan_* symbol references that need the ASan runtime at link time. Fix by setting CARGO_TARGET_*_LINKER=clang for sanitizer builds. Clang knows where its compiler-rt ASan runtime lives and links it automatically when given -fsanitize=address. This resolves the undefined __asan_* symbols without using global RUSTFLAGS (which would break proc-macro builds). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The -fuse-ld=lld flag added for standard Linux builds was being inherited into CARGO_TARGET_*_RUSTFLAGS for sanitizer builds. This caused clang to delegate to lld, which cannot find the ASan runtime libraries, resulting in thousands of undefined symbol errors from LLVM archives. Skip the lld flag when a sanitizer is configured so clang uses its native linker and provides the ASan runtime. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add an explicit guard that fails with a clear error if sanitizer is configured without a target, which would produce invalid env var names like CARGO_TARGET__RUSTFLAGS. Addresses Copilot review feedback. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
-Z sanitizer=address already links Rust's bundled ASan runtime (librustc-stable_rt.asan.a). Passing -C link-arg=-fsanitize=address additionally caused clang to inject libclang_rt.asan, producing hundreds of duplicate __ubsan_handle_* symbol errors. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Sanitizer job builds with --target, placing the binary at target/<triple>/debug/solx instead of target/debug/solx. Override CARGO_BIN_EXE_SOLX via GITHUB_ENV so test_manually finds the binary. Coverage job relied on $(which solx) which fails in container context. Add binary-path output to build-rust action and reference it explicitly in coverage.yaml, replacing all $(which solx) occurrences. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
hedgar2017
force-pushed
the
ci/coverage-pr-comments
branch
from
c4f9f5d to
4c0058a
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 7 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
… failures GITHUB_ENV persisted the debug-target binary path across all steps, including the release step where the binary doesn't exist yet. Move the override to an inline export in the debug step only. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 7 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Use inputs.target instead of ${TARGET} env var in both build-rust
and rust-unit-tests actions for self-contained sanitizer builds
- Add .exe suffix to binary-path output on Windows
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 7 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
cargo llvm-cov cleans workspace packages, deleting the instrumented solx binary that the coverage report steps need. Copy it to a stable path before running unit tests. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 7 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Move pull-requests: write from workflow-level to coverage job only - Disable Codecov PR comment to avoid duplicates with custom summary Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 7 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
1 participant
Summary
pull_requesteventsllvm-cov reportmshick/add-pr-commentTest plan
🤖 Generated with Claude Code