test: reach 100 percent statement, branch, function, and line coverage - #16
Merged
Merged
Conversation
… and the peer class Adds unit and integration tests for every path the coverage gate flagged: the attached type guards' rejection sides, hop-chain and dedup edge shapes, errors.ts's taxonomy, ps-proc-info's alive/lstart branches (own pid, dead pid, EPERM, missing ps binary, in-flight dedup), paths.ts (XDG_RUNTIME_DIR, non-numeric socket basenames), fs-key-store and fs-registry-store's corrupt/missing/schema-invalid file handling, uds-transport's connection lifecycle (readLines end-of-stream, InboundConnection.close, connectWrite against a dead socket), file-transfer's oversize/refusal/sweep paths, and the CcPeer class's send/subscribeIdle error paths, inbound frame handling (foreign auth tokens, malformed JSON, unknown control actions, hop-chain propagation), and the pacer's refill wait. Fixes a real deadlock found by the new close-lifecycle test: UdsTransport's ListeningSocket.close() destroyed accepted sockets after awaiting server.close(), but net.Server#close only invokes its callback once every connection has ended, so closing with an open connection hung forever. Destroying accepted sockets before calling close fixes it. Removes two runtime guards that had become structurally unreachable: buildRegistryEntry read procStart through an optional chain even though start() already guarantees a non-empty value before calling it, and handleConnection checked this.ownKey for definedness even though it is only ever invoked as the listener callback registered after ownKey is set. Both now take the guaranteed value as a parameter instead, turning a runtime fallback that could never fire into a type-level guarantee.
…vidence The tengu_harbor_kite_limits dynamic override has never been served to this account — it is absent from the Statsig evaluations cache — so the effective peer-guard limits are the code defaults and maxQueuedPeerMessages is 50. The 55-messages-queued non-firing therefore reflects queue accounting (messages parked behind the approval dialog do not count toward the undelivered-peer-message queue) rather than a raised cap.
Extract listeningPort and hostnameOf as pure, exported functions so their impossible-through-the-public-API sides are directly unit coverable rather than left as untested defensive branches. listeningPort throws when the underlying server ever reports a named-pipe address or null, which our own call site (always TCP host:port) cannot trigger, surfacing that violated assumption loudly instead of silently returning port 0. hostnameOf replaces a split()[0] fallback chain (whose second nullish coalescing was structurally dead, since split always returns a non-empty array) with indexOf/slice, so every branch is a real, constructible input rather than a type-only case. Added coverage for both, plus a POST /messages case exercising the priority and fromMode optional fields together, reaching 100 percent branches across the suite. Also drops a dead eslint-disable comment: the shared config's noInlineConfig makes inline disables inert, so the existing file-scoped rule override in eslint.config.ts was already the only thing suppressing the object-assign rule here.
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. |
…to stryker Two independent bugs made the mutation gate meaningless: stryker's CLI only auto-discovers stryker.conf.json/.js/.mjs/.cjs, never .ts, so "stryker run" with no argument silently ran with command-line defaults (no thresholds, the whole src tree as the mutate scope) rather than this repository's own config; and @stryker-mutator/vitest-runner@10.0.0 joins nested test names in a format vitest 5 no longer produces, so a mutant run's testNamePattern regex matches nothing and every mutant executes zero tests, misreporting as Survived regardless of whether the real suite would actually kill it (confirmed directly: manually reproducing one mutant and running vitest against it directly failed the covering test exactly as expected, while stryker still reported it Survived with "Ran 0.00 tests per mutant"). Passing stryker.config.ts explicitly to the CLI fixes the first bug. For the second, no fixed vitest-runner release exists yet against vitest 5 as of this pin; downgrading vitest and its coverage provider to the 4.x line stryker-mutator/vitest-runner was actually built against removes the test-name mismatch, confirmed by the same mutant re-reporting Killed once real tests execute again. Both pins carry a documented reason per the dependency-pinning policy and should be revisited once an upstream fix ships.
…ma guards Now that the vitest-runner/vitest version pin gives a trustworthy mutation score, work through the genuine survivors it surfaces: - pacer.ts: fold the refill guard's separate zero-elapsed branch into Math.max(0, elapsed) and the msUntilNextToken guard into Math.max(0, 1 - tokens), removing two comparisons that were only ever equivalent-in-effect at their own boundary (both branches produced byte-identical state there) with no test able to distinguish them; the simplified arithmetic keeps the same behaviour for every real input while shedding the unkillable comparison entirely. Added coverage for refill running via msUntilNextToken directly (not only after a preceding tryReserve), the exact fractional wait value, and a clock moving backward. - hop-chain.ts: multi-id joinChain output (the separator was untested with a single-element chain) and appendHop landing exactly at the grammar maximum without trimming. - envelope.ts: the exact escaped-body content (not just absence of the raw tag), a multi-entry hop-chain's comma separator in the built wire form, and a crafted body carrying an unescaped closing tag earlier in its own content — the greedy body capture runs to the LAST closing tag, so re-escaping on rebuild changes the string and assertRoundTrips must reject it, exactly the case the check exists to catch. Also asserts absent optional fields are omitted keys, not keys explicitly set to undefined, on both the from-only and from-less parses. - schemas/guards.test.ts: the key-file token test now supplies every required field (a missing pidDomain masked the token regex outcome entirely) with both a rejecting and an accepting token, and the envelope attributes test adds a fromSession charset boundary. - stryker.config.ts: ignoreStatic, with a documented, independently reproduced link to the still-open vitest-runner bug it works around (a module-load-time mutant that crashes test collection is misreported Survived rather than Killed).
…direct testing filterRoster awaited each entry's checkEntry call sequentially in a for-loop; roster() may probe dozens of live sessions per call and each check is an independent I/O round-trip with no shared state, so Promise.all runs them concurrently instead, a real latency win as the roster grows. checkEntry's own reason field was untestable from outside: filterRoster only returns the admitted entries, so a corrupted verdict object or a blanked-out reason string could never surface through the public return value even though it changed real behaviour internally. Exporting checkEntry lets a direct test assert the exact verdict shape and reason for every rejection path. Also drops two guards that were provably redundant given the schema's own guarantees: entry.messagingSocketPath is always a non-empty string by the point the own-socket check runs (the no-socket branch above it already returned otherwise), so comparing it against an absent ownSocketPath needs no separate undefined check; entry.procStart is always a defined, non-empty string, so an absent lstart already fails a plain inequality against it without a redundant explicit check. Both guards produced byte-identical results at every real input either way, adding a branch with no distinguishable behaviour to test against.
None of the three had a production caller: parseEnvelope splits a captured hop-chain directly (the wire grammar's own regex already constrains each id to valid hex before the split ever runs, so parseChain's per-id re-validation was always redundant there), and serializeAttributes joins the array directly since its own emptiness check already precedes the call (joinChain's empty-to-undefined branch never had anywhere to matter). Neither cc-peer.ts nor the package's published exports referenced any of the three - only their own unit tests did, which this removes with them. Only appendHop and checkChain were ever actually exercised by the protocol implementation, and both already had real coverage against the same live-verified boundaries. appendHop's own trim also folds into a single Math.max/slice expression: slicing from a non-positive start is a no-op in JS, so a chain already at or under the maximum returns unchanged without a separate length comparison whose true and false branches produced an identical result at the exact boundary anyway (trimming to N elements when you already have exactly N is inherently a no-op, regardless of which branch decided to do it).
Path helpers: exact spoolDir/uploadsDir segments, not just a substring check. stageFile: the boundary sits at exactly MAX_FILE_BYTES (accepts) versus one byte over (rejects), the staged filename's sha256 and uuid segments are each exactly 8 hex characters (an unsliced sha256 or uuid would break that exact shape), and the written file is owner-only (0600). materialiseAttachment: an attachment whose file_size alone is wrong (hash still matches the real bytes) fails integrity verification on its own, isolating that half of the OR from the sha256 check the existing test already covered; the delivered path uses the same 8-character prefix shape as staging. capAttachments: a batch of exactly the cap size passes through whole, the mirror case to the existing over-the-cap test. sweepSpool: an hour-old file survives against the real one-day cutoff (the multiply-vs-divide arithmetic mistake this guards would place the cutoff a fraction of a millisecond from "now", sweeping it up too); a file landing exactly on the cutoff survives, since only strictly-older files are removed; and a pass processing more entries than the sweep batch always leaves at least one untouched.
Add a dedicated wire.test.ts exercising every schema in wire.ts: one
fully-shaped valid object per schema (kills every literal/enum-member
mutation the fixture's own values happen to use, plus whole-object
and nested-object ObjectLiteral mutations that would otherwise still
validate an empty-required-field input), one deliberately incomplete
object per schema, an out-of-charset from address for every schema
carrying the address regex, and exhaustive membership checks for every
enum (status, state, from_mode, drop reason).
Length-boundary mutations (.min/.max swapped) fall out of using
realistic, moderate-length fixture values throughout rather than
single characters or the exact limit constants: a normal id or slug
string fails both an inverted min() and an inverted max() in the
direction that actually distinguishes them, so no separate boundary
test was needed for most fields.
Extract YieldReasonSchema as its own bare, uncaught enum: the object
field wraps it in .catch("claim"), which recovers a corrupted "claim"
member to exactly "claim" anyway, so testing the field's own parsed
value can never tell a healthy enum from a broken one for that specific
member. The bare schema's safeParse has no such fallback to mask a
failure.
guards.test.ts gains exhaustive .is() coverage for every registry enum
(NameSource, PeerStatus, SessionKind, PeerFeature) alongside a
rejected-value check for each.
stryker.config.ts: replace the earlier ignoreStatic fix with a
narrower exclusion of only define-schema.ts. ignoreStatic solved the
one collection-crashing mutant it was added for, but every schema
definition in this codebase is itself a top-level const, so it
silently zeroed out mutation testing for the entire schemas
directory — confirmed directly: wire.ts and registry.ts reported zero
mutants under ignoreStatic despite having genuine, real survivors
moments earlier. Excluding just the three-line helper keeps that
side effect from reaching the schemas it don't apply to.
CI's mutation gate failed on a clean checkout with "Cannot find TestRunner plugin vitest. In fact, no TestRunner plugins were loaded" - stryker's plugin auto-discovery didn't resolve @stryker-mutator/vitest-runner against a fresh pnpm install's strict, non-flat node_modules layout, even though the identical config ran fine locally against an already-populated node_modules. Naming the plugin explicitly in the plugins array removes the dependency on auto-discovery finding it either way.
|
🎉 This PR is included in version 1.1.6 🎉 The release is available on: Your semantic-release bot 📦🚀 |
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
1 participant
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
.is()guards' false side.UdsTransport.listen().close(): connections were destroyed afterserver.close()'s callback instead of before, and that callback only fires once every connection has ended — a listener with any open connection would hang on close forever.src/api/server.tsby extractinglisteningPortandhostnameOfas pure, exported functions:listeningPortthrows instead of silently defaulting to port 0 when the server ever reports something other than an AddressInfo (impossible through our own call site, so the throw path is unit-tested directly rather than left as an uncovered defensive branch);hostnameOfreplaces asplit(":")[0]fallback chain (whose second??was structurally dead code) withindexOf/slice, making every branch a real, constructible input.Test plan
pnpm test:coverage— 100% statements/branches/functions/lines (645/645, 302/302, 154/154, 600/600)pnpm test:mutation) — running separately, will report before merge