Repository navigation
test: delete low-value and trivial Rust tests - #6193
Conversation
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 3 minutes. View limit details
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 |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2761ce2aab
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| assert_eq!(HOP_ID % 2, 0); | ||
| assert_eq!(RELAY_COST % 2, 0); | ||
| assert_eq!(HOP_PATH % 2, 1); |
There was a problem hiding this comment.
If RELAY_COST, HOP_PATH, or ROUTE_COST is accidentally changed to another value with the same parity, this reduced test and the Rust encode/decode round trips will still pass, while the draft and JavaScript peer continue using the registered IDs and interoperability breaks. Keep the literal assertions or replace them with independent golden wire bytes rather than checking only parity.
AGENTS.md reference: rs/moq-net/AGENTS.md:L3-L5
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 8617fcf: every dropped wire code point is pinned again in one spec-value table per registry (ietf::tests covers HOP_ID, RELAY_COST, HOP_PATH, ROUTE_COST and the other extension values). (Written by Claude Opus 5.5)
Duplicates, derive/clap/serde restatements, constant restatements and tautologies. Per-field merge pins fold into table tests. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Duplicates of the shared golden vectors, cross-level duplicates, and asserts that cannot fail. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Duplicates, derive and third-party restatements, tautologies and old-bug replays. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Constants and defaults restated as literals, constructors read back, fixed-string round trips, and asserts that restate the implementation line for line. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Setup Options, message parameters, message types, roles, and session and stream error codes each check every value against the draft, so a same-parity renumber fails. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
2761ce2 to
8617fcf
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8617fcf51e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| @@ -124,36 +124,3 @@ impl FromStr for AudioCodec { | |||
| Ok(Self::Unknown(s.to_string())) | |||
| } | |||
| } | |||
There was a problem hiding this comment.
Removing codec_kind_discriminants_are_stable leaves no repository-wide assertion for this public enum's implicit as values. If a future codec kind is inserted anywhere but the end, the change compiles while silently renumbering every later variant; retain this guard or assign explicit discriminants so the required append-only invariant is enforced.
AGENTS.md reference: rs/AGENTS.md:L47-L48
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in ac843a5: AudioCodecKind now has explicit discriminants AAC = 0 through Ec3 = 7, so the type enforces the append-only values itself. A kind inserted mid-list without a number collides with the next explicit discriminant and fails to compile. (Written by Claude Opus 5.5)
The deleted discriminant test was the only guard on the append-only as values; explicit discriminants make the type enforce it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Merge summary for head
Enabling auto-merge. (Written by Claude Opus 5.5) |
Problem
An audit of the Rust tests found tests that cost upkeep without catching anything: copies of other tests, checks of derive/clap/serde/std behavior, restated constants, asserts that cannot fail, and replays of one old bug that a stronger test already covers. Tests should be worth their weight long-term: cover edge cases, not old bugs (the guidance itself lands in moq-dev/moq#6181).
Approach
Test-only, except one no-op source change (step 4). 194 test attributes removed across 26 crates (9869 to 9675), about 2,500 lines net. Every deletion was re-checked against current
main, and for duplicates the covering test was confirmed to still exist and cover it.Four steps, committed separately:
test: drop trivial constant and getter tests): 43 tests deleted and 9 more trimmed of literal-only asserts (Defaultliterals in moq-relay, moq-bench, moq-mux, hang; moq-net code points, restored in step 3). Deleted: a constant orDefaultfield equal to a literal (7, plus 2 wire code point tests now in step 3); a constructor or getter read back, oris_emptyon a fresh value (9); enum-to-str or codec-string round trips of a literal table (8); clap or serde-derive-only parsing (7); asserts that restate the implementation line for line (8); duplicates found on the way (2). Kept whenever the literal comes from an independent source: spec bytes, golden vectors, the kernel header, the WAVE/JWA/HTTP specs, another crate's default, or a codec running on fixed bytes.test(moq-net): pin wire code points ...): every wire code point or error code the sweep dropped is back, as a single[(name, const, spec)]table test per registry: IETF Setup Options and message parameters (ietf::tests, every extension draft value), SUBSCRIBE_NAMESPACE message types, moq-lite roles, and session and stream error codes (the existing round-trip tests now check each code against its draft value too). A same-parity renumber now fails.AudioCodecKinddiscriminants made explicit (fix(hang): ...): instead of keepingcodec_kind_discriminants_are_stable, the enum spells outAAC = 0throughEc3 = 7, so a kind inserted mid-list fails to compile. Same values as before.Audit rows, by reason
tests/integration/alpn.rs(25): every pinned-version handshake over QUIC and WebTransport is repeated inbroadcast.rs, which also moves a frame.diff.rs(8): case-for-case copies of the shared golden vectors (tests/pattern.json,tests/vectors.json). The one moq-pattern assertion missing from the vectors (a/bmatches/a//b/) became a new vector; js/pattern passes it too.cli_flag_overrides_toml_{stats_enabled,cluster_id}andcluster_node_round_triprepeatprovenance_merge_tablerows,merge_into_relay_fragment, andcluster_connect_object_form_round_trip.encodenon-empty (sign then verify is stronger),find_key.cli_does_not_clobber_toml_*pins becameprovenance_merge_tablerows, one per value shape not yet in the table (duration,Option<String/u64/bool>, enum, path,Vec<PathBuf>, URL, socket address, flattened fields, cfg-gatedlisten.unixandcluster.lan.app), each checking value and origin. moq-tokio's 8test_toml_*_survives_update_frompins became one table that checks the whole config survives the re-parse. moq-auth's Display table folded into thefrom_strtable.PartialEq/Copy/Clone/Defaultand serde_json errors, moq-tokio clap flag parsing (6), moq-netOrd::max(2), moq-muxClock: Copy, hangRole: Default, moq-rtmphmac_test3(an RFC vector for the hmac crate), moq-transcode example arg parsing (2).resolve().timeout == DEFAULT_TIMEOUT), moq-rtmp identity serializers (4) plus a setter and constructor, moq-video&Surfaceimmutability,Display, and a bitrate no-op that cannot fail on what it names, moq-mux mkvlet _ = run(..), moq-netpath::test_empty, kio accessors (2), moq-sock IPv4 bind, moq-cli CORS default, moq-ffiorigin_lifecycle, moq-gstframe_micros_is_stateless, moq-hls config constructor.audio_fragment_does_not_absorb_a_pause(its Opus durations are filled before the boundary logic it claims to test), moq-srtreordered_frame_is_clamped_to_the_first_packet(covered throughEgressbya_reorder_is_not_a_rewind).Counts (
#[..test]attributes, before to after)Impact
AudioCodecKindkeeps the same discriminants, now explicit). One new golden vector inrs/moq-pattern/tests/pattern.json.Alternatives / deliberately skipped
#[ignore]tests, and the moq-netstats.rssource-grep tests (maintainer call).tests/cluster_*.rsfiles, including the Lite04 unknown-publisher triangle the audit nominates as an old-bug pin. A separate slow-test pass owns those. Files edited by moq-dev/moq#6174.rs/moq-quic: quinn's tests in our hard fork.broadcast_*same-version tests;test_cli_disable_verify_flag(pins our owndefault_missing = "true").main: moq-authtest_key_from_str_invalid_json(only test of the base64url bad-input branch), moq-netfetches_serde_roundtrips(hand-mapped serde) anddetached_charge_is_noop(separate no-pool path), moq-clicors_origin_allows_specific_allowlist(can fail), moq-gsta_restarted_pad_is_no_longer_counted_as_ended(merging its assert would change the covering test's scenario). Some rows no longer exist.Follow-ups
data_sections_stay_off_the_wire_when_emptyduplicates the first assert ofempty_video_and_audio_sections_omitted; outside the audit's list, left alone.Checks
68a396ecb(mergedmain):just checkpasses up tonix flake check, which cannot build the runner VM image in this sandbox (unsharedenied; unrelated, frommain). The steps after it pass when run by hand: Rusttest-changed9155 of 9156 passed, the one failure ismoq-uringa_steered_group_serves_a_shared_porthitting the sharedRLIMIT_MEMLOCKon this box (untouched crate, environmental); JStest-changedpasses.(Written by Claude Opus 5.5)
🤖 Generated with Claude Code