[fix] Fix LoggerRunSettings verbosity being silently overridden when using --settings#16044
[fix] Fix LoggerRunSettings verbosity being silently overridden when using --settings#16044nohwnd wants to merge 2 commits into
Conversation
…task When dotnet test is run with --settings <runsettings>, the MSBuild VSTest task always injected --logger:Console;Verbosity=X. This caused AddLoggerToRunSettings to remove the existing console logger entry (from LoggerRunSettings in the settings file) and replace it with one carrying only the MSBuild-derived verbosity — discarding the user's configured verbosity. Root cause: two cooperating issues. 1. TestTaskUtils.CreateCommandLineArguments always included Verbosity=X in the auto-injected --logger arg even when a settings file was in use. 2. LoggerUtilities.AddLoggerToRunSettings unconditionally removed and replaced an existing logger, losing its Configuration when the new logger had no Configuration of its own. Fix: - When isRunSettingsEnabled=true (settings file provided), omit Verbosity from the auto-injected logger arg so the settings file can supply it. - In AddLoggerToRunSettings, when the incoming logger has no Configuration (no CLI params) but an existing logger does, preserve the existing Configuration rather than discarding it. Fixes #10369 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR fixes an interaction between the MSBuild VSTest task and LoggerRunSettings where dotnet test --settings ... could silently override a console logger’s configured verbosity (e.g., forcing minimal even when the runsettings specified normal). It does so by (1) stopping MSBuild from injecting Verbosity=... when a settings file is used, and (2) preserving existing logger <Configuration> when a logger is re-added without new parameters.
Changes:
- Update MSBuild task argument construction to omit
Verbosity=...on the auto-injected--logger:when--settingsis provided. - Update
LoggerUtilities.AddLoggerToRunSettingsto preserve an existing logger’s<Configuration>when the incoming logger has no configuration. - Add unit tests covering both the MSBuild argument behavior and the runsettings merge behavior.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/Microsoft.TestPlatform.Build/Tasks/TestTaskUtils.cs | Avoid injecting console logger verbosity when a runsettings file is in use. |
| src/vstest.console/Processors/Utilities/LoggerUtilities.cs | Preserve existing logger configuration when re-adding a logger without CLI parameters. |
| test/Microsoft.TestPlatform.Build.UnitTests/TestTaskUtilsTests.cs | Tests for verbosity injection behavior with/without --settings. |
| test/vstest.console.UnitTests/Processors/EnableLoggersArgumentProcessorTests.cs | Tests ensuring configuration is preserved/overridden appropriately when enabling loggers. |
nohwnd
left a comment
nohwnd
left a comment
There was a problem hiding this comment.
Review: RunSettings Logger Verbosity Fix
The two-part fix is logically sound and well-targeted at the reported bug. Tests cover the primary scenarios clearly.
One finding worth discussing (see inline):
The isRunSettingsEnabled guard in TestTaskUtils.cs applies to both VSTestTask (Console logger) and VSTestTask2 (MSBuildLogger). For the Console logger this is the intended fix; for MSBuildLogger it silently drops the MSBuild-derived verbosity when a settings file is in use and the file doesn't configure that logger — which is the common case. This may be acceptable by design, but it's worth a conscious decision and an explicit test.
No issues found in:
LoggerUtilities.cs— theConfigurationpreservation is correct. The checklogger.Configuration is null && existingLogger.Configuration is not nullis precisely scoped. It also applies to explicit--logger:consoleCLI invocations (not just the MSBuild task path), which is consistent and desirable.- Null safety, public API surface, IPC, or cross-TFM concerns — none present.
🧠 Reviewed by Expert Code Reviewer
🧠 Reviewed by Expert Code Reviewer 🧠
| vsTestVerbosity = "normal"; | ||
| } | ||
| else if (quietTestLogging.Contains(taskVsTestVerbosity)) | ||
| builder.AppendSwitchUnquotedIfNotNull("--logger:", loggerToUse); |
There was a problem hiding this comment.
[Backward Compatibility — VSTestTask2]
The isRunSettingsEnabled branch applies to both VSTestTask (Console logger) and VSTestTask2 (MSBuildLogger). For VSTestTask2 + settings file, the MSBuild-derived verbosity is now silently dropped:
- Before:
--logger:Microsoft.TestPlatform.MSBuildLogger;Verbosity=minimal(or the MSBuild-mapped value) - After:
--logger:Microsoft.TestPlatform.MSBuildLogger(no verbosity — falls back to the logger's internal default)
If the settings file doesn't contain a <Logger> entry for Microsoft.TestPlatform.MSBuildLogger (the common case), the MSBuildLogger will ignore the MSBuild-derived verbosity entirely. This is a silent behavior change for anyone using VSTestTask2 (i.e., dotnet test via the MSBuild tooling task) with a .runsettings file.
The fix for the Console logger (the bug's subject) is correct; consider whether VSTestTask2 should be treated the same way or whether it needs to retain the MSBuild verbosity pass-through when no settings-file configuration is present for it. A test covering VSTestTask2 + settings file would make the intent explicit.
There was a problem hiding this comment.
Good catch. The fix now scopes the verbosity-suppression to task is VSTestTask only. VSTestTask2 (MSBuildLogger) always injects the MSBuild-derived verbosity regardless of whether a settings file is present, since MSBuildLogger verbosity is driven by MSBuild rather than user settings. A test covering VSTestTask2 + settings file is also added to make the intent explicit.
🔧 Iterated by PR Iteration Agent 🔧
…k2 always injects MSBuild-derived verbosity The 'don't inject Verbosity when settings file is present' fix was applied to both VSTestTask (Console logger) and VSTestTask2 (MSBuildLogger). For VSTestTask2, the MSBuildLogger verbosity is driven by MSBuild, not the user's settings file, so it must always receive the MSBuild-derived verbosity. Scope the suppression to task is VSTestTask only. Also adds a test explicitly covering VSTestTask2 + settings file. 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.
Review: LoggerRunSettings Verbosity Fix — Updated
The previous finding (VSTestTask2 verbosity silently dropped when a settings file is present) has been addressed in the follow-up commit. The task is VSTestTask type guard correctly scopes the "no verbosity injection" path to the Console logger only — VSTestTask2 (MSBuildLogger) continues to inject the MSBuild-derived verbosity regardless of whether a settings file is present, which is the intended semantics.
Full analysis of the two-part fix:
Part 1 — TestTaskUtils.cs: The branching logic is correct. When isRunSettingsEnabled && task is VSTestTask, only --logger:Console is injected (no verbosity), deferring to the .runsettings configuration. The else path (VSTestTask2 or no settings file) preserves the original verbosity-injection behavior. The three new tests cover all cases: VSTestTask with settings, VSTestTask without settings, VSTestTask2 with settings.
Part 2 — LoggerUtilities.cs: The logger.Configuration is null && existingLogger.Configuration is not null guard is precisely scoped. It correctly preserves .runsettings Configuration when no new parameters are supplied (whether from the MSBuild task path or from a plain --logger:console CLI invocation), and lets explicit CLI parameters like --logger:console;verbosity=quiet override as before. Null safety is sound — the existingLoggerIndex >= 0 guard makes the array access safe.
Description alignment: Accurate. Both root causes and both fix components are described correctly. The test names in the description match the actual test names.
No issues found.
🧠 Reviewed by Expert Code Reviewer 🧠
🧠 Reviewed by Expert Code Reviewer 🧠
Summary
Fixes #10369
Root Cause
When running
dotnet test --settings my.runsettingswith<Verbosity>normal</Verbosity>inside<LoggerRunSettings>, the verbosity is silently overridden and always set tominimal.Two cooperating issues:
1.
TestTaskUtils.CreateCommandLineArgumentsalways injects verbosityEven when a settings file is provided, the MSBuild VSTest task auto-injects
--logger:Console;Verbosity=minimal(or whatever MSBuild-derived verbosity applies). This happens regardless of whether the user configured the console logger in their settings file.2.
LoggerUtilities.AddLoggerToRunSettingsdiscards existingConfigurationWhen processing the injected
--logger:Console;Verbosity=minimal,AddLoggerToRunSettingsfinds the existing console logger in theLoggerRunSettings(from the.runsettingsfile), removes it, and replaces it with the newly-constructed one — losing the user's<Verbosity>normal</Verbosity>configuration.Fix
Part 1 —
TestTaskUtils.csWhen
isRunSettingsEnabled = true(a settings file is in use), omitVerbosity=Xfrom the auto-injected logger argument. The settings file is the authoritative source for the logger configuration.Part 2 —
LoggerUtilities.csIn
AddLoggerToRunSettings: when the incoming logger has noConfiguration(i.e. no CLI parameters were supplied for it) but the existing logger in runsettings does have aConfiguration, preserve the existingConfigurationrather than discarding it.Tests
TestTaskUtilsunit tests:CreateArgumentShouldNotInjectVerbosityWhenSettingsFileIsProvidedandCreateArgumentShouldInjectVerbosityWhenNoSettingsFileIsProvidedEnableLoggersArgumentProcessorunit tests:ExecutorInitializeShouldPreserveExistingConfigurationWhenNoNewParametersAreProvidedandExecutorInitializeShouldOverrideExistingConfigurationWhenNewParametersAreProvidedBehavior Change
dotnet test --settings my.runsettingswith<Verbosity>normal</Verbosity>would always show minimal test output regardless of the settings file.