[fix] Fix backslash normalization in VSTestCLIRunSettings on Unix#15795
[fix] Fix backslash normalization in VSTestCLIRunSettings on Unix#15795nohwnd wants to merge 4 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:
|
|
Done ✅ Removed
|
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 🧠
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.