Repository navigation
test: delete tests that cover nothing beyond their neighbors - #6187
Conversation
7b1948b to
7e83147
Compare
|
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 7 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: 7e83147a72
ℹ️ 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".
| // The two registries are wire contracts (draft-lcurley-moq-lite, Error Codes). | ||
| test("the code tables keep their registry ranges", () => { |
There was a problem hiding this comment.
Restore the exact numeric assertions for these wire registries. The remaining range checks still pass if, for example, SessionCode.Version changes from 0x15 to another value below 32, while use-site tests reference the same incorrect constant and therefore also pass; JS would then send a code that Rust or another published peer interprets differently.
AGENTS.md reference: AGENTS.md:L84-L86
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Restored as one spec table per registry: the full session and stream code tables from draft-lcurley-moq-lite, in 5e9460e. (Written by Claude Opus 5.5)
| test("Epoch: the registry code point is a bare varint", () => { | ||
| // Odd values are length-prefixed byte strings, even ones bare varints. | ||
| expect(SetupOption.Epoch).toBe(0x40b68n); | ||
| expect(SetupOption.Epoch % 2n).toBe(0n); |
There was a problem hiding this comment.
Retain the SetupOption.Epoch === 0x40b68n assertion or an equivalent byte fixture. If this constant is accidentally changed to any other even value, the parity assertion and the setup round trips all continue to pass because both encoder and decoder use the same wrong key, but negotiation with conforming peers silently stops working.
AGENTS.md reference: AGENTS.md:L84-L86
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Restored in a new setup_options.test.ts that pins every SETUP option key to its draft value, Epoch 0x40b68 included, in 5e9460e. (Written by Claude Opus 5.5)
| test("SubscribeNamespace: decode rejects the wrong draft version", async () => { | ||
| // Modern 0x50 is draft-18+ only. |
There was a problem hiding this comment.
Keep explicit checks that legacy SubscribeNamespace is 0x11 and modern SubscribeNamespace is 0x50. The neighboring version and round-trip tests construct and decode messages through the same class IDs, so either ID can be changed to another value without failing them even though published peers will no longer recognize the messages.
AGENTS.md reference: AGENTS.md:L84-L86
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Restored as one table pinning legacy 0x11 and draft-18 0x50, in 5e9460e. (Written by Claude Opus 5.5)
7e83147 to
5e9460e
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5e9460e5b0
ℹ️ 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".
| @@ -19,8 +19,6 @@ test("the file picker opens before yielding to media loading", () => { | |||
| try { | |||
| source.prompt(); | |||
| expect(click).toHaveBeenCalledTimes(1); | |||
There was a problem hiding this comment.
Restore the assertions for input.type and input.accept. If File.prompt() stops assigning either property, this remaining click-only test still passes even though the default <input> is a text field and no usable file picker opens; losing accept also exposes unsupported file types despite the source only decoding image, video, and audio.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Keeping this deleted: the assertion restates literals the implementation assigns, and the repo rule is that tests cover edge cases, not trivial values. (Written by Claude Opus 5.5)
| // The exact frame duration does not imply encoder flush lateness. | ||
| test("keeps a 2.5ms Opus frame exact without a catalog hint", () => { |
There was a problem hiding this comment.
Restore the assertion that the default Opus catalog advertises an 80 ms warmup. The surviving toEncoderConfig test only covers the 20 ms frame duration, and no remaining publisher test ties this catalog hint to the 80 ms preroll also used by Rust in rs/moq-mux/src/codec/opus/mod.rs; because the JS publisher and watcher share Util.Opus.WARMUP, a mistaken change would remain self-consistent in JS tests while cross-language playback begins presenting at the wrong point.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Restored as a focused test pinning the default Opus catalog warmup to 80 ms, citing RFC 7845 section 4.6 and Rust's moq_mux WARMUP, in e8036dd. (Written by Claude Opus 5.5)
Drop language tautologies, exact duplicates, copied constants, assertion-free lifecycle tests, workflow literal greps, and a retired-action pin. Fold the three GOAWAY version rejections into one test.each, and inline the snapshot encoder's default group budget now that no test imports it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Delete tests that pin a constant, default, or wire code point to a hand-typed literal, read back a field just passed in, or restate the implementation line for line. Keep range and parity invariants, golden vectors, cross-language checks, and evaluated workflow conditions. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Encoder and decoder share each constant, so a renumbering passes every round trip while breaking interop. Restore the code points the earlier commits dropped as one table per registry: session and stream error codes, SETUP option keys, the SUBSCRIBE_NAMESPACE message IDs, the hang track names, the feedback track and suffix, and the catalog timescale default. The SETUP table replaces the cluster and epoch parity checks. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Rust's moq-mux declares the same 80ms WARMUP, and subscribers in either language prime their decoder by it, so it is a cross-language value. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
5e9460e to
e8036dd
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e8036ddeff
ℹ️ 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".
| test("a decoder's frame cap cannot be lifted after construction", () => { | ||
| const decoder = new Decoder(); |
There was a problem hiding this comment.
Restore the exact default-cap assertion. If DEFAULT_MAX_FRAME_SIZE changes while Decoder continues to consume the same constant, this immutability test and the remaining round-trip tests still pass; an increase silently weakens the zip-bomb memory bound, while any drift makes the documented JS default disagree with rs/moq-flate/src/lib.rs.
AGENTS.md reference: AGENTS.md:L18-L18
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Keeping this deleted. The 64 MiB cap is a local resource bound, not a wire contract: each decoder enforces its own, so JS and Rust drifting apart breaks no peer. A literal copy cannot detect drift from Rust either; it only makes a deliberate change edit two lines, and the constant's diff is already in front of the reviewer. The lower bound that does matter stays pinned as an invariant in stream.test.ts (MAX_GROUP_CACHE_BYTES <= DEFAULT_MAX_FRAME_SIZE), and the over-cap rejection is still exercised against the constant. (Written by Claude Opus 5.5)
| test("the session defaults match moq-net", () => { | ||
| expect(LIMITS).toEqual({ announces: 100_000, subscriptions: 10_000, fetches: 10_000 }); | ||
| }); | ||
| import { Interests, SlotMap, Slots } from "./limits.ts"; |
There was a problem hiding this comment.
Keep an assertion for the exact LIMITS values. The surviving tests inject caps of 2, so they verify enforcement mechanics but cannot detect the production defaults drifting from Rust's 100000/10000/10000; raising them would let an untrusted peer retain more announcements, subscriptions, or fetches without any test failure.
AGENTS.md reference: AGENTS.md:L18-L18
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Keeping this deleted. LIMITS is a local admission budget, not a wire value: each side caps what its peer may hold, so a JS/Rust mismatch breaks no interop. The test was a hand-typed copy that never reads moq-net, so it cannot catch drift from Rust; it only makes a deliberate change touch two lines, and that change is already visible in review. The surviving tests cover what can actually break: enforcement, release, and the TOO_MANY_REQUESTS close. (Written by Claude Opus 5.5)
# Conflicts: # sh/gh/queue.test.ts
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8253617ccf
ℹ️ 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".
| @@ -109,5 +85,4 @@ test("one job runs one CI dispatch with one checkout and cache restore", () => { | |||
| expect(steps.filter((step) => step.uses === "./.github/actions/nix")).toHaveLength(1); | |||
| expect(steps.filter((step) => step.uses === "./.github/actions/rust-cache")).toHaveLength(1); | |||
| expect(steps.filter((step) => step.run?.includes("nix develop"))).toHaveLength(1); | |||
There was a problem hiding this comment.
Restore the exact final-step dispatch assertion. If the Check step is accidentally changed from just ci to another nix develop command, every surviving assertion here still passes because line 87 only counts commands containing nix develop; since check.yml is the merge queue's sole Linux gate, that could leave the required Check green without running the selected lint and test suites.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Declining, per the maintainer's decision on this PR. The rule here is that only evaluated workflow ifs stay; a literal copy of the run string is the YAML restated in the test. A change to Check's command already shows up as a diff of the gate in review, and Check failing to run the suites would surface the next time a lint or test breaks. The "every setup step is gated" loop, which tests logic, stays. (Written by Claude Opus 5.5)
| expect(steps.find((s) => s.name === "Upload bindings")?.with?.name).toBe("${{ matrix.language }}-bindings"); | ||
| }); | ||
|
|
||
| test("wrapper gates preserve trusted tag inputs and inspect only their publisher", async () => { |
There was a problem hiding this comment.
Restore coverage of release-ffi.yml's partial-failure behavior. If the bindings job loses always() or the native build matrix loses fail-fast: false, one unrelated target failure can skip or cancel binding generation even after the required Linux metadata artifact succeeded; the remaining tests begin with the shared coordinator and wrappers and never verify this producer workflow behavior.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Declining, per the maintainer's decision on this PR. Every assertion in that test was a literal YAML check: fail-fast: false, needs, substrings of the if (always(), !inputs.reuse-native), and step names and with.name values copied from release-ffi.yml. None of it evaluates the if across job results, so it restated the workflow rather than testing behavior. The PR keeps only evaluated workflow ifs, like obs-build in cpp.test.ts. (Written by Claude Opus 5.5)
Problem
A test audit of the non-Rust suites found tests that cost upkeep but cover no edge case another test (or the language) doesn't already cover. Tests should earn their weight long-term: edge cases, not old bugs.
Approach
Delete the high-confidence rows, each re-verified on current
main:Language or library behavior
js/net/src/path.test.ts: "isEmpty checks correctly", "length property works correctly", "JSON serialization works" (string equality,String.length,JSON.stringify). Empty normalization stays covered by "Path constructor handles empty paths" and thePath.empty()prefix/join/relative tests.Duplicates
js/watch/src/audio/ring-buffer.test.ts: "should return 0 when not initialized" (body identical to "should return 0 when no data available").js/net/src/connection/cert.test.ts: hex round trip, covered byjs/net/src/util/hex.test.ts"round-trips".js/net/src/lite/goaway.test.ts: the three identical draft-01/02/03 rejections fold into onetest.each, keeping a case perhasGoawayswitch label.sh/gh/setup.test.ts: the twocheck.ymltrigger tests;sh/gh/queue.test.tsasserts the same triggers and evaluates the sameifacross events.Tautologies and copied constants
js/watch/src/audio/ring-buffer.test.ts: "should not throw when writing to buffer" (every other write test does this and asserts more).js/json/src/snapshot/snapshot.test.ts: the default budget equals the constant it is assigned from.DEFAULT_MAX_GROUP_BYTESexisted only for that test, so the encoder now readsGroup.MAX_GROUP_CACHE_BYTESdirectly (themaxGroupBytesdoc already states the default).js/net/src/util/limits.test.ts: a literal copy ofLIMITSthat never reads moq-net, so it cannot detect drift.js/publish/src/source/device.test.ts: checked the test fixture (typeof navigator.userAgent), not product code.No assertions
py/moq-ffi/tests/test_smoke.py(hasattrchecks andMoqClient(...) is not None; the moq-rs suite in the same pytest run drives these classes end to end).test_exit.pystays.py/moq-rs/tests/test_local.py::test_origin_lifecycleand its Go twinTestOriginLifecycle.Workflow YAML literal checks
sh/gh/ffi.test.ts: "bindings reuse Linux metadata even when an unrelated target fails", plus the type field it alone used. Every assertion was a literal copy fromrelease-ffi.yml:fail-fast: false,needs, substrings of theif(always(),!inputs.reuse-native), and step names andwith.namevalues. None evaluated theif, so it pinned the YAML, not the partial-failure behavior.sh/gh/cpp.test.ts: dropped the step-string,needs, andifliteral copies. Kept the one piece of logic: evaluatingobs-build'sifacross branch and tag results.Old-bug pins
js/watch/src/audio/ring-buffer.test.ts: "misplaces the frames when the ring rate disagrees" asserted the Opus published at any sample rate other than 48000 decodes to noise; file publishing hits it by default #2352 symptom, not a contract.sh/dispatch-query.test.py: the retiredci-check/ci-testrefusal; the dispatcher'scaserefuses any unknown action.Trivial constant, default, and getter tests (second commit)
Widened scope: tests whose expected value is a hand-typed copy of the implementation rather than an independent source (Rust fixture, spec vector, computed invariant). 22 whole tests deleted and 14 more trimmed to their real assertions, net of the wire-registry tables restored in the third commit.
Non-wire constant or default equals a literal
js/net: extension offer defaults,Reloadenableddefault, track priority midpoint, and themaxAge/timescaledefaults.js/flate:DEFAULT_MAX_FRAME_SIZEand its default (the read-only check stays);js/pattern:MAX_SEGMENTSnow compares against the value read before the write attempt, not32.js/publish: the Opus frame duration and jitter defaults (20 ms is also asserted by thetoEncoderConfigtest); the file input'stype/acceptliterals.cpp/obs: the Opus encoder's default bitrate and preset.sh/gh:targets.length === 9and=== 5inffi.test.ts(uniqueness and union stay); the copiedrun/CI_BASEstrings and finalrunliteral insetup.test.ts(the "every setup step is gated" loop stays); themoq-shmatrix literal and the redundantrelease-swift-libbranch check inqueue.test.ts.Trivial getters, constructors, and lookup tables
js/roomuserFieldsread-back;js/auththe two algorithm-list read-backs (invalid, case, and non-string rejections stay);js/hangcatalog fields that parse and read back (codec,label,enabledx2 audio/video), the known mode/compression lookups, OpussupportsRateover its ownSAMPLE_RATES;js/p2ptheprotocolpass-through;js/watchSAB sizes and "should start stalled" (the stalled and length getter tests cover it);demo/carladriverPathtemplate andnextGeartable.py/moq-rsuniffi optional-field defaults andError.Transport.reason;go/wrappertheMoqErrorTransport.Reasonread-back;dart/moqTransportMoqException.reason.Pins that removed things stay removed
queue.test.ts:obs.jobs.release,cpp.jobs.msvc,swift.ymlabsent.ffi.test.ts: the retired per-language nightly job names.Wire code points: one spec table per registry (third and fourth commits)
Encoder and decoder share each constant, so a renumbering passes every round trip while breaking interop. Instead of scattered single asserts, each wire registry is now one table test checked against the draft:
js/net/src/error.test.ts: the full session and stream error code tables (draft-lcurley-moq-lite, Error Codes). The range test stays.js/net/src/ietf/setup_options.test.ts(new): every SETUP option key, from moq-transport and eachdraft-lcurley-moq-*IANA section. It replaces the cluster and epoch parity checks.js/net/src/ietf/ietf.test.ts: the SUBSCRIBE_NAMESPACE message IDs (0x11 legacy, 0x50 from draft-18).hang/*.jsontrack names (js/room), thefeedback.jsontrack and.echosuffix (js/hang), and the catalog clock's microsecond timescale when omitted.warmupof 80 ms (js/publish), which RFC 7845 section 4.6 recommends and Rust'smoq_mux::codec::opus::WARMUPalso declares.Declined restoring (local resource bounds, not wire values, so a hand-typed copy pins nothing an interop peer depends on): the 64 MiB
DEFAULT_MAX_FRAME_SIZEliteral (its lower bound stays as theMAX_GROUP_CACHE_BYTESinvariant) and theLIMITSliteral.Kept on purpose: golden byte vectors, Rust-shared fixtures, the OBS encoder ID (persisted by OBS), Dart's microsecond-to-
Durationgetters (they pin the unit),priority.test.ts's draft value, and every evaluated workflowif.Declined restoring (literal YAML, per the maintainer): the Check step's
just ci "$CI_BASE"command insetup.test.tsand the ffi partial-failure test above.Impact
No public API or wire change. Tests only, plus inlining one internal, non-exported constant in
@moq/json.sh/gh/queue.test.tsis merged with moq-dev/moq#6154, which already dropped therelease-swift-libbranch check.Skipped
setup.test.tsstep counts,nightly.test.pycache grep) and the "keep, reframe" old-bug pins.connect.test.tsandwatch/broadcast.test.ts: removing them means removing the shims, which is a code change.js/qmuxSessionError.streamErrorCodeandWebSocketStream.urlread-backs: touchingjs/qmuxpulls its build into the gate, which fails onmainuntil moq-dev/moq#6191 adds the missing doc comments. Worth a two-line follow-up after it lands.Follow-ups
sh/gh/queue.test.tsruns only nightly (just gh test), so a workflow trigger that breaks the main-PR policy can merge unnoticed. moq-dev/moq#6154 fixes the currentsmoke.ymlviolation but leaves the test nightly-only; moving it intojust gh checkis still open.(Written by Claude Opus 5.5)
🤖 Generated with Claude Code