Close the remaining dotnet-test skill-coverage gaps - #976
Conversation
The coverage report left four dotnet-test skills with uncovered teaching points. All four are now at 100%. filter-syntax had no eval at all, so every point it teaches was uncovered by construction. Add one with five stimuli - a VSTest category filter with an exclusion, an MTP project on the .NET 9 SDK where the filter has to be forwarded past the SDK's own command line, xUnit v3's framework-specific filter options (including the query language, whose trait selector is the `[trait]` point the report flagged), TUnit's path-shaped treenode filter, and a VSTest-to-xUnit-v3 translation of four CI filter arguments. Five stimuli is also the trial floor: a new eval cannot be allowlisted. generate-testability-wrappers was missing the whole no-DI branch of the skill. Add an ambient-context stimulus (a shipped library whose public static API cannot take constructor injection) covering Step 5, the `AsyncLocal<T>` validation item, the `[ThreadStatic]` pitfall, and the `readonly` field. Add a `sealed` grader on the custom-wrapper stimulus, a lifetime rubric item for the AddSingleton/AddTransient choice, and assertion graders so the TimeProvider and MockFileSystem samples have to actually assert rather than only construct the fake. grade-tests was never exercised on an input large enough to trigger the row cap, so the cap, the collapsed overflow, the summary-leads rule and the report-once rule were all untested. Add a 62-test fixture (44 sound, 11 null-check-only, 7 assertion-free) and a stimulus that asks for a report that stays readable in a PR comment. writing-mstest-tests covered the collection half of the assertion table but not the null/identity/emptiness/absence half. Add a stimulus that rewrites four generic IsTrue checks, covering Assert.AreSame, Assert.IsNull, Assert.IsEmpty and Assert.DoesNotContain. Both generate-testability-wrappers and grade-tests cross the five-trial floor with their new stimulus, so their allowlist lines go with them. The four dotnet-maui entries were already stale on main - cf62633 raised those evals over the floor without retiring their exemptions, which the gate reports as an error - so they are removed here too. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 8251c107-60a7-4e3a-a18f-9b3b403aeeda
Skill Coverage Report
|
There was a problem hiding this comment.
Pull request overview
This PR closes the remaining dotnet-test skill-coverage gaps by adding new eval stimuli and representative fixtures for filter-syntax, generate-testability-wrappers, grade-tests, and writing-mstest-tests, and by removing now-unneeded entries from the eval-quality underpowered allowlist.
Changes:
- Added new/expanded
eval.yamlstimuli and rubric/graders to cover previously-uncovered teaching points. - Introduced new fixture projects (including a large MSTest suite and a static “no DI” library) used by the new stimuli.
- Removed stale/now-unnecessary paths from
eng/eval-quality/underpowered-allowlist.txt.
Show a summary per file
| File | Description |
|---|---|
| tests/dotnet-test/writing-mstest-tests/eval.yaml | Adds a new stimulus to drive coverage for more-specific MSTest assertions (null/identity/emptiness/absence). |
| tests/dotnet-test/grade-tests/eval.yaml | Adds a new stimulus that requires a readable per-test grading report for a larger suite. |
| tests/dotnet-test/grade-tests/fixtures/large-suite/Catalog.Tests/ProductCatalogTests.cs | Adds a 62-test MSTest suite used by the new grading stimulus. |
| tests/dotnet-test/grade-tests/fixtures/large-suite/Catalog.Tests/Catalog.Tests.csproj | Adds the test project for the large-suite fixture. |
| tests/dotnet-test/grade-tests/fixtures/large-suite/Catalog.Core/ProductCatalog.cs | Adds the production code used by the large-suite fixture. |
| tests/dotnet-test/grade-tests/fixtures/large-suite/Catalog.Core/Catalog.Core.csproj | Adds the library project for the large-suite fixture. |
| tests/dotnet-test/generate-testability-wrappers/fixtures/no-di-library/RetentionPolicy.cs | Adds a static, no-DI library API used by the new ambient-context/time-control stimulus. |
| tests/dotnet-test/generate-testability-wrappers/fixtures/no-di-library/Contoso.Retention.csproj | Adds the project file for the no-DI library fixture. |
| tests/dotnet-test/generate-testability-wrappers/eval.yaml | Adds/strengthens graders and adds a new ambient-context/time-control stimulus. |
| tests/dotnet-test/filter-syntax/eval.yaml | Adds a new eval spec with multiple stimuli covering framework-specific filter syntaxes and forwarding. |
| tests/dotnet-test/filter-syntax/fixtures/vstest-mstest/Contoso.Billing.Tests.csproj | Adds a VSTest+MSTest fixture project for filter examples. |
| tests/dotnet-test/filter-syntax/fixtures/vstest-mstest/BillingTests.cs | Adds MSTest category-tagged tests used by filter-syntax stimuli. |
| tests/dotnet-test/filter-syntax/fixtures/tunit/PortalTests.cs | Adds a TUnit fixture for demonstrating --treenode-filter path selectors. |
| tests/dotnet-test/filter-syntax/fixtures/tunit/Contoso.Portal.Tests.csproj | Adds the TUnit fixture project file. |
| tests/dotnet-test/filter-syntax/fixtures/mtp-xunit-v3/UnitTests.cs | Adds xUnit v3 tests with traits used by MTP/xUnit v3 filter stimuli. |
| tests/dotnet-test/filter-syntax/fixtures/mtp-xunit-v3/IntegrationTests.cs | Adds additional xUnit v3 tests/namespaces used by filter stimuli constraints. |
| tests/dotnet-test/filter-syntax/fixtures/mtp-xunit-v3/global.json | Pins the SDK used by the MTP+xUnit v3 fixture. |
| tests/dotnet-test/filter-syntax/fixtures/mtp-xunit-v3/Contoso.Catalog.Tests.csproj | Adds the MTP+xUnit v3 fixture project file. |
| tests/dotnet-test/filter-syntax/fixtures/mtp-nunit-sdk9/ShippingTests.cs | Adds an MTP+NUnit fixture for filter-forwarding stimuli. |
| tests/dotnet-test/filter-syntax/fixtures/mtp-nunit-sdk9/global.json | Pins the SDK used by the MTP+NUnit fixture. |
| tests/dotnet-test/filter-syntax/fixtures/mtp-nunit-sdk9/Contoso.Shipping.Tests.csproj | Adds the MTP+NUnit fixture project file. |
| eng/eval-quality/underpowered-allowlist.txt | Removes allowlist entries that are now above the trial floor (and removes stale entries). |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 22/22 changed files
- Comments generated: 4
Verified by building every new fixture project rather than by inspection. Catalog.Tests could not compile twice over: it used MSTest attributes and Assert without `using Microsoft.VisualStudio.TestTools.UnitTesting;` (the MSTest package does not contribute that as a global using here, even with ImplicitUsings enabled), and it pinned Microsoft.NET.Test.Sdk 17.11.1 while MSTest 3.8.0 requires >= 17.13.0, which fails restore with NU1605. With both fixed the project builds clean and all 62 tests pass, which also confirms the arithmetic the graded rows depend on. The TUnit fixture used `Task` without `using System.Threading.Tasks;` and does not enable ImplicitUsings, so it did not compile either. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 8251c107-60a7-4e3a-a18f-9b3b403aeeda
There was a problem hiding this comment.
Copilot's findings
Suppressed comments (1)
tests/dotnet-test/generate-testability-wrappers/fixtures/no-di-library/RetentionPolicy.cs:16
- RetentionPolicy mixes DateTimeOffset values with DateTime.UtcNow.
DateTime.UtcNow - createdAtandlastSweep.AddDays(1) < DateTime.UtcNowwon’t compile (DateTime vs DateTimeOffset), andNextSweepcurrently returns a DateTime in the true branch despite returning DateTimeOffset.
public static bool IsExpired(DateTimeOffset createdAt, TimeSpan retention)
=> DateTime.UtcNow - createdAt > retention;
public static DateTimeOffset NextSweep(DateTimeOffset lastSweep)
=> lastSweep.AddDays(1) < DateTime.UtcNow
? DateTime.UtcNow
: lastSweep.AddDays(1);
- Files reviewed: 22/22 changed files
- Comments generated: 0 new
|
❌ Evaluation ran but produced no results. The evaluate job completed but no |
|
✅ Evaluation passed for |
PR #976 landed independently and covers some of the same ground as this branch. Two conflicts, both resolved toward main where the work overlaps: - tests/dotnet-test/filter-syntax/eval.yaml (add/add). Both sides wrote this eval from scratch. Main's is better: its fixtures carry real test classes rather than bare project files, so the scenarios read like a repo instead of a lookup quiz. Took main's eval and fixtures wholesale and deleted the four fixture directories this branch had added. - eng/eval-quality/underpowered-allowlist.txt. Main still lists find-untested-sources; this branch raises it to 5 scenarios, so the line has to go under the shrink-only rule. Kept the removal. Also de-duplicated one scenario that both sides added independently: generate-testability-wrappers gained an ambient-context case on each side. Main's framing is stronger - a shipped NuGet library with locked public signatures and a parallel suite, versus this branch's plain console app - so this branch's version is dropped. Its one unique guard is folded into main's scenario instead: a ThreadStatic negative grader plus the matching rubric line, since [ThreadStatic] is the specific wrong answer that does not flow across await points and so breaks exactly the parallel-safety property main's prompt asks for. grade-tests kept both sides: main's 62-test report-readability case and this branch's D/F-band case with production code present are testing different things. The four dotnet-maui allowlist errors this branch reported as pre-existing are fixed in main, so the gate now passes with no errors at all, with and without --base-ref. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: ad6ff32a-d441-4a7b-b474-2bfaee764740
Brings in #976 (dotnet-test skill-coverage gaps). Three resolutions: * underpowered-allowlist.txt - took this branch's version. Both sides shrink the ledger; this branch removes a superset (it also raised code-testing-agent, coverage-analysis and find-untested-sources over the floor), so main's retained find-untested-sources line is now stale and the gate errors on stale entries. * generate-testability-wrappers - both sides added a scenario for the same property (the ambient seam for a no-DI codebase) on the same fixture path, fixtures/no-di-library, with different contents. Git saw no conflict on the fixture because the file names differ, but the result would have been two csproj in one directory copied into both scenarios. Kept main's scenario and fixture, dropped this branch's duplicate. The overfitting judge on run 30611635547 named this branch's rubric specifically ("requiring AsyncLocal<T>, IDisposable scoping and ThreadStatic ... all directly mirroring the skill's code template"); main's states the same properties as outcomes instead. Ported over the one item main lacked: the UTC/DateTimeKind preservation guard for the regression #906 fixed. * Docs - main gave filter-syntax a direct eval, so this branch's claim that all four reference skills deliberately have none is no longer true. Narrowed it to the three that still do, and recorded that filter-syntax's approach has no verdict yet (its evaluation landed during the PAT-pool outage). Validation: eval-quality gate clean (plain and --base-ref), 22/22 gate self-tests, 21/21 adapter tests, markdownlint 0 errors, all 96 eval specs load under vally 0.9.0's loadEvalSpec, merged no-di-library fixture builds, skill-menu budget 14,972/15,000. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 024b3241-d7af-418a-b9cd-3bb9ee9bf0ee
* Capture the repo's hard-won skill-authoring lessons as guidance Mines ~35 merged PRs (#830-#976), issue #899 and eng/eval-quality/README.md into reusable guidance so contributors do not have to rediscover it. - Add .agents/skills/improve-skill-quality: a triage playbook for evals that regress, return no verdict, or fail to activate. Classify before rewriting - broken fixtures, underpowered trial counts, forced tools, stale spec keys and harness errors have all masqueraded as skill regressions. Two reference docs carry the content patterns and the symptom/cause/fix catalogue with citations. - Rewrite .agents/skills/create-skill-test. It documented a pre-Vally schema (scenarios:/assertions:/setup.copy_test_files) that no longer loads: all 97 eval specs use stimuli:/graders:/environment. Anyone following it authored a broken eval. Also folds in the trial floor, dormancy-guard and fixture rules. - Extend .agents/skills/create-skill with the description-as-router rules and a "write for delta over the baseline model" section. - CONTRIBUTING.md: add the quality playbook, document eval sizing, and correct the eval snippet and the stale "raise runs in dotnet-skills.experiment.yaml" advice - an experiment-level runs overwrites every eval's own value. - .gitignore: .agents/skills/ was silently swallowing the repo's own authoring skills. Replaced with .agents/skills/* plus negations for the five owned directories, so installed third-party skills stay ignored. Validated: markdownlint clean, check_eval_quality.py clean, all relative links resolve, frontmatter parses, descriptions under 1024 chars, bodies under 500 lines. Claims fact-checked against the repo by a review pass. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 771c704d-9ac9-4d5f-8ccb-54c62066630f * Address multi-model review: correct the sign-test arithmetic and eval exceptions Three review rounds (Copilot, Claude Opus 4.8, GPT-5.6 Sol/Terra) against the implementation in eng/vally-adapter/adapt.mjs. Statistics. The docs conflated the gate's two independent bars. adapt.mjs gates on (1) counted trials >= 5, else `underpowered`, and (2) p <= 0.05 on a sign test over the *discordant* (non-tie) trials. So "at 5-7 trials one tie makes a pass unreachable" was wrong: at 6 trials 5W/1T/0L is 5 discordant and passes at p = 0.031, and at 7 trials two ties are survivable. Every trial table is now keyed on discordant trials. eng/eval-quality/README.md carried the same over-generalisation and is corrected too, since these docs restate it. Eval coverage. "Every skill needs an eval" and "no direct eval for a disable-model-invocation skill" contradicted each other. Reference skills are now explicitly exempt and covered through their consumers, and filter-syntax is no longer described as the sole answer-graded exception - platform-detection uses the same pattern. Fixtures. "Every buildable fixture must build" forbade the intentionally broken fixtures the same document demonstrates. Now: healthy fixtures build, deliberately broken ones fail only for the reason their stimulus is about. Triage routing. The first-match classifier sent fixture failures to the harness row before the fixture row could match, and treated any positive-but-failing record as a power problem even though adapt.mjs requires `conclusive` first. Added an inconclusive row, moved the fixture row up, and scoped "quote a losing trial" to content fixes so it no longer blocks the no-results path. Also: expect_tools does not assert activation (the harness reports it separately); CONTRIBUTING claimed evaluations run automatically on plugin changes when a maintainer must trigger /evaluate against a bound commit; the minimal eval example is labelled as a sub-floor skeleton; create-skill scaffolds into plugins/<plugin>/skills/ rather than a non-existent top-level skills/. Validated: markdownlint clean, check_eval_quality.py clean, adapt.test.mjs 21 pass, links and anchors resolve. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 771c704d-9ac9-4d5f-8ccb-54c62066630f * Address review: document that agent evals sit outside the verdict flow Abhitej's review found one actionable gap plus three nits; all four are small enough to land here rather than as follow-ups. Agent evals. create-skill-test told authors to write tests/<plugin>/agent.<name>/ eval.yaml, size it for the trial floor, and validate with run-skill-evals.sh -- none of which applies. The canonical experiment declares `evals: tests/*/!(agent.*)/eval.yaml`, so agent specs are excluded: no verdict is computed, the floor does not apply, and the runner drops them even when named explicitly because --eval-filter is intersected with that glob. Step 1 now says so, Step 10 shows the EXPERIMENT_FILE route for actually exercising one, and the checklist and pitfalls are scoped accordingly. Nits: aligned eng/eval-quality/README.md check 8 on "stimuli" (it still said "scenarios" while every authoring doc says stimuli); added a .gitignore comment so the next repo-owned authoring skill gets a negation instead of being silently untracked; and noted in improve-skill-quality that eng/skill-validator/src/docs/InvestigatingResults.md documents the retired skill-validator evaluate schema, so the vally-adapter one is the guide to read. Validated: markdownlint clean, check_eval_quality.py clean, 22/22 eval-quality self-tests, gitignore negations still exact. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 771c704d-9ac9-4d5f-8ccb-54c62066630f * Generalize two triage rows that were written in test-skill vocabulary The triage catalogue is meant to apply to every plugin, but two rows stated their rule in dotnet-test terms, so an author on another plugin would read past them: - the split-brain fixture row was framed entirely around Cobertura `line-rate`; the rule is that a fixture must never state the same fact twice in two places that disagree, whatever the format. Cobertura is now the named example. - the volume-comparison row said "both arms write tests" / "do not reward test count"; the rule is that when both arms produce the same kind of artifact the judge falls back on comparing quantity. Also generalized the one-line summary of the first rule in SKILL.md Step 4 so the detail and the summary do not disagree. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 771c704d-9ac9-4d5f-8ccb-54c62066630f * Correct the environment.skills guidance and a stale cross-reference Two suppressed findings from the Copilot review summaries, both real. environment.skills. The Step 5 skeleton documented it as "extra skills loaded in the isolated arm only", which is wrong in both halves. The experiment declares `vary: /environment/skills` and supplies the value for each arm itself (`[]` for baseline, `plugins/<plugin>/skills/<skill>` for skilled), so whatever a skill eval declares is replaced in every arm -- it cannot add a skill to one arm, and in a skill eval it does nothing at all. The key is meaningful only in `agent.*` evals, which the experiment does not vary. Removed it from the generic skeleton, documented where it does apply, and pointed at an existing agent eval to copy rather than reproducing a spelling: the three agent specs in this repo disagree about whether entries are bare names or relative paths, so the doc should not assert one. Stale cross-reference. Moving the fixture row to the top of the triage table last round left the sentence below it still saying a fixture failure "also matches the two rows above it", when it is now the first row. Reworded to say why it leads. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 771c704d-9ac9-4d5f-8ccb-54c62066630f --------- Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 771c704d-9ac9-4d5f-8ccb-54c62066630f
`disable-model-invocation: true` drops a skill from the CLI's `<available_skills>` menu, so the experiment's skilled arm -- which loads exactly one skill -- cannot reach it either. The gate already reasoned about this, but `report_uncovered()` skipped any skill that had an eval, so it only reported the *better* case (no eval, visibly zero evidence) and stayed silent on the worse one (an eval scoring baseline against baseline and labelling the result a pass or a fail). Two evals landed in that blind spot after the reasoning was written down: tests/dotnet-test/platform-detection (#974) and tests/dotnet-test/filter-syntax (#976). Both READMEs still described them as deliberately un-evaluated. - report reference skills that carry a direct eval, naming the spec - self-test both sides of the trigger via a new silent_case helper - correct eng/eval-quality/README.md and plugins/dotnet-test/README.md Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 3d666b74-a00f-4ed0-a3ce-2d3427cf0d23
The skill-coverage report left four
dotnet-testskills with uncovered teaching points. All four are now at 100%.filter-syntaxgenerate-testability-wrappersgrade-testswriting-mstest-testsfilter-syntaxIt had no
eval.yaml, so every point it teaches was uncovered by construction. New eval with five stimuli — five is also the trial floor, and a new eval cannot be allowlisted:[trait]point the report flagged. The third request in that stimulus is constrained to a single filter option so the two-flag combine the skill documents elsewhere is genuinely not an answer, and the grader is not in tension with the skill;--treenode-filter;Four fixtures follow the existing
tests/dotnet-test/run-tests/fixtures/*precedent: explicit usings, self-contained trivially-true bodies, no production types, package versions and TFMs matched to their run-tests equivalents. The xUnit fixture carries two namespaces so the namespace constraint in stimulus 3 is load-bearing — filtering on the tag alone over-selects.generate-testability-wrappersThe whole no-DI branch of the skill was untested. New ambient-context stimulus (a shipped library whose entire public API is static, so there is nothing to construct and no container to register into) covering Step 5, the
AsyncLocal<T>validation item, the[ThreadStatic]pitfall and thereadonlyfield. Also: asealedgrader on the custom-wrapper stimulus, a rubric item for theAddSingleton/AddTransientlifetime choice, and assertion graders so theTimeProviderandMockFileSystemsamples have to actually assert rather than only construct the fake.grade-testsNever exercised on an input large enough to trigger the row cap, so the cap, the collapsed overflow, the summary-leads rule and the report-once rule were all untested. New 62-test fixture (44 sound, 11 null-check-only, 7 assertion-free) and a stimulus that asks for a report that stays readable in a PR comment.
writing-mstest-testsCovered the collection half of the assertion table but not the null / identity / emptiness / absence half. New stimulus rewrites four generic
Assert.IsTruechecks, coveringAssert.AreSame,Assert.IsNull,Assert.IsEmptyandAssert.DoesNotContainwith deterministic graders.Allowlist
generate-testability-wrappersandgrade-testscross the five-trial floor with their new stimulus, so theirunderpowered-allowlist.txtlines go with them. The fourdotnet-mauientries were already stale onmain— cf62633 raised those evals over the floor without retiring their exemptions, which the gate reports as an error — so they are removed here too. The gate was red onmainbefore this change and is green now.Verification
eng/skill-coverage/Measure-SkillCoverage.ps1reports 100% for all four skills.eng/eval-quality/check_eval_quality.pyreports No errors in both plain and--base-ref origin/mainmodes (4 errors before), and its 20 self-tests pass.skill-validator checkwas not run locally (its build needs a blocked NuGet/npm download), but it only inspectsplugins/, which this change does not touch.