Skip to content

Conversation

@saulshanabrook
Copy link
Member

This PR adds support for running the tests locally to produce code coverage and in CI uploading to Codecov.

This should allow us to see for each PR how it affects the overall code coverage and what added lines are covered by tests/examples. I have been finding myself wanting this, as I develop new features to make sure I covered all the cases with examples.

I don't suggest we require 100% coverage for new lines added. However, even just seeing the lines can help you understand if your tests are covering what you think they are and possibly encourage covering more lines.

It can also help see what parts of our code base are not used in any examples.

@saulshanabrook saulshanabrook requested a review from a team as a code owner January 19, 2026 01:44
Copilot AI review requested due to automatic review settings January 19, 2026 01:44
Copy link
Contributor

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

This PR adds code coverage support to the egglog project by integrating cargo-llvm-cov for local coverage generation and Codecov for CI-based coverage reporting and tracking.

Changes:

  • Added Codecov badge to README and comprehensive documentation for generating coverage reports locally
  • Created new coverage and doctest Makefile targets to support coverage generation
  • Integrated coverage reporting into CI workflow with automatic uploads to Codecov

Reviewed changes

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

File Description
README.md Added Codecov badge, new Code Coverage section with local setup instructions, reorganized Parallelism section, and improved consistency by capitalizing "CodSpeed"
Makefile Added coverage target using cargo-llvm-cov, extracted doctest as separate target, updated .PHONY declarations
.gitignore Added lcov.info to ignore list for generated coverage reports
.github/workflows/build.yml Updated test job to install cargo-llvm-cov, run coverage generation, and upload results to Codecov

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@codspeed-hq
Copy link

codspeed-hq bot commented Jan 19, 2026

CodSpeed Performance Report

Merging this PR will not alter performance

Comparing saulshanabrook:code-coverage (c4a9e05) with main (f028092)1

Summary

✅ 22 untouched benchmarks
⏩ 190 skipped benchmarks2

Footnotes

  1. No successful run was found on main (57fdb7d) during the generation of this report, so f028092 was used instead as the comparison base. There might be some changes unrelated to this pull request in this report.

  2. 190 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports.

@codecov-commenter
Copy link

Welcome to Codecov 🎉

Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests.

Thanks for integrating Codecov - We've got you covered ☂️

saulshanabrook and others added 3 commits January 18, 2026 19:46
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
@saulshanabrook saulshanabrook added the status:needs decision Needs feedback from a project meeting to know whether to move forward label Jan 26, 2026
@oflatt
Copy link
Member

oflatt commented Jan 28, 2026

I'm in favor of coverage! We could do an 80% threshold or something like that

@saulshanabrook
Copy link
Member Author

@oflatt Do you mean a target of 80% coverage for adjusted lines? The default status I believe is "auto" which means that it will fail the pull request has less coverage than the main in terms of % of the total code.

Then there is the ability to set a "patch" status that will check only based on the % of coverage on the changed lines, instead of all lines.

I would say maybe merge this with defaults then see how it acts and tweak based on that?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

status:needs decision Needs feedback from a project meeting to know whether to move forward

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants