diff --git a/.claude/skills/execute-implement/SKILL.md b/.claude/skills/execute-implement/SKILL.md index 3d33158ffa..9c9b1fed1f 100644 --- a/.claude/skills/execute-implement/SKILL.md +++ b/.claude/skills/execute-implement/SKILL.md @@ -24,6 +24,7 @@ You will receive: - Apply the minimal change set needed to satisfy acceptance signals. 3. **Self-check** - Review for style, safety, and regression risk. + - Every new or changed decision/branch needs at least one test that actually exercises it — use `fieldworks-test-coverage` to verify with `test.ps1 -Coverage`, not just "a test file exists nearby." Document any accepted gap (e.g. requires a live/manual round-trip) rather than letting it pass silently. - For FieldWorks branches that will become PRs, use the review policy in `.github/instructions/review-analyzer.instructions.md` or run `pr-preflight` before handoff when risk or scope warrants it. 4. **Update status** - Record what changed and any follow-up notes. diff --git a/.claude/skills/fieldworks-test-coverage/SKILL.md b/.claude/skills/fieldworks-test-coverage/SKILL.md new file mode 100644 index 0000000000..9bb45786d8 --- /dev/null +++ b/.claude/skills/fieldworks-test-coverage/SKILL.md @@ -0,0 +1,43 @@ +--- +name: fieldworks-test-coverage +description: Check that new or changed FieldWorks code is actually exercised by tests, using test.ps1 -Coverage. Use whenever writing tests for a change, before claiming a bug fix or feature is covered, or when reviewing whether "tests pass" also means the new logic runs under test. +model: haiku +--- + + +You confirm that new/changed behavior is exercised by tests, not merely that some test file exists nearby. "Tests pass" is not the same claim as "the new code ran during the test" — this skill closes that gap. + + + +You will receive: +- The diff or file list for the change +- The test project(s) that build against the changed files + + + +1. **Identify the decision surface** + - For each changed method, find the new/changed branches, guards, and edge cases — not just "a test file touches this class." + - Pure decisions (parsing, gating, mapping) are cheap to cover directly. Side-effecting code wrapped around registry/filesystem/global-singleton calls is often already excluded by design in this repo (e.g. `AvaloniaOptionsDialogLauncher.Apply`) — check whether the surrounding test file already documents that scope boundary before assuming it's a gap. +2. **Run coverage for the touched project(s)** + - `.\test.ps1 -TestProject -Coverage [-NoBuild] [-TestFilter "..."]` + - This collects via coverlet.collector's "XPlat Code Coverage" data collector (works under plain vstest.console.exe on any VS edition — the older `/EnableCodeCoverage` flag in `Build/SetupInclude.targets` requires VS Enterprise and will silently fail to collect on Community/Build Tools installs). + - Reads the merged report from `Output//TestResults/CoverageReport/Summary.txt` (and `index.html` for line-by-line detail) that the script prints automatically. +3. **Check the new lines specifically** + - Open `index.html` for the changed file(s) and confirm the new/changed lines show as covered (green), not just that the file's overall percentage moved. + - A changed file can show high coverage while the new branch itself is dead — always check the specific lines, not the file average. +4. **Close real gaps, document accepted ones** + - If a new decision is uncovered and cheaply testable, write the test (see repo test file conventions — extract pure decisions into small static/testable methods when the surrounding code is otherwise hard to unit test, as with `AvaloniaDialogHost.ResolveEffectiveOwner`). + - If a gap is accepted (e.g. it requires a live/manual/desktop-only round-trip), say so explicitly in the PR description or handoff — do not let it pass silently as "tests pass." + + + +- Never report coverage as a percentage alone; tie the claim to the specific new/changed lines. +- Do not chase repo-wide coverage — scope to the current change's touched files. +- Prefer extracting a testable pure decision over mocking heavy framework/registry/filesystem dependencies to force coverage of an untestable method. + + + +- See `verify-test` for the broader build/test verification flow this slots into. +- See `execute-implement`'s self-check step, which references this skill for new logic. +- `test.ps1 -Coverage` requires the local dotnet tool manifest to be restored once per checkout: `dotnet tool restore` (installs ReportGenerator, used to render the summary/HTML from the raw `coverage.cobertura.xml`). + diff --git a/.claude/skills/verify-test/SKILL.md b/.claude/skills/verify-test/SKILL.md index 82cead5710..27c8cf7296 100644 --- a/.claude/skills/verify-test/SKILL.md +++ b/.claude/skills/verify-test/SKILL.md @@ -23,6 +23,7 @@ You will receive: - Execute builds/tests or manual checks as appropriate. - For FieldWorks, run `./build.ps1 -Clean` before validation when switching branches or worktrees, upgrading package versions, suspecting stale `Obj/` or `Output/` artifacts, or any time you need a fully clean validation baseline. - After the clean step, rerun the normal scripted verification commands such as `./build.ps1`, `./test.ps1`, or the narrow scripted slice you are validating. + - When the change adds or edits behavior (not docs/test-only changes), also run `./test.ps1 -Coverage` for the touched project(s) — see `fieldworks-test-coverage` — and confirm the new/changed lines are actually exercised, not just that the suite passes. 3. **Capture results** - Record pass/fail outcomes and any artifacts. 4. **Report** @@ -38,4 +39,5 @@ You will receive: - Use FieldWorks build/test scripts when applicable. - Use `rubric-verify` when execution-free scoring or explicit hard-gate review is requested. +- Use `fieldworks-test-coverage` to check that new/changed lines are exercised, not just that tests pass. diff --git a/.config/dotnet-tools.json b/.config/dotnet-tools.json new file mode 100644 index 0000000000..dbcca1d66f --- /dev/null +++ b/.config/dotnet-tools.json @@ -0,0 +1,13 @@ +{ + "version": 1, + "isRoot": true, + "tools": { + "dotnet-reportgenerator-globaltool": { + "version": "5.3.11", + "commands": [ + "reportgenerator" + ], + "rollForward": false + } + } +} \ No newline at end of file diff --git a/.github/rubrics/fieldworks-rubric.base.yaml b/.github/rubrics/fieldworks-rubric.base.yaml index db175ed181..391cb20759 100644 --- a/.github/rubrics/fieldworks-rubric.base.yaml +++ b/.github/rubrics/fieldworks-rubric.base.yaml @@ -74,8 +74,8 @@ categories: evidence: API diff or behavioral comparison note - id: test_coverage weight: 0.20 - check: New or changed behavior has at least one corresponding test. - evidence: test names covering the change + check: New or changed behavior has at least one corresponding test that actually exercises the new/changed lines (not merely a nearby test file). + evidence: test names covering the change, ideally cross-checked with `test.ps1 -Coverage` (see `fieldworks-test-coverage`) - id: integrity weight: 0.25 diff --git a/.github/workflows/CI.yml b/.github/workflows/CI.yml index 5d156c7443..a701722a40 100644 --- a/.github/workflows/CI.yml +++ b/.github/workflows/CI.yml @@ -35,9 +35,17 @@ jobs: id: test shell: powershell run: | - .\test.ps1 -Configuration Debug -NoBuild -TestFilter 'TestCategory!=LongRunning&TestCategory!=ByHand&TestCategory!=SmokeTest&TestCategory!=DesktopRequired' + .\test.ps1 -Configuration Debug -NoBuild -Coverage -TestFilter 'TestCategory!=LongRunning&TestCategory!=ByHand&TestCategory!=SmokeTest&TestCategory!=DesktopRequired' if ($LASTEXITCODE -ne 0) { exit $LASTEXITCODE } + - name: Upload coverage report + if: ${{ !cancelled() }} + uses: actions/upload-artifact@v7 + with: + name: coverage-report + path: Output/Debug/TestResults/CoverageReport + if-no-files-found: warn + - name: Summarize native test results if: ${{ always() }} shell: powershell diff --git a/Directory.Packages.props b/Directory.Packages.props index 0f8810af03..3803a44b3b 100644 --- a/Directory.Packages.props +++ b/Directory.Packages.props @@ -164,6 +164,10 @@ + + diff --git a/Src/Directory.Build.props b/Src/Directory.Build.props index 2d546ecf9f..01ddb76415 100644 --- a/Src/Directory.Build.props +++ b/Src/Directory.Build.props @@ -35,4 +35,5 @@ + diff --git a/test.ps1 b/test.ps1 index 864e4ba1c5..6f0a4ee3c9 100644 --- a/test.ps1 +++ b/test.ps1 @@ -44,6 +44,13 @@ Internal switch used when test.ps1 is invoked from build.ps1 -RunTests. Skips acquiring/releasing the same-worktree lock because the parent build already owns it. +.PARAMETER Coverage + Collect code coverage via the coverlet.collector "XPlat Code Coverage" data collector (works under + plain vstest.console.exe on any Visual Studio edition, unlike the VS-Enterprise-only "Code Coverage" + collector). Writes one coverage.cobertura.xml per test host under Output//TestResults, + then renders a merged summary + HTML report via the local ReportGenerator tool + (Output//TestResults/CoverageReport). + .EXAMPLE .\test.ps1 Runs all tests in Debug configuration (builds first if needed). @@ -69,6 +76,10 @@ .\test.ps1 -SkipManaged -TestProject TestGeneric Uses the environment variable equivalent of -AllowAssertDialogs. Clear the variable after debugging. +.EXAMPLE + .\test.ps1 -TestProject "Src/Common/FwAvalonia/FwAvaloniaTests" -Coverage + Runs FwAvaloniaTests with code coverage collection and prints a coverage summary. + .NOTES FieldWorks is x64-only. Tests run in 64-bit mode. #> @@ -86,6 +97,7 @@ param( [switch]$AllowAssertDialogs, [switch]$SkipDependencyCheck, [switch]$SkipWorktreeLock, + [switch]$Coverage, [ValidateSet('user', 'agent', 'unknown')] [string]$StartedBy = 'unknown' ) @@ -179,6 +191,36 @@ function New-TestRunSettingsForAssertDialogMode { return $runSettingsPath } +function New-TestRunSettingsForCoverage { + param( + [string]$SourcePath, + [string]$Configuration, + [bool]$Coverage + ) + + if (-not $Coverage) { + return $SourcePath + } + + # coverlet.collector's "XPlat Code Coverage" data collector instruments every module with a + # matching .pdb next to the test assembly, and restores it afterward. FieldWorks test projects + # all share a single Output/ folder rather than per-project bin folders, so with + # the default MaxCpuCount=0 (parallel testhosts), two testhosts can end up instrumenting/restoring + # the same shared product DLL (e.g. XMLViews.dll) at the same time, which fails with + # "being used by another process". Force serial testhost execution only for coverage runs. + [xml]$runSettings = Get-Content -LiteralPath $SourcePath -Raw + $runSettings.RunSettings.RunConfiguration.MaxCpuCount = '1' + + $runSettingsDir = Join-Path $PSScriptRoot "Output/$Configuration" + if (-not (Test-Path $runSettingsDir)) { + New-Item -ItemType Directory -Force -Path $runSettingsDir | Out-Null + } + + $runSettingsPath = Join-Path $runSettingsDir 'Test.coverage.runsettings' + $runSettings.Save($runSettingsPath) + return $runSettingsPath +} + $allowAssertDialogsForRun = $AllowAssertDialogs -or (Test-EnvironmentSwitchEnabled -Name 'FW_TEST_ALLOW_ASSERT_DIALOGS') function Add-UniquePath { @@ -280,6 +322,28 @@ function Get-NUnitTestAdapterPaths { return $adapterPaths.ToArray() } +function Get-CoverageAdapterPath { + param( + [string]$RepoRoot + ) + + # vstest.console.exe (unlike `dotnet test`) does not auto-discover data collectors from NuGet + # packages; the coverlet.collector build folder must be passed explicitly via /TestAdapterPath, + # same as the NUnit adapter above. + $packagesPropsPath = Join-Path $RepoRoot 'Directory.Packages.props' + $version = Get-CentralPackageVersion -PackagesPropsPath $packagesPropsPath -PackageName 'coverlet.collector' + if ([string]::IsNullOrWhiteSpace($version)) { + return $null + } + + $adapterPath = Join-Path $RepoRoot "packages/coverlet.collector/$version/build/netstandard2.0" + if (Test-Path -LiteralPath (Join-Path $adapterPath 'coverlet.collector.dll') -PathType Leaf) { + return $adapterPath + } + + return $null +} + # ============================================================================= # Environment Setup # ============================================================================= @@ -641,6 +705,10 @@ try { -SourcePath $runSettingsSourcePath ` -Configuration $Configuration ` -AllowDialogs $allowAssertDialogsForRun + $runSettingsPath = New-TestRunSettingsForCoverage ` + -SourcePath $runSettingsPath ` + -Configuration $Configuration ` + -Coverage $Coverage $vstestArgs = @() $vstestArgs += $testDlls @@ -653,6 +721,16 @@ try { $vstestArgs += "/TestAdapterPath:$adapterPath" } + if ($Coverage) { + $coverageAdapterPath = Get-CoverageAdapterPath -RepoRoot $PSScriptRoot + if ($coverageAdapterPath) { + $vstestArgs += "/TestAdapterPath:$coverageAdapterPath" + } + else { + Write-Host "[WARN] -Coverage requested but coverlet.collector build folder was not found under packages/coverlet.collector. Run a build first so it restores." -ForegroundColor Yellow + } + } + # Logger configuration - verbosity goes with the console logger $verbosityMap = @{ 'quiet' = 'quiet'; 'q' = 'quiet' @@ -672,6 +750,10 @@ try { $vstestArgs += "/ListTests" } + if ($Coverage) { + $vstestArgs += '/Collect:XPlat Code Coverage' + } + # ============================================================================= # Run Tests # ============================================================================= @@ -736,6 +818,9 @@ try { foreach ($adapterPath in $nunitAdapterPaths) { $singleArgs += "/TestAdapterPath:$adapterPath" } + if ($Coverage -and $coverageAdapterPath) { + $singleArgs += "/TestAdapterPath:$coverageAdapterPath" + } $singleArgs += "/Logger:trx;LogFileName=${dllName}_${timestamp}.trx" $singleArgs += "/Logger:console;verbosity=$vstestVerbosity" @@ -743,6 +828,10 @@ try { $singleArgs += "/TestCaseFilter:$TestFilter" } + if ($Coverage) { + $singleArgs += '/Collect:XPlat Code Coverage' + } + & $vstestPath $singleArgs 2>&1 | Tee-Object -Variable singleTestOutput $singleExitCode = $LASTEXITCODE if ($singleExitCode -ne 0 -and $overallExitCode -eq 0) { @@ -767,6 +856,40 @@ try { $script:testExitCode = $overallExitCode } + + # ============================================================================= + # Coverage report (only when -Coverage was requested) + # ============================================================================= + + if ($Coverage -and -not $ListTests) { + $coverageFiles = Get-ChildItem -Path $resultsDir -Filter "coverage.cobertura.xml" -Recurse -ErrorAction SilentlyContinue + if (-not $coverageFiles -or $coverageFiles.Count -eq 0) { + Write-Host "[WARN] -Coverage was requested but no coverage.cobertura.xml files were produced under $resultsDir." -ForegroundColor Yellow + } + else { + Write-Host "" + Write-Host "Generating coverage report from $($coverageFiles.Count) file(s)..." -ForegroundColor Cyan + $coverageReportDir = Join-Path $resultsDir "CoverageReport" + $coverageReportsGlob = Join-Path $resultsDir "*/coverage.cobertura.xml" + try { + # Restore the local tool manifest first (cheap no-op once already restored) so a fresh + # checkout/CI runner doesn't just warn and skip the report. + & dotnet tool restore 2>&1 | Out-Null + & dotnet tool run reportgenerator "-reports:$coverageReportsGlob" "-targetdir:$coverageReportDir" "-reporttypes:TextSummary;Html" 2>&1 | + Where-Object { $_ -notmatch '^\d{4}-\d\d-\d\dT' } # drop ReportGenerator's timestamped progress lines + $summaryPath = Join-Path $coverageReportDir "Summary.txt" + if (Test-Path -LiteralPath $summaryPath) { + Write-Host "" + Get-Content -LiteralPath $summaryPath | Write-Host + Write-Host "" + Write-Host "Full HTML coverage report: $(Join-Path $coverageReportDir 'index.html')" -ForegroundColor Gray + } + } + catch { + Write-Host "[WARN] ReportGenerator failed (is '.config/dotnet-tools.json' restored? run 'dotnet tool restore'): $_" -ForegroundColor Yellow + } + } + } } } finally {