Skip to content

Don't forward MSBuild terminal-logger args to the test host (fixes #52114, #52229)#54434

Closed
Evangelink wants to merge 1 commit into
mainfrom
dev/amauryleve/fix-tl-leak-to-test-app
Closed

Don't forward MSBuild terminal-logger args to the test host (fixes #52114, #52229)#54434
Evangelink wants to merge 1 commit into
mainfrom
dev/amauryleve/fix-tl-leak-to-test-app

Conversation

@Evangelink

Copy link
Copy Markdown
Member

Summary

Fixes #52114 and #52229.

Running dotnet test against an MTP test project with an MSBuild terminal-logger switch — for example dotnet test -tl:off or dotnet test --tl:off — currently:

  1. Splits unmatched CLI tokens into "binlog args" → MSBuild, and everything else → test application arguments.
  2. Forwards the test application arguments to the child test host.
  3. The test host doesn't know about --tl, fails with Unknown option '--tl', exits with code 5, and prints its entire help output.

So the user sees a wall of noise and the terminal logger remains on during the build.

Fix

Terminal-logger switches are an MSBuild concern. They are added to the MSBuild argument list (alongside the binlog switches that were already routed there) and removed from the test application argument list.

Changes:

  • LoggerUtility.IsTerminalLoggerArgument — new internal predicate that recognizes:
    • short names -tl, --tl, /tl, -ll, --ll, /ll, -tlp, --tlp, /tlp
    • long names terminalLogger, livelogger, terminalLoggerParameters with the same three prefixes
    • bare switches and switches with :value or =value (e.g. -tl:off, --terminalLogger=on, /tlp:default=off)
  • MSBuildUtility.GetBuildOptions — after SeparateBinLogArguments, partitions the remaining unmatched tokens; terminal-logger tokens are concatenated onto the MSBuild argument sequence, the rest stay as test-application arguments.

Design notes

Why a dedicated predicate instead of a full MSBuild argument recognizer?

The bug is reported specifically for terminal-logger switches, and any broader filter would risk silently dropping options the test host legitimately accepts (e.g. -p: global properties already flow through MSBuild via OptionValuesToBeForwarded and have a different role on the test-app side). A narrow, well-scoped predicate is also easier to reason about, easier to test, and easier to extend if we find additional MSBuild-only switches leaking in the future.

Why colocate the predicate with the binlog logic in LoggerUtility?

It's the same shape of code — recognize an MSBuild diagnostic-output switch so it can be routed correctly — and it lives next to its closest analog, IsBinLogArgument. Putting it on a hypothetical new helper class would only spread the related concepts across files.

Why match : and = separators?

MSBuild accepts both forms on long-name switches (e.g. -bl:foo.binlog and -bl=foo.binlog). Mirroring that here avoids the predicate disagreeing with how the build invocation will actually parse the same token.

Why exact-match (not StartsWith)?

The pre-existing TerminalLoggerDetector.TryFind in the VSTest path uses StartsWith(prefix + name, ...), which would mis-classify e.g. --terminalLoggerExtra as a terminal-logger switch. The new predicate requires either an exact name match or an immediate : / = separator after the name. The included unit tests pin this down.

Alternatives considered

  1. Filter args in LoggerUtility.SeparateBinLogArguments itself. Rejected because that helper is also used from RunCommand, PublishCommand, PackCommand, and DotNetCommandFactory, and the bug is specific to the MTP test path. Changing the shared helper would either need a new overload or risk subtly affecting other commands.
  2. Pre-process parseResult.UnmatchedTokens before MTP parsing. Possible, but the natural seam is already GetBuildOptions, which is the function that builds the two argument lists. Moving the partition there keeps the change local and obvious.
  3. Catch unknown options on the test host side and silently ignore -tl. That would mask other genuine "unknown option" mistakes, and would still produce a confusing build experience (terminal logger remains on, which is the user-visible part of the bug).

Tests

test/dotnet.Tests/CommandTests/Test/LoggerUtilityTests.cs — 36 cases covering:

  • All combinations of - / -- / / prefixes with each of the six terminal-logger names, both bare and with :value / =value separators.
  • Negative cases: empty string, binlog switches, unrelated switches, near-misses (-tlNoSuchOption, --terminalLoggerExtra, /tlinvalid), missing prefix, too-many-dash prefix, positional argument.

All 36 pass locally.

Fixes #52114 and #52229. With `dotnet test` in MTP mode, options such as `-tl:off` / `--terminalLogger:off` were placed into the test application's argument list and forwarded to the test host process, which rejected them with `Unknown option '--tl'` and produced exit code 5 plus a wall of help output.

Adds `LoggerUtility.IsTerminalLoggerArgument` to recognize MSBuild terminal-logger switches (`tl`, `terminalLogger`, `ll`, `livelogger`, `tlp`, `terminalLoggerParameters`) with `-`/`--`/`/` prefixes and `:`/`=` value separators, and updates `MSBuildUtility.GetBuildOptions` to partition unmatched tokens so that those switches reach the build invocation only.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings May 25, 2026 21:01
@Evangelink Evangelink requested a review from a team as a code owner May 25, 2026 21:01
@Evangelink

Copy link
Copy Markdown
Member Author

Closing as duplicate of #54310, which I had opened in an earlier session and missed when starting this one. #54310 covers the same fix with an equivalent LoggerUtility.IsTerminalLoggerArgument predicate plus end-to-end GetBuildOptions tests. Sorry for the churn.

@Evangelink Evangelink closed this May 25, 2026
@Evangelink Evangelink deleted the dev/amauryleve/fix-tl-leak-to-test-app branch May 25, 2026 21:02
@Evangelink Evangelink review requested due to automatic review settings May 25, 2026 21:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Can't disable timeline view on dotnet test with MTP

1 participant