diff --git a/.agents/skills/test-oracle-reviewer/SKILL.md b/.agents/skills/test-oracle-reviewer/SKILL.md index d159c865b2..793f611472 100644 --- a/.agents/skills/test-oracle-reviewer/SKILL.md +++ b/.agents/skills/test-oracle-reviewer/SKILL.md @@ -1,6 +1,6 @@ --- name: test-oracle-reviewer -description: Judge whether a test's expected value is justified independently of the code under test. Use when reviewing tests added or changed in a pull request, auditing a module's tests, or whenever expected bytes, snapshots, or mock values may have been copied from the implementation's own output. +description: Judge whether a test's expected value is justified independently of the code under test, and define the `// oracle:` marker that records its source. Use when reviewing tests added or changed in a pull request, auditing a module's tests, marking a test's source, or whenever expected bytes, snapshots, or mock values may have been copied from the implementation's own output. --- # Test oracle reviewer @@ -8,7 +8,7 @@ description: Judge whether a test's expected value is justified independently of A test whose expected value came from the code under test detects change, not bugs. For each test, ask whether the expected value can be justified without running the code. -Independent sources: a spec example, an external capture, a hand-derived value, an input the test chose itself and asserts was honored, a real property such as round-trip or `encode` writing exactly `size()` bytes, or a reference model. +Independent sources: a spec example, an external capture, a hand-derived value, an input the test chose itself and asserts was honored, a named law such as round-trip, a property a document states, such as `encode` writing exactly `size()` bytes, or a reference model. Self-derived sources: a snapshot of current output, a value computed through the code or a shared helper, a production constant or message copied into the test, a mock that returns what is then asserted, `is_ok()` or `!is_empty()` on an input that no requirement says must succeed, or a restatement of what a derive or the type system guarantees. Verify instead of guessing. @@ -17,10 +17,48 @@ A value pasted from the code's output is independent once it checks out against A search-engine or AI summary is not a source; cite the primary document. Classify each test as independent, self-derived, or unknown, with evidence, and report per assertion when a test mixes classes. -When a value checks out, add a one-line citation comment to the test, such as `// §
example` or `// hand-derived; see PR #N`, so the next review is cheap. +When a value checks out, add its marker, as below, so the next review is cheap. When a value is wrong, report a bug; do not adjust the value or the code. Unknown is an outcome, not a guess in either direction. -In a pull request review, comment on each self-derived test with the independent source to use and leave the fix to the author. +## Markers + +Mark where an expected value comes from with `// oracle: ` on the test, on a case row, or on one assertion when a test mixes sources, so one search finds every test that rests on a source. +Strong kinds name an independent source, and a test, case, or assertion marked with one is deleted only when its source changes or is shown not to decide what it asserts: + +- `spec §
`: normative text other than a contract or intent file, such as an RFC, a Microsoft open specification, or `docs/JMUX-spec.md`. +- `contract §
`: a `CONTRACT*.md`. +- `intent §
`: a statement in an intent file. +- `regression `: a bug report, as a Jira key such as `DGW-123` or a GitHub issue such as `#123` or `owner/repo#123`, or a fuzz crash file under `fuzz/`. +- `reference `: what another implementation or a reference model does, recorded or compared live. +- `property `: a law of the code under test named by one of `round-trip`, `idempotent`, `involution`, `commutative`, `associative`, or `monotonic`. + +Write every path from the repository root, with forward slashes and exact case, such as `crates/gateway-db/INTENT.md`, because markers sit at every depth and a root-relative path reads and resolves the same from each. +A file in another repository is cited as `/@:`, with `` from that repository's root, such as `Devolutions/devolutions-gateway@43958682a1b2:docs/agent-identity/CONTRACT.agent-identity.md`, and `` a commit of at least 12 hex digits, never a branch or tag, because a pinned commit never changes and so resolves the same every time. +A `spec` document outside `docs/`, such as `RFC 6455` or `MS-RDPBCGR`, is named rather than given as a path. +`§
` names the `id` of an anchor the file places before a heading as ``, or else a heading, in any case, or the number a heading starts with, so `§3.2` matches `3.2 Framing`. +Cite the anchor when the file has one, because it survives a renamed heading or a renumbered section. +When a pinned section changes upstream, re-check the test against the new text, then bump the pin or change the test, never the expected value alone, because a value changed without its pin cites text that decides the old one. + +A `property` law is accepted only by its well-known name, because the name is its definition, while any other property needs a written source: it takes the kind of the document that states it, and is `scaffold` until one does. +An assertion whose expected value comes from running the code under test is self-derived even over generated inputs, so it is `mutant` or `scaffold`, never `property`. +A codec's `round-trip` test never replaces its spec or capture vectors, because a round-trip passes symmetric bugs. +A hand-derived value, a checked snapshot, or an input the test chose and asserts was honored takes the kind of the source that requires it. + +Weak kinds name a self-derived source, which detects change rather than bugs, so any intentional change, such as a refactor or a new feature, may delete a test, case, or assertion marked with one without justification unless `test-pruner` protects it: + +- `mutant `: a test or assertion kept to catch mutants that no strong source decides. +- `scaffold`: a test or assertion written for feedback while implementing that no strong source decides. + +No `scaffold` marker merges: before the pull request does, give what it marks a strong source, keep it as `mutant` when `test-pruner` finds it alone catches a fault, or delete it. +When it checks a property or intention no document states, ask whether and where to specify it, and mark it with that source once specified, since a confirmation alone is not a source; a protected test takes this path rather than deletion. + +Mark a strong source whenever one decides the value, so a test found through a mutant but decided by a spec is `spec`, because whether a test may go depends on whether a source still requires it, not on how it was found. +An unmarked test starts as unknown: verify it as above, and turn a citation in another form into the marker it supports. +A marker is a claim: verify it like any other origin. + +## Review + +In a pull request review, comment on each self-derived test that an independent source decides, naming that source, and on each changed test without a marker; leave the fix to the author. When that source needs a small non-test change, such as an injectable deadline the test can choose, request the change. -For mutation evidence on the same pull request, use `test-pruner` in pull request mode. +For mutation evidence on the same pull request, use `test-pruner` in review mode. diff --git a/.agents/skills/test-pruner/SKILL.md b/.agents/skills/test-pruner/SKILL.md index 6601597d58..ac9c7dc5f3 100644 --- a/.agents/skills/test-pruner/SKILL.md +++ b/.agents/skills/test-pruner/SKILL.md @@ -1,65 +1,75 @@ --- name: test-pruner -description: Prune, strengthen, and consolidate tests using oracle classification and cargo-mutants evidence, for one Rust crate or module or for a pull request's diff. Use when the user asks to remove redundant tests, dedupe or consolidate tests with rstest or proptest, audit test value, clean up AI-written tests, or get a mutation-backed review of a pull request's tests. For a quick oracle-only check of a pull request's tests, use `test-oracle-reviewer` instead. +description: Prune, strengthen, and consolidate tests using oracle classification and mutation evidence, cargo-mutants for Rust and Stryker.NET for C#, for one crate, project, or module or for one change's diff. Use when asked to remove redundant tests, consolidate tests into cases or properties, audit test value, clean up AI-written tests, finalize a change's tests before its pull request opens, or get a mutation-backed review of a pull request's tests. For a quick oracle-only check, use `test-oracle-reviewer` instead. --- # Test pruner -Scope each run to one crate or module, or to one pull request's diff, so mutation runs stay affordable and results stay reviewable. +Scope each run to one crate, project, or module, or to one change's diff, so mutation runs stay affordable and results stay reviewable. Decide from two independent measurements: whether the expected value is justified independently of the code, and which faults only this test catches. Mutation testing alone cannot tell a test that matches itself from a correct one, and oracle review alone cannot tell which tests are the sole guard for a fault. +Tool mechanics: `references/cargo-mutants.md` for Rust, `references/stryker.md` for C#; load only the language in scope. + +Editing mode is for tests you own, such as a change's diff before its pull request opens: commit the changes yourself and hand the evidence tables (oracle table, kill matrix, challenge results, survivors, and found bugs) to whoever posts them as a pull request comment. +Review mode is for another's pull request: post a review and change nothing in its branch, applying mutants and challenge patches only in the clone below. +Run mutants in a clone of their own and address its files by absolute path, since a relative path resolves against whichever checkout the shell starts in, and an applied mutant left in a shared checkout corrupts every other run there. ## Measure Invoke `test-oracle-reviewer` on every test in scope and record independent, self-derived, or unknown with evidence. -Run cargo-mutants on the scoped source as described in `references/mutants.md`, adding domain mutants from its catalogs because generic mutants miss most domain faults. -Build the kill matrix of mutant by test from the cargo-mutants logs; it needs no per-test reruns. +Run the mutation tool as the reference describes, adding domain mutants from `references/domain-mutants.md` because generic mutants miss most domain faults. +Build the kill matrix of mutant by test from the tool's outputs; only the cases the reference names need per-test reruns. +A unique kill is a mutant caught by exactly one test in scope. +Make no deletion decision about a test until some mutant of the code it exercises is caught, by it or another test, since zero unique kills over nothing caught measures nothing and the invariant then passes trivially; challenge such a test instead. For tests with several assertions, record which assertion fails for each kill. Run the scoped suite repeatedly on the clean tree, for example 15 times, to find flaky tests before trusting any kill. ## Decide -Keep independent tests; merge them into rstest cases only when they check the same property and differ only in data. +Keep independent tests; merge them into cases of one parametrized test only when they check the same property and differ only in data. Never delete an independent test on mutant evidence: the mutant sample is tiny next to the real fault space, and a spec vector costs nothing to keep. -Strengthen a self-derived test with unique kills by replacing its expected value with an independent one; if none exists, keep it and mark it `// oracle: self-derived; sole guard for `. -A strengthened test that fails is a finding, never a task to make it pass. +Strengthen a self-derived test with unique kills by replacing its expected value with an independent one; if none exists, keep it with a `mutant` marker, as `test-oracle-reviewer` defines, naming each fault it alone catches in words, since mutant ids and line numbers change between runs. +A strengthened or gap test that fails is a finding, never a task to make it pass: report the test, the independent expected value and its source, and the code's actual output to whoever assigned the run in editing mode, or to the author in review mode, and change neither test nor code meanwhile, since whichever is edited first becomes the specification. Delete a self-derived test with no unique kills. A test of unknown oracle with unique kills has already passed the challenge; strengthen it. -Challenge a test of unknown oracle without unique kills. +Challenge a test of unknown oracle without unique kills, unless it is protected below, since its fate does not depend on the answer. +No `scaffold` marker outlives the pass: resolve each as `test-oracle-reviewer` requires, sending its questions to whoever assigned the run, or to the author in review mode. Deletion is a set decision, not a per-test verdict. Two tests that both catch mutant M each have zero unique kills, and deleting both loses M. Choose the deletion set so the caught mutants afterward equal the caught mutants before. The same rule applies to assertions: a self-derived one that never fails across its test's kills is a deletion candidate. -Never propose deleting a regression test tied to an issue or fuzz crash, a test of untrusted-input parsing or bounds checks, or a doctest; strengthen or merge those only. -Treat a suite that deliberately uses snapshots as a change detector: dedupe it, do not delete it. +Protected tests are never proposed for deletion: a regression test tied to an issue or fuzz crash, a test of untrusted-input parsing or bounds checks, and a doctest. +That holds whatever their marker, because a refactor of a parser is where a dropped bounds check goes unnoticed. +Treat a suite that deliberately uses snapshots as a change detector as protected: dedupe it, do not delete it. Report flaky tests separately; deleting one hides a bug. ## Challenge Give a fresh-context agent the test, the code under test, and the mutants the rest of the suite already catches, and ask for one mutant that this test catches and the others miss, as a patch only. Apply and run the patch yourself; arguments do not count. -Add any mutant it finds to the domain catalog regardless of the verdict. +Record any mutant it finds in the evidence regardless of the verdict, and propose a reusable one for `references/domain-mutants.md` in a separate pull request, since neither mode edits skills. If it finds none, ask the human who wrote or prompted the test for the source of its expected value, and accept a source you can check, never a confirmation. -Without one, delete the test, or hand a protected test to the file's CODEOWNERS. +Without one, delete the test. ## Consolidate -Keep rstest case names readable, with `#[case::name]` or enum-valued `#[values]`. -Use proptest only for a real property; an assertion that re-implements the unit is a self-derived test with random inputs. +Consolidate into the case and property forms `test-writer` gives for the language, naming every case, since the kill matrix reports each case by its name. +Use a property test only for a real property; an assertion that re-implements the unit is a self-derived test with random inputs. Round-trip passes symmetric bugs, so every codec module keeps spec or capture vectors. -Do not add a never-panics property where a fuzz target already covers it. +Do not add a never-panics or never-throws property where a fuzz target already covers it. ## Invariant and output The caught-mutant set never shrinks except through deletions a human approves in a pull request. -Rerun the caught set after every deletion set and every consolidation, verify by rerunning rather than reasoning, and revert on any loss. -Triage surviving mutants as equivalent, coverage gap, or bug lead; a mutant that survives because its code is unreachable is a bug lead. +Rerun the caught set after every deletion set and every consolidation, verify by rerunning rather than reasoning, and revert on any loss; compare runs made with identical tool settings over the same scope. +After ruling out the scoping errors and tool artifacts the reference names, triage each survivor as equivalent (no observable change), unconstrained (an observable change no independent requirement decides, such as a log message's wording), coverage gap (a change an independent requirement rejects), or bug lead (the original code looks wrong, or is unreachable in every supported configuration). +Close each coverage gap with a test at the seam `tdd` defines, marked with the requirement that rejects the change and written with `test-writer`; a survivor only a test of internals could catch is not a gap but equivalent or a bug lead, as above. +A suite of independent tests yields few deletions; a pass may end in consolidations and gap tests only, or in no change. -In crate mode, open separate pull requests for deletions, consolidations, and strengthenings, or separate commits when amending an open pull request; they are different reviews. -Put the oracle table, kill matrix, challenge results, survivors, and found bugs in a pull request comment; the pull request body is the squash commit. +In editing mode, commit deletions, consolidations, strengthenings, and gap tests separately, each as soon as it is verified, so an interrupted run loses little; they are different reviews. Change only tests and source comments; never modify non-test source otherwise. -In pull request mode, post one review instead: inline comments on the affected tests, with the tables in `
` blocks. -Deletions, consolidations, strengthenings, and small non-test changes that enable an independent oracle become requests to the author, with the current caught set as the bar. +In review mode, post one review: inline comments on the affected tests, with the tables in `
` blocks. +Deletions, consolidations, strengthenings, gap tests, and small non-test changes that enable an independent oracle become requests to the author, with the current caught set as the bar. diff --git a/.agents/skills/test-pruner/references/mutants.md b/.agents/skills/test-pruner/references/cargo-mutants.md similarity index 72% rename from .agents/skills/test-pruner/references/mutants.md rename to .agents/skills/test-pruner/references/cargo-mutants.md index 884e2ba2c8..bde8e62eb3 100644 --- a/.agents/skills/test-pruner/references/mutants.md +++ b/.agents/skills/test-pruner/references/cargo-mutants.md @@ -1,14 +1,12 @@ # cargo-mutants mechanics -cargo-mutants reports whether the suite caught each mutant. -It has no user-defined mutation operators and keeps per-test results only in its logs, so both are built on its outputs. +cargo-mutants keeps per-test results only in its logs, so the kill matrix is parsed from them. These notes target cargo-mutants 27.1. ## Run Scope with `--file '/src//**'` for a crate or module, `--re '|'` for a pull request's changed functions, or `--in-diff ` for its whole diff; build time dominates, so scope first. A test-only diff yields no mutants under `--in-diff`, so scope with `--re` to the production functions the changed tests call, including unchanged ones. -Make no deletion decision on an empty or near-empty viable sample; with nothing to catch, every test has zero unique kills and the invariant passes trivially. Package directories vary; take the path from `cargo metadata` rather than assuming `crates/`. ```sh @@ -56,49 +54,9 @@ $expected = Select-String -Path -Pattern '^test (\S+)(?: - shoul } ``` -A unique kill is a mutant caught by exactly one test in scope. For assertion-level evidence, read the `panicked at :` location of each failure; an assertion whose line never appears across its test's kills catches nothing the others miss. ## Rerun -Rerun with the same scope and flags after each change. Run the scoped tests on the clean tree first; with `--baseline skip`, a suite broken by a test edit marks every mutant caught. Any mutant that moved from caught to missed without an approved deletion explaining it means revert. - -## Domain mutants - -Write each as a small patch; apply it, run the scoped tests, revert it with `git apply -R`; caught means any test failed. -Patches are pull request evidence, not repository files. - -Codec catalog: - -- Swap endianness on one field read or write. -- Length prefix off by one in either direction. -- Remove one bounds check or remaining-length test. -- Wrong padding count, or padding read but not skipped. -- Swap two adjacent fields of the same type. -- Flip one bit in a flags field. -- Write one byte fewer than `size()` reports. -- Return `Ok` on a malformed-input branch that returns `Err`. -- Accept a reserved or out-of-range enum value. - -Async and resource catalog: - -- Remove or shorten a deadline. -- Detach a task instead of aborting it, or hold a `JoinHandle` where an abort-on-drop handle is required. -- Return early before a drain or flush. -- Swallow an error as `Ok`. -- Skip recording a state flag. -- Swap two status or close codes. - -Decision and validation catalog, for each security or policy decision: remove it, weaken it, key it on the wrong identity, or apply it to the wrong object. - -- Always return `Ok` from the check. -- Accept any candidate by dropping a filter. -- Look up by the wrong key, such as a name hash instead of a thumbprint. -- Stop after the first candidate. -- Ignore one input source, such as caller-provided roots. -- Skip verification of a candidate's chain. -- Swap error precedence or report the wrong error. - -Add every mutant the challenger produces that the rest of the suite misses. diff --git a/.agents/skills/test-pruner/references/domain-mutants.md b/.agents/skills/test-pruner/references/domain-mutants.md new file mode 100644 index 0000000000..63a6abf1d1 --- /dev/null +++ b/.agents/skills/test-pruner/references/domain-mutants.md @@ -0,0 +1,37 @@ +# Domain mutants + +Neither mutation tool takes user-defined operators, so write each domain mutant as a patch. +Take it, or rebuild a challenger's patch, with `git diff --output=` from an edit you then revert, since a hand-written hunk header is easily wrong and a Windows PowerShell redirect re-encodes the patch as UTF-16 with CRLF, which `git apply` rejects. +`git apply` it, run the scoped tests, and `git apply -R` it; caught means a test failed, and a patch that does not build proves nothing until it does. +Patches are pull request evidence, not repository files. + +Codec catalog: + +- Swap endianness on one field read or write. +- Length prefix off by one in either direction. +- Remove one bounds check or remaining-length test. +- Wrong padding count, or padding read but not skipped; accept non-zero pad bits. +- Swap two adjacent fields of the same type. +- Flip one bit in a flags field. +- Write one byte fewer than the size computation reports. +- Return success (`Ok`, `true`) on a malformed-input branch that returns an error or throws. +- Accept a reserved or out-of-range enum value. + +Async and resource catalog: + +- Remove or shorten a deadline. +- Detach a task instead of aborting it, or hold a handle that neither aborts on drop nor cancels on dispose where one is required. +- Return early before a drain or flush. +- Swallow an error as success. +- Skip recording a state flag. +- Swap two status or close codes. + +Decision and validation catalog, for each security or policy decision: remove it, weaken it, key it on the wrong identity, or apply it to the wrong object. + +- Always return success from the check. +- Accept any candidate by dropping a filter. +- Look up by the wrong key, such as a name hash instead of a thumbprint. +- Stop after the first candidate. +- Ignore one input source, such as caller-provided roots. +- Skip verification of a candidate's chain. +- Swap error precedence or report the wrong error. diff --git a/.agents/skills/test-pruner/references/stryker.md b/.agents/skills/test-pruner/references/stryker.md new file mode 100644 index 0000000000..13bb6ef2c3 --- /dev/null +++ b/.agents/skills/test-pruner/references/stryker.md @@ -0,0 +1,69 @@ +# Stryker.NET mechanics + +Stryker.NET lists the tests that killed each mutant in its JSON report, but only when bail is disabled, so the kill matrix is read from the report. +These notes target Stryker.NET 5.0.0 with xUnit v2 on the VsTest runner. + +## Install + +In the solution directory, `dotnet new tool-manifest` then `dotnet tool install dotnet-stryker --version 5.0.0`; the manifest it writes is a repository file, not output, so leave it untracked unless the pull request adopts it. + +## Run + +Run the clean suite first with a TRX log, because the report names tests by id and the TRX carries the id with the display name: + +```powershell +dotnet test .csproj --logger "trx;LogFileName=clean.trx" --results-directory /trx +``` + +Confirm the TRX lists the tests you expect before trusting any result, because `dotnet test` passes when it discovers none, as with a wrong project or filter. + +Then, from the test project directory: + +```powershell +dotnet stryker --mutate "**//*.cs" --disable-bail --break-on-initial-test-failure --reporter json --reporter cleartext --output --skip-version-check +``` + +`--mutate` takes several globs, `!` excludes, and `.cs{..}` limits to a character span; Stryker mutates the whole project in memory before filtering, so compile-error warnings about files outside the scope are noise. +`--disable-bail` runs every covering test to completion; by default Stryker aborts a mutant's run at the first failure, and `killedBy` then holds only the tests that had failed by then, a partial list that makes kills look unique. +`--break-on-initial-test-failure` stops the run when the clean suite fails, which a test edit can cause, because a matrix built over failing tests proves nothing. +For a change's diff, `--since:` selects its mutants; a revision expression such as `HEAD~1` or an abbreviated SHA is rejected. +A test-only diff still yields mutants under `--since`: every mutant covered by a test in a changed test file counts as changed. +Pass `--project .csproj` only when the test project references more than one project, and `--solution .sln` when it targets .NET Framework, where Stryker requires it. +Coverage analysis, `perTest` by default, runs only the tests covering each mutant, so the matrix spans the whole test project without scoping tests and a unique kill is unique across the project; restricting to a test class needs `test-case-filter` in `stryker-config.json`, since the 5.0.0 command line rejects it as a flag. +Stryker builds the test project itself and writes a `.gitignore` of `*` into `--output`, so the output stays untracked wherever it is. +Build and the initial run take a few minutes before the first mutant, and a whole library of a few thousand lines can run for most of an hour, so scope first; timeouts are computed per mutant from the initial run, and `additional-timeout` and `timeout-ratio` are config-file options. + +## Kill matrix + +`/reports/mutation-report.json` holds `files[].mutants[]` with `id`, `mutatorName`, `replacement`, `location`, `status` (`Killed`, `Survived`, `NoCoverage`, `Timeout`, `CompileError`, `Ignored`), and `coveredBy` and `killedBy` lists of test ids, and `testFiles[].tests[]` with `id` and `name`. +`name` is the method's qualified name without arguments, so every row of a `[Theory]` shares one name; `id` is the VsTest test case id, which the TRX carries as `UnitTest/@id` with the display name and arguments. +Key on the id throughout: display names collide when xUnit truncates long string arguments. + +```powershell +$report = Get-Content -Raw /reports/mutation-report.json | ConvertFrom-Json +[xml]$trx = Get-Content -Raw /trx/clean.trx +$name = @{} +foreach ($t in $trx.TestRun.TestDefinitions.UnitTest) { $name[$t.id] = $t.name } +$report.files.PSObject.Properties | Where-Object Name -like '**' | ForEach-Object { + foreach ($m in $_.Value.mutants | Where-Object status -eq 'Killed') { + foreach ($id in $m.killedBy) { "$($m.id)`tL$($m.location.start.line) $($m.mutatorName): $($m.replacement)`t$id`t$($name[$id])" } + } +} +``` + +Read kills only from `killedBy`; `coveredBy` lists the tests that ran against a mutant, not those that failed. +The script lists `Killed` mutants only; attribute a `Timeout` by applying it by hand, as below, and running its `coveredBy` tests one at a time before counting it caught. +One row with an argument xUnit cannot serialize makes the whole theory one test id, so the matrix cannot tell its rows apart; `test-writer`'s C# forms avoid this. +Stryker keeps no per-test output, so for assertion-level evidence apply the mutant by hand from `location` and `replacement`, run the killing test with `dotnet test --filter "FullyQualifiedName~."`, and read the failing assertion from the stack trace. +Mutant ids are assigned in order over the whole project, so an edit anywhere can renumber them, and lines shift with edits above; across runs match mutants by file, `mutatorName`, `replacement`, and the text of the mutated line in `files[].source`, and mutants sharing all four by their order in the file, which test and comment edits leave unchanged. + +## Rerun + +Refresh the clean run and its TRX before each rerun. +For a change's diff, measure and rerun with `--mutate` over the files a `--since` run mutated, never comparing `--since` runs: a test edit changes which mutants `--since` selects, so a lost kill drops out instead of surviving. +Do not use `--with-baseline`; it reuses results for mutants it considers unchanged, the opposite of a rerun. +Identical settings include `mutation-level` and any `test-case-filter`, since either changes the mutant population or the tests that can kill it. + +## Pitfalls + +Verify a surprising status by applying the mutant by hand: Stryker can report as survived a mutant in a static initializer, whose value is computed before the mutant switches on, or a replacement that would not compile on its own, and a machine at full load turns slow kills into timeouts. diff --git a/.agents/skills/test-reviewer/SKILL.md b/.agents/skills/test-reviewer/SKILL.md index ef09b4f529..3b45616e5b 100644 --- a/.agents/skills/test-reviewer/SKILL.md +++ b/.agents/skills/test-reviewer/SKILL.md @@ -1,6 +1,6 @@ --- name: test-reviewer -description: Review changed Rust tests against the `test-writer` rules, including a named bug, an independent oracle, non-branching `rstest` bodies, checked snapshots, and properties under `proptest`. Use when a diff or pull request adds or changes Rust tests, or when asked to review test organization or parametrization. +description: Review changed Rust or C# tests against the `test-writer` rules, including a named bug, an independent oracle, non-branching parametrized bodies, checked snapshots, and real properties. Use when a diff or pull request adds or changes tests, or when asked to review test organization or parametrization. --- # Test reviewer @@ -9,7 +9,7 @@ Invoke `test-writer` and use its rules as review criteria without following its Review only changed tests unless the user requests a broader audit. Run `test-oracle-reviewer` on each changed test; this skill covers a test's shape, not where its expected values come from. When a test's name and assertions do not say which bug it catches, ask the author rather than guess. -When the change touches an invariant from an applicable intent or contract file that no test checks, ask what pins it and whether `proptest` could exercise it across generated inputs. -A new or changed `expect-test` snapshot is a behavior claim; check that the pull request intends it and that the value is right. +When the change touches an invariant from an applicable intent or contract file that no test checks, ask what pins it and whether a property test could exercise it across generated inputs. +A new or changed snapshot is a behavior claim; check that the pull request intends it and that the value is right. Report each violation with its exact location and a concrete correction, and group repeated instances that share one correction. -Return no findings when the changed tests already comply. \ No newline at end of file +Return no findings when the changed tests already comply. diff --git a/.agents/skills/test-writer/SKILL.md b/.agents/skills/test-writer/SKILL.md index b255c7953c..5d068d2f88 100644 --- a/.agents/skills/test-writer/SKILL.md +++ b/.agents/skills/test-writer/SKILL.md @@ -1,31 +1,35 @@ --- name: test-writer -description: Design Rust tests that catch bugs, choosing between an example test, `rstest` cases, an `expect-test` snapshot, a `proptest` property, and a fuzz target. Use whenever adding or changing Rust unit, integration, snapshot, or property tests. +description: Design tests that catch bugs, choosing between an example test, parametrized cases, a snapshot, a property, and a fuzz target, in Rust (`rstest`, `expect-test`, `proptest`) or C# (xUnit `[Theory]`, CsCheck). Use whenever adding or changing unit, integration, snapshot, or property tests in Rust or C#. --- # Test writer First name the bug the test catches that existing tests miss; without one it costs runtime and maintenance and guards nothing. Take the expected value from a source independent of the code under test, as `test-oracle-reviewer` defines; a value derived from the implementation detects change, not bugs. +Mark that source as `test-oracle-reviewer` defines, and assert only what it decides, because an assertion past it pins how the code works today; add one only for feedback while implementing, marked `scaffold`, and never in place of a decision `tdd` says to ask for. +Test at the seam `tdd` defines, the public boundary where the behavior is observed. +A test that fails while the code still does what its marked source requires asserts past that source: narrow it to what the source decides, or delete it unless `test-pruner` protects it; a snapshot that fails only on representation is re-read and updated instead. Reach nothing beyond loopback, because an external host makes a failure unreproducible. +Each rule here takes a form per language: `references/rust.md` for Rust, `references/csharp.md` for C#; load only the language in scope. ## Properties Look for a property before writing example tests, because a property holds across generated inputs while examples show only the cases someone thought of. Candidates are a round-trip through encode and decode, a size computation matching the bytes written, and an invariant from the applicable intent or contract file that speaks of inputs or state, such as one that must survive any sequence of operations. -Test a property with `proptest`, reusing the crate's existing strategies and adding new ones beside them, so each type's generator is written once. -With no property that must hold across many inputs, write example tests; a `proptest` whose assertion re-implements the code under test checks the code against itself. -A parser of untrusted input gets a fuzz target in `fuzz/` rather than a never-panics `proptest`, because the fuzzer is coverage-guided and keeps a corpus, which a few hundred random cases are not. +Reuse the existing generator for each type and add new ones beside it, so each type's generator is written once. +With no property that must hold across many inputs, write example tests; a property whose assertion re-implements the code under test checks the code against itself. +A parser of untrusted input gets a fuzz target rather than a never-panics property, because the fuzzer is coverage-guided and keeps a corpus, which a few hundred random cases are not; where no fuzz harness exists to add one to, a property over malformed inputs stands in, and a pull request comment says the parser is not fuzzed. ## Snapshots -Snapshot structured values such as a parsed config, a decoded message, or a CLI error with `expect-test`; the expected value sits beside the assertion and `UPDATE_EXPECT=1` rewrites every stale one after a representation change. -Taking or updating a snapshot is the oracle, so read and check it, because `UPDATE_EXPECT=1` accepts whatever the code produced; when a spec or capture defines the value, assert that instead. -Move a snapshot or blob longer than a screen into a sibling file through `expect_file!` or `include_bytes!`, so the source shows the test rather than its data; keep short byte vectors inline so each field can carry its comment. +Snapshot structured values such as a parsed config or a decoded message, with the expected value beside the assertion. +Taking or updating a snapshot is the oracle, so read and check it, because its value is whatever the code produced; when a spec or capture defines the value, assert that instead. +Move a snapshot or blob longer than a screen into a sibling file, so the source shows the test rather than its data; keep short byte vectors inline so each field can carry its comment. ## Examples and cases -Parametrize with `rstest` only when every case checks the same behavior with different data, and name each case with `#[case::name]` so a failure names the case, not an index. -A body that branches on its parameters, through an `if`, a `match`, or an optional expected value, is several tests behind one name, so a failure reports a case instead of the behavior that broke; write one test per behavior and share setup through a helper. -A `#[values]` cross product is right only when every combination is a real scenario; one that needs an `if` to skip or adjust a combination is that branching. -Keep each regression test standalone and named for its issue, because its setup reproduces the bug's exact conditions and a case table would generalize them away. \ No newline at end of file +Parametrize only when every case checks the same behavior with different data, and name each case so a failure names the case, not an index. +A body that branches on its parameters, through a conditional or an optional expected value, is several tests behind one name, so a failure reports a case instead of the behavior that broke; write one test per behavior and share setup through a helper. +A cross product of parameters is right only when every combination is a real scenario; one that needs a conditional to skip or adjust a combination is that branching. +Keep each regression test standalone and named for its issue, because its setup reproduces the bug's exact conditions and a case table would generalize them away. diff --git a/.agents/skills/test-writer/references/csharp.md b/.agents/skills/test-writer/references/csharp.md new file mode 100644 index 0000000000..3d19522d8a --- /dev/null +++ b/.agents/skills/test-writer/references/csharp.md @@ -0,0 +1,16 @@ +# C# forms + +The repository's C# tests are xUnit v2; each rule in `SKILL.md` takes these forms there. + +Properties use CsCheck: compose `Gen` with LINQ and `Sample` it; each type's generator lives in one static class of the test project. +CsCheck rather than FsCheck, because its generators shrink without per-type shrinkers, so a composed generator costs one declaration, and it adds no FSharp.Core to a C# test project. +Releases after CsCheck 2.14.1 need .NET 6 or later, so a .NET Framework test project pins 2.14.1, the last on .NET Standard 2.0. +A failing `Sample` prints a seed; replay it with `seed:` or the `CsCheck_Seed` environment variable, and pin the failure by keeping the shrunk value in a regression test rather than the seed, since a generator change moves the seed's meaning. + +Snapshots are written by hand: the expected value inline or, past a screen, in a sibling file copied into the test output. +Nothing rewrites a stale value, so edit each by hand after reading it. + +Cases are `[Theory]` with `[InlineData]` rows for literal data, whose display name carries the arguments, so a failure names the row. +Other data goes through `[MemberData(nameof())]` whose rows hold only values xUnit can serialize, such as a string key the test resolves to its data, because one row it cannot serialize turns the whole theory into one test case that no filter or mutation report can split. +A cross product is a `[MemberData]` member that yields it. +A regression test is a separate `[Fact]`. diff --git a/.agents/skills/test-writer/references/rust.md b/.agents/skills/test-writer/references/rust.md new file mode 100644 index 0000000000..d259c41880 --- /dev/null +++ b/.agents/skills/test-writer/references/rust.md @@ -0,0 +1,14 @@ +# Rust forms + +Each rule in `SKILL.md` takes these forms in Rust. + +In a doctest, write the marker as a hidden line, `# // oracle: ...`, so it stays out of the rendered documentation. + +Properties use `proptest`, whose generators are the crate's strategies. + +Snapshots use `expect-test`, whose `UPDATE_EXPECT=1` rewrites every stale value after a representation change. +Use `expect_file!` for a snapshot in a sibling file and `include_bytes!` for a blob. + +Cases are `rstest`, each named with `#[case::name]`. +A cross product is enum-valued `#[values]`, so each combination's name says what it checks. +A regression test is not an `rstest` case. diff --git a/.github/skills/code-review/SKILL.md b/.github/skills/code-review/SKILL.md index 5c04aee74f..7dec1e9d8b 100644 --- a/.github/skills/code-review/SKILL.md +++ b/.github/skills/code-review/SKILL.md @@ -13,7 +13,7 @@ Inspect the diff, its stated goal, and repository guidance before selecting cond - Run `contract-review` when the diff changes a `CONTRACT*.md`. - Run `code-compressor` for every diff, limiting it to changed code. - Run `rust-style-reviewer` when Rust code changes, limiting it to changed lines. -- Run `test-reviewer` when Rust tests change; it runs `test-oracle-reviewer` itself, so do not add that pass separately. +- Run `test-reviewer` when Rust or C# tests change; it runs `test-oracle-reviewer` itself, so do not add that pass separately. - Run `dependency-reviewer` when a manifest or lockfile changes, limiting it to dependencies and features the diff adds or widens. - Run `docs-compressor` when documentation, rustdoc, explanatory comments, or instructional prose changes. - Run `prose-verifier` when hand-maintained prose changes in a format where source line breaks are stylistic.