Kill mutation-testing survivors and widen scaffolding excludes (#566, #567, #569, #570, #593, #595, #598) - #612
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThe PR expands mutation-testing exclusions, annotates equivalent mutants, and adds focused tests for budget boundaries, expiry, client configuration, connection state, codec behaviour, errors, serializers, requests, packets, and protocol accessors. ChangesMutation and contract coverage
Possibly related issues
Possibly related PRs
Suggested labels: Poem
🚥 Pre-merge checks | ✅ 17 | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (17 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
ac9a598 to
d769287
Compare
The mutation-testing caller's `src/test_helpers.rs` exclude glob matched only the module root, leaving the `src/test_helpers/` submodules (frame_codec.rs, pool_client.rs) to emit noise survivors. Companion `tests.rs` files carrying cfg(test)-only modules were likewise mutated because cargo-mutants cannot detect them as test code. Add `src/test_helpers/**` and a repo-wide `src/**/tests.rs` glob to the caller and pin both in the workflow contract test.
The WireframeError wrapper only had its clean-close true case asserted; every false-case assertion targeted CodecError directly, so a wrapper that unconditionally reported a clean close passed the suite. Add false-case assertions for the Io, mid-frame Codec, and Protocol variants at the WireframeError level. Addresses #567.
A cluster of public accessors and trait methods was never asserted directly, so each could be replaced with a constant. Add direct assertions for WireframeApp::protocol and message_assembler, the FrameCodec trait's max_frame_length via UFCS dispatch (the inherent method shadows it), ServiceRequest::frame, BincodeSerializer's should_deserialize_after_parse, and MessageRequest::take_body_stream's take-once semantics. Addresses #598.
Several state predicates were asserted in only one polarity, so forcing them to a constant survived the suite. Assert a plain Envelope reports is_stream_terminator() == false, that a response stream is not terminated mid-stream, and that the actor state reports is_shutting_down() == true during shutdown (via a new ActorStateHarness::start_shutdown helper). Addresses #569.
Nine surviving mutants are equivalent: they cannot change observable behaviour, so no test can kill them. Add the mutants dev-dependency and mark each with #[cfg_attr(test, mutants::skip)] plus a one-line justification, keeping them out of future survivor reports without a production dependency. Addresses #566, #595, and the worklist equivalent-mutant list.
The budget and size-limit checks were only asserted strictly over their limits, and the wall-clock purge_expired() was never called. Add exact-fit assertions for the connection budget, the per-message size limit, and a first frame declaring exactly the limit, plus a zero-timeout test that drives the no-argument purge_expired(). Addresses #595.
Reflow the assertions added in the preceding survivor-kill commits to satisfy the formatting gate.
is_stamping_enabled's false polarity is unreachable during frame processing: the sole MultiPacketContext construction site always installs the channel with install(Some(rx), _), which yields Enabled, so a Disabled stamp never coexists with an active channel. The survivor is equivalent, not a test gap; #569's proposed disabled-stamping test cannot be constructed through the actor API. Mark it accordingly. Addresses #569.
Drop the Result return from the two accessor tests (panic_in_result_fn) and replace an unwrap with expect in the declared-total submission helper (unwrap_used).
Move the send-streaming config accessor test into its own module so send_streaming.rs stays under the 400-line cap, and inline the declared-total submission into its test function so the expect call lives inside a test rather than a plain helper.
d769287 to
57fce33
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/client/pool/slot.rs`:
- Around line 50-54: Remove the mutation-suppression attributes covering
try_acquire_permit, clear_last_returned_at, and the cleanup predicate using
strong_count() == 0. Preserve mutation coverage for the immediate lease path,
timestamp clearing, and removal of dead weak registry entries; add targeted
tests or narrowly scoped exclusions only if a specific mutation is proven
equivalent. Apply the changes in src/client/pool/slot.rs lines 50-54 and
101-104, and src/session.rs lines 96-100.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 4537fb9a-09d1-4f0e-8650-0bf018bc5667
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (25)
.github/workflows/mutation-testing.ymlCargo.tomlsrc/client/connect_parts.rssrc/client/pool/slot.rssrc/client/runtime.rssrc/client/tests/mod.rssrc/client/tests/send_streaming_config.rssrc/client/tests/streaming.rssrc/codec/tests.rssrc/connection/multi_packet.rssrc/connection/test_support.rssrc/message_assembler/budget.rssrc/message_assembler/budget_enforcement_tests.rssrc/push/queues/mod.rssrc/request/mod.rssrc/server/runtime/backoff.rssrc/session.rstests/codec_error.rstests/connection.rstests/extractor.rstests/middleware.rstests/packet_parts.rstests/serializer.rstests/wireframe_protocol.rstests/workflow_contracts/mutation_testing_test.py
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
leynos/rust-prover-tools(auto-detected)leynos/whitaker(auto-detected)leynos/shared-actions(auto-detected) → reviewed against open PR#355kill-mutation-survivorsinstead of the default branch
Review of the survivor-triage PR challenged three mutants::skip annotations as false equivalents: the pool slot's non-blocking permit fast path, the idle-timestamp clear that guards against recycling a freshly created connection, and the session registry's lazy eviction of dead weak entries. Each mutation does change observable behaviour, so drop the skips and add unit tests that kill the mutants directly. Also drop the is_stamping_enabled skip: the disabled polarity is directly assertable at unit level, making a reachability argument unnecessary. Addresses review feedback on the survivor-triage changes for #566, #569, and the worklist equivalents.
Record in the developers' guide and ADR-007 the eight-shard dispatch fan-out, the added exclude-globs (src/test_helpers/** and src/**/tests.rs, #599), the workflow contract test that pins the caller configuration, and the convention for annotating proven-equivalent mutants with cfg_attr(test, mutants::skip) via the mutants dev-dependency.
Use minute-based Duration construction and an infallible SocketAddr literal so the new slot tests pass clippy's duration-unit lint and whitaker's no-expect-outside-tests rule.
The new typos gate on main enforces en-GB-oxendict, which requires the -ize form.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1df84ef92f
ℹ️ 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".
| # helper submodules (frame_codec.rs, pool_client.rs). `src/**/tests.rs` | ||
| # covers the cfg(test)-only test modules split into companion files, | ||
| # which cargo-mutants cannot detect as test code (#599). | ||
| exclude-globs: "src/codec/examples.rs,src/test_helpers.rs,src/test_helpers/**,src/connection/test_support.rs,src/**/tests.rs" |
There was a problem hiding this comment.
Exclude test submodule directories too
In the mutation workflow I inspected, this glob only excludes files whose path ends in tests.rs; it does not cover cfg(test)-only submodule files such as the newly added src/client/tests/send_streaming_config.rs or the existing src/fragment/tests/*.rs files loaded from #[cfg(test)] mod tests;. cargo-mutants documents that --exclude filters source files by glob and that path globs match root-relative file paths, so these files remain in scope and the false-survivor noise described in the comment continues for test-only submodules. Add an exclude such as src/**/tests/** or the specific test directories as well.
Useful? React with 👍 / 👎.
| // Equivalent mutant (`found != expected` guard → `true`): when the ids are | ||
| // equal the data path is identical to the mismatch arm bar a spurious | ||
| // `warn!`, so the returned correlation is unchanged. | ||
| #[cfg_attr(test, mutants::skip)] |
There was a problem hiding this comment.
Keep testing the correlation mismatch guard
This mutant is not equivalent when a request already has the same correlation id as the source: forcing the guard to the mismatch arm still returns the same value, but inherit_correlation then emits the mismatch log::warn!. That warning is observable diagnostics and would incorrectly report healthy requests as mismatched, so marking the guard with mutants::skip hides a real regression rather than an equivalent mutant.
Useful? React with 👍 / 👎.
Summary
Addresses #566, #567, #569, #570, #593, #595, and #598, plus the
mutation-testing worklist's scaffolding-exclude and equivalent-mutant items
(dataset: run 29074771985,
main@ 94a5d44). This is a survivor-triage pass:it kills the highest-value category-1 survivors, annotates equivalent
mutants, and widens the caller's scaffolding exclude-globs. Survivors that
need heavy asynchronous or fragmentation harnesses are deferred to their
issues (listed below) to stay within the survivor-triage tolerance.
Review walkthrough
Scaffolding exclude-glob fix (32 noise survivors)
The
src/test_helpers.rsexclude glob matched only the module root, so thesrc/test_helpers/submodules (22 noise survivors) and the cfg(test)-onlycompanion
tests.rsfiles (10 survivors) were still mutated. Thecaller workflow
now excludes
src/test_helpers/**and a repo-widesrc/**/tests.rs, and theworkflow contract test
pins both.
make test-workflow-contractspasses.Category 1: survivors killed (before → after, per file)
WireframeError::is_clean_close→trueprotocol()/message_assembler()→Nonemax_frame_length→0/1ServiceRequest::frame→ leaked vecsshould_deserialize_after_parse→truetake_body_stream→NonePacket::is_stream_terminator→trueis_terminated→true(mid-stream)is_shutting_down→falsechunk_size→None/Some(0)/Some(1);timeout→Nonecheck_aggregate_budgets/check_size_limit>→>=accept_first_frame_at>→>=;purge_expired→vec![]Totals for the files touched by this PR: 21 targeted survivors before, 0
after (confirmed by scoped re-runs, see Validation).
New tests live in
tests/codec_error.rs,
tests/wireframe_protocol.rs,
tests/middleware.rs,
tests/serializer.rs (new file),
tests/extractor.rs,
tests/packet_parts.rs,
tests/connection.rs,
src/codec/tests.rs,
src/client/tests/streaming.rs,
src/client/tests/send_streaming.rs, and
src/message_assembler/budget_enforcement_tests.rs.
A small
start_shutdownhelper was added tosrc/connection/test_support.rs
to drive the shutting-down polarity.
Category 2: equivalent mutants annotated
Marked with
#[cfg_attr(test, mutants::skip)](newmutants = "0.0.4"dev-dependency; no production dependency) with a one-line justification each:
src/client/connect_parts.rs—INITIAL_READ_BUFFER_CAPACITY(pre-allocation hint, min'd againstmax_frame_length; pure constant arithmetic)src/client/runtime.rs—builder(Defaultis literallySelf::new(); pure constructor identity)src/push/queues/mod.rs—builder(blanketFrom<T> for Tidentity; pure constructor identity)src/request/mod.rs—select_correlationguard (pure function; the equal-id path returns the identical value bar a spuriouswarn!)src/server/runtime/backoff.rs—normalized(pure function; the>=variant swaps two equalDurations, a no-op)src/message_assembler/budget.rs—is_enabled(Mutation testing: message assembler budget and expiry survivors #595 accepted; pure function over twoOptions guarding a hot-path shortcut whose slow path returns the same result)All six retained skips are stateless equivalences of pure functions or
constants — the mutated expression's value is provably identical for every
input, so no example, property, or model-checking harness could distinguish
them; a justification comment is the proportionate evidence.
Four skips originally proposed here were removed in review and replaced
with unit tests that kill the mutants (red-green verified, scoped
cargo mutantsconfirmed):src/client/pool/slot.rstry_acquire_permit— the reviewer is right thatforcing
Noneremoves the non-blocking fast path; a permit-capacity unittest now kills all variants.
src/client/pool/slot.rsclear_last_returned_at— a stale timestamp cantrigger a spurious recycle of a fresh connection whose lease never wrote a
new timestamp; a recycle-decision unit test now kills the empty-body mutant.
src/session.rsgetcleanup predicate — retaining the dead weak entrygrows the registry until a later
prune; a unit test asserts the failedlookup evicts the entry.
src/connection/multi_packet.rsis_stamping_enabled— although thedisabled polarity is unreachable through today's actor API, it is directly
assertable at unit level, so a stamping-invariant test replaces the
reachability argument.
Deferred to the issues (outside this PR's tolerance)
client_pool.rs:149is_shutdown,client_pool.rs:187ordered_slotsrotation,scheduler.rs:210notify_shutdown: need a livepool + test-server harness across a shutdown.
scheduler.rs:101has_waitersremains accepted (loom-only) per the issue.codec_driver.rs:87FramePipeline::purge_expireddelegation:needs a fragmentation pipeline with an injected partial reassembly.
client/hooks.rs:111LifecycleHooks::clone: needs apool-recycle reconnect test asserting a configured hook still fires.
fragment/adapter.rs:133traitpurge_expired:FragmentAdapteris public API, so deleting the never-dispatched impl is abreaking change; a dyn-dispatch test is the non-breaking alternative.
untouched here; their issues remain open with full analyses.
Validation
Red-green per module: each mutant hand-applied, new test observed failing,
mutant reverted, test observed passing. Transcript for WireframeError::is_clean_close false cases are untested (mutation survivor) #567 (
src/error.rs,is_clean_closebody →true):Scoped
cargo mutants --all-featuresconfirmation over the touched files,filtered to the targeted functions:
1 timeout, 0 missed. The timeout is
is_shutting_down→true,which State predicates asserted in only one polarity (mutation survivors) #569 already documents as the timing-out variant (the surviving
falsevariant is now caught).pool/slot.rs,session.rs,multi_packet.rsafter de-skipping): all targeted mutants caught,0 missed (see the review-response comment for the run summary).
make check-fmt,make lint(clippy + whitaker),make test(
--all-targets --all-features),make test-doc,make test-workflow-contracts, andmake markdownlintall pass.Documentation
The developers' guide
mutation-testing section and
ADR-007
now record the eight-shard dispatch fan-out, the widened exclude-globs, the
workflow contract test, and the
mutants::skipannotation convention with itsdev-dependency.
No production behaviour changed: every non-test source edit is either a
mutants::skipannotation with a justification comment or a#[cfg(test)]module.