[fix] Fix LoggerRunSettings verbosity being silently overridden when using --settings

[Jakub Jareš (nohwnd)]

Summary

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

Fixes #10369

Root Cause

When running dotnet test --settings my.runsettings with <Verbosity>normal</Verbosity> inside <LoggerRunSettings>, the verbosity is silently overridden and always set to minimal.

Two cooperating issues:

1. TestTaskUtils.CreateCommandLineArguments always injects verbosity

Even 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.AddLoggerToRunSettings discards existing Configuration

When processing the injected --logger:Console;Verbosity=minimal, AddLoggerToRunSettings finds the existing console logger in the LoggerRunSettings (from the .runsettings file), removes it, and replaces it with the newly-constructed one — losing the user's <Verbosity>normal</Verbosity> configuration.

Fix

Part 1 — TestTaskUtils.cs

When isRunSettingsEnabled = true (a settings file is in use), omit Verbosity=X from the auto-injected logger argument. The settings file is the authoritative source for the logger configuration.

Part 2 — LoggerUtilities.cs

In AddLoggerToRunSettings: when the incoming logger has no Configuration (i.e. no CLI parameters were supplied for it) but the existing logger in runsettings does have a Configuration, preserve the existing Configuration rather than discarding it.

Tests

• TestTaskUtils unit tests: CreateArgumentShouldNotInjectVerbosityWhenSettingsFileIsProvided and CreateArgumentShouldInjectVerbosityWhenNoSettingsFileIsProvided
• EnableLoggersArgumentProcessor unit tests: ExecutorInitializeShouldPreserveExistingConfigurationWhenNoNewParametersAreProvided and ExecutorInitializeShouldOverrideExistingConfigurationWhenNewParametersAreProvided

Behavior Change

• Before: dotnet test --settings my.runsettings with <Verbosity>normal</Verbosity> would always show minimal test output regardless of the settings file.
• After: The verbosity from the settings file is respected.

🔍 Triaged by Issue Repro Triage & Auto-Fix 🔍

[Copilot]

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 --settings is provided.
• Update LoggerUtilities.AddLoggerToRunSettings to 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.

[Jakub Jareš (nohwnd)]

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 — the Configuration preservation is correct. The check logger.Configuration is null && existingLogger.Configuration is not null is precisely scoped. It also applies to explicit --logger:console CLI 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 🧠

[Jakub Jareš (nohwnd)]

