Fix O(n²) output accumulation in data-driven tests causing large TRX files

[Evangelink]

Fixes #7908

Problem

When running data-driven tests (e.g. [DataRow]), the StdOut, StdErr, and Trace string builders in TestContextImplementation were not cleared between data row iterations. Each subsequent row's TestResult.LogOutput included all previous rows' output, causing O(n²) growth in the TRX file.

For N data rows each producing X bytes of console output, the total output written to the TRX was X·N·(N+1)/2 instead of X·N. With 93 tests producing verbose logging, this easily produced a file large enough to cause OutOfMemoryException when AzDO's TrxResultParser tried to parse it.

Root Cause

In TestMethodInfo.InvokeAsync(), after each test method invocation:

• GetAndClearDiagnosticMessages() properly cleared diagnostic messages ✓
• GetResultFiles() properly cleared result files (comment: "clear the result files to handle data driven tests") ✓
• GetOut(), GetErr(), GetTrace() returned accumulated content but never cleared the underlying SynchronizedStringBuilder ✗

The same TestContextImplementation instance is reused across all data rows within a single test method execution.

Fix

Added GetAndClearOut(), GetAndClearErr(), GetAndClearTrace() methods to TestContextImplementation (matching the existing GetAndClearDiagnosticMessages pattern) and used them in TestMethodInfo.InvokeAsync().

The non-clearing GetOut()/GetErr()/GetTrace() methods are preserved — they are used by other call sites (assembly init, class init, class cleanup, assembly cleanup) that use separate TestContextImplementation instances and don't need clearing.

Tests

• Unit tests: Three tests in TestMethodInfoTests that call InvokeAsync twice and verify output from the first invocation does not leak into the second (covering StdOut, StdErr, DebugTrace).
• Acceptance test: TrxReportDataDrivenOutputTests — runs an actual MSTest data-driven test with Console.WriteLine output across 3 [DataRow] iterations, then parses the TRX XML and verifies each UnitTestResult's <StdOut> contains only its own row's output marker. This test is implementation-agnostic and survives refactoring.

[Copilot]

Pull request overview

Addresses excessive TRX size in data-driven MSTest runs by preventing StdOut/StdErr/Trace from accumulating across data row iterations within the same TestContextImplementation instance.

Changes:

• Add GetAndClearOut/Err/Trace() to TestContextImplementation to support per-invocation output capture and reset.
• Update TestMethodInfo.InvokeAsync() to use the new clearing accessors when populating TestResult log fields.
• Add unit + acceptance regression coverage to ensure data-driven output does not leak across invocations/rows.

Show a summary per file

File	Description
test/UnitTests/MSTestAdapter.PlatformServices.UnitTests/Execution/TestMethodInfoTests.cs	Adds unit tests verifying StdOut/StdErr/Trace are cleared between InvokeAsync calls.
test/IntegrationTests/MSTest.Acceptance.IntegrationTests/TrxReportTests.cs	Adds an acceptance regression test that inspects TRX XML for data-driven per-row stdout isolation.
src/Adapter/MSTestAdapter.PlatformServices/Services/TestContextImplementation.cs	Introduces GetAndClear* methods for stdout/stderr/trace buffers.
src/Adapter/MSTestAdapter.PlatformServices/Execution/TestMethodInfo.cs	Switches to GetAndClear* so output doesn’t accumulate across data rows.

Copilot's findings

• Files reviewed: 4/4 changed files
• Comments generated: 4

[Copilot]

The XML parsing loop skips UnitTestResults without a <StdOut> element (if (stdOut is null) continue;). Given the test’s summary says each data row’s StdOut should contain its marker, this can allow a false-positive pass if StdOut is missing/empty. Consider asserting that the expected markers are present (e.g., total markers across all results equals 3, or each of the 3 data-row results has exactly one marker) so the test can’t silently succeed when output isn’t written.

[Evangelink]

Summary

Workflow: Test Expert Reviewer 🧪
Date: 2026-04-29
Repository: microsoft/testfx

Key Findings

Unit tests (TestMethodInfoTests.cs) — Well structured. The three new InvokeShouldClear* tests correctly use the TestFramework.ForTestingMSTest convention (no [TestMethod] needed — TestContainer discovers public methods by convention). The two-invocation pattern with interleaved body replacement accurately simulates data-driven re-use of the same TestContextImplementation instance. Assertions are specific, include failure messages, and use both positive (.Contain) and negative (.NotContain) checks. ✅

Acceptance test (TrxReportDataDrivenOutputTests) — Two related weaknesses:

1. Vacuous pass risk (line 139): if (stdOut is null) { continue; } means if all StdOut nodes are absent, no assertions ever execute and the test passes. A regression dropping output entirely would go undetected.

