test: Cover the Variant object header size matrix end to end - #422
Merged
CurtHagenlocher merged 2 commits intoAug 23, 2026
Merged
Conversation
apache#421 fixed `MakeObjectHeader` / `ParseObjectHeader` writing `field_id_size` and `offset_size` into each other's bits, and pinned both helpers to literal header bytes. That coverage stops at the helper: it says nothing about which widths `VariantValueWriter` asks for, or whether the body it emits is laid out at the widths the header declares. `VariantObjectHeaderSizeTests` builds real objects over the matrix of reachable widths — the dictionary is padded with names sorting ahead of the object's own to drive field IDs into the 2- and 3-byte bands, and the first field's value is padded to drive the end offset into them. Each case asserts the emitted header byte, decodes the object body with a decoder written from the spec rather than through `VariantEncodingHelper`, checks that the widths were forced for the reason intended (largest ID, end offset), and reads the object back through the library. A second theory covers objects of 300 fields, which pack `is_large` into bit 6 alongside the two size fields. Width 4 is unreachable from a real object: a 4-byte field ID needs a dictionary of more than 16,777,216 entries and a 4-byte offset needs more than 16 MiB of field data, so those cells stay covered at the helper level. Verified by reintroducing the transposition in `MakeObjectHeader`: 8 of the 13 cases fail, exactly the ones where `field_id_size != offset_size`. The 5 symmetric cases pass either way, which is why the bug survived until now. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Adds end-to-end unit tests that validate Variant object header encoding (field_id_size, offset_size, and is_large) across the reachable size matrix by writing real objects via VariantValueWriter, decoding the raw bytes according to the spec, and then round-tripping via VariantObjectReader. This extends prior helper-level coverage (from #421) to ensure the writer chooses the intended widths and that the emitted body layout matches the declared header widths.
Changes:
- Introduces
VariantObjectHeaderSizeTestscovering the cross-product of reachablefield_id_sizeandoffset_sizecombinations (plus large-objectis_largecases). - Builds fixtures that force field ID width via a padded, sorted metadata dictionary and force offset width via padded field data.
- Decodes object bodies with a spec-driven decoder (independent of
VariantEncodingHelper) and validates header byte, widths, IDs/offsets, and library round-trip.
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
The spec-side decoder asserted that field offsets strictly increase. Field IDs are sorted by name, but the values they point at may be stored in any physical order — `VariantObjectReader.GetFieldValue` says as much, which is why it slices from a field's start rather than between two offsets. The assertion therefore pinned the current writer's layout in a block whose stated job is checking the writer against the spec, and a spec-legal reordering would have failed it. Assert what the spec fixes instead: the last offset ends the data region, and every field start lies inside it. Reintroducing the transposition in `MakeObjectHeader` still fails the same 8 of 13 cases. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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's Changed
Additional test coverage for #420.
#421 fixed
MakeObjectHeader/ParseObjectHeaderwritingfield_id_sizeandoffset_sizeinto each other's bits, and pinned both helpers to literal header bytes. That coverage stops at the helper: it says nothing about which widthsVariantValueWriterasks for, or whether the body it emits is laid out at the widths the header declares.VariantObjectHeaderSizeTestsbuilds real objects over the matrix of reachable widths — the dictionary is padded with names sorting ahead of the object's own to drive field IDs into the 2- and 3-byte bands, and the first field's value is padded to drive the end offset into them. Each case asserts the emitted header byte, decodes the object body with a decoder written from the spec rather than throughVariantEncodingHelper, checks that the widths were forced for the reason intended (largest ID, end offset), and reads the object back through the library. A second theory covers objects of 300 fields, which packis_largeinto bit 6 alongside the two size fields.Width 4 is unreachable from a real object: a 4-byte field ID needs a dictionary of more than 16,777,216 entries and a 4-byte offset needs more than 16 MiB of field data, so those cells stay covered at the helper level.