[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.

[Jakub Jareš (nohwnd)]

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 🔧

[Jakub Jareš (nohwnd)]

Commit pushed: 7930a74

🔧 Iterated by PR Iteration Agent 🔧

[Jakub Jareš (nohwnd)]

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 🧠

[Copilot]

Pull request overview

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

[Jakub Jareš (nohwnd)]

Commit pushed: 59a900e

🔧 Iterated by PR Iteration Agent 🔧

[Jakub Jareš (nohwnd)]

Commit pushed: 9283b20

🔧 Iterated by PR Iteration Agent 🔧

[Copilot]

Pull request overview

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

Files not reviewed (1)

• src/Microsoft.TestPlatform.TestHostProvider/Resources/Resources.Designer.cs: Language not supported

[Jakub Jareš (nohwnd)]

Commit pushed: c529206

🔧 Iterated by PR Iteration Agent 🔧

[Copilot]

Copilot wasn't able to review this pull request because it exceeds the maximum number of files (300). Try reducing the number of changed files and requesting a review from Copilot again.

[github-actions]

Review: Verbosity-Skipping Refinement (2026-08-14)

This commit supersedes the earlier unconditional "skip verbosity when any settings file is present" approach with a targeted check: verbosity is only omitted when the settings file specifically configures the console logger verbosity. That is a meaningful improvement — without it, a settings file that merely sets MaxCpuCount would have silently suppressed verbosity injection, degrading output.

Two-part coordination is correct. HasConsoleLoggerVerbosity = true → --logger:Console (no Verbosity=) → LoggerUtilities.AddLoggerToRunSettings with loggerParameters = null → logger.Configuration stays null → existingLogger.Configuration (from runsettings) is preserved. The handshake works cleanly.

VSTestTask vs VSTestTask2 split is correctly bounded. VSTestTask2 (MSBuildLogger) always needs MSBuild-derived verbosity and is unaffected by the hasConsoleLoggerVerbosityInRunSettings path.

Exception handling in HasConsoleLoggerVerbosity is appropriate — XmlException, IOException, UnauthorizedAccessException are caught and false is returned. File.Exists guards the path before loading, so ArgumentException for invalid path characters won't reach XDocument.Load.

Tests look correct. Assert.DoesNotMatchRegex + Assert.Contains together verify no Verbosity= is injected while the logger itself is still added. The acceptance test's StdOutputContains("PassingTest") + ValidateSummaryStatus(1,1,1) correctly distinguishes normal (prints individual test names) from minimal (does not) verbosity.

One low-severity finding (inline): Descendants() matches <Verbosity> anywhere in the subtree rather than specifically under <Configuration>. Not a practical risk, but a tighter scope would be more precise.

---

🧠 Reviewed by Expert Code Reviewer

🧠 Reviewed by Expert Code Reviewer 🧠

[github-actions]

[Correctness — minor] HasConsoleLoggerVerbosity uses logger.Descendants() which searches the entire subtree of each <Logger> element for any <Verbosity> node. A <Verbosity> element placed outside <Configuration> — e.g., under a custom vendor extension block — would incorrectly suppress verbosity injection even though the structured <Configuration><Verbosity> path is absent.

The semantically precise check would scope to only <Configuration> children:

foreach (var configuration in logger.Elements())
{
    if (!configuration.Name.LocalName.Equals("Configuration", StringComparison.OrdinalIgnoreCase))
        continue;
    foreach (var element in configuration.Elements())
    {
        if (element.Name.LocalName.Equals("Verbosity", StringComparison.OrdinalIgnoreCase))
            return true;
    }
}

In practice no settings file would put <Verbosity> outside <Configuration>, so this is low risk, but the tighter check would make the intent explicit and match the schema.

[Copilot]

Copilot wasn't able to review this pull request because it exceeds the maximum number of files (300). Try reducing the number of changed files and requesting a review from Copilot again.

[github-actions]

Review: Test Assertion Portability Fix (2026-08-14 21:25)

The only new commit since the previous expert review (21:07Z) is 781b3c0 ("Fix settings argument assertion"), which splits one platform-sensitive assertion:

// Before
Assert.Contains($"--settings:\"{settingsFile}\"", commandline);
// After
Assert.Contains("--settings:", commandline);
Assert.Contains(settingsFile, commandline);

This is correct: the original assertion could fail on platforms where CommandLineBuilder.AppendSwitchIfNotNull produces different quoting. Splitting into two assertions is the standard pattern for cross-platform CLI argument tests in this repo.

No production code changed. The HasConsoleLoggerVerbosity implementation, LoggerUtilities.AddLoggerToRunSettings fix, and acceptance test added in 6e566cb were already reviewed and are sound. The low-severity Descendants() scope finding noted in the previous pass remains open but is not a practical risk given the schema.

---

🧠 Reviewed by Expert Code Reviewer 🧠

🧠 Reviewed by Expert Code Reviewer 🧠

[Copilot]

Copilot wasn't able to review this pull request because it exceeds the maximum number of files (300). Try reducing the number of changed files and requesting a review from Copilot again.

[github-actions]

Review: LoggerRunSettings verbosity fix

Dimension	Status
Correctness of HasConsoleLoggerVerbosity	✅ pass (two nits below)
Correctness of LoggerUtilities config preservation	✅ pass
Correctness of KnownExtensionInstanceFactory pattern	✅ pass
Edge cases / interaction between the two fix parts	✅ correct
Backward compatibility (VSTestTask vs VSTestTask2 split)	✅ correct
Exception handling in XML parsing	⚠️ one nit
XmlNode aliasing in LoggerUtilities	⚠️ cosmetic note

3 findings — all nit/informational, none merge-blocking.

Core logic assessment

The two-part fix composes correctly end-to-end:

1. HasConsoleLoggerVerbosity reads the settings file and returns true if <Verbosity> is present anywhere under the matched console logger element. When true, TestTaskUtils injects --logger:Console without a Verbosity=X suffix.
2. LoggerUtilities.AddLoggerToRunSettings then finds the existing logger from the settings file via GetExistingLoggerIndex (OrdinalIgnoreCase on both FriendlyName and URI — consistent with the detection logic), and because logger.Configuration is null (no CLI params), it preserves the settings file's Configuration node. The ConsoleLogger is initialized with the settings-file verbosity intact.

The precedence rule is also correct: if the user explicitly passes --logger:Console;Verbosity=normal (non-null Configuration), the preservation condition is false and the CLI arg wins. That's the right semantics.

The VSTestTask vs VSTestTask2 split at line 54–55 is the right boundary — MSBuildLogger verbosity is MSBuild-derived and shouldn't be suppressed based on runsettings content.

The KnownExtensionInstanceFactory cast (_requestData as RequestData)? is safe (returns null if not RequestData) and not a layering violation — CrossPlatEngine already depends on Common. The dedup via _initializedLoggers.Contains(type) works correctly for both the URI and type-based initialization paths.

---

🧠 Reviewed by Expert Code Reviewer

🧠 Reviewed by Expert Code Reviewer 🧠

[github-actions]

nit: File.Exists is redundant here — XDocument.Load already catches IOException (which covers FileNotFoundException, DirectoryNotFoundException, network errors, etc.) and returns false. The File.Exists pre-check introduces a classic TOCTOU window: the file could be deleted, renamed, or its directory unmounted between the check and the load. Since the IOException catch handles all of those cases correctly, the guard can be dropped:

// Before
if (!File.Exists(settingsFile))
    return false;

// After: just rely on the IOException catch below — cleaner and race-free

The File.Exists path also returns false for permission errors and UNC unavailability, which masks the reason the function returned early (the caller only sees "no verbosity configured"). Not a correctness issue in practice, but removing the check simplifies the code.

[github-actions]

[RunSettings Validation] Descendants() traverses the entire subtree under the matched <Logger> element, not just Configuration/Verbosity. A settings file with an unusual (but syntactically valid) structure like:

<Logger friendlyName="console">
  <PluginOptions>
    <Verbosity>x</Verbosity>   <!-- hypothetical third-party config -->
  </PluginOptions>
</Logger>

...would produce a false-positive and suppress verbosity injection. In that case LoggerUtilities preserves the whole Configuration node (which may be null or may not contain a real verbosity value), so the ConsoleLogger silently uses its default.

The canonical schema puts <Verbosity> exactly one level inside <Configuration>:

foreach (var element in logger.Element("Configuration")?.Elements() ?? Enumerable.Empty<XElement>())
{
    if (element.Name.LocalName.Equals("Verbosity", StringComparison.OrdinalIgnoreCase))
        return true;
}

This constrains the match to exactly where the ConsoleLogger expects it and avoids the false-positive. Nit, won't occur with any settings file generated by VS or dotnet test.

[github-actions]

[RunSettings Validation] When logger.Configuration is null && existingLogger.Configuration is not null, we assign the same XmlNode instance to the incoming logger. That's fine (the existing logger is about to be removed from the list at line 84, so there's only one live reference), but it's worth noting the implicit invariant: if InitializeLogger or anything downstream ever re-parents this node into a different XmlDocument, you'll get an InvalidOperationException ("The node belongs to a different document").

