Use MSBuild CommandLineParser for terminal logger detection#53835
Use MSBuild CommandLineParser for terminal logger detection#53835MichalPavlik wants to merge 1 commit intomainfrom
Conversation
Replace custom TerminalLoggerDetector parsing logic (TryFind, Switch) with Microsoft.Build.CommandLine.Experimental.CommandLineParser to support response files and align with MSBuild's switch parsing.
There was a problem hiding this comment.
Pull request overview
Updates dotnet test’s terminal logger detection to use MSBuild’s command-line parsing implementation, addressing cases where switches come from response files (e.g., Directory.Build.rsp) and aligning behavior with MSBuild switch parsing rules.
Changes:
- Replace custom
-tl/-tlptoken scanning withMicrosoft.Build.CommandLine.Experimental.CommandLineParserinTerminalLoggerDetector. - Add a new unit test suite for
TerminalLoggerDetector.ProcessTerminalLoggerConfiguration. - Add a
Microsoft.Build.Runtimedependency to support the new MSBuild command-line parser usage.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| test/dotnet.Tests/CommandTests/VSTest/TerminalLoggerDetectorTests.cs | Adds tests for terminal logger detection behavior (switches, env vars, tlp defaults). |
| src/Cli/dotnet/dotnet.csproj | Adds MSBuild runtime package dependency needed for the new parser usage. |
| src/Cli/dotnet/Commands/Test/VSTest/TestCommand.cs | Replaces custom parsing with MSBuild CommandLineParser-based detection for terminal logger switches. |
| public void ProcessTerminalLoggerConfiguration_WithExplicitSwitch_ReturnsExpectedMode(string tlArg, TerminalLoggerMode expectedMode) | ||
| { | ||
| var parseResult = Parser.Parse($"dotnet test {tlArg}"); | ||
|
|
||
| var result = TerminalLoggerDetector.ProcessTerminalLoggerConfiguration(parseResult); |
There was a problem hiding this comment.
Parser.Parse(...) here likely won't compile because this file doesn't import/alias the CLI parser type. Most other tests use using Parser = Microsoft.DotNet.Cli.Parser; (to avoid ambiguity with System.CommandLine parser types). Add that alias (or fully-qualify) so Parser.Parse resolves correctly.
| [Fact] | ||
| public void ProcessTerminalLoggerConfiguration_WithEnvironmentVariable_ReturnsExpectedMode() | ||
| { | ||
| string? previousValue = Environment.GetEnvironmentVariable("MSBUILDTERMINALLOGGER"); | ||
| try | ||
| { | ||
| Environment.SetEnvironmentVariable("MSBUILDTERMINALLOGGER", "off"); | ||
|
|
||
| var parseResult = Parser.Parse("dotnet test"); | ||
| var result = TerminalLoggerDetector.ProcessTerminalLoggerConfiguration(parseResult); | ||
|
|
||
| result.Should().Be(TerminalLoggerMode.Off); |
There was a problem hiding this comment.
These tests don’t currently cover the scenario that motivated the change (switches coming from response files, especially the implicit Directory.Build.rsp). Consider adding a test that creates a temp directory with Directory.Build.rsp containing -tl:off, sets Environment.CurrentDirectory to that directory, parses dotnet test, and asserts TerminalLoggerMode.Off so the regression is protected.
| @@ -466,21 +465,19 @@ static bool CheckIfTerminalIsSupportedAndTryEnableAnsiColorCodes() | |||
|
|
|||
| bool TryFromCommandLine(IReadOnlyList<string> unmatchedTokens, [NotNullWhen(true)] out string? value) | |||
| { | |||
| Switch? terminalLogger = TryFind(unmatchedTokens, ["tl", "terminalLogger", "ll", "livelogger"]); | |||
| if (terminalLogger == null) | |||
| var parser = new Microsoft.Build.CommandLine.Experimental.CommandLineParser(); | |||
| var switches = parser.Parse(unmatchedTokens); | |||
|
|
|||
| string[]? tlValues = switches.TerminalLogger; | |||
| if (tlValues is not { Length: > 0 }) | |||
There was a problem hiding this comment.
unmatchedTokens are parsed twice (once in TryFromCommandLine and again in FindDefaultValue) by constructing a new CommandLineParser each time. Consider parsing once and reusing the resulting switches for both checks to reduce duplication and avoid unnecessary allocations.
Fixes #50441
Replaces custom TerminalLoggerDetector parsing logic (TryFind, Switch) with Microsoft.Build.CommandLine.Experimental.CommandLineParser to support response files and align with MSBuild's switch parsing.