2. <= 1 instead of == 1 (line 148): The comment says "exactly ONE marker" but IsLessThanOrEqualTo(markerCount, 1) also passes when markerCount == 0. Combined with the continue, an implementation that clears output before capturing it would satisfy both assertions.

Recommendations

• Add a positive assertion that at least N results (matching the [DataRow] count) have non-null StdOut with exactly one marker.
• Consider changing IsLessThanOrEqualTo(markerCount, 1) → AreEqual(1, markerCount) once the positive presence check is in place.

---

Generated by Test Expert Reviewer

🧪 Test quality reviewed by Test Expert Reviewer 🧪

[Evangelink]

Summary

Workflow: Expert Code Reviewer 🧠
Date: 2026-04-29
Repository: microsoft/testfx

Key Findings

No blocking issues found. Analysis covered correctness, threading & concurrency, performance, API compatibility, cross-TFM, resource management, security, and defensive coding.

Positive observations:

• Root cause fix is correct: GetAndClearOut/Err/Trace() are added to TestContextImplementation and called from TestMethodInfo.InvokeAsync(), preventing output accumulation across data-row iterations. The same TestContextImplementation instance is reused for all data rows, so clearing between iterations is the right fix.
• Preserves other call sites: The non-clearing GetOut()/GetErr()/GetTrace() are kept for TestClassInfo.cs and UnitTestRunner.cs where each context is single-use (class/assembly init/cleanup).
• Consistent threading pattern: The new read-then-clear methods follow the exact same pattern as the pre-existing GetAndClearDiagnosticMessages() extension (GetDiagnosticMessages() → ClearDiagnosticMessages()). Each SynchronizedStringBuilder operation is individually synchronized via [MethodImpl(MethodImplOptions.Synchronized)], consistent with all other callers.
• Tests are properly registered: The unit test class extends TestContainer (ForTestingMSTest framework), which discovers any public Task/void parameterless method — no [TestMethod] attribute needed. All three new methods satisfy that contract.
• Acceptance test covers the regression end-to-end: Generates a real MSTest data-driven test, runs it with TRX reporting, and verifies each UnitTestResult's StdOut contains at most one row-specific marker.
• SynchronizedStringBuilder.Clear(): This method already existed in the class (already used by ClearDiagnosticMessages). The PR simply wires it up for stdout/stderr/trace.

Recommendations

None — the change is minimal, surgical, and correct.

---

Generated by Expert Code Reviewer 🧠

🧠 Reviewed by Expert Code Reviewer 🧠

[Evangelink]

@copilot address review comments

[Copilot]

@copilot address review comments

Addressed all review comments in commit 28103bb:

• Fixed reversed Assert.IsGreaterThanOrEqualTo arguments ((results.Count, 3) → (3, results.Count))
• Changed Assert.IsLessThanOrEqualTo(markerCount, 1) → Assert.AreEqual(1, markerCount) to enforce exactly one marker per result
• Added resultsWithOutput counter with a post-loop assertion to prevent vacuous passes when StdOut is absent

[Evangelink]

@copilot resolve the merge conflicts in this pull request

[Copilot]

@copilot resolve the merge conflicts in this pull request

Merge conflicts resolved. Main had already implemented the atomic GetAndClear() approach on SynchronizedStringBuilder with slightly different method names (GetAndClearOutput/GetAndClearError vs GetAndClearOut/GetAndClearErr). I kept main's version in the conflict resolution — it's cleaner and addresses the threading concern from the earlier review comment.

[Copilot]

Copilot's findings

• Files reviewed: 2/2 changed files
• Comments generated: 1

[Evangelink]

Summary

Workflow: Test Expert Reviewer 🧪
Date: 2026-04-30
Repository: microsoft/testfx

Key Findings

Both issues raised in the previous Test Expert review (April 29, commit facdab3) have been addressed in commit 28103bb:

1. ✅ IsLessThanOrEqualTo(markerCount, 1) → AreEqual(1, markerCount, ...) — The acceptance test now enforces exactly one marker per row result, catching both the accumulation regression and a missing-output regression.

2. ✅ Vacuous-pass risk — resultsWithOutput counter added; Assert.IsGreaterThanOrEqualTo(3, resultsWithOutput, ...) at the end of the loop ensures at least 3 results actually had StdOut. Tests that silently drop output are now caught.

Unit tests (TestMethodInfoTests.cs) — Three tests (ClearStdOut, ClearStdErr, ClearDebugTrace) correctly follow the TestFramework.ForTestingMSTest convention (public parameterless methods on a TestContainer — no [TestMethod] required). The two-invocation pattern with interleaved body replacement accurately simulates data-driven re-use of the same TestContextImplementation. Assertions are specific, include failure messages, and use positive + negative checks. ✅

