Test coverage: NuGet/update/install files reverted by #654 (issue #630)#671
Merged
Conversation
PR #654 reverted the #629 NuGet-client migration and its coverage tests (#647) together, restoring UpdateCommand, PackageInstallationService, and NugetService to their pre-migration versions and deleting their dedicated test files. This re-covers that current (pre-migration) code on main. Product change (minimal, behavior-preserving test seam only): - NugetService: add internal static HttpGetAsync delegate defaulting to the shared HttpClient.GetAsync (production behavior byte-for-byte unchanged); route the four flat-container/registration GETs through it so download, version-resolution, and dependency-parsing paths run offline in tests. - Document two genuine, non-unit-coverable ceilings in source via <remarks> referencing issue #630: the %USERPROFILE%\.nuget\packages Create() branch (GetFolderPath ignores env redirection) and the defensive ContainsKey short-circuit in InstallPackageRecursiveAsync (unreachable via public API). Tests (meaningful use-case first, then residual-branch units): - NugetServiceOfflineTests.cs (29): restore/download/version-resolution/ source-handling, transitive + diamond dedup, listed-version filtering, nuspec dependency parsing, error paths. [DoNotParallelize]; the HttpGetAsync seam and cache/env vars are saved and restored per test. - PackageInstallationServiceTests.cs (16): version resolution (config/pinned/ ignoreConfig), already-present transitive top-up, fresh-install merge, EnsurePackageAsync success/failure, workspace init. - UpdateCommandTests.cs (11): version compare, updates found/none, preview mode, per-package + build-tools failures, runtime handling, JSON/output. Coverage: UpdateCommand 147/147 (100%), PackageInstallationService 131/131 (100%), NugetService 302/307 (98.37%; the 5 uncovered lines are the two documented ceilings above). Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Two review-driven fixes, both raising real coverage without gaming:
1. Correctness (transitive deps under a version range): ResolveDependenciesAsync
now normalizes a dependency's version via ParseMinimumVersion before recursing
into GetPackageDependenciesAsync. FetchDirectDependenciesAsync only strips
brackets ("[2.0.0, )" -> "2.0.0, "), so the raw value built a malformed
flat-container URL and silently dropped dependencies nested under a
range-versioned package. The offline fakes now match the slash-delimited
"/<id>/<version>/" segment so a malformed URL genuinely 404s, making the
transitive test fail without the fix.
2. Test hygiene / no machine mutation: extract a GetUserProfileDirectory seam
(defaults to Environment.GetFolderPath(UserProfile)) so the default
%USERPROFILE%\.nuget\packages create-branch is exercised hermetically against
a temp dir instead of being a documented ceiling that would otherwise mutate
the real user profile. Added a test pinning the seam's production default.
NugetService.cs 98.4% -> 99.4% (only the genuinely-unreachable defensive guard
remains, documented). PackageInstallationService.cs / UpdateCommand.cs stay 100%.
Debug + Release (both projects) 0W/0E; 97/97 scoped tests pass.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Improves unit-test coverage for the restored (pre-migration) NuGet/update/install code paths, primarily by adding offline/deterministic tests and introducing limited test seams in NugetService, plus a small correctness fix for transitive dependency walking when dependency versions are specified as ranges.
Changes:
- Add offline
NugetServicetests using an injectable HTTP GET seam and hermetic user-profile resolution. - Add behavior tests for
winapp updateandPackageInstallationServicewith controllable fakes. - Fix
NugetService.GetPackageDependenciesAsynctransitive recursion to normalize range versions viaParseMinimumVersion.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/winapp-CLI/WinApp.Cli/Services/NugetService.cs | Adds test seams for HTTP/profile path and fixes transitive dependency recursion for range versions. |
| src/winapp-CLI/WinApp.Cli.Tests/UpdateCommandTests.cs | Adds behavior coverage for winapp update handler paths via controllable fakes. |
| src/winapp-CLI/WinApp.Cli.Tests/PackageInstallationServiceTests.cs | Adds branch-level coverage for install/version-resolution/top-up behavior using an in-memory INugetService fake. |
| src/winapp-CLI/WinApp.Cli.Tests/NugetServiceOfflineTests.cs | Adds hermetic, offline coverage for NuGet download/extract/deps/version-listing/cache-path behaviors. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // transitive nuspec URL is well-formed. FetchDirectDependenciesAsync only strips brackets | ||
| // (e.g. "[2.0.0, )" -> "2.0.0, "), which would otherwise build a malformed flat-container | ||
| // URL and silently drop transitive dependencies nested under a range-versioned package. | ||
| var transitiveDeps = await GetPackageDependenciesAsync(depId, ParseMinimumVersion(depVersion), cancellationToken); |
Collaborator
Author
There was a problem hiding this comment.
Good catch — fixed in 23f80bd. An open-lower-bound/unparseable range (e.g. (,2.0.0]) normalizes to empty, so the loop now skips the transitive fetch when the minimum version is empty (mirroring the existing on-disk ResolveDependenciesAsync guard) instead of building a malformed .../<id>//<id>.nuspec URL. The direct dependency is still returned; added a covering test with a unique package id to avoid the static DependencyCache colliding with other tests.
Contributor
Build Metrics ReportBinary Sizes
Test Results✅ 3492 passed, 4 skipped out of 3496 tests in 601.9s (+58 tests, +73.9s vs. baseline) Test Coverage✅ 94.8% line coverage, 89% branch coverage · ✅ +1.0% vs. baseline CLI Startup Time33ms median (x64, Updated 2026-07-17 03:14:55 UTC · commit |
…review) Addresses the Copilot PR-review finding on the ParseMinimumVersion normalization: an open-lower-bound or otherwise unparseable range (e.g. "(,2.0.0]") normalizes to an empty string, which would recurse into GetPackageDependenciesAsync with an empty version and build a malformed ".../<id>//<id>.nuspec" flat-container URL (pointless work + a bogus empty-version cache key). Skip the transitive fetch when the minimum version is empty, mirroring the existing on-disk ResolveDependenciesAsync guard; the direct dependency is already in the result set. Added a covering test (unique package id to avoid the static DependencyCache colliding with other tests). 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.
What
Raises per-file test coverage to ≥95% for three files that PR #654 restored to their pre-migration state (issue #630):
Commands/UpdateCommand.csServices/PackageInstallationService.csServices/NugetService.csNugetService.cs's only remaining uncovered lines are a genuinely-unreachable defensive guard inInstallPackageRecursiveAsync(public entry seeds an empty dict andResolveDependenciesAsyncskips present deps before recursing), documented in-source with a<remarks>referencing #630. No[ExcludeFromCodeCoverage], nocoverage.runsettingsedits.Product changes (minimal, behavior-preserving)
internal static Functest seams onNugetService:HttpGetAsync(defaults to the sharedHttpClient.GetAsync— the single real network boundary; tests inject canned in-memory responses) andGetUserProfileDirectory(defaults toEnvironment.GetFolderPath(UserProfile); lets the default%USERPROFILE%\.nuget\packagescreate-branch be exercised hermetically against a temp dir instead of mutating the real user profile). Both default delegates are byte-for-byte the original behavior.ResolveDependenciesAsyncnow normalizes a dependency's version viaParseMinimumVersionbefore recursing intoGetPackageDependenciesAsync. Previously the raw value fromFetchDirectDependenciesAsync(which strips only brackets,"[2.0.0, )"→"2.0.0, ") built a malformed flat-container URL, silently dropping transitive dependencies nested under a range-versioned package. No-op for exact versions; the sibling on-disk path and the direct consumer already applied this normalization. The offline test fakes now match the slash-delimited/<id>/<version>/URL segment so this path genuinely 404s without the fix (the transitive test fails if the fix is reverted).Tests
NugetServiceOfflineTests— restore/download/version-resolution/transitive+diamond-dedup, listed-version filtering, nuspec parsing, HTTP-error paths, the hermetic default-fallback create-branch, and the range-versioned transitive walk.[DoNotParallelize]; all process-wide seams + NuGet env vars saved/restored per test.PackageInstallationServiceTests,UpdateCommandTests— version resolution, already-present top-up merges, updates-found/all-up-to-date/preview, per-package + build-tools failure paths.Validation
WinApp.CliandWinApp.Cli.Tests, warnings-as-errors): 0 W / 0 E each.pr-reviewskill (7 specialists + a GPT multi-model cross-check) before opening; the two accepted findings (real-profile mutation risk + a fake that masked the range-version bug) are fixed in this branch.This wave re-covers the pre-migration NuGet client that #654 restored (#654 reverted the #629 migration + its #647 tests together). If maintainers re-attempt the migration, these three test files and the two seams will likely need rework. The
ParseMinimumVersioncorrectness fix has standalone value while this code ships.