Repository navigation
Test coverage: NuGet/update/install files reverted by #654 (issue #630) - #671
Merged
Alexandre Zollinger Chohfi (azchohfi) merged 3 commits intoJul 17, 2026
Merged
Alexandre Zollinger Chohfi (azchohfi) merged 3 commits into
Alexandre Zollinger Chohfi (azchohfi) merged 3 commits into
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>
Alexandre Zollinger Chohfi (azchohfi)
deleted the
azchohfi-azchohfi-tests-nuget-revert
branch
July 17, 2026 03:26
4 tasks done
Nikola Metulev (nmetulev)
added a commit
that referenced
this pull request
Sep 4, 2026
## Description This reverts #654, which reverted #629 (and its stacked coverage PR #647). #629 migrated `NugetService` off hand-rolled NuGet v3 HTTP calls and onto the official NuGet client libraries, so `init` / `restore` / `update` honor the user's `nuget.config` hierarchy: custom and private package sources, credentials, `packageSourceMapping`, and `globalPackagesFolder`. Nothing was wrong with that migration. It was reverted purely because it pinned `NuGet.*` to the prerelease `7.9.0-rc.36120` from the public dnceng `dotnet-tools` feed, and the ADO security gate rejects consuming a prerelease from a secondary public feed. That was always documented as temporary: the `NuGet.UseSystemTextJsonDeserialization` switch needed to keep the Native AOT publish clean had not shipped in a nuget.org stable yet. **`NuGet 7.9.0` is now stable on nuget.org**, which is exactly the condition #654 named for re-landing. So the pins point at `7.9.0` stable, and `nuget.config` plus `.pipelines/release-nuget.config` go back to main's minimal form: no `dotnet-tools` source, no `packageSourceMapping`, no upstream note. ### Beyond a plain re-revert About 100 commits landed on main between the revert and now, so this is not a clean re-application. The notable resolutions: - **`NugetService.cs` takes the migrated implementation wholesale.** The only two commits that touched it after the revert (#671, #715) added seams and env-var endpoint overrides that exist solely to serve the hand-rolled implementation, so nothing real is dropped. - **`PackageInstallationServiceTests.cs` / `UpdateCommandTests.cs`** were add/add conflicts. Main's versions came from #671 and cover the "already present, resolve deps via feed" branch and the string-inequality update gate, both of which the migration deletes. The migrated versions win; one still-relevant test from main (`UpdateCommand_ExposesNameShortDescriptionAndSetupSdksOption`) was ported forward. - **Deleted `NugetServiceOfflineTests.cs`** (748 lines). Every method targeted the removed `HttpGetAsync` / `GetUserProfileDirectory` seams. The restored NuGet suite is a superset for everything still reachable. - **Interface drift ported into the restored tests**: `IsPackageInstalled` on `FakeSignToolNugetService`, the widened `IBuildToolsService.RunBuildToolAsync` signature, and the runtime-install step moving from `IWorkspaceSetupService` to `IWindowsAppRuntimeService`. ### Feed configuration, and why the pipeline env vars go away #715 added `WINAPP_NUGET_FLAT_CONTAINER` / `_REGISTRATION` / `_AUTH_PREFIX` so the network-isolated ADO legs could reach `pde-oss_Internal`. Those variables only ever configured the hand-rolled endpoints, which no longer exist. `ci.yml` and `release.yml` already copy `release-nuget.config` to both `%APPDATA%\NuGet\NuGet.Config` and the repo root and then run `NuGetAuthenticate@1`, and the NuGet client picks all of that up on its own, including the credential provider. So the env blocks are removed from the `Build CLI` and `Bundle MSIX Packages` steps. The one place that still needed an explicit feed is the live-feed integration tests. Those helpers used to hand-roll registration and flat-container HTTP against a hardcoded `api.nuget.org`; they now go through `PackageMetadataResource` / `FindPackageByIdResource` against the same source the test config declares, and that source is overridable with a new `WINAPP_TEST_NUGET_SOURCE`. `AGENTS.md` is updated with the `nuget.config`-based equivalent for corp machines, replacing the old env-var recipe. ### Follow-on fixes from review and from testing the branch These came out of the review passes and from running the branch against a real app, and are in the later commits rather than the re-land itself: - **A fully cached dependency graph now restores without querying sources** (#762). Reading the dependency *list* from a cached package's local `.nuspec` was not enough, because each declared range was still resolved to a concrete version against the feeds only. So an already-extracted graph failed offline, or under a `packageSourceMapping` that no longer maps a transitive package anywhere. Resolution now falls back to the lowest fully installed cache entry (completion marker required) before failing. Deliberately confined to the failure paths, so a reachable source always decides the version and online restores select exactly what they did before. - **`winapp restore` works after `winapp init` on a .NET project.** For .NET projects `init` records versions as PackageReferences rather than a `winapp.yaml`, and `restore` then exited 1 telling the user to run `dotnet restore` by hand. It runs it instead. - **Feed failures explain themselves.** On a project whose `nuget.config` pins a blocked or unreachable feed, `init` reported a bare `Failed to get version for Microsoft.WindowsAppSDK`; the NuGet layer's message naming the source and the reason was logged at debug level only. It is now surfaced and included in the returned error. - **An unsatisfiable diamond involving a float and an exact prerelease pin is no longer accepted.** `RangesHaveCommonVersion` decides a single-point intersection exactly, since prerelease eligibility is not expressible as a numeric interval (`1.*` and `[1.5.0-preview]` intersect at a non-empty point yet share no version). Kept narrow on purpose: widening the existing witness heuristic instead would wrongly fail cases like `1.*` with `(1.2.0, 1.8.0)`. - Code-quality findings: deterministic disposal of the test listeners' port probes, cleanup catches narrowed to the exceptions those blocks exist to swallow, explicit `.Where(...)` filters, and a guard on the one `Path.Combine` whose segment comes from an externally supplied package id. ## Usage Example The behavior this restores: `winapp restore` (and `init` / `update`) picks up a `nuget.config` like this and restores the SDK packages from a private mirror. ```xml <?xml version="1.0" encoding="utf-8"?> <configuration> <packageSources> <clear /> <add key="contoso" value="https://pkgs.dev.azure.com/contoso/_packaging/winsdk-mirror/nuget/v3/index.json" /> </packageSources> </configuration> ``` ```powershell winapp restore ``` ## Related Issue Closes #653. Closes #762. Re-lands #629 and #647, reverting #654. ## Type of Change - ✨ New feature - 🐛 Bug fix - ♻️ Refactoring - 🔧 Config/build ## Checklist - [x] New tests added for new functionality (if applicable) - [x] Tested locally on Windows - [x] [docs/usage.md](../docs/usage.md) updated (if CLI commands changed) - [x] Shipped skills updated in `plugins/winapp/skills/` (if CLI commands/workflows changed) ## Additional Notes **Validation** - `dotnet build winapp.sln -c Debug -p:TreatWarningsAsErrors=true`: 0 warnings, 0 errors. - Full suite green on two consecutive runs: 4643 total, 4638 passed, 5 skipped, 0 failed. - Release Native AOT publish clean for both `win-x64` and `win-arm64`. Confirmed `NuGet.UseSystemTextJsonDeserialization` still reaches ILC as a trim feature on 7.9.0 stable (`--feature:` and `--runtimeknob:` both present in the generated `.ilc.rsp`), and no Newtonsoft.Json assets land in the published output. - End to end against the published AOT binary: `winapp init` in a directory whose `nuget.config` clears nuget.org, declares a single custom mirror, and sets `globalPackagesFolder` restored all 18 SDK packages from that mirror into the project-local folder. This exercises the actual feature, not just the build. - The #762 tests were verified non-vacuous: the offline and mapping-excluded cases fail without the fix and pass with it. **For reviewers** The highest-risk part is the conflict resolution, not the migration itself, since the migration already went through review in #629. The resolutions above are the places worth a careful look. I also ran an independent review pass over the conflict resolution; it confirmed `NugetService.cs` and friends are byte-identical to their pre-revert state apart from the documented `UpdateCommand` port-forward, and it caught the dropped `UpdateCommandTests` metadata test that is now restored. `NugetServiceCachedGraphTests` is marked `[DoNotParallelize]`: it warms a real global-packages folder, then reconfigures and re-reads it, and that two-phase sequence proved intermittently sensitive to the 32-way parallel test run. Two deliberate omissions: running `scripts/build-cli.ps1` also regenerates `docs/npm-usage.md` (line endings only) and bumps a stale schema-version string in `src/winapp-npm/src/winapp-commands.ts` from `0.5.1` to `0.6.1`. Both are pre-existing drift on main and unrelated to this change, so they are left out to keep the diff focused. ## Screenshots / Demo N/A ## AI Description <!-- ai-description-start --> _This section is auto-generated by AI when the PR is opened or updated. To opt out, delete this entire section including the marker comments._ <!-- ai-description-end --> --------- Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Co-authored-by: Nikola Metulev <nmetulev@users.noreply.github.com> Copilot-Session: e5368fbc-50a6-45dc-b933-31d87643b19a
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.