test(coverage): gate all six shipping packages at 100% line and branch - #321
Merged
Conversation
Coverlet's assembly filter is exact-match, so `[Celerity]*` compiled to
^Celerity$ and matched only the Celerity.Collections assembly. Since the
2.0.0 package split, Celerity.Hashing, Celerity.Primitives and the three
showcase packages were outside the gate entirely — any of them could have
dropped to 0% with CI green.
Widen the filter to all six, run the three showcase test projects into the
same report, and backfill everything the wider scope exposed. Coverage goes
from 98.67% line / 97.20% branch (on the one measured assembly) to
100% / 100% across all six: 8902/8902 lines and 3514/3514 branches. The
floor is raised from 95/90 to 100/100 to match.
Six guards no test can reach are hoisted into small private helpers carrying
[ExcludeFromCodeCoverage] with a justification that states the proof:
- Deque / IndexedPriorityQueue growth clamps 2^30-element arrays
- Frozen{Dictionary,Set} count ceilings 2^30 materialized keys
- Cuckoo/Xor sizing floors dead given the ctor's own
argument validation
- XorFilter peel-retry exhaustion the seed schedule is
key-independent, so no
hasher can stall it
The Deque clamp additionally gets a real [MemoryIntensiveFact(3100)] test
that allocates ~3 GiB to prove it saturates rather than wrapping negative;
it is excluded so the gate does not depend on the runner's headroom.
XorFilter.TryBuild is split into a retry driver and TryPeel. Only the driver
is unreachable — an individual peel attempt does stall and reseed in
practice, and that path stays measured.
coverage_report.py now accepts a repeatable --input and merges reports on
(source file, line number), since four test projects contribute and the
showcase projects also exercise Celerity.Collections transitively.
Closes #314
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR expands and hardens the repository’s coverage infrastructure so CI enforces 100% line and 100% branch coverage across all six shipping packages, fixing the prior gap where most shipped assemblies were silently unmeasured due to coverlet’s exact-match assembly filter behavior. It also backfills missing edge-case tests and introduces a Cobertura-merge flow so multiple test projects contribute to one unified gate and report.
Changes:
- Widen coverage scope to all six shipping assemblies and raise the CI floor to 100/100, including merging multiple Cobertura inputs into a single report.
- Add extensive boundary/contract tests to close previously uncovered branches across collections, primitives, and showcase packages.
- Refactor a few “unreachable guard” paths into
[ExcludeFromCodeCoverage(Justification=...)]helpers to keep the 100% gate meaningful without lowering thresholds.
Reviewed changes
Copilot reviewed 29 out of 29 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| src/coverage.runsettings | Updates coverlet include/exclude settings to target all shipping assemblies and document filter behavior. |
| src/Celerity/Collections/XorFilter.cs | Splits construction into driver/peel attempt helpers and introduces exclusions for provably unreachable guards. |
| src/Celerity/Collections/IndexedPriorityQueue.cs | Factors growth-clamp logic into an excluded helper and keeps growth semantics intact. |
| src/Celerity/Collections/FrozenCeleritySet.cs | Moves an unreachable element-count guard into an excluded helper with justification. |
| src/Celerity/Collections/FrozenCelerityDictionary.cs | Moves an unreachable key-count guard into an excluded helper with justification. |
| src/Celerity/Collections/Deque.cs | Factors Array.MaxLength clamp into an excluded helper and adds related test coverage. |
| src/Celerity/Collections/CuckooFilter.cs | Factors defensive floors into an excluded helper and reuses it for sizing expressions. |
| src/Celerity.Tests/Utils/VarIntBoundaryCoverageTests.cs | Adds tests for VarInt mid-encode exhaustion and signed-wrapper failure arms. |
| src/Celerity.Tests/Primitives/RandomSourceRejectionSamplingTests.cs | Adds deterministic tests for Lemire rejection-sampling retry loops. |
| src/Celerity.Tests/Primitives/GuidV7GeneratorCoverageTests.cs | Adds tests for GuidV7Generator’s wall-clock Next() path and its monotonicity contract. |
| src/Celerity.Tests/Collections/SketchDegenerateParameterTests.cs | Adds tests for probabilistic sketch clamps/saturation corners and formatting. |
| src/Celerity.Tests/Collections/SetSourceCountBranchTests.cs | Tests both sides of the (source as ICollection<T>)?.Count ?? 0 sizing hint branch across sets. |
| src/Celerity.Tests/Collections/SetExplicitICollectionMemberTests.cs | Pins explicit ICollection<T> behavior (silent Add on duplicates, IsReadOnly false). |
| src/Celerity.Tests/Collections/OversizedSourceAndResidualGuardTests.cs | Adds tests for FenwickTree counted-source ceiling and Trie enumerator “sticky exhausted” behavior. |
| src/Celerity.Tests/Collections/FilterEdgeCaseCoverageTests.cs | Adds tests for Cuckoo victim-cache paths and Xor construction reseed behavior. |
| src/Celerity.Tests/Collections/EnumSetUnderlyingTypeCoverageTests.cs | Adds coverage for EnumSet conversion arms across underlying enum widths and bounds. |
| src/Celerity.Tests/Collections/EnumMapUnderlyingTypeCoverageTests.cs | Adds coverage for EnumMap key/index conversions and strongly-typed Keys/Values enumerators. |
| src/Celerity.Tests/Collections/EnumeratorInvalidationAndClearCoverageTests.cs | Adds coverage for Reset() invalidation paths and Clear/TrimExcess early-outs + reference clearing. |
| src/Celerity.Tests/Collections/DequeGrowthTests.cs | Adds memory-intensive test asserting growth saturates at Array.MaxLength. |
| src/Celerity.Tests/Collections/CapacityArgumentValidationTests.cs | Adds comprehensive EnsureCapacity/TrimExcess guard tests and fills LongDictionary.TrimExcess coverage. |
| src/Celerity.Sentinel.Tests/SentinelCoverageGapTests.cs | Adds coverage for Sentinel first-seen filter optional paths, snapshot guards, and formatting. |
| src/Celerity.Ring.Tests/RingCoverageGapTests.cs | Adds coverage for ring/pool corner cases: replicas, wraparound, TryGetNode success paths, last-node removal. |
| src/Celerity.Cardinality.Tests/CardinalityCoverageGapTests.cs | Adds coverage for Cardinality surface metadata, saturation handling, mode transitions, and Clear behavior. |
| scripts/coverage_report.py | Makes --input repeatable and merges multiple Cobertura reports at (file,line) granularity. |
| docs/testing.md | Updates documentation to reflect six-assembly gating and multi-project report merging. |
| CONTRIBUTING.md | Updates contributor guidance to reflect the 100/100 gate and the new-package checklist item. |
| CLAUDE.md | Updates agent guidance to reflect the 100/100 gate and exact-match filter behavior. |
| CHANGELOG.md | Adds an Unreleased “Fixed” entry describing the coverage-gate scope correction. |
| .github/workflows/coverage.yml | Installs required SDKs, runs core + showcase test projects, merges reports, and enforces 100/100. |
- docs/testing.md said "all three shipping assemblies" in the TL;DR — a leftover from before the showcase packages joined the gate. - coverage.runsettings / coverage.yml headers credited ReportGenerator; the repo dropped that dependency in favour of scripts/coverage_report.py. - The <Include> comment said "one entry each" while the config uses a single comma-separated list. Corrected, and documented WHY it must stay that way: the collector reads one child node per key, so splitting it into separate <Include> elements silently honours only the first. Verified against this suite — three separate elements produced a report containing only the Celerity package, i.e. exactly the bug #314 was about. - Trimmed the CHANGELOG entry to the repo's stated convention. - The GuidV7 wall-clock test assumed a monotonic clock; an NTP step between the two straddling readings would invert the range and fail a correct generator. Bounds are now ordered. - Added a note to the local coverage instructions to clear stale results first: the four reports merge by source path, SourceLink derives that path from the build's git state, and mixing commits double-counts every file. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…verage-6896fc # Conflicts: # CHANGELOG.md
… work BTreeDictionary/BTreeSet (#305) and IHashProvider64 (#318) landed on main while this was open, taking the merged result to 99.95% line / 99.82% branch. Backfills the five gaps they left: - RemoveFromInternal promotes a deleted internal key's in-order predecessor or successor by walking down to the flanking leaf. In a two-level tree that child IS a leaf, so the descent loop body never ran. With MinDegree 16 a third level needs ~1000 entries, so the new tests build 5000 and delete every one against a SortedSet/SortedDictionary oracle — a descent that stopped a level early would promote the wrong key while still leaving the right count, so the reconciliation is order-sensitive. - SeekLowerBound's `while (node is not null)` only fails on a null root, i.e. EnumerateRange over an empty tree. - ICollection<KVP>.Contains matches on key AND value; only the fully-matching case was exercised, leaving the short-circuit arm untaken. Note the results are bound to locals first: Assert.Contains(pair, collection) walks the enumerator and never calls the method under test. - Hash64Source's `IsNative64 ? … : null` initializer: Native is read only by Hash64, which callers invoke only when IsNative64 is true, so a 32-bit-only THasher never triggers it and the false arm is unobservable. Hoisted into CreateNative() carrying the exclusion and the reasoning. Also addresses the second Copilot review: find_inputs' docstring promised an ordering across all globs that the implementation does not provide (it preserves pattern order, sorting within each). The order is presentational only, since parse() merges — docstring corrected to say so. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Coverage
|
…unds Ordering the bounds only covered a backward clock step BEFORE Next(). A step after it can leave the embedded reading above both samples: if the clock ticks a millisecond between `before` and Next(), then steps back before `after`, the timestamp exceeds max and a correct generator fails. A second of slack closes that without weakening what the test is for. It exists to prove Next() reads the wall clock in the right epoch and unit, and those failures are not close calls — a .NET-ticks or 1900 epoch is wrong by decades, seconds-for-milliseconds by a factor of a thousand. Millisecond exactness here only bought the ability to fail on a clock adjustment. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The table is presented as the complete set of source-level exclusions, but CreateNative was added during the post-merge backfill and never listed. Adds the row and states the completeness claim explicitly, with the grep that checks it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Benchmarks11 regressions Highlights
Collections (460)
Hashers (111)
Same-runner A/B (sharded 8-way): main ( |
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
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.
What
Takes the suite to 100% line and 100% branch coverage across all six shipping packages, and raises the CI floor to match.
Along the way this fixes #314: coverlet's assembly filter is exact-match, so
[Celerity]*compiled to^Celerity$and matched only theCelerity.Collectionsassembly. Since the 2.0.0 package split,Celerity.Hashing,Celerity.Primitivesand the three showcase packages had been outside the gate entirely — any of them could have dropped to 0% with CI green.Tests: 4420 → 5013.
What was uncovered
Nothing exotic, which is the point — it was the code no one writes a test for:
EnsureCapacity(-1)/TrimExcess(< Count)argument guards on 16 collection typesICollection<T>.Add/IsReadOnlyon 8 sets (these delegate toTryAdd, so unlike the publicAddthey must be silent on a duplicate — that contract was untested)Clear()on empty / on reference-type instantiations(source as ICollection<T>)?.Count ?? 0in the set constructorsEnumMap/EnumSetbyte-, ushort- and ulong-backed enums — only the defaultintenum had ever been exercised, leaving theUnsafe.SizeOf<TEnum>()conversion arms deadLongDictionary.TrimExcess— zero hits, the whole methodIRandomSource);GuidV7Generator.Next()HyperLogLogprecision 5 and 6; the CuckooFilter victim slot removal pathFenwickTree's oversized-source rejection — reachable for free via anICollection<T>that lies aboutCount, because the guard runs before any allocationTrie.Enumerator's exhausted state is sticky — aforeachstops at the firstfalseand never tests itThe six exclusions
Everything reachable got a real test. Six guards are genuinely unreachable and are hoisted into small private helpers carrying
[ExcludeFromCodeCoverage(Justification = "…")]that states the proof:Deque.ClampToArrayMaxLength[MemoryIntensiveFact(3100)]test that allocates ~3 GiB and asserts growth saturates atArray.MaxLengthrather than wrapping negative — excluded so the gate doesn't depend on whether the runner had the headroom to run it.IndexedPriorityQueue.ClampGrowthEnsureCapacitycallsResizedirectly, so capacity can't be pre-inflated into it;_elementswould pass the 2 GiB single-object array limit.FrozenCelerityDictionary/FrozenCeleritySetcount ceilingsList<string>, so 2³⁰ keys needs an 8.6 GBstring[]. A source that merely reports a hugeICollection.Countcan't reach it — that count is only a capacity hint.CuckooFilter.AtLeastOne/XorFilter.AtLeastOnef = ceil(log2(8/p))withp ∈ (0,1)enforced givesf ≥ 4;blockLength ≥ 10for any source.XorFilter.BuildOrThrow/TryBuildMaxConstructionAttemptsseeds — a constant hasher makes peeling easier, since theHashSetcollapses every key to one.These are array-size impossibilities and arithmetic dead code, not "hard to test". Each stays in the source as defence-in-depth against a future change to the surrounding validation.
Notable refactor
XorFilter.TryBuildis split into a retry driver andTryPeel. Only the driver is unreachable — an individual peel attempt genuinely does stall and reseed, and that path stays measured. Without the split, excluding the loop-exhaustion branch would have meant excluding the entire peeling algorithm.Infrastructure
src/coverage.runsettingslists all six assemblies, with a comment explaining the exact-match filter so the next package split can't repeat this silently.coverage.ymlinstalls both SDKs (the showcase test projects are net10.0-only) and runs all four test projects.coverage_report.pytakes a repeatable--inputand merges reports on (source file, line number) — necessary because the showcase projects also exerciseCelerity.Collectionstransitively, so a line covered by any run counts as covered.CONTRIBUTING.mdandCLAUDE.mdadvertised the gate as library-wide, which wasn't true;docs/testing.mddocumented the narrowed scope without flagging it as a gap. All three corrected, plus a new-package checklist item.Merged with v2.4.0
BTreeDictionary/BTreeSet(#305) andIHashProvider64<T>(#318) landed while this was open, which dropped the merged result to 99.95% line / 99.82% branch. Five more gaps closed to restore 100%:RemoveFromInternal's descent to the flanking leaf (both trees, both directions). It replaces a deleted internal key with its in-order predecessor or successor, walking down withwhile (!cursor.IsLeaf). In a two-level tree that child is a leaf, so the loop body never ran.MinDegree == 16means a third level needs ~1000 entries, so the new tests build 5000 and delete every one against aSortedSet/SortedDictionaryoracle — a descent stopping one level early would promote the wrong key while leaving the right count, so the reconciliation is order-sensitive.SeekLowerBound'swhile (node is not null)— only fails on a null root, i.e.EnumerateRangeover an empty tree.ICollection<KVP>.Containsmatches on key and value; only the fully-matching case was exercised. (Worth noting for future tests:Assert.Contains(pair, collection)walks the enumerator and never calls the method under test — the results have to be bound to locals first.)Hash64Source'sIsNative64 ? … : null—Nativeis read only byHash64, which callers invoke only whenIsNative64is true, so a 32-bit-onlyTHashernever triggers the initializer and the false arm is unobservable. Hoisted intoCreateNative()with the exclusion and the reasoning.Verification
Verified in the exact CI configuration (core on net8.0, showcase on net10.0, merged): 5013 tests pass, gate exits 0 at 100/100. Also verified on net10.0 throughout. The
Dequememory test ran (not skipped) on a 64 GB dev box.One caveat worth stating: the four reports merge by source-file path, and SourceLink derives that path from the build's git state. CI builds all four in one job at one commit so the paths agree, but mixing reports across commits locally double-counts every file —
docs/testing.mdnow says to clear stale results first.Closes #314
🤖 Generated with Claude Code