Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 44 additions & 0 deletions ai-plans/0066-0-resolve-first-stryker-survivor-queue.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
# Resolve the First Stryker Survivor Queue

## Rationale

The first mutation-testing triage pass covered the two smallest scoped runs, `Light.PortableResults.Validation.OpenApi` and `Light.PortableResults.AspNetCore.Mvc`, and produced twenty survivors. Twelve of them are one gap — the `target`-provided branch of the typed validation OpenAPI helpers is never exercised — and that branch calls `WithErrorExample` with a `null` message, which is the symptom described in issue #57. Testing it now would encode the current behavior as the expected contract, so it is deferred and resolved together with that bug.

The remaining eight are independent and are the subject of this plan. Two are false survivors that expose a blind spot the tooling documentation does not yet describe, three are not killable and need justified suppression, and three are genuine guard-clause gaps of exactly the kind `tests/AGENTS.md` already asks for solitary tests. Resolving them leaves a survivor queue whose every remaining entry has a recorded reason, which is the state that makes future runs comparable.

## Acceptance Criteria

- [x] `tests/AGENTS.md` documents mutants in static initializers and static constructors as a blind spot, with the measured evidence and the reason the reused test host cannot kill them, so a future reader does not re-triage the same two mutants.
- [x] The two unreachable switch arms in `RegisterBuiltInValidationErrors` and the redundant `builder` guard in `ProducesPortableValidationProblemFor` carry narrow `Stryker disable once` comments with justifications; no global `ignore-mutations` setting is used.
- [x] Tests cover the null-builder guards of both typed validation OpenAPI helper families and the null-context guard of `BaseLightActionResult.ExecuteResultAsync`, asserting `ArgumentNullException`.
- [x] A re-run of both projects reports no survivor other than the twelve deferred to #57 and the two known-false static-initializer mutants; `AspNetCore.Mvc` reports zero survivors.
- [x] The baseline table in `tests/AGENTS.md` is replaced for both projects with full count vectors measured at the resulting commit, keeping the provenance the existing table records.
- [x] `dotnet test Light.PortableResults.slnx` passes, and production code changes are limited to suppression comments — no behavior changes, no CI changes, no mutation score thresholds.

## Technical Details

### Suppressions

Three mutants cannot be killed by any test that respects the contract, so they are suppressed at the source with the reason recorded in the report:

- `BuiltInValidationErrorContractRegistrationExtensions.cs`, the `ErrorMetadataTypeContract` arm — `BuiltInValidationErrorContracts.Contracts` is this method's only data source and contains schema and no-metadata contracts exclusively. The arm stays: it is correct for a registry that later gains a type contract, and removing it to satisfy the tool would be the restructuring `tests/AGENTS.md` forbids.
- The same method's `default:` arm message — `ErrorMetadataContract` declares a `private protected` constructor and has exactly three sealed subclasses, so no fourth kind can exist and the arm is unreachable by construction.
- `PortableValidationOpenApiRouteHandlerBuilderExtensions.cs`, the `ArgumentNullException.ThrowIfNull(builder)` guard — the delegated `ProducesPortableValidationProblem` guards the same parameter, so the observable contract is identical with or without it. The existing `ProducesPortableValidationProblemFor_ShouldRejectNullBuilder` test passes either way, which is why it did not kill the mutant.

Match the mutator to the suppression (`Statement` for the two statement mutants, `String` for the message) rather than disabling all mutators at the site.

### Guard clause tests

The `EnsureBuilder` overloads in `BuiltInValidationErrorBuilderExtensions` are reached by all thirty-six public typed helpers, so one test per builder family is sufficient: one helper on `PortableProblemOpenApiBuilder` and one on `PortableValidationProblemOpenApiBuilder`, each invoked on a null builder. Without the guard these produce `NullReferenceException` instead of the documented `ArgumentNullException`, which is what makes the mutants killable. `ValidationOpenApiExtensionGuardTests` is the established home for these.

`BaseLightActionResult.ExecuteResultAsync` needs the equivalent for a null `ActionContext`, reached through the sealed `LightActionResult`. The MVC test project currently has no unit-level test class; the integration tests exercise the app end to end and cannot reach this guard.

### Blind spot: static initializers

Applying the two false survivors to the source by hand fails the suite — emptying `BuiltInValidationErrorContracts.Contracts` fails 46 of 112 tests in `Light.PortableResults.Validation.OpenApi.Tests`, and blanking the built-in schema id string fails 52 of 112 — while Stryker reports both as survived. Both mutants sit in code reachable only during static initialization of a static property, which a reused test host runs once per process, before or independently of mutant activation.

Document this next to the existing Safe Mode entry, in the same shape: what the tool reports, why, and what a reader must do instead. The consequence for triage is that a survivor in a static initializer, static constructor, or a helper called only from one is verified by applying it by hand and running the affected test project, not by reading the report.

### Re-measurement

Both projects are re-run at the end to produce the recorded baseline; together they take roughly four minutes. The suppressed mutants move from `Survived` to `Ignored`, which changes the `Ignored` count that `tests/AGENTS.md` currently explains as block-removal filtering alone — that explanation needs to account for user suppressions once they exist.
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,9 @@ this ErrorMetadataContractsBuilder builder
switch (contract)
{
case ErrorMetadataTypeContract typeContract:
// The built-in registry holds schema and no-metadata contracts exclusively, so this arm is
// unreachable today. It stays correct for a registry that later gains a type contract.
// Stryker disable once Statement : unreachable - the built-in registry never holds a type contract
builder.ForCode(code, typeContract.MetadataType);
break;
case ErrorMetadataSchemaContract schemaContract:
Expand All @@ -33,6 +36,9 @@ this ErrorMetadataContractsBuilder builder
builder.ForCode(code);
break;
default:
// ErrorMetadataContract declares a private protected constructor and has exactly three sealed
// subclasses, so no fourth contract kind can exist and this arm cannot be reached.
// Stryker disable once String : unreachable - ErrorMetadataContract permits no fourth subclass
throw new InvalidOperationException(
$"The error metadata contract '{contract.GetType().FullName}' is not supported."
);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,9 @@ public static RouteHandlerBuilder ProducesPortableValidationProblemFor<TValidato
)
where TValidator : IPortableValidationOpenApiContract
{
// The delegated ProducesPortableValidationProblem guards the same parameter, so the observable contract is
// identical with or without this guard.
// Stryker disable once Statement : equivalent - ProducesPortableValidationProblem guards the same parameter
ArgumentNullException.ThrowIfNull(builder);

return builder.ProducesPortableValidationProblem(
Expand Down
25 changes: 14 additions & 11 deletions tests/AGENTS.md
Original file line number Diff line number Diff line change
Expand Up @@ -37,13 +37,13 @@ Revisit `coverage-analysis: off` when Stryker's MTP per-test coverage support ([
dotnet stryker -p Light.PortableResults.AspNetCore.Shared.csproj

# One-file configuration smoke check (~4 minutes)
# Expect: 2,398 tests, 67 killed, 0 NoCoverage, 100.00%
# Expect: 2,401 tests, 67 killed, 0 NoCoverage, 100.00%
dotnet stryker -p Light.PortableResults.csproj -m '**/Result.cs'
```

The smoke-check vector is tied to the current `Result.cs` and its tests; update it after intentional changes alter the mutant inventory. All mutants surviving with a 0.00% score suggests fallback to the `vstest` runner, while any non-zero `NoCoverage` count suggests `coverage-analysis: off` was lost.

`-p` selects the mutated project, not the tests: Stryker runs every test project transitively referencing it (`AspNetCore.Shared` → 337 tests, `Light.PortableResults` → all 2,398). Cross-project kills are legitimate (sociable tests) and nearly free. Never pass `-tp` (it does not narrow tests) or `--since` (any non-C# file in the diff, e.g. an `ai-plans/` document, degrades it to a full run). Reports go to `StrykerOutput/<timestamp>/reports/` (gitignored): JSON for agents (filter `"status"` for both `"Survived"` and `"Timeout"`; investigate the timeout cause before survivor triage), HTML for humans.
`-p` selects the mutated project, not the tests: Stryker runs every test project transitively referencing it (`AspNetCore.Shared` → 337 tests, `Light.PortableResults` → all 2,401). Cross-project kills are legitimate (sociable tests) and nearly free. Never pass `-tp` (it does not narrow tests) or `--since` (any non-C# file in the diff, e.g. an `ai-plans/` document, degrades it to a full run). Reports go to `StrykerOutput/<timestamp>/reports/` (gitignored): JSON for agents (filter `"status"` for both `"Survived"` and `"Timeout"`; investigate the timeout cause before survivor triage), HTML for humans.

### Cost and baseline

Expand All @@ -60,18 +60,20 @@ The smoke-check vector is tied to the current `Result.cs` and its tests; update
| `AspNetCore.Mvc` | 33 | 11 |
| `AspNetCore.MinimalApis` | 32 | 11 |

Baseline for sources at commit `04aee20`, remeasured with `dotnet-stryker` 4.16.0, concurrency 8 and 30,000 ms additional timeout (both pinned in `stryker-config.json`), `Debug`, Apple M3 Max (16 logical cores):
Baselines carry per-row provenance, because rows are re-measured individually as survivors are triaged. A row measured as part of the change that produced it cites the issue rather than a hash the commit cannot contain; find it with `git log --grep "Closes #<n>"`. All runs used `dotnet-stryker` 4.16.0, concurrency 8 and 30,000 ms additional timeout (both pinned in `stryker-config.json`), `Debug`, Apple M3 Max (16 logical cores):

| Project | Tests run | Elapsed | Killed | Timeout | Survived | CompileError | Ignored | NoCoverage |
| --- | ---: | ---: | ---: | ---: | ---: | ---: | ---: | ---: |
| `AspNetCore.Mvc` | 102 | 0:53 | 16 | 0 | 1 | 11 | 5 | 0 |
| `AspNetCore.MinimalApis` | 237 | 1:22 | 16 | 0 | 0 | 11 | 5 | 0 |
| `AspNetCore.Shared` | 337 | 2:29 | 31 | 0 | 0 | 3 | 10 | 0 |
| `Validation.OpenApi` | 163 | 2:38 | 57 | 0 | 19 | 2 | 36 | 0 |
| Project | Provenance | Tests run | Elapsed | Killed | Timeout | Survived | CompileError | Ignored | NoCoverage |
| --- | --- | ---: | ---: | ---: | ---: | ---: | ---: | ---: | ---: |
| `AspNetCore.Mvc` | `#66` | 103 | 0:43 | 17 | 0 | 0 | 11 | 5 | 0 |
| `AspNetCore.MinimalApis` | `04aee20` | 237 | 1:22 | 16 | 0 | 0 | 11 | 5 | 0 |
| `AspNetCore.Shared` | `04aee20` | 337 | 2:29 | 31 | 0 | 0 | 3 | 10 | 0 |
| `Validation.OpenApi` | `#66` | 165 | 2:20 | 59 | 0 | 14 | 2 | 39 | 0 |

`Validation.OpenApi` was run twice consecutively with the 30,000 ms setting; both runs produced 57 killed, 0 timeout, 19 survived, and a 75.00% score, completing in 2:32 and 2:38. At 20,000 ms, one of two runs still timed out a mutant known to survive. At the 5,000 ms default, 21 mutants were reported as timeouts, masking most survivors and inflating the score to 98.68%.
Both `Validation.OpenApi` survivor groups are accounted for and neither is a missing test: twelve are the `target`-provided branch of the typed helpers, deferred to the bug in #57 so that tests are written against the corrected contract, and two are the known-false static-initializer mutants described in the blind spots below. Its 39 `Ignored` are 36 block-removal plus the three triage suppressions.

`Ignored` is not user suppression: all 56 baseline entries are `Block removal` mutants discarded deterministically by Stryker's built-in "block already covered" filter because another active mutant exists inside the block. A different reason or count should be investigated.
At the 5,000 ms default additional timeout, `Validation.OpenApi` reported 21 mutants as timeouts, masking most of these survivors and inflating the score to 98.68%. At 20,000 ms, one of two runs still timed out a mutant known to survive.

`Ignored` covers two distinct things, and the report carries the reason for each. Most entries are `Block removal` mutants discarded deterministically by Stryker's built-in "block already covered" filter because another active mutant exists inside the block. The rest are `Stryker disable once` suppressions from triage, which carry the justification written at the source. Check the reasons, not just the count: a `Block removal` count that moves without a source change should be investigated.

`NoCoverage` must be zero — a non-zero value means `coverage-analysis: off` is no longer taking effect. Compare full count vectors at equal concurrency, not percentages.

Expand All @@ -96,5 +98,6 @@ If a survivor can only be killed by asserting on something incidental — exact
Treat that list as observed, not fixed: any new `out`/`ref` code joins it silently. Stryker announces it as `[INF] Safe Mode! Stryker will remove all mutations in <method>` on the console only — no log file is written — and the discarded mutants are simply absent from the JSON report. The durable way to recover the current set is to filter the report for `"status": "CompileError"`; those sites are the only trace left, and their enclosing methods are the ones running blind.

When changing a method in that set, mutation score carries no information about it and line coverage only proves execution. Adequacy has to be argued by hand: enumerate the behaviors the method promises and point at the test constraining each one. State that reasoning in the pull request, because no tool in this repository can check it.
- Mutants reachable only during static initialization are reported as survived even when the suite kills them. Measured on `BuiltInValidationErrorContracts`: emptying the `Contracts` registry fails 46 of 112 tests and blanking the built-in schema id string fails 52 of 112, yet Stryker reported both as `Survived`. Both sites run only while a static property initializer executes, which a reused test host runs once per process, independently of mutant activation. Verify any survivor in a static initializer, a static constructor, or a helper called only from one by applying the mutation to the source by hand and running the affected test project — the report cannot settle it. Do not add tests for such a survivor before that check: the two above already had covering assertions.
- `Timeout` counts as killed. The pinned 30,000 ms additional timeout reduced `Validation.OpenApi` from 21 timeouts to zero in two consecutive concurrency-8 runs, but no finite value makes classification independent of hardware and load. Investigate any future timeout as either a genuine hang or insufficient headroom; do not assume it represents a killed mutant.
- The MTP runner is a preview (stryker-mutator/stryker-net#3094); verify surprising results against a plain `dotnet test` run.
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
using System;
using System.Threading.Tasks;
using FluentAssertions;
using Xunit;

namespace Light.PortableResults.AspNetCore.Mvc.Tests;

public sealed class LightActionResultGuardTests
{
[Fact]
public async Task ExecuteResultAsync_ShouldRejectNullContext()
{
var actionResult = new LightActionResult(Result.Ok());

var act = async () => await actionResult.ExecuteResultAsync(null!);

await act.Should().ThrowAsync<ArgumentNullException>();
}
}
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
using System;
using FluentAssertions;
using Light.PortableResults.AspNetCore.OpenApi;
using Light.PortableResults.AspNetCore.OpenApi.ErrorContracts;
using Microsoft.AspNetCore.Builder;
using Xunit;
Expand All @@ -24,4 +25,20 @@ public void ProducesPortableValidationProblemFor_ShouldRejectNullBuilder()

act.Should().Throw<ArgumentNullException>();
}

[Fact]
public void TypedProblemHelpers_ShouldRejectNullBuilder()
{
var act = static () => ((PortableProblemOpenApiBuilder) null!).WithEqualToError<int>();

act.Should().Throw<ArgumentNullException>();
}

[Fact]
public void TypedValidationProblemHelpers_ShouldRejectNullBuilder()
{
var act = static () => ((PortableValidationProblemOpenApiBuilder) null!).WithEqualToError<int>();

act.Should().Throw<ArgumentNullException>();
}
}
Loading