test(codec): freeze moqt-16 wire vectors#39
Merged
Conversation
Co-Authored-By: Paperclip <noreply@paperclip.ing>
Author
|
🔗 Paperclip issue: BLO-16455 |
1 similar comment
Author
|
🔗 Paperclip issue: BLO-16455 |
Author
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Looks good. This is a self-contained, test-only change (fixture JSON + integration test + one new pub const), and it's low risk:
MOQ_WIRE_FIXTURE_VERSIONconst andmoqt-16-v1.jsonmanifest version string match; the test assertsmanifest.vectors.len() == 28, which matches the 28 fixture keys.- Verified the referenced APIs (
KeyValuePairs::set_intvalue/set_bytesvalue,TrackExtensions) exist inmoq-transport/src/coding— no hallucinated surface. - No SQL, no LLM trust boundaries, no conditional side effects, no CI toolchain pin risk (
is_multiple_ofrequires Rust ≥1.87; CI usesdtolnay/rust-toolchain@stable/actions-rust-lang/setup-rust-toolchain, so this is fine on current stable). - Byte-exact + round-trip assertions in
assert_fixtureare a sound pattern for pinning wire format against accidental regressions.
Suggestions (1)
- [native-codex]
moq-transport/tests/wire_vectors.rs:1— Since these vectors are hand-authored hex rather than captured from an external/spec reference implementation, this test guards against accidental encoder/decoder drift but doesn't independently verify MOQT-16 spec conformance. Worth a one-line doc note (or leave as-is, since the PR title already scopes this correctly as "freeze," not "validate against spec").
Strengths
- Clear separation of fixture data (JSON) from test logic, with a version marker tying them together.
- Covers unknown-parameter / unknown-extension / unknown-control-message paths, which are exactly the cases most likely to silently break.
Recommended Action
No blocking issues. Safe to merge once CI is green.
reviewed head: 15eaa9a
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.
Summary
moqt-16-v1fixture identityVerification
cargo fmt --allcargo test -p moq-transport(236 unit tests + 1 wire-vector integration test passed)Paperclip
https://paperclip.blockcast.net/BLO/issues/BLO-16455
Consumed by the dependent
pim-multicast-gatewayintegration PR for BLO-16455.