[fix] Add TreatErrorMessagesAsWarnings parameter to TRX logger

[Jakub Jareš (nohwnd)]

🤖 Automated Fix

Fixes issue #10391: Data Collector Errors Cause TRX Result to be Failed.

Root Cause

In TrxLogger.TestMessageHandler, the case TestMessageLevel.Error: branch unconditionally sets TestResultOutcome = TestOutcome.Failed. This means any error-level message — including those from data collectors that fail during session teardown — marks the entire TRX result as failed, even when all tests actually passed.

Fix

Adds a new TreatErrorMessagesAsWarnings logger parameter. When set to true, error-level run messages no longer update TestResultOutcome, so the overall TRX outcome reflects actual test results rather than data collector errors.

Usage:

dotnet test ... --logger "trx;TreatErrorMessagesAsWarnings=true"

or with vstest.console:

vstest.console.exe ... /logger:"trx;TreatErrorMessagesAsWarnings=true"

Error messages are still recorded in the TRX file's ErrorInfo section — they are not silenced, just excluded from the pass/fail decision.

Changes

• Constants.cs: Add TreatErrorMessagesAsWarnings string constant (follows the same pattern as WarnOnFileOverwrite)
• TrxLogger.cs: Read the parameter in Initialize(TestLoggerEvents, Dictionary<string, string?>) and skip setting TestResultOutcome = Failed when _treatErrorMessagesAsWarnings is true
• TrxLoggerTests.cs: Unit tests for default behavior (backward compatible) and new parameter behavior
• LoggerTests.cs: Acceptance tests using OutOfProcDataCollector (which logs errors in SessionEnded_Handler) to verify the real end-to-end pipeline:
  • Without parameter: TRX outcome is Failed even when only PassingTest ran ✓
  • With TreatErrorMessagesAsWarnings=true: TRX outcome is Completed ✓

Default Behavior

The default remains unchanged (backward compatible): TreatErrorMessagesAsWarnings defaults to false, preserving existing behavior where any error message marks the run as failed.

🔍 Triaged by Issue Repro Triage & Auto-Fix 🔍

[Copilot]

Pull request overview

Adds an opt-in TRX logger parameter to prevent error-level run messages (notably from data collectors) from forcing the overall TRX run outcome to Failed, addressing #10391 while keeping default behavior unchanged.

Changes:

• Introduces a new TRX logger parameter TreatErrorMessagesAsWarnings.
• Updates TrxLogger.TestMessageHandler to avoid setting the overall outcome to Failed on error-level messages when the parameter is enabled.
• Adds unit + acceptance coverage to validate default behavior and the new opt-in behavior end-to-end.

Reviewed changes

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

File	Description
src/Microsoft.TestPlatform.Extensions.TrxLogger/Utility/Constants.cs	Adds the new logger parameter key constant.
src/Microsoft.TestPlatform.Extensions.TrxLogger/TrxLogger.cs	Parses the new parameter and gates whether error-level messages flip the overall TRX outcome to Failed.
test/Microsoft.TestPlatform.Extensions.TrxLogger.UnitTests/TrxLoggerTests.cs	Adds unit tests for default outcome behavior vs. opt-in behavior.
test/Microsoft.TestPlatform.Acceptance.IntegrationTests/LoggerTests.cs	Adds acceptance tests verifying TRX ResultSummary outcome changes with/without the new parameter when a data collector logs an error.

[Jakub Jareš (nohwnd)]

Expert Review — [fix] Add TreatErrorMessagesAsWarnings parameter to TRX logger

Activated dimensions: RunSettings Validation & Inference, Backward Compatibility & Rollback Safety, Error Reporting & Diagnostic Clarity, Null Safety & Boundary Validation

Summary: The change is well-structured and correctly follows the WarnOnFileOverwrite pattern for parameter parsing. Backward compatibility is preserved by defaulting to false. Tests cover the happy path and recording behavior. One correctness issue found in the parse-error fallback path.

---

🔴 Finding: Parse-error fallback defaults to true (suppresses failures silently)

File: TrxLogger.cs, line 153

When TreatErrorMessagesAsWarnings is present in the parameters dictionary but fails bool.TryParse (e.g. TreatErrorMessagesAsWarnings=yes or TreatErrorMessagesAsWarnings=1), the code falls back to true. This means a misconfigured value silently opts the user into suppressing error-level messages from counting as failures — the opposite of the safe default.

Unlike WarnOnFileOverwrite (where true means "warn = cautious"), true here means "mask failures = less cautious". The correct parse-error fallback is false (same as the not-found default).

See the inline comment for the fix.

---

✅ What looks correct

• Backward compatibility: _treatErrorMessagesAsWarnings defaults to false when the parameter is absent — existing behavior is fully preserved.
• Error recording: Even with TreatErrorMessagesAsWarnings=true, the RunInfo is still added to _runLevelErrorsAndWarnings with TestOutcome.Error, so errors appear in the TRX ErrorInfo section.
• Initialize call ordering: _treatErrorMessagesAsWarnings is set before Initialize(events, testRunDirectory) is called; the simpler overload doesn't reset it, which is consistent with how _warnOnFileOverwrite is handled.
• Acceptance tests: SampleDataCollector.SessionEnded_Handler calls LogError unconditionally, giving the tests a reliable error source. The netstandard2.0 target on OutOfProcDataCollector.csproj matches the test's GetTestDllForFramework("OutOfProcDataCollector.dll", "netstandard2.0") call.

