Add coverage analysis review #001 (88.9% lines, 82.3% fns, 8 suggestions)

This commit is contained in:
2026-08-02 07:59:59 +00:00
parent 6d6142978a
commit 9fb85417b5

View File

@@ -0,0 +1,457 @@
---
status: open
last_updated: 2026-08-02
reviewed_artifacts:
- src/lib.rs
- src/error.rs
- src/macros.rs
- src/data_access.rs
- src/validation.rs
- src/schema.rs
- src/offset_map.rs
- src/layout_builder.rs
- src/sequential_reader.rs
- src/tunion.rs
- src/engine.rs
- tests/{engine_integration,error_paths,poc_roundtrip,tunion_dispatch}.rs
tool: cargo-llvm-cov
invocation: cargo llvm-cov --html --output-dir target/coverage
reviewer: coverage pass (post-rebrand sanity check)
---
# Coverage Analysis #001
## Purpose
First dedicated code-coverage pass for the alktype crate, run immediately
after the rebranding cleanup (commit `6d61429`). This pass has two goals:
1. Establish a coverage baseline before any new development begins, so
future passes can measure deltas.
2. Identify the weakly-covered areas that should be tightened up before
publishing — while the surface is still small and the cost of adding
tests is low.
Headline result: **coverage is in reasonable shape for a crate that was
built task-by-task with per-task tests.** Line coverage is **88.9%**
(4557/5125), function coverage is **82.3%** (433/526), all 285 tests pass,
and the build is clean. The gaps concentrate in three categories: (a)
defensive overflow guards that are structurally hard to reach, (b) an
asymmetry between `is_valid()` (heavily tested) and `validate()` (rarely
called) in the validation layer, and (c) the `error::Display` impl which
is never exercised. None of the gaps indicate a logic defect — the code
under test was reviewed during the rebranding pass and is consistent.
## Methodology
- `cargo llvm-cov --json --output-path /tmp/coverage.json` then per-file
line + function breakdown from the JSON export.
- `cargo llvm-cov --text` to extract the exact uncovered source lines per
file, mapped back to the source for attribution.
- Each gap classified by testability: trivial (pure function / Display
impl), easy (existing test patterns extend naturally), medium (new
error-path scaffolding), or hard (overflow guards needing
`usize::MAX`-adjacent inputs).
- No `--all-features` flag — the crate has no feature flags currently
(`[features] default = []` in `Cargo.toml`).
## Summary Statistics
| Severity | Count |
|----------|-------|
| Critical | 0 |
| Warning | 0 |
| Suggestion | 8 (S1S8) |
No correctness findings — this is a coverage pass, not a logic review.
The logic review was performed as part of the rebranding sanity check
(commit `6d61429` message documents the full sweep). Everything below is
a suggestion ordered by leverage, not severity.
---
## Per-File Coverage
| File | Lines | Covered | % | Fns | Covered | % |
|------|------:|--------:|--:|----:|--------:|--:|
| src/error.rs | 9 | 0 | 0.0% | 1 | 0 | 0.0% |
| src/macros.rs | 144 | 109 | 75.7% | 19 | 17 | 89.5% |
| src/data_access.rs | 459 | 388 | 84.5% | 62 | 48 | 77.4% |
| src/validation.rs | 433 | 368 | 85.0% | 51 | 45 | 88.2% |
| src/sequential_reader.rs | 1115 | 965 | 86.5% | 82 | 58 | 70.7% |
| src/layout_builder.rs | 1036 | 926 | 89.4% | 95 | 72 | 75.8% |
| src/schema.rs | 464 | 423 | 91.2% | 64 | 56 | 87.5% |
| src/offset_map.rs | 605 | 560 | 92.6% | 63 | 54 | 85.7% |
| src/tunion.rs | 423 | 395 | 93.4% | 49 | 45 | 91.8% |
| src/engine.rs | 437 | 423 | 96.8% | 40 | 38 | 95.0% |
| **TOTAL** | **5125** | **4557** | **88.9%** | **526** | **433** | **82.3%** |
`engine.rs` and `tunion.rs` are in good shape. The gaps concentrate in
`error.rs` (the `Display` impl), the macro-generated validation code
(`macros.rs`), and the error-path arms across the layout and reader
modules.
---
## Suggestions
### S1. `AlkTypeError::Display` impl is completely unexercised (error.rs, 0%)
**File**: `src/error.rs:3043`
**Problem**: The `Display` impl for `AlkTypeError` is never called by any
test. All four arms — `Schema`, `Offset`, `Access`, `Validation` — are
uncovered. This is the only file at 0% and the only place where a public
trait implementation has zero coverage.
The `Display` impl is public API surface: consumers format errors for
logging, error responses, and debugging. A typo or a `field_path`/`reason`
swap in the format strings would not be caught by any test today.
**Fix**: ~10 lines of unit tests. Construct one error of each variant and
assert `format!("{e}")` contains the expected substrings:
- `AlkTypeError::Schema("bad")``"schema error: bad"`
- `AlkTypeError::Offset { field_path: "f", reason: "r" }` → `"offset
error at f: r"`
- `AlkTypeError::Access { field_path: "f", reason: "r" }` → `"access
error at f: r"`
- `AlkTypeError::Validation(...)` → `"validation error: ..."` (use a
synthetic `ValidationError` or skip the substring check on the inner
message)
Add a `std::error::Error::source()` call to confirm it returns `None`
(the impl is the blanket `impl std::error::Error for AlkTypeError {}`).
**Lift**: 9 uncovered lines → 100% for `error.rs`. Trivial effort, no
new infrastructure.
---
### S2. `validate()` vs `is_valid()` asymmetry in the validation layer (validation.rs, macros.rs)
**Files**: `src/validation.rs` (the `Int64Validator`, `Uint64Validator`,
`StringValidator`, `BytesValidator`, `EnumValidator`, `TimestampValidator`
`validate()` methods); `src/macros.rs` (the macro-generated
`validate()` methods for `Int8/16/32`, `Uint8/16/32`, `Float32/64`,
`Struct/Union/Array/Record/Boolean`)
**Problem**: The tests heavily exercise `is_valid()` (the bool check)
but rarely call `validate()` (the `Result`-returning method). The
`AlkTypeEngine::validate_json` public API calls `validator.validate()`,
so the `Result` paths are the ones consumers actually hit when they want
error messages — but they're uncovered.
Specifically uncovered:
- `Int64Validator::validate` / `Uint64Validator::validate` — the `Ok`
and `Err` arms for both. `is_valid()` is tested; `validate()` is not.
- `StringValidator::validate` — the `maxLength` exceeded arm and the
non-string arm. The `Ok` arm is hit via `is_valid()` but not
`validate()`.
- `BytesValidator::validate` — same shape as StringValidator.
- `EnumValidator::validate` — the whole method (it is a no-op that
returns `Ok(())`, but it should be called at least once).
- `TimestampValidator::validate` — the `Ok` arm (valid RFC 3339) and the
`Err` arm (non-timestamp string).
- Macro-generated `validate()` for all numeric / type-check validators —
the `Ok` arm and the `Err` arm of each.
The factory error arms are also uncovered: `string_factory`,
`bytes_factory`, `enum_factory`, `timestamp_factory` each have a branch
that rejects a keyword value that isn't `true` (or `true`/object for
string/bytes). Only the `Ok` path of each factory is exercised.
**Fix**: ~40 lines of unit tests. For each validator, call
`validator.validate(&instance)` on a valid instance (assert `Ok(())`) and
an invalid instance (assert `Err`, check the error message substring).
For the factories, build a validator with a malformed keyword value
(e.g. `{"AlkType:String": 42}`, `{"AlkType:Enum": false}`) and assert
`build_validator` returns `Err(AlkTypeError::Schema(_))`.
**Lift**: ~65 uncovered lines in `validation.rs`, ~35 in `macros.rs`.
Brings both files above 95% line coverage. Easy — the test patterns
already exist for `is_valid()`, just call the `Result`-returning twin.
---
### S3. Engine `read_field` for String and Struct (engine.rs, ~12 lines)
**File**: `src/engine.rs:267282`
**Problem**: `AlkTypeEngine::read_field` in aligned mode is tested for
`Uint8`, `Uint32`, and `String` (the last only via a string-specific
test). The `AlkTypeKind::String` arm at line 267 and the
`AlkTypeKind::Struct` arm at line 279 are uncovered — the Struct arm
returns `FieldValue::Struct { start, end }` and is the path consumers
use to get a byte range for nested-struct recursion.
The two `AlkTypeError::Offset` error arms at lines 210216 (field schema
not found / no `AlkType:*` kind) are also uncovered. These fire when the
`OffsetMap` and the schema tree disagree about a field's existence — a
defensive path, but it's public API surface that should produce a
sensible error.
**Fix**: ~20 lines of tests, extending the existing
`read_field_aligned_*` tests in `src/engine.rs`:
- A test with a nested struct field, asserting `read_field` returns
`FieldValue::Struct { start, end }` matching the `OffsetMap` range.
- A test calling `read_field` on a field path that doesn't exist in the
schema tree (but does exist in the offset map by construction — or
skip this if the two are always in sync; the error arm is defensive).
- A `String` read via `read_field` (the existing
`read_field_aligned_reads_string_length_prefixed` test covers this —
verify it is actually hitting line 267 and not just the data_access
call).
**Lift**: 12 uncovered lines, lifts `engine.rs` from 96.8% to ~100%.
Easy.
---
### S4. `SequentialReader` error paths and uncommon reads (sequential_reader.rs, ~117 lines)
**File**: `src/sequential_reader.rs` (many locations — see below)
**Problem**: This is the largest gap area by line count. The uncovered
lines fall into four groups:
1. **Union error paths** (~30 lines): the `read_union_value` overflow
guards (`variant_start + disc_size overflows`, `after_disc +
variant_size overflows`, `union end ... overflows`), the missing-
properties and missing-discriminator-field schema errors, and the
unknown-discriminator-value access error for field-name unions. These
are defensive guards, but the "unknown discriminator value" path is a
real consumer-facing error that should produce a clear message.
2. **Array error paths** (~15 lines): the `array schema is not an
object`, `declared fixed count but minItems is absent`, `items schema
has no AlkType:* kind`, `array size × stride overflows`, and `array
end overflows` arms. The `items schema has no AlkType:* kind` arm is
a real validation error a consumer would hit with a malformed schema.
3. **Discriminator helper arms** (~10 lines): `read_byte_discriminator`
only exercises the `Uint8` arm; the `Uint16` and `Uint32` arms are
uncovered. `discriminator_string_value` only exercises the `String`
arm; `U8`/`U16`/`U32`/`Enum` are uncovered.
4. **Miscellaneous** (~60 lines): the `schema()` accessor, the
"walked backwards" guards (defensive), the
`resolve_and_walk_variant` `Union`-as-variant arm (nested unions),
the `resolve_variant_schema` `None`-return arms, and several
`panic!("expected ...")` arms in test helpers (these are unreachable
by construction — they're match exhaustiveness guards).
**Fix**: ~50 lines of tests, split by group:
- **Group 1**: feed a `read_next` call a buffer with a byte-discriminator
union whose discriminator value isn't in the mapping (the existing
`union_unknown_discriminator_returns_access_error` covers the byte
case; add the field-name case). Add a field-name union whose
`properties` is missing entirely (schema error).
- **Group 2**: an array with `items` set to a schema with no
`AlkType:*` keyword (schema error). An array with `minItems` set but
`maxItems` absent and a count prefix that's too short (access error —
already partly covered by `buffer_too_short_returns_access_error`).
- **Group 3**: a `read_next` with a `Uint16` byte discriminator and a
`Uint32` byte discriminator (extend the existing
`byte_discriminator_union_reads_value` with a parametrized disc type).
A `discriminator_string_value` test with `U8`/`Enum` field
discriminators (extend `field_discriminator_union_reads_value`).
- **Group 4**: a `schema()` accessor one-liner. A nested-union-as-
variant test (a union whose mapping points at another union). The
"walked backwards" guards and `panic!("expected ...")` arms are
unreachable by construction — leave them.
**Lift**: ~60 uncovered lines (excluding the unreachable guards), lifts
`sequential_reader.rs` from 86.5% to ~92%. Medium effort — the test
patterns exist, but the error-path scaffolding needs a few crafted
schemas and buffers.
---
### S5. `LayoutBuilder` error paths (layout_builder.rs, ~85 lines)
**File**: `src/layout_builder.rs` (many locations)
**Problem**: Similar shape to S4 but on the write-side. The uncovered
lines are almost all `AlkTypeError::Offset` construction arms for
defensive guards: `type_size returned None for fixed kind`, `offset +
size overflows`, `prefix + data size overflows`, array/union error
constructions, and the `unreachable!` exhaustiveness guard. Three
`panic!("expected Offset, ...")` arms in test helpers are unreachable by
construction.
The one error path that's a real consumer-facing validation error is the
"TUnion variant must be AlkType:Struct" arm (lines 528, 591) — a
consumer who points a union mapping at a non-struct variant should get
a clear error. This is uncovered for both byte-offset and field-name
discriminators.
**Fix**: ~15 lines of tests:
- A union mapping whose variant is `AlkType:Uint32` (not a struct) —
assert `build` returns `Err(AlkTypeError::Offset { .. })` with a
reason containing "variant must be AlkType:Struct". Test both
byte-offset and field-name discriminator variants.
- The remaining ~70 lines are overflow guards and `unreachable!` — leave
them (same reasoning as S8 below).
**Lift**: ~6 uncovered lines (the two "variant must be Struct" arms).
The rest is defensive. Low effort for the consumer-facing error; the
guards are S8 territory.
---
### S6. `schema.rs` unused public accessors (schema.rs, ~34 lines)
**File**: `src/schema.rs`
**Problem**: Several public API methods are never called by any
internal code or test:
- `AlkTypeKind::as_str()` — ~15 of the 19 match arms are uncovered (only
`Uint32`, `Float32`, `Float64`, `Timestamp` are hit by existing
tests). The method is public; consumers use it for error messages and
debugging.
- `AlkTypeKind::needs_endian()` — completely uncovered. The method
exists but no internal caller uses it (the engine and data_access
layer infer endianness need from the kind directly).
- `AlkTypeKind::is_composite()` — completely uncovered. Same situation.
- `AlkTypeKind::get_alktype_kind_enum()` — the strict (boolean-form-only)
variant. Tests use `get_alktype_kind_loose_enum` instead.
**Fix**: ~15 lines of tests:
- A `as_str()` round-trip test: `for kind in [all 19 variants] { assert
kind.as_str().parse::<AlkTypeKind>().unwrap() == kind }`.
- A `needs_endian()` test asserting `true` for the multi-byte kinds and
`false` for the rest.
- An `is_composite()` test asserting `true` for `Struct/Union/Array/
Record` and `false` for the rest.
- A `get_alktype_kind_enum()` test asserting it returns `None` for the
object-annotation form (mirroring the existing
`get_alktype_kind_ignores_object_keyword` test for the loose variant).
**Lift**: ~34 uncovered lines, lifts `schema.rs` from 91.2% to ~100%.
Trivial — these are pure functions over the enum.
**Question**: `needs_endian()` and `is_composite()` are currently unused
internally. If they have no external consumer either (this is a fresh
crate, no consumers yet), consider whether they're dead code that should
be removed, or kept as part of the intended public API. If kept, test
them; if removed, the coverage gap disappears. This is a design
decision, not a coverage decision.
---
### S7. `offset_map.rs` defensive guards and one missing-properties path (offset_map.rs, ~35 lines)
**File**: `src/offset_map.rs`
**Problem**: The uncovered lines are almost all `AlkTypeError::Offset`
construction arms for defensive guards: `type_size returned None`,
`element kind has no fixed size`, `array schema is not an object`,
`TArray is missing 'items'`, `could not resolve TArray items schema`,
`TArray element schema has no AlkType:* kind`. One `unreachable!`
exhaustiveness guard and one `round_up`/`align_up` early-return arm.
The one path that's a real consumer-facing error is the "struct schema
has no 'properties' object" arm — a nested struct missing `properties`
should produce a schema error. This is uncovered.
**Fix**: ~5 lines: a test with a nested struct whose `properties` is
absent, asserting `OffsetMap::compute` returns
`Err(AlkTypeError::Schema(_))`.
**Lift**: ~3 uncovered lines (the missing-properties arm). The rest is
defensive (S8). Low effort.
---
### S8. Defensive overflow guards — the long tail (all modules, ~150 lines total)
**Files**: `src/data_access.rs`, `src/layout_builder.rs`,
`src/sequential_reader.rs`, `src/tunion.rs`, `src/offset_map.rs`
**Problem**: The single largest category of uncovered lines across the
crate is `checked_add(...).ok_or_else(|| ... overflows usize)` guards.
These appear in `data_access.rs` (the read/write array and string
functions), `layout_builder.rs` (every offset computation),
`sequential_reader.rs` (every offset + size computation in the union/
array/record readers), `tunion.rs` (the discriminator offset + size
guards), and `offset_map.rs` (the array offset computations).
They are structurally hard to test because they require
`usize::MAX`-adjacent inputs to trigger the overflow. The code is simple
— a `format!` constructing an error message — and the guards are
correct-by-construction (they wrap a `checked_add` that returns `None`
on overflow).
**Fix**: Two options, in increasing fidelity:
1. **Skip them** — these are defensive guards over trivial format-
string construction. They're low-risk gaps. Document in this review
that they're intentionally left, and move on. This is the
recommended path for v1.
2. **Targeted overflow tests** — for each `checked_add` site, construct
an input where `offset + size` would overflow `usize` (e.g. `offset =
usize::MAX - 1`, `size = 2`). This requires crafted buffers or
`var_sizes` entries near `usize::MAX`. ~20 tests, ~150 lines, covers
the full long tail. High effort, low value — the code is a format
string.
**Lift**: up to ~150 lines if option 2 is taken. Option 1 is
recommended — the effort/value ratio is poor and the guards are
correct-by-construction.
---
## Recommended Order
1. **S1** (error.rs Display) — trivial, 9 lines, the only 0% file.
Do this first; it's a 5-minute fix.
2. **S2** (validate() paths) — easy, ~100 lines across validation.rs and
macros.rs. Largest single leverage: lifts two files above 95% and
exercises the public `validate_json` error-return path.
3. **S6** (schema.rs accessors) — trivial, ~15 lines, lifts schema.rs to
~100%. Also resolve the "keep or remove `needs_endian`/
`is_composite`" design question.
4. **S3** (engine String/Struct read) — easy, ~20 lines, lifts engine.rs
to ~100%.
5. **S4** (sequential reader error paths) — medium, ~50 lines for ~60
lifted lines. The discriminator-helper arms and the unknown-value
error paths are the high-value subset; the overflow guards are S8.
6. **S5** (layout_builder "variant must be Struct") — low effort, ~6
lines for the consumer-facing error. The rest is S8.
7. **S7** (offset_map missing-properties) — low effort, ~3 lines. The
rest is S8.
8. **S8** (overflow guards) — recommended to skip for v1. Document as
intentionally deferred; revisit if the crate ever handles untrusted
offsets from untrusted input.
After S1S7 (skipping S8), estimated coverage: **~95% lines, ~92%
functions**. The remaining ~5% is the overflow-guard long tail.
---
## Notes
- All line numbers refer to the tree at commit `6d61429` (the rebranding
cleanup commit). The coverage was measured on that tree.
- No critical or warning findings: this pass confirms the gaps are
testing-only. The underlying logic was reviewed during the rebranding
sweep (the full source was read; all `AlkType:*` keyword strings, the
`AlkTypeKind` enum, the parsers, the validators, and the public API
are consistent).
- The crate has no feature flags (`[features] default = []`), so
`--all-features` is not needed. If feature flags are added later
(e.g. `no_std` per OQ-002), this pass should be re-run with
`--all-features` to catch the cfg-gated paths.
- The `panic!("expected ...")` arms in test helpers across
`sequential_reader.rs`, `tunion.rs`, `layout_builder.rs`, and
`data_access.rs` are unreachable by construction (they're match
exhaustiveness guards on `AlkTypeError` variants in `assert!`
matchers). They show as uncovered but are not test gaps — they're
defensive code in test helpers.