Skip to content

ci: publish repository coverage via codecov#19

Draft
Ramlaoui wants to merge 4 commits into
mainfrom
codex/docs-readme-coverage
Draft

ci: publish repository coverage via codecov#19
Ramlaoui wants to merge 4 commits into
mainfrom
codex/docs-readme-coverage

Conversation

@Ramlaoui
Copy link
Copy Markdown
Collaborator

@Ramlaoui Ramlaoui commented May 9, 2026

Summary

  • upload Python and Rust coverage reports to Codecov from CI
  • show the repository coverage badge at the top of README.md
  • remove the custom PR coverage summary script and README coverage section

Testing

  • validated .github/workflows/ci.yml parses
  • not run full project test suites locally

Notes

  • the README badge tracks the main branch coverage in Codecov
  • PR coverage updates come from Codecov uploads in the CI workflow

@Ramlaoui Ramlaoui changed the title docs: add README coverage guidance ci: automate PR coverage reporting May 9, 2026
@Ramlaoui Ramlaoui force-pushed the codex/docs-readme-coverage branch from 712bda5 to 773ab1d Compare May 9, 2026 13:08
@github-actions
Copy link
Copy Markdown

github-actions Bot commented May 9, 2026

Coverage Report

Scope Covered Lines Total Lines Line Coverage
Python 221 623 35.47%
Rust 1286 5200 24.73%

Artifacts:

  • Python XML/HTML coverage: atompack-py/coverage/
  • Rust LCOV coverage: coverage/rust.lcov

@Ramlaoui Ramlaoui changed the title ci: automate PR coverage reporting ci: publish repository coverage via codecov May 9, 2026
@Ramlaoui
Copy link
Copy Markdown
Collaborator Author

Ramlaoui commented May 9, 2026

This is blocked on Codecov onboarding, not on the coverage generation code itself.

Current status:

  • the workflow can generate both Python and Rust coverage reports
  • the repository coverage badge / PR reporting cannot be completed until LeMaterial/atompack is enabled in Codecov
  • that requires org-level GitHub App access or someone who already has Codecov org access to enable the repo and provide the upload token

Concretely, we cannot finish the Codecov path unless one of these happens:

  1. a LeMaterial org admin grants the Codecov GitHub App access to atompack
  2. the repo is already enabled in Codecov and someone provides the CODECOV_TOKEN
  3. we abandon Codecov and switch to a GitHub-only badge flow instead

Marking this PR as draft until that dependency is resolved.

@Ramlaoui Ramlaoui marked this pull request as draft May 9, 2026 18:08
@speckhard
Copy link
Copy Markdown
Contributor

Thanks for working on this! Progress looks great. Had claude take a look:

## General

Direction is good and the diff is mostly mechanical. A few real issues to flag before this leaves draft:

1. **`fail_ci_if_error: true` plus a soft skip step is contradictory.** When `CODECOV_TOKEN` is set, any transient Codecov hiccup (network, transient 5xx, rate limit) will hard fail CI. When it is not set, the soft-skip step prints a message but the artifact still uploads. On a forked PR (where secrets are typically masked) the token is empty, so this works there. But once the token is provisioned, you have made every CI run depend on Codecov uptime. Recommend `fail_ci_if_error: false`, at least until the workflow has burned in. Worth confirming with Victor / Alex what level of strictness they want.
2. **The two `if:` branches both run when `CODECOV_TOKEN` is unset.** That is fine, but the "skip" step prints a `echo`, which buries the fact that no coverage was uploaded behind a green check. If the secret is misconfigured in production, no one will notice. Consider emitting `::warning::` instead of plain `echo` so it surfaces in the workflow summary.
3. **The `coverage` job is gated by `needs: [rust, python]`.** If either fails, coverage never runs, which is fine. But this also means the coverage artifact will never be available for a debugging session on a PR where one suite is red. Consider whether the coverage job should be allowed to run independently (e.g. `needs: []` with its own checkout + setup) so a partial signal still gets uploaded. Not a blocker, more a workflow-ergonomics call.
4. **The Codecov badge in `README.md` points at the `main` branch.** If you ever rename the default branch or fork, this will silently 404 to a "unknown" badge. Worth a comment in the README near the badge linking the Codecov project, or moving the badge inside an HTML comment-protected block.
5. **No mention of why the previous "custom PR coverage summary script" was removed.** PR body says it is removed, but the diff does not show a deletion of any script. If the script lives elsewhere, link the file. If this PR is also supposed to delete it, the diff is incomplete.
6. **Question for Ali**: was this workflow run end-to-end in his fork with a real `CODECOV_TOKEN` so we know `make py-coverage` and `make rust-coverage` actually produce the files the upload step expects? The PR body says "validated `ci.yml` parses, not run full project test suites locally", which is not the same thing.

