Collect coverage on a Debug build for accurate line coverage (#630)#637
Merged
Conversation
Follow-up to #632. That PR excluded generated code from the coverage denominator but still collected coverage on an optimized Release build, where the C# compiler often drops or merges the sequence points for standalone block braces (and duplicate returns). The Microsoft coverage engine then reports many '{'/'}' lines as hits=0 even when the method fully executes, deflating line coverage and capping control-flow-heavy files at ~70-80%. Collecting on a Debug build maps every line faithfully. This raises the honest hand-written baseline from the understated ~48.8% (Release) to ~59.5% (Debug) with no test changes -- e.g. ManifestService.cs reads 81.7% in Debug vs the 62-71% seen in Release. - coverage-report.ps1: default -Configuration Debug (+ rationale in help). - build-cli.ps1: build + run the test/coverage pass in Debug so the CI-posted cobertura line-rate matches local runs; pass -p:TreatWarningsAsErrors=true on the Debug build to keep the Release-only warning gate. The shipped CLI is still the Release `dotnet publish`; only the test run moves to Debug. - README: document why Debug, that the Release artifact is still validated by the sample/npm E2E suites, and correct the baseline figure. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Switches .NET coverage collection to Debug for more accurate line mapping while retaining Release publishing.
Changes:
- Defaults coverage reporting to Debug.
- Builds/tests in Debug with warnings treated as errors.
- Documents the rationale and corrected baseline.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
scripts/coverage-report.ps1 |
Makes Debug the default coverage configuration. |
scripts/build-cli.ps1 |
Builds and runs coverage tests in Debug. |
src/winapp-CLI/WinApp.Cli.Tests/README.md |
Documents Debug coverage behavior and baseline. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
Build Metrics ReportBinary Sizes
Test Results✅ 1637 passed, 1 skipped out of 1638 tests in 461.2s (+64.1s vs. baseline) Test Coverage❌ 59.5% line coverage, 52% branch coverage · ✅ +11.5% vs. baseline CLI Startup Time36ms median (x64, Updated 2026-07-15 08:28:23 UTC · commit |
This was referenced Jul 15, 2026
azchohfi
added a commit
that referenced
this pull request
Jul 15, 2026
## Test coverage: non-UI CLI commands Part of the CLI test-coverage push (tracking #630). This PR raises meaningful line coverage across the non-UI command surface (cert, sign, package, run, manifest, init, restore, update, tool, msstore, complete, unregister, and the root/help wiring), targeting **≥95% per hand-written command file** with real, use-case-driven tests plus unit tests for the branch/error paths. ### Per-file Debug line coverage (all 21 in-scope non-UI command files ≥95%) `100%` — UpdateCommand, RestoreCommand, CompleteCommand, SignCommand, MSStoreCommand, CertGenerateCommand, CertInfoCommand, CertInstallCommand, GetWinappPathCommand, CreateExternalCatalogCommand, ManifestGenerateCommand, **ManifestUpdateAssetsCommand**. `≥95%` — InitCommand 95.1 · CreateDebugIdentityCommand 95.5 · ToolCommand 97.5 · WinAppRootCommand 98.4 · ManifestAddAliasCommand 98.5 · **RunCommand 99.1** · UnregisterCommand 99.1 · PackageCommand 99.5. No `[ExcludeFromCodeCoverage]`, no command exclusions. ### Product code (3 minimal, behavior-preserving edits) 1. **RunCommand** — two test seams isolating OS boundaries on the execution-alias launch path: `internal Func<string, FileInfo?> ResolveAliasProxy` (alias-proxy resolution under `%LOCALAPPDATA%\Microsoft\WindowsApps`) and `internal Func<ProcessStartInfo, Process?> ProcessStarter`. Both default to the production call, so runtime behavior is unchanged. This lets tests exercise all the surrounding validation / debug / cancellation / error-handling logic without registering a real alias proxy or spawning the resolved binary. Follows the existing `UpdateNotificationService` internal-seam precedent. 2. **MSStoreCommand** — same `ProcessStarter` seam for the MSStoreCLI process start (defaults to `Process.Start`), so the null-process defensive branch is testable. 3. **ManifestUpdateAssetsCommand** — deleted a provably-dead `if (imagePath == null)` guard. `ImageArgument` is a required argument (arity `ExactlyOne`, no default), so System.CommandLine rejects a missing value before the handler runs and `GetValue(ImageArgument)` is never null inside `InvokeAsync`. The guard was unreachable dead code; deleting it (rather than excluding it) is the honest coverage fix. ### Notable test work - **RunCommand debug-output + Ctrl+C cleanup** (both the AUMID and execution-alias paths): a fake debug loop cancels the token *during* the loop, then the test asserts the package's processes are terminated and the loop's exit code is still returned. Deterministic (no timing race), and fails if the cleanup branch is removed. - **ToolCommand** stdout/stderr forwarding + argument-quoting are asserted against real captured console output (a space-containing arg is forwarded as a single quoted token), not just exit codes. - **Package (fake-msix)** tests assert the user-visible signed/unsigned guidance messages. - A test helper launches `cmd.exe` via its fixed `System32` path rather than the `%ComSpec%` environment variable, to avoid any PATH/env hijack in the test host. ### Review Reviewed with the repo `pr-review` skill before opening (7 specialists + a multi-model cross-check): **0 critical / 0 high**; all medium/low findings addressed (padding tests upgraded to real behavior assertions, the two debug-cancel branches covered, the dead guard deleted, the ComSpec/PATH hijack removed, signed/unsigned and search-limit-hint messages asserted). Coverage is measured on a **Debug** build (see #637) because optimized Release under-counts standalone brace lines and inlined static helpers. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
azchohfi
added a commit
that referenced
this pull request
Jul 15, 2026
## Summary Raises hand-written line coverage on the **Manifest & image-asset service** surface to **≥95% per file** (measured in Debug), with use-case-driven tests. **Test-only — no product code changes.** ## Per-file coverage (authoritative full Debug run: `pwsh scripts/coverage-report.ps1 -Configuration Debug`) | File | Before | After | |------|-------:|------:| | Services/ManifestService.cs | 81.7% | **96.5%** | | Services/ImageAssetService.cs | 82.2% | **96.7%** | | Services/ManifestTemplateService.cs | 86.9% | **97.5%** | | Services/AppxManifestDocument.cs | 91.1% | **95.6%** | | Helpers/ManifestFileReferenceHelper.cs | 93.9% | **96.0%** | ## Tests 112 net-new executed cases (78 `[TestMethod]`s + 39 `[DataRow]`s) across 5 classes. Use-case-driven: manifest generate/edit, identity/capabilities/extensions, all execution-alias status branches, template expansion + ASCII/id sanitization, namespace-aware `XDocument` reads/writes, raster + SVG rendering, asset/ICO generation, file-reference resolution — plus malformed/edge/error paths. All passing; Release build clean (warnings-as-errors). ## Notes - **Measured in Debug.** Optimized Release drops sequence points on standalone brace lines and inlined static helpers, under-reporting control-flow-heavy files. Reproduce the numbers with `-Configuration Debug` (the switch to Debug-by-default is #637). - **`[DoNotParallelize]`** on the two GDI+-heavy test classes is a test attribute only — it avoids a `System.Drawing` encoder-lookup race under MSTest's 32-worker method-level parallelism. Production renders assets sequentially, so this is a test-only artifact. - Remaining uncovered lines are defensive/unreachable guards (e.g. GUID-reject where the format requires a `.`; `XDocument.Parse` throws before it could return a null `Root`) — verified empirically and left in place. **No `[ExcludeFromCodeCoverage]`, no product changes.** - `PngHelper.CreateRasterPng` renders test PNG fixtures in one shared place (dedup from review). ## Review Reviewed with the repo `pr-review` skill (7 specialist dimensions + independent multi-model cross-check): **0 critical, 0 high**. The single medium finding (duplicated raster-PNG helper) is resolved in this branch. --------- Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
azchohfi
added a commit
that referenced
this pull request
Jul 15, 2026
## Test coverage: Project / workspace / setup services Part of the CLI test-coverage push (tracking #630). This PR raises meaningful line coverage across the workspace-setup and project-detection service surface, targeting **≥95% per hand-written file** with real, use-case-driven tests plus unit tests for the branch/error paths. ### Per-file Debug line coverage (authoritative, full run) | Service | Coverage | |---|---| | CppWinrtService | 100% | | GitignoreService | 100% | | SlugGenerator | 100% | | WinmdsLockfileService | 98.1% | | WinmdService | 95.7% | | DotNetService | 95.5% | | WorkspaceSetupService | 95.1% | | ProjectDetectionService | 93.2% ⛔ documented ceiling | | DirectoryPackagesService | 92.7% ⛔ documented ceiling | **7/9 files ≥95%.** The two below-gate files are honest, documented ceilings — their remaining uncovered lines are genuinely-defensive `UnauthorizedAccessException` / `IOException` catch handlers and null-guards (`DocumentElement == null`, `Attributes == null`) that valid input can't reach. No `[ExcludeFromCodeCoverage]`, no service exclusions, no flaky TOCTOU tests were used to paper over them. ### Product code The product diff is **pure dead-code deletion (28 lines, 0 insertions), zero behavior change, zero test seams**: - `ProjectDetectionService.HasFileWithExtension` — deleted (zero call sites). - `WorkspaceSetupService.InitializeConfigurationAsync` — deleted a duplicate no-config guard that is provably unreachable: its sole caller (`SetupWorkspaceAsync`) already returns for the identical `RequireExistingConfig && !configService.Exists()` condition before invoking it. Both deletions are honest coverage improvements (removing dead lines rather than excluding them). Verified: full `WorkspaceSetupService` suite passes with zero behavior change; build is clean (0 warnings, warnings-as-errors). ### Notable test work - Real deny-ACL graceful-degradation tests for `ProjectDetectionService`'s filesystem access-denied handlers (self-mark inconclusive if run elevated so they never pass vacuously; strip all ACEs in cleanup). - Real COM-interop `SAFEARRAY` exercise for `SlugGenerator`'s interop hashing overloads (unmanaged memory freed via try/finally, no leak on assert failure). - Consolidated `FakeConfigurableDevModeService` into `FakeDevModeService` (behavior-preserving defaults) to remove a duplicate test fake. ### Test-project change - `AllowUnsafeBlocks` in `WinApp.Cli.Tests.csproj` (test project only) to build the real SAFEARRAY for the SlugGenerator interop tests. ### Review Reviewed with the repo `pr-review` skill before opening: **0 critical / 0 high**; all medium/low findings addressed (reverted the one product visibility seam back to `private`, deleted the two dead-code blocks, hardened the unmanaged-memory and deny-ACL cleanup paths, folded the duplicate fake). Coverage is measured on a **Debug** build (see #637) because optimized Release under-counts standalone brace lines and inlined static helpers. --------- Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
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.
Follow-up to #632. That PR fixed the biggest coverage distortion (generated code inflating the denominator), but it still collected coverage on an optimized Release build — where the Microsoft coverage engine reports many standalone block-brace lines (
{/}) ashits=0even when the code executes (it drops or merges their sequence points). That deflates line coverage and caps control-flow-heavy files at ~70–80%.Fix
Collect coverage on a Debug build, which maps lines faithfully (the standard configuration for .NET coverage). No test content changes.
coverage-report.ps1: default-Configuration Debug(+ rationale in help).build-cli.ps1: build + run the test/coverage pass in Debug so the CI-posted coberturaline-ratematches local runs. Pass-p:TreatWarningsAsErrors=trueon the Debug build to keep the warning-as-error gate thatDirectory.Build.propsotherwise applies only to Release. The shipped CLI is still the Releasedotnet publish— only the test run moves to Debug, and the Release artifact stays exercised end-to-end by the sample/npm E2E suites.README: document why Debug and correct the baseline figure.Impact on the honest baseline
Same tests, ~11 points higher — the gap was pure Release brace under-counting. This makes the number CI posts on PRs honest and unblocks the per-file 95% target for the follow-up coverage PRs (the ~70–80% "ceiling" was a measurement artifact, not real code that couldn't be covered).
Review
Ran the repo's
pr-reviewskill (7 specialists + a GPT multi-model cross-check) and validated at runtime:-p:TreatWarningsAsErrors=true→ 0 warnings, 0 errors.publishoutput, not the solutionbin; the CI metrics baseline transitions cleanly.Closes out the Release-measurement half of #630.