Skip to content

feat(make): add coverage targets with LCOV for editor integration#3

Open
andybrown668 wants to merge 1 commit intomainfrom
andy/pr-coverage-make
Open

feat(make): add coverage targets with LCOV for editor integration#3
andybrown668 wants to merge 1 commit intomainfrom
andy/pr-coverage-make

Conversation

@andybrown668
Copy link

Type of Change

  • New Feature
  • Bug Fix
  • Documentation
  • Performance Improvement
  • Test/CI
  • Refactor
  • Other:

Related Issues

N/A

Summary of Changes

Add Make coverage targets with LCOV for editor integration.

  • Add .config/make/coverage.mak: coverage-unit, coverage-e2e, coverage-combined.
  • Emit LCOV only (lcov.info at repo root for unit/combined) for Cursor/VS Code.
  • Use BFD linker when available to avoid LLD duplicate-symbol under coverage.
  • Ignore lcov.info in .gitignore; update Makefile help and doc/development.md.

Checklist

  • I have read and followed the CONTRIBUTING.md guidelines
  • Passed make pre-commit
  • Added/updated necessary tests
  • Documentation updated (if needed)
  • CI/CD passed (if applicable)

Impact

  • Breaking change (compatibility)
  • Requires doc/config/deployment update
  • Other impact:

Additional Notes

Single-commit PR split from andy/documentation for easier review. Includes minimal doc/development.md addition for coverage targets (full doc flow is in other PRs).

Verification: make pre-commit and optionally make coverage-unit (requires cargo install cargo-llvm-cov and rustup component add llvm-tools-preview).


Thank you for your contribution! Please ensure your PR follows the community standards (CODE_OF_CONDUCT.md) and sign the CLA if this is your first contribution.

- Add .config/make/coverage.mak: coverage-unit, coverage-e2e, coverage-combined
- Emit LCOV only (lcov.info at repo root for unit/combined) for Cursor/VS Code
- Use BFD linker when available to avoid LLD duplicate-symbol under coverage
- Ignore lcov.info in .gitignore; update Makefile help and development.md

Made-with: Cursor
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds developer-facing coverage support via cargo-llvm-cov, producing LCOV reports intended for editor integrations (Cursor/VS Code), and documents the new workflow.

Changes:

  • Introduces make coverage-unit, make coverage-e2e, and make coverage-combined targets (LCOV output).
  • Updates Make help text and adds a development guide section documenting coverage usage.
  • Ignores generated lcov.info in .gitignore.

Reviewed changes

Copilot reviewed 3 out of 4 changed files in this pull request and generated 3 comments.

File Description
doc/development.md New development guide documenting build/test/quality and the new coverage targets.
Makefile Extends the help header to list documentation and coverage targets.
.gitignore Ignores generated lcov.info.
.config/make/coverage.mak Adds coverage targets and supporting variables/checks for LCOV generation.

Comment on lines +24 to +31
@cargo llvm-cov --version >/dev/null 2>&1 || (echo "Install: cargo install cargo-llvm-cov && rustup component add llvm-tools-preview"; exit 1)

.PHONY: coverage-deps
coverage-deps: ## Install cargo-llvm-cov and llvm-tools-preview if missing
@command -v cargo >/dev/null 2>&1 || (echo "cargo not found. Ensure Rust is installed and in PATH."; exit 1)
@cargo llvm-cov --version >/dev/null 2>&1 || \
(echo "Installing coverage tools..."; cargo install cargo-llvm-cov && rustup component add llvm-tools-preview)
@cargo llvm-cov --version >/dev/null 2>&1 || (echo "Install failed. Run: cargo install cargo-llvm-cov && rustup component add llvm-tools-preview"; exit 1)
Copy link

Copilot AI Mar 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

coverage-check/coverage-deps claim to verify/install both cargo-llvm-cov and llvm-tools-preview, but the logic only checks whether cargo llvm-cov exists. If a developer already has cargo-llvm-cov installed but is missing llvm-tools-preview (or rustup itself), these targets can still pass the dependency step and then fail later during cargo llvm-cov test/report. Update the checks to also verify the LLVM tools are available (e.g., via rustup which llvm-profdata/llvm-cov) and make rustup component add llvm-tools-preview run when the component is missing, regardless of whether cargo-llvm-cov is already installed.

