Repository navigation
Invoke build tools directly in sample tests instead of Invoke-Expression - #726
Merged
Nikola Metulev (nmetulev) merged 9 commits intoAug 12, 2026
Merged
Conversation
The sample Pester tests ran dotnet, npm, npx, and cargo by composing a command string and passing it to Invoke-Expression, and Invoke-WinappCommand did the same for the CLI itself. Temp directory paths flow into those strings, so a path containing a quote or other parser-significant character would be re-interpreted as syntax rather than passed through as data. Call the executables directly. Invoke-WinappCommand keeps its single -Arguments string parameter so all 71 call sites are unchanged; a small ConvertTo-ArgumentList tokenizer splits that string into discrete arguments, honoring single and double quotes, and the result is splatted onto the resolved executable.
Copilot started reviewing on behalf of
Alexandre Zollinger Chohfi (azchohfi)
August 11, 2026 18:57
View session
Contributor
There was a problem hiding this comment.
Pull request overview
Removes Invoke-Expression from sample tests and invokes build tools directly.
Changes:
- Directly invokes
dotnet,npm,npx, andcargo. - Adds argument tokenization for
Invoke-WinappCommand. - Passes npm package paths directly.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
samples/SampleTestHelpers.psm1 |
Reworks CLI and npm invocation. |
samples/dotnet-app/test.Tests.ps1 |
Directly invokes dotnet. |
samples/electron/test.Tests.ps1 |
Directly invokes npm and npx tools. |
samples/rust-app/test.Tests.ps1 |
Directly invokes Cargo. |
samples/sparse-app/test.Tests.ps1 |
Directly invokes dotnet. |
samples/tauri-app/test.Tests.ps1 |
Directly invokes npm and Cargo. |
samples/winui-app/test.Tests.ps1 |
Directly invokes dotnet. |
samples/winui-solution/test.Tests.ps1 |
Directly invokes dotnet. |
samples/winui-unpackaged-app/test.Tests.ps1 |
Directly invokes dotnet. |
samples/wpf-app/test.Tests.ps1 |
Directly invokes dotnet. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+82
to
+85
| } elseif ($ch -eq '"' -or $ch -eq "'") { | ||
| $quote = $ch | ||
| $hasContent = $true | ||
| } elseif ([char]::IsWhiteSpace($ch)) { |
Contributor
Build Metrics ReportBinary Sizes
Test Results✅ 4555 passed, 5 skipped out of 4560 tests in 584.7s (+1 test, -133.7s vs. baseline) Test Coverage✅ 89.1% line coverage, 82.4% branch coverage · ✅ no change vs. baseline CLI Startup Time46ms median (x64, Try This BuildInstalls 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://raw.githubusercontent.com/microsoft/winappCli/main/scripts/winapp-pr.ps1))) 726Switching between builds often?Put the tool on your PATH once: & ([scriptblock]::Create((irm https://raw.githubusercontent.com/microsoft/winappCli/main/scripts/winapp-pr.ps1))) -AddToPathThen this build is just: winapp-pr 726Run Updated 2026-08-12 18:53:16 UTC · commit |
PowerShell unrolls a single-element array on output, so a one-token command such as "restore" came back as a String rather than String[]. Splatting a scalar string passes it one character at a time, turning `winapp restore` into `winapp r e s t o r e`, which failed with a non-zero exit. This only affected callers that resolve winapp from PATH; the npx branch prepends an element and so always had an array, which is why cpp-app and flutter-app failed while electron passed. Return with a unary comma so the array survives the pipeline.
ConvertTo-ArgumentList carries the whole risk of this change and nothing tested it directly, which is how the single-element unrolling defect reached CI. The sample matrix catches it, but only after a full run. Add scripts/tests/SampleTestHelpers.Tests.ps1 covering the return shape, the splat behavior that actually broke, and the argument forms the 71 call sites use. Point the existing scripts Pester run at the scripts/tests directory rather than the single MS Learn file so anything added there runs too.
Alexandre Zollinger Chohfi (azchohfi)
marked this pull request as draft
August 11, 2026 22:51
Alexandre Zollinger Chohfi (azchohfi)
marked this pull request as ready for review
August 12, 2026 04:35
Nikola Metulev (nmetulev)
deleted the
azchohfi-sample-tests-direct-invocation
branch
August 12, 2026 23:32
This was referenced Sep 24, 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
The sample Pester tests invoked
dotnet,npm,npx, andcargoby composing a command string and handing it toInvoke-Expression:Invoke-WinappCommandinSampleTestHelpers.psm1did the same for the CLI, including interpolating a resolved project path into thedotnet runfallback. Temp directory paths flow through these strings, so a path containing a quote or other parser-significant character would be re-interpreted as syntax rather than passed through as data.How
Call the executables directly, so arguments never round-trip through the parser.
Invoke-WinappCommandkeeps its existing single-Argumentsstring parameter, so all 71 call sites are unchanged. A smallConvertTo-ArgumentListhelper splits that string into discrete arguments — honoring single and double quotes — and the result is splatted onto the resolved executable:Changing every call site to pass arrays would have been the alternative, but it touches 71 lines across 14 files for no additional safety, since the tokenizer reproduces the same splitting the parser was doing.
Validation
All 15 sample scripts parse clean. The tokenizer was checked against the argument shapes actually used in these tests, including quoted paths with spaces and quoted values containing
=:Test-only change; no product code is touched.