test: raise NuGet-client migration (#629) coverage to >=95% per file#647
Merged
Conversation
Verify and close the remaining coverage gaps on the product files that PR #629 (NugetService migration to the official NuGet client libraries) changed/added, bringing them to the >=95%-per-file bar used across the CLI. Per-file line coverage (scoped Debug run), before -> after: - NugetService.cs 91.6% -> 99.2% (249/251) - NugetService.Dependencies.cs 80.0% -> 95.4% (272/285) - NugetPackageDownloader.cs 78.0% -> 91.5% (75/82, documented ceiling) - NugetSourceProvider.cs 99.1% -> 100% - PackageInstallationService.cs 64.2% -> 100% - UpdateCommand.cs 92.4% -> 100% New/extended tests (MSTest v4, in-process/local feeds, no real network): - NugetServiceCoverageTests: already-cached short-circuit, corrupt/renamed cached nuspec, SDK-channel latest filtering + no-match errors, version-less dependency skip, cancellation while resolving, all-sources protocol failure (deterministic closed HTTPS loopback), no-eligible-sources diagnosis. - NugetPackageDownloaderCoverageTests: content-endpoint failover against an in-process v3 flat-container feed (opts into allowInsecureConnections and asserts the content endpoint was hit + the diagnostic error is surfaced), plus direct CollectingLogger unit tests. - Extended PackageInstallationServiceTests (-> 100%), UpdateCommandTests (-> 100%), and NugetServiceVersionRangeTests data rows. Product change is behavior-preserving: NugetPackageDownloader.CollectingLogger is widened from private to internal purely so its message-capture logic can be unit-tested (production wiring unchanged), plus source-documented honest-ceiling comments on the genuinely untestable defensive lines (byIdResource-null guard, best-effort OS temp-file delete, structural braces). Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
… >=95% pr-review found NugetPackageDownloader.cs at 91.5% (below the >=95% bar): the best-effort temp-file delete catch was uncovered. Add a minimal behavior- preserving DeleteTempFile seam (defaults to File.Delete) so a test can force the cleanup to fail and assert the package download still succeeds -- a real contract that a failed temp cleanup must not fail the user's download. Brings NugetPackageDownloader.cs to 95.18%; the remaining uncovered lines are the documented byIdResource-null defensive guard and compiler brace artifacts. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Expands NuGet migration coverage with deterministic local-feed tests and minimal test seams.
Changes:
- Covers NuGet resolution, caching, downloads, and failure paths.
- Expands package installation and update-command tests.
- Adds injectable cleanup and logger visibility seams.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
NugetPackageDownloader.cs |
Adds test seams and coverage notes. |
UpdateCommandTests.cs |
Covers additional update paths. |
PackageInstallationServiceTests.cs |
Covers installation and version merging. |
NugetServiceVersionRangeTests.cs |
Adds range-intersection cases. |
NugetServiceTests.cs |
Covers invalid-version fallback comparison. |
NugetServiceCoverageTests.cs |
Adds local-feed NuGet coverage tests. |
NugetPackageDownloaderCoverageTests.cs |
Tests diagnostics, logging, and cleanup failures. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| var downloader = new NugetPackageDownloader(sourceProvider) | ||
| { | ||
| // Force the best-effort temp-file cleanup to throw after the package has transferred. | ||
| DeleteTempFile = _ => throw new IOException("simulated temp-file cleanup failure"), |
Collaborator
Author
There was a problem hiding this comment.
Good catch — fixed in 4466b4c. The test forces DeleteTempFile to throw specifically to cover the swallow path, which disabled the product's own cleanup of the buffer created under %TEMP%. It now captures the path handed to the seam and deletes it in the inally (and asserts the seam was actually invoked). The product paths are unchanged, so NugetPackageDownloader coverage holds at 95.18%.
# Conflicts: # src/winapp-CLI/WinApp.Cli.Tests/PackageInstallationServiceTests.cs # src/winapp-CLI/WinApp.Cli.Tests/UpdateCommandTests.cs
The temp-file cleanup-failure test forces NugetPackageDownloader's DeleteTempFile seam to throw, which by design disables the product's own cleanup of the download buffered under %TEMP%. The test only deleted its feed root, so it leaked one random-named .nupkg into the system temp directory on every run. Capture the path handed to the seam and delete it in the finally; also assert the seam was actually invoked. No product-path change: line 139 still invokes the throwing seam and the swallow catch is still exercised, so NugetPackageDownloader coverage is unchanged. Addresses the Copilot auto-review comment on PR #647. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
azchohfi
added a commit
that referenced
this pull request
Jul 16, 2026
…#654) Reverts the NuGet-client migration (#629) and its stacked coverage PR (#647). Tracks #653. ## Why After migrating `NugetService` to the official NuGet client libraries, `main` builds fail on ADO due to a security issue: #629 pins the `NuGet.*` packages to the prerelease **`7.9.0-rc.36120`** sourced from the dnceng **dotnet-tools** public feed (not nuget.org). The security gate flags consuming a prerelease from a secondary public feed. This was always intended as temporary — the `NuGet.UseSystemTextJsonDeserialization` switch needed to keep the Native AOT publish clean isn't in a nuget.org stable yet. ## What this reverts - **#647** first (reverse-merge order, since it's stacked on #629): removes the added coverage tests and the `NugetPackageDownloader.cs` test seams. - **#629** next: restores the hand-rolled `NugetService`, and removes the `NuGet.*` RC pins, the `dotnet-tools` source + `packageSourceMapping` from `nuget.config`, the `.pipelines/release-nuget.config` upstream, and the `Directory.Packages.props` pins. ## Conflict resolution notes Several coverage PRs merged after #629 built on top of its surface, so a plain revert left conflicts. Resolved as follows: - `FakeNugetService.cs` — restored to its pre-#629 form. - `BaseCommandTests.cs` — reverted #629's NuGet-specific changes but **kept** the generic `ParseAndInvokeWithCaptureAsync(..., CancellationToken)` overload, which is unrelated test infrastructure used by UI-command cancellation tests from #645. - Deleted `PackageInstallationServiceTests.cs` and `UpdateCommandTests.cs` (both new in #629; later expanded by #640/#641 for the migrated code, which no longer exists). - Deleted `PackageInstallationServiceCacheMarkerTests.cs` (added by #641; exercised the migrated `NugetService` against a local folder feed via `NugetFeedTestHelpers` + `INugetService.IsPackageInstalled`, all of which are removed here). ## Validation - `dotnet build winapp.sln -c Debug` — **0 warnings / 0 errors** - Test project builds clean; UI-command and NuGet-service test suites pass (447 passed / 0 failed / 1 skipped). - No remaining references to `dotnet-tools`, `7.9.0`, or `packageSourceMapping` in `nuget.config` / `Directory.Packages.props` / `.pipelines/release-nuget.config`. ## Re-landing This migration should be reapplied **only once stable `NuGet.Protocol 7.9.0` and all related `NuGet.*` packages are published on nuget.org** (not on dnceng dotnet-tools or any other secondary public feed). At that point: repoint the pins to the nuget.org stable, and drop the `dotnet-tools` source, `packageSourceMapping`, and the release-nuget.config upstream. See #653. --------- Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
This was referenced Jul 16, 2026
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.
What
Raises test coverage of the recently-merged NuGet-client migration (#629, which rewrote
NugetServiceonto the official NuGet client libraries) to ≥95% per hand-written file, continuing the coverage effort tracked in #630 for those files.Per-file coverage (Debug)
Remaining uncovered lines are source-documented honest ceilings (defensive guards that no real/in-memory feed shape can drive — e.g. a
FindPackageByIdResource-null source) and compiler brace artifacts. No[ExcludeFromCodeCoverage]on hand-written logic and nocoverage.runsettingsexclusions.Tests
New/expanded coverage against in-process local-folder and loopback-HTTP feeds (no network): the already-fully-cached short-circuit, a corrupt/renamed cached
.nuspec, SDK-channel (preview/experimental) latest-version filtering and its no-match errors, versionless dependencies, cancellation while resolving a dependency, an all-sources protocol failure reading a dependency graph, the "no eligible sources" diagnosis, per-source failover diagnostics (a content-endpoint failure surfaced instead of a plain "not found"),CollectingLoggermessage capture, package-installation paths,update-command paths, and best-effort temp-file cleanup failure.Product changes (behavior-preserving test seams)
NugetPackageDownloader.CollectingLogger:private→internalso its error-message-capture logic can be unit-tested directly (production constructs and uses it identically).NugetPackageDownloader.DeleteTempFile: a newinternal Action<string>seam defaulting toFile.Delete, so a test can force the best-effort temp-file cleanup to fail and assert the download still succeeds — a real contract that a failed cleanup must not fail the user's download. Production behavior is identical to callingFile.Deletedirectly.Validation