Skip to content

Polish project restore output modes - #791

Closed
Nikola Metulev (nmetulev) wants to merge 2 commits into
nmetulev-improve-run-progressfrom
nmetulev-polish-project-restore-output
Closed

Polish project restore output modes#791
Nikola Metulev (nmetulev) wants to merge 2 commits into
nmetulev-improve-run-progressfrom
nmetulev-polish-project-restore-output

Conversation

@nmetulev

Copy link
Copy Markdown
Member

Summary

  • pass quiet verbosity to project-mode dotnet restore while preserving default, JSON, and interactive terminal behavior
  • remove duplicate restore command text from verbose decision traces while keeping displayed commands redacted
  • add direct JSON/quiet restore routing regressions and update project-mode docs and setup guidance

Validation

  • dotnet test src\winapp-CLI\WinApp.Cli.Tests\WinApp.Cli.Tests.csproj -c Release --filter "FullyQualifiedName~ProjectRunServiceTests" (195 passed)
  • Release CLI against a throwaway two-project solution: quiet emitted no restore invocation or normal restore progress; verbose emitted one restore invocation
  • .\scripts\validate-plugin-package.ps1
  • git diff --check
  • .\scripts\build-cli.ps1 reached NativeAOT publish, then stopped at the machine's known missing Desktop Development for C++ linker prerequisite

Comment thread src/winapp-CLI/WinApp.Cli.Tests/ProjectRunServiceTests.cs Fixed
Comment thread src/winapp-CLI/WinApp.Cli.Tests/ProjectRunServiceTests.cs Fixed
Honor quiet verbosity during project restores, avoid duplicate verbose invocations, and document JSON and quiet routing.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@nmetulev
Nikola Metulev (nmetulev) force-pushed the nmetulev-improve-run-progress branch from de57385 to 3c6fd63 Compare August 26, 2026 23:22
Copilot AI balanced review requested due to automatic review settings August 26, 2026 23:22
@nmetulev
Nikola Metulev (nmetulev) force-pushed the nmetulev-polish-project-restore-output branch from 2526b8c to 00665be Compare August 26, 2026 23:22
Use scoped disposal for the JSON and quiet restore stderr capture writers.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Polishes project-mode restore output across default, verbose, quiet, JSON, and interactive modes.

Changes:

  • Applies quiet verbosity to restore commands when appropriate.
  • Removes duplicate verbose restore-command traces while retaining redacted invocation output.
  • Adds regression tests and updates user/setup documentation.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.

Show a summary per file
File Description
ProjectRunService.cs Resolves restore verbosity and simplifies debug traces.
ProjectRunService.Arguments.cs Supports optional restore verbosity arguments.
ProjectRunServiceTests.cs Tests quiet and JSON restore routing.
plugins/winapp/skills/winapp-setup/SKILL.md Documents output-mode behavior.
docs/usage.md Clarifies restore/build verbosity and routing.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

@github-actions

Copy link
Copy Markdown
Contributor

Build Metrics Report

Binary Sizes

Artifact Baseline Current Delta
CLI (ARM64) 38.63 MB 38.61 MB 📉 -22.5 KB (-0.06%)
CLI (x64) 38.75 MB 38.73 MB 📉 -20.5 KB (-0.05%)
MSIX (ARM64) 16.03 MB 16.02 MB 📉 -6.5 KB (-0.04%)
MSIX (x64) 17.02 MB 17.00 MB 📉 -19.5 KB (-0.11%)
NPM Package 33.43 MB 33.40 MB 📉 -28.4 KB (-0.08%)
NuGet Package 33.47 MB 33.45 MB 📉 -23.3 KB (-0.07%)

Test Results

4621 passed, 5 skipped out of 4626 tests in 567.1s (+5 tests, -190.4s vs. baseline)

Test Coverage

89.1% line coverage, 82.4% branch coverage · ✅ no change vs. baseline

