Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 18 additions & 0 deletions .claude/CLAUDE.md
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,24 @@ Server-managed collections use `new_collection`/`collection_more`/`collection_re

For enums, it also creates `<Enum><Variant>Generator` for each data variant. Implementation is split across `struct_gen.rs`, `enum_gen.rs`, and `utils.rs`.

### Coverage Annotations and Ratchet

**CRITICAL: You MUST NOT increase the numbers in `.github/coverage-ratchet.json` without first asking for and then receiving explicit human permission to do so.** This applies even if you believe the annotation is justified. The ratchet numbers can only decrease; any increase requires human approval. Do not work around this by restructuring code to avoid coverage instead of writing tests. Do NOT blindly add `// nocov` -- each annotation must be justified, and your first instinct should be to write a test instead.

This project enforces 100% code coverage. When code is genuinely untestable, use these annotations:

- `// nocov` at the end of a line -- excludes that line from line coverage.
- `// nocov start` / `// nocov end` on their own lines -- excludes a block from line coverage.

The coverage check script (`scripts/check-coverage.py`) automatically removes `// nocov` annotations from lines that turn out to be covered. The ratchet file tracks the total count of excluded lines and prevents the count from increasing.

The following patterns are **automatically excluded** and do NOT need `// nocov`:
- Structural syntax (closing braces, punctuation-only lines)
- `#[cfg(test)]` modules
- `todo!()` and `unreachable!()` calls
- Continuation lines inside multi-line `unreachable!()`/`todo!()` calls
- `#[ignore]`d test bodies

### Testing Conventions

- Place tests in `tests/` as integration tests, not as inline `#[cfg(test)] mod tests` in source files.
Expand Down
3 changes: 3 additions & 0 deletions .github/coverage-ratchet.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
{
"nocov": 564
}
42 changes: 39 additions & 3 deletions .github/workflows/ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -157,11 +157,13 @@ jobs:
name: coverage
runs-on: ubuntu-latest
permissions:
contents: read
contents: write # needed for ratchet auto-update
pull-requests: write # needed for creating PRs on main
steps:
- uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2
# persist-credentials needed for git push in the auto-update step below
- uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 # zizmor: ignore[artipacked]
Comment thread
DRMacIver marked this conversation as resolved.
with:
persist-credentials: false
ref: ${{ github.head_ref || github.ref }}

Comment thread
DRMacIver marked this conversation as resolved.
- name: Install Rust
run: rustup toolchain install stable --profile minimal -c llvm-tools-preview
Expand All @@ -177,6 +179,40 @@ jobs:
- name: Run coverage
run: just check-coverage

- name: Auto-update coverage annotations and ratchet
if: success()
env:
GH_TOKEN: ${{ github.token }}
Comment on lines 181 to +185
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'm confused why we need an auto-update job - shouldn't CI fail at PR-time if coverage-ratchet.json doesn't match the coverage job?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

It should, but we've seen enough things go out of sync when multiple merges happen that I wanted a fallback.

I'd be happy to remove this if we add "require branches to be up to date before merging" but that's a bit of a pain in the ass.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The code as written has confusing semantics to a reader. The intent of the person who wrote the code (you) was that we should be alerted if an edge case occurs that we don't expect to occur. A reasonable reader of the code instead reads this as "this is an integral part to the coverage ratchet workflow and can't be removed". There should be a comment here at least!

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yeah, that's fair. I'll add a comment in.

run: |
if git diff --quiet; then
echo "No coverage annotation changes needed"
exit 0
fi

git config user.name "github-actions[bot]"
git config user.email "github-actions[bot]@users.noreply.github.com"
git add -u
git add .github/coverage-ratchet.json

if git diff --cached --quiet; then
exit 0
fi

if [ "${{ github.event_name }}" = "pull_request" ]; then
git commit -m "Auto-update coverage annotations and ratchet"
git push
else
BRANCH="auto/coverage-ratchet-update"
git checkout -b "$BRANCH"
git commit -m "Auto-update coverage annotations and ratchet"
git push -f origin "$BRANCH"
gh pr create \
--title "Auto-update coverage ratchet" \
--body "The coverage ratchet has tightened on main. This PR updates the ratchet file and removes unnecessary annotations." \
--base main \
--head "$BRANCH" || true
fi
Comment thread
DRMacIver marked this conversation as resolved.

conformance:
name: conformance
runs-on: ubuntu-latest
Expand Down
7 changes: 5 additions & 2 deletions justfile
Original file line number Diff line number Diff line change
Expand Up @@ -38,11 +38,14 @@ check-docs:
docs:
cargo +nightly docs-rs --open

check-lint: check-format check-clippy
check-nocov-style:
scripts/check-nocov-style.py

check-lint: check-format check-clippy check-nocov-style

check-coverage:
# requires cargo-llvm-cov and llvm-tools-preview
RUST_BACKTRACE=1 cargo llvm-cov --all-features --fail-under-lines 30 --show-missing-lines
scripts/check-coverage.py
Comment thread
DRMacIver marked this conversation as resolved.

check-conformance:
cargo build --release --manifest-path tests/conformance/rust/Cargo.toml
Expand Down
Loading
Loading