fix(cli): ask only the runs that covered the suite whether a test stopped - #202
Merged
Conversation
…pped `VanishedProvider` decided absence with one membership test — is this fingerprint in the current slice? — and had no notion of whether the sessions in that slice tried to run it. A `dotnet test --filter` run in the inner loop therefore produced one false `stopped running` per unselected test, and #191's Fisher gate did not fix that: it only helps where the filtered runs dominate the *baseline* too. The commonest shape still fired at full strength. Measured on a real store — seventeen full runs of `SampleApp.MSTest` then three `--filter` runs of one test — the report produced **twenty** `stopped running` findings, each reading "ran in 17 of 17 earlier runs" at p = 8.8e-4, which is the best any twenty-run window can do. It now produces none. Absence is only evidence in a session that asked. So each run is classified by how much of the suite it covered — its distinct tests against `PartialSessionShare = 0.50` of the largest run in the window — and the kinds that read absence set the small ones aside. The anchor is the largest run and deliberately not the median: four full runs and sixteen filtered ones, which is what ten minutes of an inner loop produces, has a median of one test and every run in it measures as typical against that. A report is scoped to exactly one assembly, so the runs being compared are always runs of the same suite. `SessionView.Tests`, not `QuickStatistics.distinctTests` as the issue proposes. `SessionAssemblies.Project` nulls that field for any multi-assembly session, so it is absent exactly on solution-wide runs; the count the CLI already derives from executions is the same number and needs no plumbing. A re-split rather than a filter of the window's own slices. The two differ on a store that interleaves full and filtered runs: dropping partial sessions out of a current slice of three filtered runs leaves nothing to ask about, where taking the three most recent full runs still finds a test that genuinely stopped. Both the "now" and the "before" are re-derived over the covering runs, which also fixes the baseline half — a test present in every run that asked for it no longer reads as a 5-of-17 occasional visitor. `AnalysisWindow.SliceSizeFor` is extracted so the re-split narrows on the same rule the window does and the two cannot drift. The trade, recorded deliberately rather than left to be discovered. A count cannot separate a filter from a deletion and no threshold makes it able to: nine tests missing from seventeen is the same table whether they were excluded or removed. The line is at a half, and it is biased towards silence — the kind is capped at `Severity.Low` because a disappearance is usually something the developer did on purpose a minute ago, whereas the false positive arrives once per unselected test on every filtered run for as long as it stays in the window. So a deletion removing more than half a suite is not reported, and a filter selecting more than half still produces false absences. Both are in the command reference and in known limitations. Not suppressed silently. `VanishedEvidence` publishes `partialSessionsSetAside`, the headline says "full runs" and a `set aside` metric gives the number where any were, and the summary carries `partialSessions` with a caveat line worded as an observation rather than a discount — only this kind sets them aside, because a filtered run's *outcomes* are as true as any other run's and it is only its silences that mean nothing. The schema moves to 1.13. Verified end to end: on a store of full runs only the envelope is identical apart from the schema version and the new field. Item 3 of the issue — populating `TestSession.TotalTestsExpected` — is deliberately not done here, and cannot be done as written. MSTest exposes no test count to any hook the adapter occupies; xUnit needs a `CreateDiscoverer` override and NUnit's `ITest.TestCaseCount` silently becomes post-filter under the adapter's `PreFilter` setting. And the field is modelled host-wide, so `SessionAssemblies.Project` nulls it before the CLI sees a session. A follow-up will file it as a per-assembly discovered count. Refs #140 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TMQcZLBGFTd8wz3mcfn9vc
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
There are small but user-visible/documentation issues in the updated text outputs/comments (notably the Vanished headline string missing “runs/full runs” in one clause) that should be corrected before approval.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR refines the CLI’s Vanished finding so “stopped running” is evaluated only against sessions that plausibly covered the full suite, preventing filtered runs (e.g., dotnet test --filter) from generating false disappearances. It introduces a window-relative “partial session” classification, re-slices Vanished over only full-suite runs, and updates the JSON contract and docs accordingly.
Changes:
- Classify sessions as “partial” based on distinct test count vs the window’s largest run, and surface partial-session counts in the report summary.
- Update
VanishedProviderto re-slice over full-suite runs only (setting partial sessions aside) and publishpartialSessionsSetAsideinVanishedEvidence. - Bump report schema to 1.13 and update tests + documentation to reflect the new output fields and wording.
File summaries
| File | Description |
|---|---|
| tests/Xping.Cli.Tests/Report/VanishedProviderTests.cs | Adds coverage for filtered runs, re-splitting behavior, and set-aside evidence/headline wording. |
| tests/Xping.Cli.Tests/Report/ShareableOutputTests.cs | Updates expected VanishedEvidence and SummaryDto shapes for the new fields. |
| tests/Xping.Cli.Tests/Report/SessionViewTests.cs | Adds tests pinning window-relative partial-session classification and boundary behavior. |
| tests/Xping.Cli.Tests/Report/ReportEnvelopeTests.cs | Adjusts seeded data and schema assertions for the new summary field/version. |
| tests/Xping.Cli.Tests/Commands/CliSurfaceTests.cs | Updates CLI schema version expectations to 1.13. |
| src/Xping.Cli/Report/Windowing/AnalysisWindow.cs | Extracts SliceSizeFor to share split sizing logic with re-slicing providers. |
| src/Xping.Cli/Report/Rendering/TextReportRenderer.cs | Emits a caveat line when partial sessions are present in the window. |
| src/Xping.Cli/Report/Providers/VanishedProvider.cs | Re-slices over full-suite sessions only; publishes set-aside count in evidence. |
| src/Xping.Cli/Report/LocalAnalysisConstants.cs | Introduces PartialSessionShare (0.50) and documents the tradeoff. |
| src/Xping.Cli/Report/Indexes/SessionView.cs | Adds IsPartial flag (window-derived) with documentation. |
| src/Xping.Cli/Report/Contract/ReportEnvelope.cs | Bumps schema to 1.13 and documents the contract change. |
| src/Xping.Cli/Report/Contract/EvidenceHeadline.cs | Updates Vanished headline/metrics to reflect “full runs” + set-aside metric. |
| src/Xping.Cli/Report/Contract/EnvelopeBuilder.cs | Adds partialSessions to the summary DTO construction. |
| src/Xping.Cli/Report/AnalysisContext.cs | Computes per-window partial-session classification and count. |
| docs/known-limitations.md | Documents the limitation/tradeoff around filters vs deletions and the 50% threshold. |
| docs/cli/command-reference.md | Updates Vanished documentation and JSON example for schema 1.13 + partialSessions. |
Review details
- Files reviewed: 16/16 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Collaborator
Author
|
Follow-ups filed, so the two things this PR deliberately leaves undone are tracked rather than lost:
With both tracked, |
Two findings from review. "absent from the last 3" was unambiguous only while every run in the window was one this kind counted. Once runs are set aside it is not: the last three runs and the last three *full* runs are different runs, and the first clause saying "earlier full runs" does not repair the second. A headline is read, and pasted, a clause at a time, so a clause that is only true given the one before it is a clause that will be quoted false. Both clauses carry the noun where anything was set aside, and the sentence stays byte for byte what it was where nothing was — on an ordinary store "the last 3" can mean nothing but the last three runs, and the shorter sentence is the true one there. The two headline tests now pin the whole sentence rather than asserting a substring of it, which is what let the second clause drift in the first place. `SeedVanishing` had two consecutive `<summary>` blocks: the edit that added the sizing rationale kept the original summary and added a second rather than merging them. One summary, and the sizing argument stays in the remarks where it was. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TMQcZLBGFTd8wz3mcfn9vc
Collaborator
Author
|
Both Copilot findings addressed in 33ab0a2 — both valid, and one of them was mine rather than a style preference. The headline clause was the substantive one, and worse than the "grammatically incomplete" framing suggests. The two headline tests now assert the whole sentence rather than a substring of it, which is what let the second clause drift in the first place. The duplicated 926 CLI tests still pass. The doc examples in |
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
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.
The issue is still valid
#191(which closed the sibling issue #165) replacedVanished's appearance-count gate with aone-sided Fisher exact test. That narrowed the symptom but did not fix it — it only helps where the
filtered runs also dominate the baseline. The commonest shape still fired at full strength, and
#191's own commit message records that table as one it intends to report, because it cannot tell
it from a deletion.
Measured on a real store, the same shape #191 used to measure itself — seventeen full runs of
SampleApp.MSTest, then threedotnet test --filterruns naming one test:stopped runningfindings8.8e-4 is the best any twenty-run window can do, so no tightening of the gate would have reached it.
What this does
Absence is only evidence in a session that asked. A run under a
--filterdid not fail to see thetests it excluded; it never looked for them.
So each run is classified by how much of the suite it covered —
SessionView.IsPartial, its distincttests against
PartialSessionShare = 0.50of the largest run in the window — andVanishedProviderreads the window as the sequence of runs that covered the suite, re-split into its own "now" and
"before".
Three deliberate departures from the issue's proposal:
SessionView.Tests, notQuickStatistics.distinctTests.SessionAssemblies.Projectnullsthat field for any multi-assembly session, so it is absent exactly on solution-wide runs. The count
the CLI already derives from executions is the same number and needs no plumbing or null-handling.
filtered — has a median of one test, against which every run in it measures as typical. A report is
scoped to exactly one assembly, so the runs being compared are always runs of the same suite.
full and filtered runs: dropping partial sessions out of a current slice of three filtered runs
leaves nothing to ask about, where taking the three most recent full runs still finds a test that
genuinely stopped. Re-deriving both sides also fixes the baseline half — a test present in every
run that asked for it no longer reads as a 5-of-17 occasional visitor.
AnalysisWindow.SliceSizeForis extracted so the re-split narrows on the same rule the window doesand the two cannot drift.
The trade, recorded rather than left to be discovered
A count cannot separate a filter from a deletion, and no threshold makes it able to: nine tests
missing from seventeen is the same table whether they were excluded or removed. The line is at a
half, biased towards silence on purpose — the kind is capped at
Severity.Lowbecause adisappearance is usually something the developer did a minute ago, whereas the false positive arrives
once per unselected test on every filtered run for as long as it stays in the window.
So a deletion removing more than half a suite is not reported, and a filter selecting more
than half still produces false absences. Both are stated in the command reference and in
known limitations, and both are pinned by tests.
Not suppressed silently
VanishedEvidencepublishespartialSessionsSetAside; the headline says "full runs" and aset asidemetric gives the number, but only where any were — with nothing set aside there isnothing for the word to distinguish the runs from. The summary carries
partialSessionswith acaveat line worded as an observation rather than a discount: only this kind sets these runs aside,
because a filtered run's outcomes are as true as any other run's and it is only its silences that
mean nothing. Schema moves to 1.13.
Verification
dotnet build Xping.Sdk.sln— 0 warnings, 0 errors.baseline, the evidence wording in both directions, and the documented cost.
new field — the unfiltered path is unmoved.
Two things worth a reviewer's eye
Item 3 of the issue is deliberately not done, and cannot be done as written. Investigated per
framework:
post-filter. This is the framework the issue's reproduction was recorded on.
CreateDiscovereroverride countingFindcallbacks, degrading tonullonthe run-selected-tests path.
ITest.TestCaseCount, but NUnit3TestAdapter's opt-inPreFiltersilently turnsit into a post-filter number — a fact-shaped number that can quietly be the wrong fact is worse
than a null for a statistical gate.
TotalTestsExpectedis modelled host-wide, soSessionAssemblies.Projectnulls it before the CLI sees a session. PopulatingGetTotalTestsExpected()alone would change nothing observable.A follow-up should file it as a per-assembly discovered count, not a host-wide field.
One existing fixture was adjusted.
ReportEnvelopeTests.SeedVanishingseeded a three-test suiteand deleted two of them — over the half line, so those tests broke. They use
Vanishedonly as aconvenient way to produce several findings, so the stable set is now sized relative to the vanishing
set. Worth confirming you agree that is the right reading rather than a signal about the threshold.
The helper's remarks record why the suite is squeezed from both sides: too few stable tests and the
later runs read as partial, too many and the Benjamini-Hochberg bar tightens past what eight sessions
can produce.
Refs #140
🤖 Generated with Claude Code
https://claude.ai/code/session_01TMQcZLBGFTd8wz3mcfn9vc