🧠 Reviewed by Expert Code Reviewer

🧠 Reviewed by Expert Code Reviewer 🧠

[Jakub Jareš (nohwnd)]

[Correctness] The parse-error fallback defaults to true, which silently suppresses failure reporting when the parameter value can't be parsed (e.g. TreatErrorMessagesAsWarnings=yes).

The analogy to WarnOnFileOverwrite doesn't hold here: for WarnOnFileOverwrite, true is the cautious default (produces a visible warning instead of silently overwriting). For TreatErrorMessagesAsWarnings, true is the less cautious default — it masks failures that the user may not have intended to suppress.

If a user passes an invalid value, the safe fallback is false, which preserves the original behavior where errors mark the run as failed:

_treatErrorMessagesAsWarnings = parameters.TryGetValue(TrxLoggerConstants.TreatErrorMessagesAsWarnings, out string? treatErrorMessagesAsWarningsString)
    ? bool.TryParse(treatErrorMessagesAsWarningsString, out bool treatErrorMessagesAsWarningsValue)
        ? treatErrorMessagesAsWarningsValue
        // We found the option but could not parse the value; preserve existing behavior.
        : false
    : false;

🧠 Reviewed by Expert Code Reviewer

[Jakub Jareš (nohwnd)]

Fixed — changed the parse-error fallback from true to false so an unparseable value (e.g. TreatErrorMessagesAsWarnings=yes) preserves the existing behavior rather than silently opting into warning mode.

🔧 Iterated by PR Iteration Agent 🔧

[Jakub Jareš (nohwnd)]

Commit pushed: a066368

🔧 Iterated by PR Iteration Agent 🔧

[Jakub Jareš (nohwnd)]

Expert Review — Follow-up: [fix] Add TreatErrorMessagesAsWarnings parameter to TRX logger

The previous finding has been addressed. The parse-error fallback for TreatErrorMessagesAsWarnings is now correctly false — a malformed value (e.g. TreatErrorMessagesAsWarnings=yes) preserves existing behavior rather than silently opting the user into warning mode.

All activated dimensions checked (RunSettings Validation & Inference, Backward Compatibility & Rollback Safety, Error Reporting & Diagnostic Clarity, Null Safety & Boundary Validation):

• ✅ Parse-error fallback fixed: Both the not-found and parse-failure branches now return false, matching the safe default.
• ✅ Backward compatibility: Unchanged — false default means existing runs see no behavior change.
• ✅ Error recording preserved: _runLevelErrorsAndWarnings.Add(runMessage) still executes unconditionally regardless of the flag, so errors appear in the TRX ErrorInfo section.
• ✅ Initialize ordering: _treatErrorMessagesAsWarnings is set before Initialize(events, testRunDirectory) is called, consistent with the _warnOnFileOverwrite pattern.
• ✅ Unit tests: Cover the default path (Failed on error), the opt-in path (outcome unchanged), and the recording invariant (error still logged even with flag).
• ✅ Acceptance tests: Cover the end-to-end pipeline with OutOfProcDataCollector against both .NET Full and .NET Core runners.

No further issues found.

🧠 Reviewed by Expert Code Reviewer 🧠

🧠 Reviewed by Expert Code Reviewer 🧠

[Copilot]

Pull request overview

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

[Jakub Jareš (nohwnd)]

Expert Review — Follow-up: [fix] Add TreatErrorMessagesAsWarnings parameter to TRX logger

New commit reviewed: 6a24672 ("Potential fix for pull request finding") — changes TreatErrorMessagesAsWarnings in Constants.cs from public static string to public const string. This is a correct improvement: the value is never reassigned, so const is more accurate and allows compile-time inlining. No correctness impact.

Activated dimensions: RunSettings Validation & Inference, Backward Compatibility & Rollback Safety, Error Reporting & Diagnostic Clarity, Null Safety & Boundary Validation, Acceptance Test Coverage Design

All dimensions checked against the full current diff:

• ✅ Parse-error fallback (previously REQUEST_CHANGES): Fixed in prior commit — both the not-found and parse-failure branches return false.
• ✅ TestResultOutcome initialization: Property initializes to TestOutcome.Passed; at run completion Passed is converted to Completed — consistent with the acceptance test expecting "Completed" for a clean run.
• ✅ Error recording invariant: _runLevelErrorsAndWarnings.Add(runMessage) executes unconditionally regardless of the flag, so errors always appear in the TRX ErrorInfo section.
• ✅ Backward compatibility: Default false preserves existing behavior; no change to TRX format or wire protocol.
• ✅ Null safety: bool.TryParse(null, ...) returns false, so a null dictionary value is handled safely by the parse-failure branch.
• ✅ const declaration: internal static class Constants — no external consumers; const is correct and consistent with all other string constants in the file.
• ✅ Acceptance tests: Cover both the regression case (Failed when collector logs error without flag) and the opt-in case (Completed when flag is set), with [NetFullTargetFrameworkDataSource] and [NetCoreTargetFrameworkDataSource] coverage.

No further issues found.

🧠 Reviewed by Expert Code Reviewer 🧠

🧠 Reviewed by Expert Code Reviewer 🧠

🧠 Reviewed by Expert Code Reviewer 🧠