In practice, serialization reads outerXml / InnerXml strings from the node rather than moving it, so this is safe today. Just flagging for future maintainers who add Configuration mutation code.

[github-actions]

[Description] PR scope does not match description

The PR description mentions only a targeted 2-file bug fix for #10369, but the actual diff contains 573 changed files (35,698 additions / 17,339 deletions). Undescribed changes include:

• New MTP proxy types: MtpProxyDiscoveryManager, MtpProxyExecutionManager, MtpProxyManagerFactory, MtpDataCollectionForwarder, MtpTestNodeConverter
• Major refactoring of ParallelOperationManager, ProxyDiscoveryManager, ProxyExecutionManager
• New MicrosoftTestingPlatformDetector utility
• KnownExtensionInstanceFactory extension point on RequestData / TestLoggerManager
• IPC/serialization changes in CommunicationUtilities
• eng/expected-dll-frameworks.json and eng/expected-nupkg-file-counts.json updates

The bug fix for #10369 (TestTaskUtils + LoggerUtilities) is present and correctly implemented — but reviewers relying on the description will be unaware of the bulk of the changes. Please update the description to reflect the full scope, or split the infrastructure work into a separate PR.

🧠 Reviewed by Expert Code Reviewer

[Copilot]

Copilot wasn't able to review this pull request because it exceeds the maximum number of files (300). Try reducing the number of changed files and requesting a review from Copilot again.

[github-actions]

Review — head commit (2026-08-15)

Test change: ValidateSummaryStatus removal ✅

The deletion is correct. ValidateSummaryStatus parses the summary line for the dotnet test format (e.g. Passed! - ...) but on Unix with MTP the summary uses the VSTest console format instead — so the assertion was inherently platform-specific and not reliably verifiable cross-platform. The regression scenario (verbosity is respected → individual skipped test names appear at normal) is fully covered by StdOutputContains("SkippingTest"), and ExitCodeEquals(1) still confirms the run produced expected failures. Nothing meaningful is lost.

Prior findings status

#	Finding	Status
1	Descendants() false-positive in HasConsoleLoggerVerbosity	⚠️ still present — inline comment posted
2	File.Exists TOCTOU before try/catch	i️ still present, but benign — the race window is real but the worst outcome is a redundant IOException already handled by the catch block; not worth a blocker
3	XmlNode aliasing (logger.Configuration = existingLogger.Configuration)	i️ still present, safe today — both point to the same node but the old entry is immediately removed before serialisation

Dimension summary

Dimension	Status
Dependency & Package Integrity	✅
Cross-TFM & Framework Resolution	✅
IPC Transport & Protocol Stability	✅
Error Reporting & Diagnostic Clarity	✅
RunSettings Validation & Inference	⚠️ Descendants() scope too broad (see inline)
Backward Compatibility & Rollback Safety	✅
Acceptance Test Coverage Design	✅ — verbosity fix coverage intact post-deletion
All other dimensions	✅ N/A

One minor nit inline. Core fix is correct, test removal is justified. Looks good overall.

---

🧠 Reviewed by Expert Reviewer

🧠 Reviewed by Expert Code Reviewer 🧠

[github-actions]

[RunSettings Validation] logger.Descendants() walks the entire subtree under <Logger>, so a <Verbosity> element nested anywhere (e.g. inside a third-party plugin-specific section) would produce a false positive and cause VSTestTask to silently omit the MSBuild-derived verbosity override. The intent is to check only the <Configuration> child.

Suggested change

	foreach (var element in logger.Descendants())
	foreach (var element in logger.Element("Configuration")?.Elements() ?? Enumerable.Empty<XElement>())

This also avoids the Descendants overhead on arbitrarily deep trees.