## Line-by-line

### `.github/workflows/ci.yml`

- **L7-8**: `permissions: contents: read` at the workflow level is correct. Good defensive default.
- **L60**: `if: github.event_name == 'pull_request' || github.ref == 'refs/heads/main'` excludes `push` events on branches. This is intentional, but it also means a developer who pushes a feature branch without opening a PR sees no coverage. That is the usual pattern, just confirm it matches what we want.
- **L65-66**: `permissions: contents: read` at the job level duplicates the workflow-level. Pick one.
- **L67-68**: `env: CODECOV_TOKEN: ${{ secrets.CODECOV_TOKEN }}` exposes the token to the entire job. The `codecov-action` reads it from the env, so this is fine. Consider scoping it to the upload step only with `env:` under that step, to reduce blast radius if any earlier step were to leak environment variables in logs.
- **L74-79**: pinning `setup-uv@v7` with `version: "0.9.16"` is correct and matches the `python` job. Good.
- **L80-84**: `actions-rust-lang/setup-rust-toolchain@v1` with `components: llvm-tools-preview` is correct for `cargo-llvm-cov`. Good.
- **L88-89**: `cargo install cargo-llvm-cov --locked` will run cold every CI run. The `Swatinem/rust-cache@v2` step caches build artifacts, not installed binaries. Consider `taiki-e/install-action@cargo-llvm-cov` which uses prebuilt binaries and shaves minutes off every run. This is the standard pattern.
- **L91**: `make py-coverage` runs before `make rust-coverage`. Both targets are independent (different language ecosystems), so this is sequential when it could be parallel matrix-style. Acceptable for v1.
- **L98-102**: `disable_search: true` + `files:` enumeration is the right pattern (forces explicit file list). Good.
- **L99**: the path `atompack-py/coverage/python-coverage.xml` is hard coded in two places (here and in the Makefile recipe). Worth a single env var, or at least a comment that they must move together.
- **L104-106**: the soft-skip step is reachable only if the token is empty. See general note 2.
- **L107-113**: artifact upload always runs (no `if`). On PRs from forks where coverage was skipped, you will still upload whatever empty/partial files the recipes produced. Consider `if: always()` if you really want it, or gate on success of the coverage steps.

### `Makefile`

- **L10-13**: the `.PHONY` list is updated to include the three new targets. Good.
- **L57**: `@command -v cargo-llvm-cov >/dev/null 2>&1 || (echo "..." && exit 1)` is the standard guard, but for CI we just installed it explicitly. Locally, developers will hit this error. Consider auto-installing in `rust-coverage` (with the `--locked` flag) so `make coverage` works on a clean checkout. Or document the install in the README near the new `make coverage` section.
- **L58**: `mkdir -p coverage` creates the dir at the project root. The `.gitignore` should be updated to ignore `coverage/` to prevent accidentally committing the lcov report. Same applies to `atompack-py/coverage/`.
- **L107-109** (`py-coverage`): inline command is long. Two specific concerns:
    - `--ignore=tests/benchmarks` is correct (benchmarks suite is opt-in). Good.
    - the `--with pytest-cov` flag adds the dep at runtime instead of through `[dev]`. That is fine for ephemeral CI but couples the recipe to `uv`'s `--with` semantics. Consider adding `pytest-cov` to the `dev` extras in `pyproject.toml` so this is reproducible without uv too.
- **L136**: the new top-level `coverage: rust-coverage py-coverage` aggregator is fine.

### `README.md`

- **L3-7**: badge HTML is centered with `<p align="center">`. Inconsistent with the rest of the README which uses bare markdown. Consider matching the existing style by using markdown `[![Codecov](...)](...)` on its own line.
- The PR body says "remove the README coverage section" but the diff only adds the badge, it does not delete any prior section. Confirm intent.

## Risks (per CLAUDE.md rule 3)

- If the `CODECOV_TOKEN` secret is misconfigured, CI on `main` will fail loudly (because of `fail_ci_if_error: true`) and block merges. Mitigation: ship with `fail_ci_if_error: false` for the first week.
- `cargo install cargo-llvm-cov --locked` on cold runners can take 90+ seconds. Mitigation: switch to prebuilt action above.
- The XML coverage path is duplicated. Mitigation: env var, or at minimum a comment.

## Suggested follow-up

- One commit changing `fail_ci_if_error` to `false` and adding a TODO to flip after burn-in.
- One commit using `taiki-e/install-action@cargo-llvm-cov`.
- Not sure if "remove custom PR coverage summary script" mention in the PR body still applies, or whether that landed in an earlier PR.

You can add me as a reviewer anytime

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