Acceptance test (TrxReportDataDrivenOutputTests) — Uses a unique GUID-named TRX file (no path collision risk), DynamicData over all TFMs for cross-framework coverage, and an inline generated test project with 3 [DataRow] entries. The assertion structure now correctly covers both the anti-regression (no accumulation) and the positive (output is present and correctly attributed). ✅

Recommendations

None — all previous concerns resolved.

---

Generated by Test Expert Reviewer 🧪

🧪 Test quality reviewed by Test Expert Reviewer 🧪

🧪 Test quality reviewed by Test Expert Reviewer 🧪

[Evangelink]

Summary

Workflow: Expert Code Reviewer 🧠
Date: 2026-04-30
Repository: microsoft/testfx

Key Findings

No blocking issues found. This re-review covers the test-only changes introduced after the 2026-04-29 review (merge-conflict resolution dropped the production code changes already present in main; the current diff consists entirely of test additions).

Verified correct:

• Assert.IsGreaterThanOrEqualTo(3, results.Count, ...) and Assert.IsGreaterThanOrEqualTo(3, resultsWithOutput, ...) — The API signature is IsGreaterThanOrEqualTo(T lowerBound, T value) which asserts value >= lowerBound. Both calls correctly assert a minimum of 3. The concern raised and addressed in PR comments about reversed arguments is resolved in the current state.
• Assert.AreEqual(1, markerCount, ...) — Correctly enforces exactly one marker per result, addressing the <= 1 vacuous-pass concern from the Test Expert review.
• resultsWithOutput counter — Post-loop assertion Assert.IsGreaterThanOrEqualTo(3, resultsWithOutput) prevents the test from passing vacuously if all StdOut nodes are null.
• Unit tests — Three new TestMethodInfoInvokeShouldClear* tests correctly reuse a single TestMethodInfo and invoke it twice with distinct output, verifying no leakage. The pattern is consistent with all existing tests in TestMethodInfoTests.

Recommendations

None — the test additions are correct and the previous Test Expert feedback has been fully addressed.

---

Generated by Expert Code Reviewer 🧠

🧠 Reviewed by Expert Code Reviewer 🧠

🧠 Reviewed by Expert Code Reviewer 🧠

[Evangelink]

Summary

Workflow: Expert Code Reviewer 🧠
Date: 2026-04-30
Repository: microsoft/testfx

Key Findings

No issues found. The single new commit since the last review (ccb4fc25) is a one-line compile error fix: ExitCodes.Success → ExitCode.Success in the acceptance test. The correction is accurate — the correct enum type in this codebase is ExitCode, not ExitCodes.

All substantive changes (production fix in TestContextImplementation/TestMethodInfo, unit tests, and acceptance test logic) were fully reviewed and found correct in the two prior runs of this workflow.

Recommendations

None.

---

Generated by Expert Code Reviewer 🧠

🧠 Reviewed by Expert Code Reviewer 🧠

🧠 Reviewed by Expert Code Reviewer 🧠

[Evangelink]

Summary

Workflow: Test Expert Reviewer 🧪
Date: 2026-04-30
Repository: microsoft/testfx

Key Findings

The only change since the previous Test Expert review (7ac5236e) is a trivial compile error fix (ExitCodes.Success → ExitCode.Success). No test logic was altered. All findings from prior reviews have been addressed.

Unit tests (TestMethodInfoTests.cs) — Three InvokeShouldClear* tests correctly follow the ForTestingMSTest convention (public parameterless async methods on TestContainer — no [TestMethod] needed). The two-invocation pattern with interleaved body replacement accurately simulates data-driven reuse of the same TestContextImplementation. FluentAssertions with both positive (.Contain) and negative (.NotContain) checks with helpful failure messages. ✅

Acceptance test (TrxReportDataDrivenOutputTests) — All previous concerns fully addressed:

• GUID-named TRX file prevents path collisions ✅
• DynamicData over all TFMs ensures cross-framework coverage ✅
• Assert.AreEqual(1, markerCount, ...) enforces exactly one marker per result (fixes previous ≤ 1 vacuous-pass risk) ✅
• Assert.IsGreaterThanOrEqualTo(3, resultsWithOutput, ...) prevents the test from passing if output is silently dropped ✅

Recommendations

None — all previous concerns resolved; tests are reliable and well-structured.

---

Generated by Test Expert Reviewer 🧪

🧪 Test quality reviewed by Test Expert Reviewer 🧪

🧪 Test quality reviewed by Test Expert Reviewer 🧪