test: add coverage tooling and de-duplicate the suite's helpers and cases - #86
Merged
Conversation
…ases Adds source-based coverage (cargo-llvm-cov) and does the first pass of a curation sweep over the test suite. Coverage - `just coverage` / `coverage-html` / `coverage-lcov` measure b2-core, mirroring what `just test` runs, so the number is as fast as the suite and pulls in no ML deps. `just coverage-all` adds the CLI adapter (its tests spawn the instrumented `b2` binary, so process-level runs count); that one is heavier on a cold cache because b2-cli depends on b2-embed. - Real-model paths stay out on purpose: they are exercised by `just eval`, not by `cargo test`, so instrumenting them would report a permanent, meaningless 0%. Shared helpers `tests/common/mod.rs` held three items while nine files carried their own byte-identical `reindexed()`, six their own `count()`, four their own `ingest_golden()`, three their own `index_conn()`, and `ingest_resolver.rs` re-declared `FixedId`/`copy_dir`/`golden_vault_copy` outright. All now live in `common`. The tracing `Capture` writer is deliberately left duplicated in the two files that need it — hoisting it would make ~28 test binaries link tracing-subscriber to serve two, and those two are documented as needing their own binary anyway. Duplicate cases folded into their canonical home - `tests/links.rs` deleted: it re-tested `src/link.rs`, which has its own unit tests. Its four genuinely-unique cases (the lowercase verb-lookalike hazard, the document-order pair, `alias` vs `caption`, the `:` separator + tail verb) moved in beside them. - `vault.rs` now asserts the unknown-ref refusal once across read/neighbors/ explain/similar, and index-first honesty once across search/list_notes; the per-op singles in `read.rs`, `explain.rs` and `list.rs` are gone. - Stamping is `stamp.rs`'s subject alone; the partial restatements in `vault.rs` and `ingest_resolver.rs` are gone. - `graph.rs`'s two neighbour tests were one edge set viewed from each end; merged. - `roundtrip.rs` drops the no-frontmatter case, which `props.rs`'s 512-case round-trip property already covers, and says so. 231 tests, from 245. No behaviour is newly untested; `cargo test -p b2-core` green. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GjVA1fAW4ncDhUJ5cGHezt
…behind #[ignore]
Second curation pass. The first one removed duplication; this one fixes tests that
claimed more than they checked and adds the ones the coverage report showed missing.
Gaps closed (b2-core lines 91.13% → 92.06%, regions 87.78% → 88.86%, functions
94.27% → 95.52%)
- `relation.rs` had **no tests at all** and `is_symmetric` was never executed —
`contradicts` being its own inverse is the one non-trivial fact in the relation
vocabulary, and nothing asserted it. Now 76.92% → 100%, covering the closed core's
membership, symmetry, and the tail-verb fallbacks for both symmetry and labels.
- `vault::query_snippet` was entirely uncovered: every fixture chunk is shorter than
the 160-char snippet budget, so the windowing branch — the thing that makes a
section-sized chunk show the matched term rather than its head — never ran. Its own
comment flags a Unicode clamp hazard. `tests/search.rs` now exercises both the
window and the head fallback.
- `search` failing fast on a model swap is documented in CLAUDE.md as load-bearing
("rather than returning silently-wrong results") and had no test. `tests/embed.rs`
now covers the swap → refusal → reindex-heals cycle, including that `open` leaves
the stored vectors alone.
- `graph_filtered_search` had the "no silent cap above the reachable set" half
pinned but not that `limit` truncates below it; added beside it.
Tests that overclaimed
- `chunks.rs::weights_are_a_lever_a_giant_heading_weight_pulls_the_cut` applied no
giant weight — it used `BreakWeights::default()`, so the `weights` lever was never
tested. It now compares default against flattened heading weights and asserts the
cuts actually move, which is what the name promised.
- `ui`'s `npm test` hand-enumerated ten files, so a new `*.test.ts` was silently
skipped until someone remembered package.json. Now node's own runner over a glob.
#[ignore]
`b2-embed/tests/batch.rs` was the repo's only `#[ignore]`, against CLAUDE.md's own
rule, and so had effectively never run. Batched-vs-single embedding equivalence is a
correctness gate for every stored vector, so it moves into the eval harness — where
real-model work belongs, and where `just eval` actually runs it on every invocation.
The convention now says so explicitly.
`cargo test -p b2-core` 236 green, `-p b2-cli` 41 green, `npm test` 10 green,
fmt/clippy clean (the two remaining clippy warnings predate this branch).
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01GjVA1fAW4ncDhUJ5cGHezt
`just coverage-app` mirrors `just check-app`: builds ui/dist first, then covers b2-desktop's own unit tests. Kept out of the other coverage recipes for the same reason clippy keeps it out of the fast gate — it needs the platform webview toolchain. A low number there is expected by design; the host is a dumb adapter and its behaviour is covered by the façade suite. Unverified in this environment: b2-desktop needs GTK system libraries that the container lacks, so the recipe is written to match check-app's working pattern rather than confirmed by a run. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GjVA1fAW4ncDhUJ5cGHezt
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe PR centralizes core integration-test fixtures, adds coverage for parsing, relations, chunking, search, graph, and embedding contracts, introduces an eval-time batch embedding gate, and adds repository coverage workflows plus frontend test discovery updates. ChangesCore validation
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant EvalRun
participant LocalEmbedder
participant BatchCheck
EvalRun->>LocalEmbedder: load model
EvalRun->>BatchCheck: validate batch embeddings
BatchCheck->>LocalEmbedder: embed batch texts
BatchCheck->>LocalEmbedder: embed texts individually
BatchCheck-->>EvalRun: pass or return validation error
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/b2-embed/examples/eval.rs`:
- Around line 395-402: Update the cosine validation in the batched-versus-single
embedding check to reject non-finite values before evaluating the 0.9999
threshold. Ensure NaN and infinities return the existing descriptive error path,
while finite cosine results retain the current threshold behavior.
In `@justfile`:
- Around line 60-61: Remove the unsupported --summary-only argument from the
cargo llvm-cov commands in the coverage, coverage-all, and coverage-app targets,
leaving their existing package and coverage options unchanged so the default
summary output is generated successfully.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: efd6cb59-12b9-489c-8e8b-f9af28da55b1
⛔ Files ignored due to path filters (1)
ui/ui/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (29)
.gitignoreCLAUDE.mdcrates/b2-core/src/link.rscrates/b2-core/src/relation.rscrates/b2-core/tests/add.rscrates/b2-core/tests/cancel.rscrates/b2-core/tests/chunks.rscrates/b2-core/tests/common/mod.rscrates/b2-core/tests/dirs.rscrates/b2-core/tests/discover.rscrates/b2-core/tests/embed.rscrates/b2-core/tests/explain.rscrates/b2-core/tests/graph.rscrates/b2-core/tests/ingest_resolver.rscrates/b2-core/tests/links.rscrates/b2-core/tests/list.rscrates/b2-core/tests/mv.rscrates/b2-core/tests/project_embed.rscrates/b2-core/tests/read.rscrates/b2-core/tests/rm_delete.rscrates/b2-core/tests/roundtrip.rscrates/b2-core/tests/search.rscrates/b2-core/tests/vault.rscrates/b2-core/tests/write.rscrates/b2-core/tests/write_frontmatter.rscrates/b2-embed/examples/eval.rscrates/b2-embed/tests/batch.rsjustfileui/package.json
💤 Files with no reviewable changes (2)
- crates/b2-embed/tests/batch.rs
- crates/b2-core/tests/links.rs
…(CodeRabbit) Two review findings on #86. `check_batch_matches_single` gated on `cos <= 0.9999`, but every comparison against a NaN is false — so a NaN cosine would pass the gate silently, which is the one failure mode a correctness check must not have. Non-finite is now rejected first, with its own message. The coverage recipes passed `--summary-only`, which cargo-llvm-cov documents as valid only alongside --json/--lcov/--cobertura. It is dropped, though not for the reason reported: 0.8.7 accepts and ignores it in text mode, so the recipes ran fine (they produced the numbers in the PR description). The flag simply buys nothing — the default text report already is the per-file summary — and would break if a later release starts enforcing the documented constraint. Noted in the justfile so it does not come back. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GjVA1fAW4ncDhUJ5cGHezt
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adds source-based coverage (cargo-llvm-cov) and does the first pass of a
curation sweep over the test suite.
Coverage
just coverage/coverage-html/coverage-lcovmeasure b2-core, mirroringwhat
just testruns, so the number is as fast as the suite and pulls in no MLdeps.
just coverage-alladds the CLI adapter (its tests spawn the instrumentedb2binary, so process-level runs count); that one is heavier on a cold cachebecause b2-cli depends on b2-embed.
just eval, not bycargo test, so instrumenting them would report a permanent, meaningless 0%.Shared helpers
tests/common/mod.rsheld three items while nine files carried their ownbyte-identical
reindexed(), six their owncount(), four their owningest_golden(), three their ownindex_conn(), andingest_resolver.rsre-declared
FixedId/copy_dir/golden_vault_copyoutright. All now live incommon. The tracingCapturewriter is deliberately left duplicated in the twofiles that need it — hoisting it would make ~28 test binaries link
tracing-subscriber to serve two, and those two are documented as needing their own
binary anyway.
Duplicate cases folded into their canonical home
tests/links.rsdeleted: it re-testedsrc/link.rs, which has its own unittests. Its four genuinely-unique cases (the lowercase verb-lookalike hazard, the
document-order pair,
aliasvscaption, the:separator + tail verb) movedin beside them.
vault.rsnow asserts the unknown-ref refusal once across read/neighbors/explain/similar, and index-first honesty once across search/list_notes; the
per-op singles in
read.rs,explain.rsandlist.rsare gone.stamp.rs's subject alone; the partial restatements invault.rsand
ingest_resolver.rsare gone.graph.rs's two neighbour tests were one edge set viewed from each end; merged.roundtrip.rsdrops the no-frontmatter case, whichprops.rs's 512-caseround-trip property already covers, and says so.
231 tests, from 245. No behaviour is newly untested;
cargo test -p b2-coregreen.Co-Authored-By: Claude Opus 5 noreply@anthropic.com
Claude-Session: https://claude.ai/code/session_01GjVA1fAW4ncDhUJ5cGHezt
Summary by CodeRabbit
Bug Fixes
:is accepted as an explanation separator.Tests
Documentation
cargo llvm-cov/justcoverage workflows; updated testing conventions; cleaned up coverage artifacts ignore rules.