CLI Startup Time

47ms median (x64, winapp --version) · ⚠️ +12ms vs. baseline

Try This Build

Installs the MSIX for your architecture, replacing any previously installed build. Needs the GitHub CLI — the command offers to install it and sign you in if it is missing.

& ([scriptblock]::Create((irm https://github.com/ghraw/microsoft/winappCli/main/scripts/winapp-pr.ps1))) 791
Switching between builds often?

Put the tool on your PATH once:

& ([scriptblock]::Create((irm https://github.com/ghraw/microsoft/winappCli/main/scripts/winapp-pr.ps1))) -AddToPath

Then this build is just:

winapp-pr 791

Run winapp-pr with no arguments to pick from a list of open PRs.


Updated 2026-08-26 23:46:12 UTC · commit 0e253b6 · workflow run

@azchohfi

Copy link
Copy Markdown
Collaborator

🤖 AI-generated review (winappcli pr-review skill) — verify before acting.

PR Review

Decision

Merge. No critical or high issues. I confirmed all four output modes behave as the updated docs describe by running the published CLI against a real WinUI app. One optional test-quality nit below.

Must fix

None — mergeable as-is.

Non-blocking

The new --json restore test asserts on a value the test built itself

  • What is wrong: The test builds restore arguments locally without passing a verbosity, then asserts those same arguments lack -v quiet. The assertion cannot fail, so it does not test production behavior.
  • Show me: Line 2946 calls BuildRestorePassArguments(solution, options) (no verbosity argument); line 2979 asserts that local args does not contain -v quiet. If ResolveRestoreVerbosity regressed to return "quiet" for JSON, the three production call sites would pass -v quiet — and this test would still pass.
  • Why it matters: This PR's purpose is pinning these output modes; a vacuous assertion leaves the --json half unpinned while reading as covered.
  • Smallest fix: Assert on the invocation recorded by a BuildAndResolveAsync run with Json: true, the way the sibling quiet test already does via dotnet.StreamingCalls.
  • Location: src/winapp-CLI/WinApp.Cli.Tests/ProjectRunServiceTests.cs:2946-2980

What was exercised

Harness: a real WinUI app (microsoft-ui-reactor's ReactorGallery — .slnx plus 8 project references), driven by a published Release winapp.exe rather than dotnet run.

  • dotnet publish -c Release — succeeded.
  • dotnet test --filter ProjectRunServiceTests — 195 passed, 0 failed.
  • winapp run (default) — stdout carries the banner and the exact dotnet restore … invocation, then live restore output. Matches the doc claim.
  • winapp run --quietstdout 0 bytes; restore/build output on stderr; zero occurrences of the invocation or the "Restoring … dependencies…" banner. Suppression confirmed.
  • winapp run --jsonstdout is 26 bytes of valid JSON ({"ProcessId":…}, parsed successfully); the invocation and ~9 KB of child output went to stderr. Confirms --json keeps default restore verbosity rather than quiet.
  • winapp run --verbose — still prints the exact dotnet restore … invocation plus the new decision trace. This addresses the main risk in the diff: replacing the LogDebug that carried the redacted arguments costs --verbose users nothing, because RunRestoreCommandAsync already echoes the invocation.
  • App built and launched end-to-end; processes cleaned up afterward.

Not exercised: the interactive-terminal path (dotnet's in-place terminal logger) — all runs redirected stdout/stderr, so only the plain-line/CI path was validated. Two of the three changed call sites (RunRestorePassAsync for SDK-less CsWinRT resolution, and the solution-sibling loop) were not hit at runtime; the gallery took the all-managed solution restore path. Both are covered by the passing unit tests and use the identical one-line expression.

@zateutsch Zach Teutsch (zateutsch) left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review was fully clean for me.

@nmetulev

Copy link
Copy Markdown
Member Author

Superseded by #789, which now includes the output-mode polish and all follow-up review fixes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants