[fix] Fix: use MSBuild output mode automatically with dotnet msbuild /t:VSTest when terminal logger is active by nohwnd · Pull Request #16058 · microsoft/vstest

nohwnd · 2026-05-24T13:04:38Z

🤖 This is an automated fix generated by the Issue Triage agent.

Root Cause

When running dotnet msbuild /t:VSTest with the MSBuild terminal logger enabled (_MSBUILDTLENABLED == '1'), test output from parallel MSBuild nodes was silently lost.

The cause: VsTestUseMSBuildOutput defaulted to False, so _VSTestConsole was always used. This target invokes vstest.console directly and writes to stdout — output that the terminal logger cannot properly attribute to individual projects when running on parallel MSBuild worker nodes.

Fix

Microsoft.TestPlatform.targets: When the terminal logger is active and the user has not set VsTestUseMSBuildOutput explicitly, default it to True. This causes _VSTestMSBuild (using VSTestTask2 / ToolTask) to be used, which routes output through MSBuild task logging — ensuring it is attributed and displayed correctly by the terminal logger.
Added $(MSBUILDENSURESTDOUTFORTASKPROCESSES) != '1' guard to the _VSTestMSBuild condition so it will not run when invoked from dotnet test (which sets this env var and uses _VSTestConsole for proper colorization).

Backward compatibility: Users who want the old console-colorized behavior can opt out with /p:VsTestUseMSBuildOutput=false.

Test

Added MSBuildLoggerIsUsedAutomaticallyWithDotnetMSBuildWhenTerminalLoggerIsActive acceptance test in DotnetTestMSBuildOutputTests that invokes dotnet msbuild -t:VSTest -tl:on without setting VsTestUseMSBuildOutput and asserts that test error output appears in stdout.

Also added InvokeDotnetMSBuildTest helper to IntegrationTestBase to support invoking dotnet msbuild with the patched dotnet in acceptance tests.

…est when terminal logger is active When running 'dotnet msbuild /t:VSTest' with the terminal logger enabled, test output from parallel MSBuild nodes was silently lost because VSTestTask wrote directly to stdout rather than through MSBuild's task logging system. Fix: default VsTestUseMSBuildOutput to True when the terminal logger is active (_MSBUILDTLENABLED == '1') so that VSTestTask2 (ToolTask-based) is used automatically. This routes test output through MSBuild's logging infrastructure, ensuring it is properly attributed and displayed. Users who need the old console-colorized behavior can opt out with /p:VsTestUseMSBuildOutput=false. The _VSTestMSBuild condition also now excludes the MSBUILDENSURESTDOUTFORTASKPROCESSES == '1' case (set by dotnet test) to avoid running both paths simultaneously. Fixes #5156 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot

Pull request overview

This PR updates the MSBuild VSTest target selection logic so that when the MSBuild terminal logger is enabled, VSTest defaults to using MSBuild task-based logging output (instead of spawning vstest.console directly), preventing test output loss from parallel MSBuild nodes when running dotnet msbuild /t:VSTest.

Changes:

Default VsTestUseMSBuildOutput to True when the MSBuild terminal logger is active and the user hasn’t set the property explicitly.
Add a guard to keep using the console path when MSBUILDENSURESTDOUTFORTASKPROCESSES=1 (e.g., dotnet test scenario).
Add an acceptance test plus a new dotnet msbuild invocation helper for acceptance tests.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File	Description
`src/Microsoft.TestPlatform.Build/Microsoft.TestPlatform.targets`	Adjusts defaulting/target-selection logic to favor MSBuild task logging when terminal logger is active.
`test/Microsoft.TestPlatform.TestUtilities/IntegrationTestBase.cs`	Adds a helper to invoke `dotnet msbuild` via the patched dotnet used by acceptance tests.
`test/Microsoft.TestPlatform.Acceptance.IntegrationTests/DotnetTestMSBuildOutputTests.cs`	Adds an acceptance test intended to validate the new default behavior under terminal logger.

    <CallTarget Targets="_VSTestConsole" Condition="$(_MSBUILDTLENABLED) == '0' OR !$(VsTestUseMSBuildOutput) OR $(MSBUILDENSURESTDOUTFORTASKPROCESSES) == '1'" />
    <!-- Proper MSBuild integration, but no custom colorization -->
-    <!-- Use the new view whe terminal logger is enabled (_MSBUILDTLENABLED==0), and user did not opt out (VsTestUseMSBuildOutput == true) -->
-    <CallTarget Targets="_VSTestMSBuild" Condition="$(_MSBUILDTLENABLED) == '1' AND $(VsTestUseMSBuildOutput)" />
+    <!-- Use the new view when terminal logger is enabled (_MSBUILDTLENABLED==1), user did not opt out (VsTestUseMSBuildOutput == true),
+         and MSBUILDENSURESTDOUTFORTASKPROCESSES is not set (set by dotnet test, which uses the console path instead). -->
+    <CallTarget Targets="_VSTestMSBuild" Condition="$(_MSBUILDTLENABLED) == '1' AND $(VsTestUseMSBuildOutput) AND $(MSBUILDENSURESTDOUTFORTASKPROCESSES) != '1'" />


+    public void MSBuildLoggerIsUsedAutomaticallyWithDotnetMSBuildWhenTerminalLoggerIsActive(RunnerInfo runnerInfo)
+    {
+        SetTestEnvironment(_testEnvironment, runnerInfo);
+
+        var projectPath = GetIsolatedTestAsset("TerminalLoggerTestProject.csproj", runnerInfo.TargetFramework);
+        // Invoke via dotnet msbuild /t:VSTest with terminal logger on and no explicit VsTestUseMSBuildOutput property.
+        // The MSBuild output path should be used automatically when the terminal logger is active.
+        InvokeDotnetMSBuildTest($@"-t:VSTest {projectPath} -tl:on -nodereuse:false /p:PackageVersion={IntegrationTestEnvironment.LatestLocallyBuiltNugetVersion}", workingDirectory: Path.GetDirectoryName(projectPath));
+


nohwnd

Expert Review — PR #16058

The fix is small, focused, and the logic is correct. The two CallTarget conditions remain mutually exclusive across all relevant scenarios (TL on/off, dotnet test vs dotnet msbuild, user opt-out).

Key checks passed:

✅ No double-invocation possible — the _VSTestConsole and _VSTestMSBuild conditions are mutually exclusive
✅ Backward compatibility preserved — VsTestUseMSBuildOutput=False explicit opt-out still works; users on non-TL MSBuild are unaffected
✅ dotnet test isolation — the MSBUILDENSURESTDOUTFORTASKPROCESSES == '1' guard correctly routes dotnet test to _VSTestConsole
✅ Two-line property default ordering is correct — the _MSBUILDTLENABLED == '1' check sets True first, the unconditional fallback sets False only when still unset
✅ Acceptance test covers the exact scenario described in the fix

One minor finding (see inline): VSTEST_CONSOLE_PATH env var in InvokeDotnetMSBuildTest appears to be dead code for the dotnet msbuild path.

🧠 Reviewed by Expert Code Reviewer

nohwnd · 2026-05-24T13:09:35Z

+
+        debugEnvironmentVariables["VSTEST_CONSOLE_PATH"] = vstestConsolePath;
+
+        IntegrationTestBase.ExecutePatchedDotnet("msbuild", arguments, out _standardTestOutput, out _standardTestError, out _runnerExitCode, debugEnvironmentVariables, workingDirectory);


[Environment Variable & Feature Flag Contracts] VSTEST_CONSOLE_PATH appears to be used by the dotnet test / _VSTestConsole path, not the dotnet msbuild / _VSTestMSBuild path (which reads VsTestConsolePath from the MSBuild property already appended as -p:VsTestConsolePath=...). Setting this env var here looks like dead code.

If ExecutePatchedDotnet/_VSTestMSBuild doesn't read VSTEST_CONSOLE_PATH, removing the line would be a clean-up. If it is read somewhere in the MSBuild task, a comment explaining why it's needed would help future readers.

This doesn't affect correctness (the MSBuild property path already ensures the right vstest.console is used), but it adds confusion.

Good catch — removed the dead VSTEST_CONSOLE_PATH assignment. The MSBuild property -p:VsTestConsolePath already handles routing the correct console path.

🔧 Iterated by PR Iteration Agent 🔧

The MSBuild path already uses -p:VsTestConsolePath to pass the console path as an MSBuild property. Setting VSTEST_CONSOLE_PATH env var here is dead code. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

nohwnd · 2026-05-24T13:13:27Z

Commit pushed: 8677ca1

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

nohwnd · 2026-05-28T06:00:46Z

Commit pushed: 07a6ff6

The test invoked 'dotnet msbuild -t:VSTest' without building the project first, so the test DLL did not exist and vstest.console failed with a file-not-found error instead of running tests. Change to '-t:Build;VSTest' to ensure the project is compiled before the test run, matching the typical real-world usage pattern. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

nohwnd · 2026-05-28T07:10:48Z

Commit pushed: 5911482

Copilot

Pull request overview

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

nohwnd

Expert Review — PR #16058 (re-review after iteration)

This is a follow-up review covering commits pushed after the initial review on 2026-05-24.

New commits reviewed:

8677ca1 — Removed dead VSTEST_CONSOLE_PATH env var from InvokeDotnetMSBuildTest (addresses the finding from the prior review ✅)
07a6ff6 — CI retrigger only, no code changes
5911482 — Fixed acceptance test to use -t:Build;VSTest instead of -t:VSTest; the project was not compiled before test execution, causing the DLL to be missing ✅

All prior findings resolved. The PR is clean:

✅ .targets logic: _VSTestConsole and _VSTestMSBuild remain mutually exclusive across all scenarios
✅ VsTestUseMSBuildOutput two-step default ordering is correct
✅ MSBUILDENSURESTDOUTFORTASKPROCESSES == '1' guard correctly isolates dotnet test from dotnet msbuild
✅ InvokeDotnetMSBuildTest passes console path via MSBuild property (correct for the VSTestConsolePath lookup in targets)
✅ Acceptance test now builds before running tests via -t:Build;VSTest

🧠 Reviewed by Expert Code Reviewer 🧠

The acceptance test was using -tl:on to activate the terminal logger, but in non-interactive (non-TTY) CI environments, the terminal logger may not set _MSBUILDTLENABLED=1 even when explicitly requested. This caused the auto-detection in Microsoft.TestPlatform.targets to not trigger, making VsTestUseMSBuildOutput remain False and the test assertions fail. Switch to explicitly setting /p:_MSBUILDTLENABLED=1 to directly simulate the terminal logger being active. This tests the auto-detection mechanism reliably regardless of TTY availability. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

nohwnd · 2026-06-02T06:18:41Z

Commit pushed: 05409a9

nohwnd

Expert Review — PR #16058 (re-review after latest commit)

This is a follow-up review covering the new commit pushed on 2026-06-02.

New commit reviewed:

05409a9 — Switch acceptance test from -tl:on to /p:_MSBUILDTLENABLED=1

Analysis:

The change is correct and well-motivated. -tl:on enables the MSBuild terminal logger, but in non-TTY CI environments MSBuild may suppress the terminal logger even when explicitly requested, leaving _MSBUILDTLENABLED unset. This would cause the auto-detection path in Microsoft.TestPlatform.targets to not trigger, making VsTestUseMSBuildOutput remain False and the test assertions fail spuriously.

By directly injecting /p:_MSBUILDTLENABLED=1, the test bypasses the TTY detection and exercises the auto-detection logic deterministically — exactly what an integration test should do.

✅ No code changes to .targets — the fix is purely a test reliability improvement
✅ The comment in the test file accurately explains why /p:_MSBUILDTLENABLED=1 is preferred over -tl:on
✅ All prior findings remain resolved; no new issues introduced

All checks passed. The PR is clean and ready for human review/merge.

🧠 Reviewed by Expert Code Reviewer 🧠

The _VSTestMSBuild target condition included ' == '1'' which made it untestable without a real terminal logger TTY. Since VsTestUseMSBuildOutput already encodes the routing decision (set to True by auto-detection when _MSBUILDTLENABLED=1, or explicitly by the user), the _MSBUILDTLENABLED check in the CallTarget conditions was redundant. Simplified the conditions to: - _VSTestConsole: runs when VsTestUseMSBuildOutput=false OR MSBUILDENSURESTDOUTFORTASKPROCESSES=1 - _VSTestMSBuild: runs when VsTestUseMSBuildOutput=true AND MSBUILDENSURESTDOUTFORTASKPROCESSES!=1 Updated the acceptance test to set VsTestUseMSBuildOutput=true directly instead of /p:_MSBUILDTLENABLED=1 (which can't be reliably set via /p: since MSBuild treats _MSBUILDTLENABLED as an internal property). Also renamed the test to better reflect what it actually tests. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

nohwnd · 2026-06-03T23:01:56Z

Commit pushed: dfc046c

Copilot

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

+    <!-- Use the MSBuild path when MSBuild output is enabled (set automatically when terminal logger is active, or explicitly by the user).
+         Not used when invoked via dotnet test (MSBUILDENSURESTDOUTFORTASKPROCESSES=1). -->
+    <CallTarget Targets="_VSTestMSBuild" Condition="$(VsTestUseMSBuildOutput) AND $(MSBUILDENSURESTDOUTFORTASKPROCESSES) != '1'" />


+    public void MSBuildLoggerIsUsedWithDotnetMSBuildWhenVsTestUseMSBuildOutputIsEnabled(RunnerInfo runnerInfo)
+    {
+        SetTestEnvironment(_testEnvironment, runnerInfo);
+
+        var projectPath = GetIsolatedTestAsset("TerminalLoggerTestProject.csproj", runnerInfo.TargetFramework);
+        // Invoke via dotnet msbuild /t:Build;VSTest with VsTestUseMSBuildOutput=true to force the MSBuild output path.
+        // VSTestTask2 routes test failure output through MSBuild error/warning messages rather than writing directly
+        // to stdout, so errors appear as "error TESTERROR: ..." in the MSBuild output.
+        // In real usage, VsTestUseMSBuildOutput is auto-detected from _MSBUILDTLENABLED when the terminal logger is
+        // active; here we set it explicitly to avoid a dependency on terminal logger TTY detection in CI.
+        InvokeDotnetMSBuildTest($@"-t:Build;VSTest {projectPath} -nodereuse:false /p:PackageVersion={IntegrationTestEnvironment.LatestLocallyBuiltNugetVersion} /p:VsTestUseMSBuildOutput=true", workingDirectory: Path.GetDirectoryName(projectPath));


nohwnd

Expert Review — PR #16058 (re-review after latest commit)

This is a follow-up review covering the new commit pushed on 2026-06-03.

New commit reviewed:

dfc046c — Simplify VSTest target conditions and fix acceptance test

Analysis:

The commit removes the redundant $(_MSBUILDTLENABLED) checks from the _VSTestConsole and _VSTestMSBuild CallTarget conditions. Since VsTestUseMSBuildOutput is already set by the PropertyGroup auto-detection (True when _MSBUILDTLENABLED == '1', False otherwise), the _MSBUILDTLENABLED check in the routing conditions was truly redundant.

Simplified conditions are correct and mutually exclusive:

_VSTestConsole: !$(VsTestUseMSBuildOutput) OR $(MSBUILDENSURESTDOUTFORTASKPROCESSES) == '1'
_VSTestMSBuild: $(VsTestUseMSBuildOutput) AND $(MSBUILDENSURESTDOUTFORTASKPROCESSES) != '1'

All routing scenarios remain correct:

✅ TL active, not opted out → VsTestUseMSBuildOutput=True → _VSTestMSBuild
✅ TL not active → VsTestUseMSBuildOutput=False → _VSTestConsole
✅ User explicit VsTestUseMSBuildOutput=True → _VSTestMSBuild (unless dotnet test)
✅ User explicit VsTestUseMSBuildOutput=False → _VSTestConsole
✅ dotnet test (MSBUILDENSURESTDOUTFORTASKPROCESSES=1) → always _VSTestConsole

Acceptance test: Switching from /p:_MSBUILDTLENABLED=1 (internal MSBuild property, not reliably settable via /p:) to /p:VsTestUseMSBuildOutput=true is the correct approach — it tests the routing decision directly without depending on MSBuild internals. ✅

All prior findings remain resolved. No new issues introduced.

🧠 Reviewed by Expert Code Reviewer 🧠

and % through the MSBuild terminal-logger output pipeline, and add matching Unescape in VSTestTask2. Add FailingTestWithSpecialChars test asset and update acceptance-test counts from 3->4 total tests / 1->2 failing. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

nohwnd · 2026-06-05T06:49:15Z

Commit pushed: 619d447

…ve duplicate GetDiagLogContents PR #16055 refactored IntegrationTestBase.cs to: - Add DiagLogsDirectory property - Refactor IsDiagAlreadyEnabled into ThrowIfDiagInArguments (throws) + IsDiagInEnvironment - Add DiagFlagPattern compiled regex - Auto-inject --diag before -- separator in InvokeVsTest/InvokeDotnetTest Preserved InvokeDotnetMSBuildTest added by this PR. Also removed the duplicate GetDiagLogContents() private methods and manual /Diag path arguments from CreateNoNewWindowTests and SerializerSelectionTests since they now rely on the base class implementation. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

nohwnd · 2026-06-05T07:10:13Z

Commit pushed: f5502a8

Copilot

Pull request overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.

+        var projectPath = GetIsolatedTestAsset("TerminalLoggerTestProject.csproj", runnerInfo.TargetFramework);
+        // Invoke via dotnet msbuild /t:Build;VSTest with VsTestUseMSBuildOutput=true to force the MSBuild output path.
+        // VSTestTask2 routes test failure output through MSBuild error/warning messages rather than writing directly
+        // to stdout, so errors appear as "error TESTERROR: ..." in the MSBuild output.
+        // In real usage, VsTestUseMSBuildOutput is auto-detected from _MSBUILDTLENABLED when the terminal logger is
+        // active; here we set it explicitly to avoid a dependency on terminal logger TTY detection in CI.
+        InvokeDotnetMSBuildTest($@"-t:Build;VSTest {projectPath} -nodereuse:false /p:PackageVersion={IntegrationTestEnvironment.LatestLocallyBuiltNugetVersion} /p:VsTestUseMSBuildOutput=true", workingDirectory: Path.GetDirectoryName(projectPath));


+    /// <summary>
+    /// Reverses MSBuildLogger.Escape. Single-pass scanner to correctly handle
+    /// sequences like <c>%%n</c> (literal <c>%n</c>, not <c>%</c> + newline).
+    /// </summary>
+    private static string? Unescape(string? input)


+        ThrowIfDiagInArguments(arguments ?? "");
+        if (collectDiagnostics && !IsDiagInEnvironment())


+        ThrowIfDiagInArguments(arguments);
+        if (collectDiagnostics && !IsDiagInEnvironment())
        {


…mmented-out assertions) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

nohwnd · 2026-06-09T06:22:56Z

Commit pushed: 8677fe2

nohwnd

Expert Review — PR #16058 (re-review after latest commits)

This is a follow-up review covering 3 new commits pushed since the previous review on 2026-06-03.

New commits reviewed:

619d447ada — Sync escaping and test counts with main (PR #16046): new %-based Escape/Unescape in MSBuildLogger + VSTestTask2
f5502a8f4a — Sync with main (PR #16055): IntegrationTestBase refactoring (DiagLogsDirectory, ThrowIfDiagInArguments, GetDiagLogContents centralized)
8677fe2c67 — Add commented-out assertions to MSBuildLoggerCanBeDisabledByBuildProperty test

`%`-based Escape/Unescape (`619d447`)

The new escaping scheme is correct and lossless:

% is escaped first (% → %%) before the other substitutions, preventing double-encoding of newly-introduced %p/%r/%n escape sequences produced by subsequent .Replace() calls. ✅
Unescape is a single-pass scanner, correctly handling edge cases like %%n (literal %n, not % followed by newline). ✅
The default: sb.Append('%'); sb.Append(next) passthrough in Unescape is forward-compatible: unknown sequences survive round-trips verbatim, preventing silent corruption if Escape is extended in the future. ✅
The old scheme was lossy (~~~~~ → ____, !!!! → ____, |||| → ____ with no way to decode back). The fix is correct. ✅

One maintenance observation: Unescape is duplicated — an identical copy exists in MSBuildLogger.cs (marked internal) and VSTestTask2.cs (marked private static). This is architecturally necessary since the two are in different assemblies, but means that if Escape is ever extended with a new sequence (e.g., %t for tab), both Unescape implementations must be updated in lockstep. Each file has a cross-reference comment pointing to the other, which helps. No blocking issue.

`IntegrationTestBase` refactoring (`f5502a8`)

ThrowIfDiagInArguments is intentionally stricter than the old IsDiagAlreadyEnabled: it throws instead of silently skipping if /Diag: is found in arguments, enforcing the auto-injection contract. The PR updates all known callers (CreateNoNewWindowTests, SerializerSelectionTests). ✅
DiagFlagPattern regex is more precise than the old string.Contains check — avoids false positives like --diagnostics being treated as a diag flag. ✅
InvokeDotnetMSBuildTest deliberately omits both ThrowIfDiagInArguments and diag injection — correct, since dotnet msbuild doesn't forward --diag to vstest.console. ✅
GetDiagLogContents promoted to base class using SearchOption.TopDirectoryOnly — semantically equivalent to the removed per-test duplicates. ✅

All prior findings remain resolved. No new issues introduced.

🧠 Reviewed by Expert Code Reviewer 🧠

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

nohwnd · 2026-06-10T06:52:53Z

Commit pushed: 527758c

Copilot

Pull request overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.

    public void InvokeVsTest(string? arguments, Dictionary<string, string?>? environmentVariables = null, bool collectDiagnostics = true)
    {
-        if (collectDiagnostics && !IsDiagAlreadyEnabled(arguments ?? ""))
+        ThrowIfDiagInArguments(arguments ?? "");
+        if (collectDiagnostics && !IsDiagInEnvironment())
        {


        debugEnvironmentVariables["VSTEST_CONSOLE_PATH"] = vstestConsolePath;

-        if (collectDiagnostics && !IsDiagAlreadyEnabled(arguments))
+        ThrowIfDiagInArguments(arguments);
+        if (collectDiagnostics && !IsDiagInEnvironment())
        {


+        var projectPath = GetIsolatedTestAsset("TerminalLoggerTestProject.csproj", runnerInfo.TargetFramework);
+        // Invoke via dotnet msbuild -t:Build;VSTest with VsTestUseMSBuildOutput=true to force the MSBuild output path.
+        // VSTestTask2 routes test failure output through MSBuild error/warning messages rather than writing directly
+        // to stdout, so errors appear as "error TESTERROR: ..." in the MSBuild output.
+        // In real usage, VsTestUseMSBuildOutput is auto-detected from _MSBUILDTLENABLED when the terminal logger is
+        // active; here we set it explicitly to avoid a dependency on terminal logger TTY detection in CI.
+        InvokeDotnetMSBuildTest($@"-t:Build;VSTest {projectPath} -nodereuse:false /p:PackageVersion={IntegrationTestEnvironment.LatestLocallyBuiltNugetVersion} /p:VsTestUseMSBuildOutput=true", workingDirectory: Path.GetDirectoryName(projectPath));


+    /// <summary>
+    /// Reverses MSBuildLogger.Escape. Single-pass scanner to correctly handle
+    /// sequences like <c>%%n</c> (literal <c>%n</c>, not <c>%</c> + newline).
+    /// </summary>
+    private static string? Unescape(string? input)
+    {
+        if (input == null)
+        {
+            return null;
+        }
+
+        var sb = new StringBuilder(input.Length);
+        for (int i = 0; i < input.Length; i++)
+        {
+            if (input[i] == '%' && i + 1 < input.Length)
+            {
+                char next = input[++i];
+                switch (next)
+                {
+                    case '%': sb.Append('%'); break;
+                    case 'p': sb.Append('|'); break;
+                    case 'r': sb.Append('\r'); break;
+                    case 'n': sb.Append('\n'); break;
+                    default: sb.Append('%'); sb.Append(next); break;
+                }
+            }
+            else
+            {
+                sb.Append(input[i]);
+            }
+        }
+
+        return sb.ToString();
+    }


nohwnd

Expert Review — PR #16058 (re-review after latest commit)

This is a follow-up review covering the new commit pushed on 2026-06-10.

New commit reviewed:

527758c0 — Fix: use consistent -t: syntax in test comment (matching code style)

Analysis:

The commit is a single-line comment-only change in DotnetTestMSBuildOutputTests.cs. The comment in MSBuildLoggerIsUsedWithDotnetMSBuildWhenVsTestUseMSBuildOutputIsEnabled previously used /t:Build;VSTest (MSBuild-native flag syntax) while the actual invocation immediately below it used -t:Build;VSTest (dotnet CLI style). The commit aligns the comment to match the code style. No logic changes.

All prior findings remain resolved. No new issues introduced.

✅ .targets logic: _VSTestConsole and _VSTestMSBuild conditions remain mutually exclusive
✅ VsTestUseMSBuildOutput two-step default ordering is correct
✅ MSBUILDENSURESTDOUTFORTASKPROCESSES == '1' guard correctly isolates dotnet test from dotnet msbuild
✅ %-based Escape/Unescape round-trip is lossless and forward-compatible
✅ Acceptance test uses -t:Build;VSTest and /p:VsTestUseMSBuildOutput=true correctly
✅ Comment now consistently uses -t: style matching the invocation

🧠 Reviewed by Expert Code Reviewer 🧠

… before build dotnet msbuild does not implicitly restore NuGet packages (unlike dotnet test or dotnet build). Without -restore, isolated test assets created by GetIsolatedTestAsset() fail with NETSDK1004 (project.assets.json not found). This was the root cause of the persistent CI failure in MSBuildLoggerIsUsedWithDotnetMSBuildWhenVsTestUseMSBuildOutputIsEnabled. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

nohwnd · 2026-06-11T07:28:10Z

Commit pushed: 426e4ca

nohwnd · 2026-06-11T07:28:14Z

Fix pushed (2f28a85)

Root cause: InvokeDotnetMSBuildTest called dotnet msbuild -t:Build;VSTest without restoring packages first. Unlike dotnet test or dotnet build, plain dotnet msbuild does not implicitly restore. Isolated test assets created by GetIsolatedTestAsset() have no pre-existing project.assets.json, so the build failed with NETSDK1004 on every CI run — the test has never passed.

Fix: Added -restore flag so the call becomes dotnet msbuild -restore -t:Build;VSTest ..., which runs NuGet restore before building.

nohwnd

Expert Review — PR #16058 (re-review after latest commit)

This is a follow-up review covering the new commit pushed on 2026-06-11.

New commit reviewed:

426e4ca8 — Add -restore to InvokeDotnetMSBuildTest so packages are restored before build

Analysis:

The commit adds a single -restore flag prepended to the dotnet msbuild invocation in InvokeDotnetMSBuildTest. dotnet msbuild -restore is a well-known MSBuild global switch (added in MSBuild 15.5+) that runs a separate Restore build pass before the specified targets. This is the canonical workaround for NETSDK1004 (project.assets.json not found) when using dotnet msbuild directly — unlike dotnet build or dotnet test, dotnet msbuild does not implicitly restore NuGet packages.

✅ The fix is correct: -restore causes MSBuild to generate project.assets.json before the Build;VSTest targets run, unblocking isolated test assets from GetIsolatedTestAsset()
✅ Placement is safe: -restore is a global switch and can appear anywhere in the arguments list; prepending it before user args has no side effects
✅ No logic changes — only InvokeDotnetMSBuildTest is affected, and the change is additive
✅ All prior findings remain resolved; no new issues introduced

All checks passed. The PR is clean and ready for human review/merge.

🧠 Reviewed by Expert Code Reviewer 🧠

Copilot AI review requested due to automatic review settings May 24, 2026 13:04

Copilot started reviewing on behalf of nohwnd May 24, 2026 13:04 View session

Copilot AI reviewed May 24, 2026

View reviewed changes

nohwnd commented May 24, 2026

View reviewed changes

This was referenced May 25, 2026

Daily Maintenance Digest — 2026-05-25 #16049

Closed

Daily Maintenance Digest — 2026-06-11 #16066

Closed

Retrigger CI

07a6ff6

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

nohwnd mentioned this pull request May 28, 2026

[fix] Fix: use MSBuild output mode automatically with dotnet msbuild /t:VSTest when terminal logger is active #16057

Closed

Copilot AI review requested due to automatic review settings May 28, 2026 07:10

Copilot started reviewing on behalf of nohwnd May 28, 2026 07:11 View session

Copilot AI reviewed May 28, 2026

View reviewed changes

nohwnd commented May 28, 2026

View reviewed changes