Suggested change
@cargo llvm-cov --version >/dev/null 2>&1 || (echo "Install: cargo install cargo-llvm-cov && rustup component add llvm-tools-preview"; exit 1)
.PHONY: coverage-deps
coverage-deps: ## Install cargo-llvm-cov and llvm-tools-preview if missing
@command -v cargo >/dev/null 2>&1 || (echo "cargo not found. Ensure Rust is installed and in PATH."; exit 1)
@cargo llvm-cov --version >/dev/null 2>&1 || \
(echo "Installing coverage tools..."; cargo install cargo-llvm-cov && rustup component add llvm-tools-preview)
@cargo llvm-cov --version >/dev/null 2>&1 || (echo "Install failed. Run: cargo install cargo-llvm-cov && rustup component add llvm-tools-preview"; exit 1)
@command -v cargo >/dev/null 2>&1 || (echo "cargo not found. Ensure Rust is installed and in PATH."; exit 1)
@command -v rustup >/dev/null 2>&1 || (echo "rustup not found. Install Rust via https://rustup.rs/"; exit 1)
@cargo llvm-cov --version >/dev/null 2>&1 || (echo "cargo-llvm-cov not found. Install: cargo install cargo-llvm-cov"; exit 1)
@rustup which llvm-profdata >/dev/null 2>&1 || (echo "llvm-tools-preview (llvm-profdata) not found. Run: rustup component add llvm-tools-preview"; exit 1)
@rustup which llvm-cov >/dev/null 2>&1 || (echo "llvm-tools-preview (llvm-cov) not found. Run: rustup component add llvm-tools-preview"; exit 1)
.PHONY: coverage-deps
coverage-deps: ## Install cargo-llvm-cov and llvm-tools-preview if missing
@command -v cargo >/dev/null 2>&1 || (echo "cargo not found. Ensure Rust is installed and in PATH."; exit 1)
@command -v rustup >/dev/null 2>&1 || (echo "rustup not found. Install Rust via https://rustup.rs/"; exit 1)
@cargo llvm-cov --version >/dev/null 2>&1 || \
(echo "Installing cargo-llvm-cov..."; cargo install cargo-llvm-cov)
@rustup which llvm-profdata >/dev/null 2>&1 || \
(echo "Installing llvm-tools-preview via rustup..."; rustup component add llvm-tools-preview)
@cargo llvm-cov --version >/dev/null 2>&1 && \
rustup which llvm-profdata >/dev/null 2>&1 && \
rustup which llvm-cov >/dev/null 2>&1 || \
(echo "Install failed. Run: cargo install cargo-llvm-cov && rustup component add llvm-tools-preview"; exit 1)

Copilot uses AI. Check for mistakes.
Comment on lines +36 to +39
export RUSTFLAGS="$(COVERAGE_RUSTFLAGS)" && \
cargo llvm-cov test --workspace --exclude e2e_test --no-fail-fast --no-report
export RUSTFLAGS="$(COVERAGE_RUSTFLAGS)" && \
cargo llvm-cov report --lcov --output-path "$(COVERAGE_LCOV_PATH)"
Copy link

Copilot AI Mar 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The coverage targets overwrite RUSTFLAGS with COVERAGE_RUSTFLAGS, which drops any user-provided RUSTFLAGS (e.g., for target-specific linking or feature cfgs). Consider appending to the existing RUSTFLAGS (or only setting it when it’s empty) so coverage runs don’t unexpectedly change a developer’s environment.

Copilot uses AI. Check for mistakes.
Comment on lines +51 to +52
- `make clippy` — Clippy
- `make check` — Compilation check
Copy link

Copilot AI Mar 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doc lists make clippy and make check, but the Make targets in .config/make/ are named clippy-check and compilation-check (and there is no check/clippy alias). Update these target names (or add aliases in the Makefiles) so the documentation matches what actually works.

Suggested change
- `make clippy` — Clippy
- `make check` — Compilation check
- `make clippy-check` — Clippy
- `make compilation-check` — Compilation check

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants