Fix log accumulation across DynamicData test invocations - #7925

Merged
Amaury Levé (Evangelink) merged 3 commits into
mainfrom
dev/amauryleve/fix-dynamic-data-log-accumulation
Apr 29, 2026
Merged

Fix log accumulation across DynamicData test invocations#7925
Amaury Levé (Evangelink) merged 3 commits into
mainfrom
dev/amauryleve/fix-dynamic-data-log-accumulation

Conversation

@Evangelink

Copy link
Copy Markdown
Member

Summary

Fixes#7846

When running DynamicData tests with multiple invocations, the TestContextImplementation output buffers (_stdOutStringBuilder, _stdErrStringBuilder, _traceStringBuilder) were never cleared between invocations. Since DynamicData tests reuse the same TestContext instance across all invocations, logs accumulated exponentially:

  • Test 1 result: logs from test 1
  • Test 2 result: logs from test 1 + test 2
  • Test 3 result: logs from test 1 + test 2 + test 3
  • ...

This caused >20 GB RAM usage for simple projects and truncated/broken log output in test summaries.

Root Cause

GetOut(), GetErr(), and GetTrace() on TestContextImplementation returned the StringBuilder contents via ToString() but never cleared them. In contrast, GetAndClearDiagnosticMessages() and GetResultFiles() already followed a get-then-clear pattern.

Fix

Replace GetOut(), GetErr(), GetTrace() with GetAndClearOutput(), GetAndClearError(), GetAndClearTrace() that clear the StringBuilder after extracting the value — matching the existing GetAndClearDiagnosticMessages() pattern.

Changes

  • TestContextImplementation.cs — Replace non-clearing getters with GetAndClearOutput/Error/Trace methods
  • TestMethodInfo.cs — Update call site in InvokeAsync finally block
  • UnitTestRunner.cs — Update call sites in assembly init/cleanup
  • TestClassInfo.cs — Update call sites in class init/cleanup
  • TestMethodInfoTests.cs — Add 3 regression tests verifying buffers don't accumulate across invocations
  • TestContextImplementationTests.cs — Update existing thread-safety test to use renamed methods

GetOut(), GetErr(), and GetTrace() on TestContextImplementation returned
accumulated buffer contents but never cleared them. When DynamicData tests
reuse the same TestContext across multiple invocations, each subsequent
result contained all logs from all previous invocations, causing exponential
memory growth (>20 GB for simple projects).
Replace these with GetAndClearOutput(), GetAndClearError(), and
GetAndClearTrace() that clear the StringBuilder after extracting the value,
matching the existing GetAndClearDiagnosticMessages() pattern.
Fixes#7846
CopilotAI review requested due to automatic review settings April 29, 2026 05:27

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Fixes MSTest adapter log buffer accumulation across multiple DynamicData invocations by switching stdout/stderr/trace retrieval to a get-then-clear pattern, preventing exponential growth in captured output and memory usage.

Changes:

  • Replace GetOut/GetErr/GetTrace with GetAndClearOutput/GetAndClearError/GetAndClearTrace in TestContextImplementation.
  • Update execution pipeline call sites (test method invoke; assembly/class init & cleanup) to use the new draining APIs.
  • Add regression unit tests to ensure output/error/trace don’t accumulate across multiple invocations.
Show a summary per file
FileDescription
test/UnitTests/MSTestAdapter.PlatformServices.UnitTests/Services/TestContextImplementationTests.csUpdates background-thread write test to use new get-and-clear APIs.
test/UnitTests/MSTestAdapter.PlatformServices.UnitTests/Execution/TestMethodInfoTests.csAdds regression tests validating buffers don’t accumulate across repeated InvokeAsync calls.
src/Adapter/MSTestAdapter.PlatformServices/Services/TestContextImplementation.csIntroduces get-and-clear methods for stdout/stderr/trace buffers.
src/Adapter/MSTestAdapter.PlatformServices/Execution/UnitTestRunner.csUpdates assembly init/cleanup log capture to drain buffers.
src/Adapter/MSTestAdapter.PlatformServices/Execution/TestMethodInfo.csUpdates per-test invocation log capture to drain buffers.
src/Adapter/MSTestAdapter.PlatformServices/Execution/TestClassInfo.csUpdates class init/cleanup log capture to drain buffers.

Copilot's findings

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

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Summary

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

Key Findings

  1. [Coverage — Minor]WritesFromBackgroundThreadShouldNotThrow in TestContextImplementationTests.cs was updated for Output and Error but not for Trace. GetAndClearTrace() also calls Clear() under concurrency and should be included in the thread-safety test.

  2. [Coverage — Important] No test in TestContextImplementationTests.cs directly verifies the clearing contract of the new GetAndClear* methods. The only test for these methods is the thread-safety check (which only asserts no exception). A dedicated read-then-clear test would document and protect the core invariant introduced by this fix.

  3. [Coverage — Minor] Regression tests for log accumulation are scoped to TestMethodInfo.InvokeAsync. The TestClassInfo (class init/cleanup) and UnitTestRunner (assembly init/cleanup) call sites that were also changed have no equivalent accumulation tests.

Recommendations

  • Add a GetAndClearOutput_ShouldReturnContentThenClearBuffer-style test (and equivalents for Error/Trace) in TestContextImplementationTests.cs to directly validate the clearing behavior.
  • Extend WritesFromBackgroundThreadShouldNotThrow to also call GetAndClearTrace() for completeness.
  • Consider adding accumulation regression tests for the class/assembly init-cleanup paths, or add a comment noting they're exercised by acceptance tests.

The existing accumulation tests in TestMethodInfoTests.cs are well-structured and correctly detect the regression: result2.LogOutput.Should().Be("invocation_output") would fail with the old non-clearing code (it would produce "invocation_outputinvocation_output"), so the core fix is properly verified.


Generated by Test Expert Reviewer 🧪

🧪 Test quality reviewed by Test Expert Reviewer 🧪

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Summary

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

Key Findings

[Threading — Minor/Accepted]GetAndClearOutput/Error/Trace each perform two individually-synchronized calls (ToString() + Clear()). Content written by a background thread in the TOCTOU window is silently discarded. This matches the pre-existing GetAndClearDiagnosticMessages() pattern and is an accepted design trade-off in this codebase. See inline comment.

Positive Observations

  • Root cause addressed correctly: The TestContextImplementation output buffers now clear after each read, consistent with the established GetAndClearDiagnosticMessages pattern. The exponential memory growth scenario is fully closed.
  • Bonus fix in multi-assembly cleanup: In UnitTestRunner.RunAssemblyCleanupAsync, the foreach over multiple TestAssemblyInfo entries previously accumulated logs across iterations (each GetOut() returned all prior logs); the new GetAndClearOutput() correctly scopes each iteration to only its own cleanup logs.
  • Regression tests are valid: The three new public async Task tests in TestMethodInfoTests.cs are properly discoverable by TestFramework.ForTestingMSTest (which discovers all public parameterless methods returning void or Task in TestContainer subclasses — no [TestMethod] attribute required).
  • Exclusive control flow: In both TestClassInfo.DoRunAsync and RunAssemblyCleanupAsync, the failure path (creates a new TestResult and returns) and the success path (lastResult +=) are mutually exclusive, so there is no double-clear or double-append risk.
  • No public API surface changed: All three renamed methods remain internal.

Recommendations

  • Consider adding a brief comment to the three new methods noting the intentional TOCTOU (consistent with GetAndClearDiagnosticMessages), to signal future maintainers that this is deliberate rather than an oversight.

Generated by Expert Code Reviewer

🧠 Reviewed by Expert Code Reviewer 🧠

Address review feedback: ToString() + Clear() as two separate synchronized
calls could lose appended text if another thread writes between them. Add
a single synchronized GetAndClear() method to SynchronizedStringBuilder
that performs both operations under one lock.
- Add focused GetAndClear{Output,Error,Trace} unit tests in
TestContextImplementationTests that verify the buffer is cleared after
the first call (returns empty on second call).
- Add GetAndClearTrace() call to WritesFromBackgroundThreadShouldNotThrow
to complete thread-safety coverage for all three buffer types.
- Consolidate regression test comments: single block comment noting that
TestClassInfo and UnitTestRunner call sites use the same GetAndClear*
methods tested in isolation.
CopilotAI review requested due to automatic review settings April 29, 2026 05:39

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot's findings

  • Files reviewed: 6/6 changed files
  • Comments generated: 0 new

@Evangelink
Amaury Levé (Evangelink) merged commit 37cd497 into mainApr 29, 2026
28 checks passed
@Evangelink
Amaury Levé (Evangelink) deleted the dev/amauryleve/fix-dynamic-data-log-accumulation branch April 29, 2026 09:06
@Evangelink

Copy link
Copy Markdown
MemberAuthor

/backport to rel/4.2

@github-actions

Copy link
Copy Markdown
Contributor

Started backporting to rel/4.2: https://github.com/microsoft/testfx/actions/runs/25104582574

Sign up for freeto 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.

Log collection is broken, >20 GB RAM usage for simple project

2 participants

@Evangelink
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Add copy buttons to all
 blocks\n(function() {\n function addCopyButtons() {\n document.querySelectorAll('pre code').forEach(function(codeBlock) {\n if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;\n codeBlock.parentElement.setAttribute('data-copy-added', 'true');\n \n var btn = document.createElement('button');\n btn.textContent = 'Copy';\n btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';\n btn.onmouseover = function() { this.style.opacity = '1'; };\n btn.onmouseout = function() { this.style.opacity = '0.7'; };\n btn.onclick = function() {\n navigator.clipboard.writeText(codeBlock.textContent).then(function() {\n btn.textContent = 'Copied!';\n setTimeout(function() { btn.textContent = 'Copy'; }, 1500);\n });\n };\n codeBlock.parentElement.style.position = 'relative';\n codeBlock.parentElement.appendChild(btn);\n });\n }\n \n addCopyButtons();\n \n // Re-run on dynamic content\n var observer = new MutationObserver(addCopyButtons);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Add Copy Buttons to Code Blocks");
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '
Skip to content

Fix log accumulation across DynamicData test invocations - #7925

Merged
Amaury Levé (Evangelink) merged 3 commits into
mainfrom
dev/amauryleve/fix-dynamic-data-log-accumulation
Apr 29, 2026
Merged

Fix log accumulation across DynamicData test invocations#7925
Amaury Levé (Evangelink) merged 3 commits into
mainfrom
dev/amauryleve/fix-dynamic-data-log-accumulation

Conversation

@Evangelink

Copy link
Copy Markdown
Member

Summary

Fixes#7846

When running DynamicData tests with multiple invocations, the TestContextImplementation output buffers (_stdOutStringBuilder, _stdErrStringBuilder, _traceStringBuilder) were never cleared between invocations. Since DynamicData tests reuse the same TestContext instance across all invocations, logs accumulated exponentially:

  • Test 1 result: logs from test 1
  • Test 2 result: logs from test 1 + test 2
  • Test 3 result: logs from test 1 + test 2 + test 3
  • ...

This caused >20 GB RAM usage for simple projects and truncated/broken log output in test summaries.

Root Cause

GetOut(), GetErr(), and GetTrace() on TestContextImplementation returned the StringBuilder contents via ToString() but never cleared them. In contrast, GetAndClearDiagnosticMessages() and GetResultFiles() already followed a get-then-clear pattern.

Fix

Replace GetOut(), GetErr(), GetTrace() with GetAndClearOutput(), GetAndClearError(), GetAndClearTrace() that clear the StringBuilder after extracting the value — matching the existing GetAndClearDiagnosticMessages() pattern.

Changes

  • TestContextImplementation.cs — Replace non-clearing getters with GetAndClearOutput/Error/Trace methods
  • TestMethodInfo.cs — Update call site in InvokeAsync finally block
  • UnitTestRunner.cs — Update call sites in assembly init/cleanup
  • TestClassInfo.cs — Update call sites in class init/cleanup
  • TestMethodInfoTests.cs — Add 3 regression tests verifying buffers don't accumulate across invocations
  • TestContextImplementationTests.cs — Update existing thread-safety test to use renamed methods

GetOut(), GetErr(), and GetTrace() on TestContextImplementation returned
accumulated buffer contents but never cleared them. When DynamicData tests
reuse the same TestContext across multiple invocations, each subsequent
result contained all logs from all previous invocations, causing exponential
memory growth (>20 GB for simple projects).
Replace these with GetAndClearOutput(), GetAndClearError(), and
GetAndClearTrace() that clear the StringBuilder after extracting the value,
matching the existing GetAndClearDiagnosticMessages() pattern.
Fixes#7846
CopilotAI review requested due to automatic review settings April 29, 2026 05:27

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Fixes MSTest adapter log buffer accumulation across multiple DynamicData invocations by switching stdout/stderr/trace retrieval to a get-then-clear pattern, preventing exponential growth in captured output and memory usage.

Changes:

  • Replace GetOut/GetErr/GetTrace with GetAndClearOutput/GetAndClearError/GetAndClearTrace in TestContextImplementation.
  • Update execution pipeline call sites (test method invoke; assembly/class init & cleanup) to use the new draining APIs.
  • Add regression unit tests to ensure output/error/trace don’t accumulate across multiple invocations.
Show a summary per file
FileDescription
test/UnitTests/MSTestAdapter.PlatformServices.UnitTests/Services/TestContextImplementationTests.csUpdates background-thread write test to use new get-and-clear APIs.
test/UnitTests/MSTestAdapter.PlatformServices.UnitTests/Execution/TestMethodInfoTests.csAdds regression tests validating buffers don’t accumulate across repeated InvokeAsync calls.
src/Adapter/MSTestAdapter.PlatformServices/Services/TestContextImplementation.csIntroduces get-and-clear methods for stdout/stderr/trace buffers.
src/Adapter/MSTestAdapter.PlatformServices/Execution/UnitTestRunner.csUpdates assembly init/cleanup log capture to drain buffers.
src/Adapter/MSTestAdapter.PlatformServices/Execution/TestMethodInfo.csUpdates per-test invocation log capture to drain buffers.
src/Adapter/MSTestAdapter.PlatformServices/Execution/TestClassInfo.csUpdates class init/cleanup log capture to drain buffers.

Copilot's findings

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

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Summary

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

Key Findings

  1. [Coverage — Minor]WritesFromBackgroundThreadShouldNotThrow in TestContextImplementationTests.cs was updated for Output and Error but not for Trace. GetAndClearTrace() also calls Clear() under concurrency and should be included in the thread-safety test.

  2. [Coverage — Important] No test in TestContextImplementationTests.cs directly verifies the clearing contract of the new GetAndClear* methods. The only test for these methods is the thread-safety check (which only asserts no exception). A dedicated read-then-clear test would document and protect the core invariant introduced by this fix.

  3. [Coverage — Minor] Regression tests for log accumulation are scoped to TestMethodInfo.InvokeAsync. The TestClassInfo (class init/cleanup) and UnitTestRunner (assembly init/cleanup) call sites that were also changed have no equivalent accumulation tests.

Recommendations

  • Add a GetAndClearOutput_ShouldReturnContentThenClearBuffer-style test (and equivalents for Error/Trace) in TestContextImplementationTests.cs to directly validate the clearing behavior.
  • Extend WritesFromBackgroundThreadShouldNotThrow to also call GetAndClearTrace() for completeness.
  • Consider adding accumulation regression tests for the class/assembly init-cleanup paths, or add a comment noting they're exercised by acceptance tests.

The existing accumulation tests in TestMethodInfoTests.cs are well-structured and correctly detect the regression: result2.LogOutput.Should().Be("invocation_output") would fail with the old non-clearing code (it would produce "invocation_outputinvocation_output"), so the core fix is properly verified.


Generated by Test Expert Reviewer 🧪

🧪 Test quality reviewed by Test Expert Reviewer 🧪

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Summary

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

Key Findings

[Threading — Minor/Accepted]GetAndClearOutput/Error/Trace each perform two individually-synchronized calls (ToString() + Clear()). Content written by a background thread in the TOCTOU window is silently discarded. This matches the pre-existing GetAndClearDiagnosticMessages() pattern and is an accepted design trade-off in this codebase. See inline comment.

Positive Observations

  • Root cause addressed correctly: The TestContextImplementation output buffers now clear after each read, consistent with the established GetAndClearDiagnosticMessages pattern. The exponential memory growth scenario is fully closed.
  • Bonus fix in multi-assembly cleanup: In UnitTestRunner.RunAssemblyCleanupAsync, the foreach over multiple TestAssemblyInfo entries previously accumulated logs across iterations (each GetOut() returned all prior logs); the new GetAndClearOutput() correctly scopes each iteration to only its own cleanup logs.
  • Regression tests are valid: The three new public async Task tests in TestMethodInfoTests.cs are properly discoverable by TestFramework.ForTestingMSTest (which discovers all public parameterless methods returning void or Task in TestContainer subclasses — no [TestMethod] attribute required).
  • Exclusive control flow: In both TestClassInfo.DoRunAsync and RunAssemblyCleanupAsync, the failure path (creates a new TestResult and returns) and the success path (lastResult +=) are mutually exclusive, so there is no double-clear or double-append risk.
  • No public API surface changed: All three renamed methods remain internal.

Recommendations

  • Consider adding a brief comment to the three new methods noting the intentional TOCTOU (consistent with GetAndClearDiagnosticMessages), to signal future maintainers that this is deliberate rather than an oversight.

Generated by Expert Code Reviewer

🧠 Reviewed by Expert Code Reviewer 🧠

Address review feedback: ToString() + Clear() as two separate synchronized
calls could lose appended text if another thread writes between them. Add
a single synchronized GetAndClear() method to SynchronizedStringBuilder
that performs both operations under one lock.
- Add focused GetAndClear{Output,Error,Trace} unit tests in
TestContextImplementationTests that verify the buffer is cleared after
the first call (returns empty on second call).
- Add GetAndClearTrace() call to WritesFromBackgroundThreadShouldNotThrow
to complete thread-safety coverage for all three buffer types.
- Consolidate regression test comments: single block comment noting that
TestClassInfo and UnitTestRunner call sites use the same GetAndClear*
methods tested in isolation.
CopilotAI review requested due to automatic review settings April 29, 2026 05:39

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot's findings

  • Files reviewed: 6/6 changed files
  • Comments generated: 0 new

@Evangelink
Amaury Levé (Evangelink) merged commit 37cd497 into mainApr 29, 2026
28 checks passed
@Evangelink
Amaury Levé (Evangelink) deleted the dev/amauryleve/fix-dynamic-data-log-accumulation branch April 29, 2026 09:06
@Evangelink

Copy link
Copy Markdown
MemberAuthor

/backport to rel/4.2

@github-actions

Copy link
Copy Markdown
Contributor

Started backporting to rel/4.2: https://github.com/microsoft/testfx/actions/runs/25104582574

Sign up for freeto 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.

Log collection is broken, >20 GB RAM usage for simple project

2 participants

@Evangelink
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Force GitHub README to respect dark mode\n(function() {\n var style = document.createElement('style');\n style.textContent = '\n .markdown-body {\n color-scheme: dark light;\n }\n .markdown-body pre { background: #161b22 !important; }\n .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }\n .markdown-body table th, .markdown-body table td { border-color: #30363d !important; }\n .markdown-body img { background: #0d1117; }\n .markdown-body blockquote { border-left-color: #8b949e; }\n .markdown-body hr { border-color: #30363d; }\n ';\n document.head.appendChild(style);\n})();", "GitHub Dark Mode README Fix"); } } catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Fix log accumulation across DynamicData test invocations - #7925

Merged
Amaury Levé (Evangelink) merged 3 commits into
mainfrom
dev/amauryleve/fix-dynamic-data-log-accumulation
Apr 29, 2026
Merged

Fix log accumulation across DynamicData test invocations#7925
Amaury Levé (Evangelink) merged 3 commits into
mainfrom
dev/amauryleve/fix-dynamic-data-log-accumulation

Conversation

@Evangelink

Copy link
Copy Markdown
Member

Summary

Fixes#7846

When running DynamicData tests with multiple invocations, the TestContextImplementation output buffers (_stdOutStringBuilder, _stdErrStringBuilder, _traceStringBuilder) were never cleared between invocations. Since DynamicData tests reuse the same TestContext instance across all invocations, logs accumulated exponentially:

  • Test 1 result: logs from test 1
  • Test 2 result: logs from test 1 + test 2
  • Test 3 result: logs from test 1 + test 2 + test 3
  • ...

This caused >20 GB RAM usage for simple projects and truncated/broken log output in test summaries.

Root Cause

GetOut(), GetErr(), and GetTrace() on TestContextImplementation returned the StringBuilder contents via ToString() but never cleared them. In contrast, GetAndClearDiagnosticMessages() and GetResultFiles() already followed a get-then-clear pattern.

Fix

Replace GetOut(), GetErr(), GetTrace() with GetAndClearOutput(), GetAndClearError(), GetAndClearTrace() that clear the StringBuilder after extracting the value — matching the existing GetAndClearDiagnosticMessages() pattern.

Changes

  • TestContextImplementation.cs — Replace non-clearing getters with GetAndClearOutput/Error/Trace methods
  • TestMethodInfo.cs — Update call site in InvokeAsync finally block
  • UnitTestRunner.cs — Update call sites in assembly init/cleanup
  • TestClassInfo.cs — Update call sites in class init/cleanup
  • TestMethodInfoTests.cs — Add 3 regression tests verifying buffers don't accumulate across invocations
  • TestContextImplementationTests.cs — Update existing thread-safety test to use renamed methods

GetOut(), GetErr(), and GetTrace() on TestContextImplementation returned
accumulated buffer contents but never cleared them. When DynamicData tests
reuse the same TestContext across multiple invocations, each subsequent
result contained all logs from all previous invocations, causing exponential
memory growth (>20 GB for simple projects).
Replace these with GetAndClearOutput(), GetAndClearError(), and
GetAndClearTrace() that clear the StringBuilder after extracting the value,
matching the existing GetAndClearDiagnosticMessages() pattern.
Fixes#7846
CopilotAI review requested due to automatic review settings April 29, 2026 05:27

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Fixes MSTest adapter log buffer accumulation across multiple DynamicData invocations by switching stdout/stderr/trace retrieval to a get-then-clear pattern, preventing exponential growth in captured output and memory usage.

Changes:

  • Replace GetOut/GetErr/GetTrace with GetAndClearOutput/GetAndClearError/GetAndClearTrace in TestContextImplementation.
  • Update execution pipeline call sites (test method invoke; assembly/class init & cleanup) to use the new draining APIs.
  • Add regression unit tests to ensure output/error/trace don’t accumulate across multiple invocations.
Show a summary per file
FileDescription
test/UnitTests/MSTestAdapter.PlatformServices.UnitTests/Services/TestContextImplementationTests.csUpdates background-thread write test to use new get-and-clear APIs.
test/UnitTests/MSTestAdapter.PlatformServices.UnitTests/Execution/TestMethodInfoTests.csAdds regression tests validating buffers don’t accumulate across repeated InvokeAsync calls.
src/Adapter/MSTestAdapter.PlatformServices/Services/TestContextImplementation.csIntroduces get-and-clear methods for stdout/stderr/trace buffers.
src/Adapter/MSTestAdapter.PlatformServices/Execution/UnitTestRunner.csUpdates assembly init/cleanup log capture to drain buffers.
src/Adapter/MSTestAdapter.PlatformServices/Execution/TestMethodInfo.csUpdates per-test invocation log capture to drain buffers.
src/Adapter/MSTestAdapter.PlatformServices/Execution/TestClassInfo.csUpdates class init/cleanup log capture to drain buffers.

Copilot's findings

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

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Summary

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

Key Findings

  1. [Coverage — Minor]WritesFromBackgroundThreadShouldNotThrow in TestContextImplementationTests.cs was updated for Output and Error but not for Trace. GetAndClearTrace() also calls Clear() under concurrency and should be included in the thread-safety test.

  2. [Coverage — Important] No test in TestContextImplementationTests.cs directly verifies the clearing contract of the new GetAndClear* methods. The only test for these methods is the thread-safety check (which only asserts no exception). A dedicated read-then-clear test would document and protect the core invariant introduced by this fix.

  3. [Coverage — Minor] Regression tests for log accumulation are scoped to TestMethodInfo.InvokeAsync. The TestClassInfo (class init/cleanup) and UnitTestRunner (assembly init/cleanup) call sites that were also changed have no equivalent accumulation tests.

Recommendations

  • Add a GetAndClearOutput_ShouldReturnContentThenClearBuffer-style test (and equivalents for Error/Trace) in TestContextImplementationTests.cs to directly validate the clearing behavior.
  • Extend WritesFromBackgroundThreadShouldNotThrow to also call GetAndClearTrace() for completeness.
  • Consider adding accumulation regression tests for the class/assembly init-cleanup paths, or add a comment noting they're exercised by acceptance tests.

The existing accumulation tests in TestMethodInfoTests.cs are well-structured and correctly detect the regression: result2.LogOutput.Should().Be("invocation_output") would fail with the old non-clearing code (it would produce "invocation_outputinvocation_output"), so the core fix is properly verified.


Generated by Test Expert Reviewer 🧪

🧪 Test quality reviewed by Test Expert Reviewer 🧪

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Summary

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

Key Findings

[Threading — Minor/Accepted]GetAndClearOutput/Error/Trace each perform two individually-synchronized calls (ToString() + Clear()). Content written by a background thread in the TOCTOU window is silently discarded. This matches the pre-existing GetAndClearDiagnosticMessages() pattern and is an accepted design trade-off in this codebase. See inline comment.

Positive Observations

  • Root cause addressed correctly: The TestContextImplementation output buffers now clear after each read, consistent with the established GetAndClearDiagnosticMessages pattern. The exponential memory growth scenario is fully closed.
  • Bonus fix in multi-assembly cleanup: In UnitTestRunner.RunAssemblyCleanupAsync, the foreach over multiple TestAssemblyInfo entries previously accumulated logs across iterations (each GetOut() returned all prior logs); the new GetAndClearOutput() correctly scopes each iteration to only its own cleanup logs.
  • Regression tests are valid: The three new public async Task tests in TestMethodInfoTests.cs are properly discoverable by TestFramework.ForTestingMSTest (which discovers all public parameterless methods returning void or Task in TestContainer subclasses — no [TestMethod] attribute required).
  • Exclusive control flow: In both TestClassInfo.DoRunAsync and RunAssemblyCleanupAsync, the failure path (creates a new TestResult and returns) and the success path (lastResult +=) are mutually exclusive, so there is no double-clear or double-append risk.
  • No public API surface changed: All three renamed methods remain internal.

Recommendations

  • Consider adding a brief comment to the three new methods noting the intentional TOCTOU (consistent with GetAndClearDiagnosticMessages), to signal future maintainers that this is deliberate rather than an oversight.

Generated by Expert Code Reviewer

🧠 Reviewed by Expert Code Reviewer 🧠

Address review feedback: ToString() + Clear() as two separate synchronized
calls could lose appended text if another thread writes between them. Add
a single synchronized GetAndClear() method to SynchronizedStringBuilder
that performs both operations under one lock.
- Add focused GetAndClear{Output,Error,Trace} unit tests in
TestContextImplementationTests that verify the buffer is cleared after
the first call (returns empty on second call).
- Add GetAndClearTrace() call to WritesFromBackgroundThreadShouldNotThrow
to complete thread-safety coverage for all three buffer types.
- Consolidate regression test comments: single block comment noting that
TestClassInfo and UnitTestRunner call sites use the same GetAndClear*
methods tested in isolation.
CopilotAI review requested due to automatic review settings April 29, 2026 05:39

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot's findings

  • Files reviewed: 6/6 changed files
  • Comments generated: 0 new

@Evangelink
Amaury Levé (Evangelink) merged commit 37cd497 into mainApr 29, 2026
28 checks passed
@Evangelink
Amaury Levé (Evangelink) deleted the dev/amauryleve/fix-dynamic-data-log-accumulation branch April 29, 2026 09:06
@Evangelink

Copy link
Copy Markdown
MemberAuthor

/backport to rel/4.2

@github-actions

Copy link
Copy Markdown
Contributor

Started backporting to rel/4.2: https://github.com/microsoft/testfx/actions/runs/25104582574

Sign up for freeto 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.

Log collection is broken, >20 GB RAM usage for simple project

2 participants

@Evangelink
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Highlight search terms from Google/DuckDuckGo/Bing referrer\n(function() {\n var ref = document.referrer;\n var terms = [];\n \n if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {\n var url = new URL(ref);\n var q = url.searchParams.get('q') || url.searchParams.get('p');\n if (q) {\n terms = q.split(/\\s+/).filter(function(t) { return t.length > 2; });\n }\n }\n \n if (terms.length === 0) return;\n \n var style = document.createElement('style');\n style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';\n document.head.appendChild(style);\n \n function highlight(node) {\n if (node.nodeType === 3) { // text node\n var text = node.textContent;\n var found = false;\n terms.forEach(function(term) {\n var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\\]\\\\]/g, '\\\\') + ')', 'gi');\n if (regex.test(text)) {\n found = true;\n var frag = document.createDocumentFragment();\n var parts = text.split(regex);\n parts.forEach(function(part, i) {\n if (i % 2 === 0) {\n frag.appendChild(document.createTextNode(part));\n } else {\n var span = document.createElement('span');\n span.className = 'userscript-highlight';\n span.textContent = part;\n frag.appendChild(span);\n }\n });\n node.parentNode.replaceChild(frag, node);\n }\n });\n } else if (node.nodeType === 1 && node.childNodes) { // element\n var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];\n if (!skipTags.includes(node.tagName)) {\n Array.from(node.childNodes).forEach(highlight);\n }\n }\n }\n \n highlight(document.body);\n \n // Re-highlight on dynamic content\n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1 || node.nodeType === 3) highlight(node);\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Highlight Search Terms"); } } catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Fix log accumulation across DynamicData test invocations - #7925

Merged
Amaury Levé (Evangelink) merged 3 commits into
mainfrom
dev/amauryleve/fix-dynamic-data-log-accumulation
Apr 29, 2026
Merged

Fix log accumulation across DynamicData test invocations#7925
Amaury Levé (Evangelink) merged 3 commits into
mainfrom
dev/amauryleve/fix-dynamic-data-log-accumulation

Conversation

@Evangelink

Copy link
Copy Markdown
Member

Summary

Fixes#7846

When running DynamicData tests with multiple invocations, the TestContextImplementation output buffers (_stdOutStringBuilder, _stdErrStringBuilder, _traceStringBuilder) were never cleared between invocations. Since DynamicData tests reuse the same TestContext instance across all invocations, logs accumulated exponentially:

  • Test 1 result: logs from test 1
  • Test 2 result: logs from test 1 + test 2
  • Test 3 result: logs from test 1 + test 2 + test 3
  • ...

This caused >20 GB RAM usage for simple projects and truncated/broken log output in test summaries.

Root Cause

GetOut(), GetErr(), and GetTrace() on TestContextImplementation returned the StringBuilder contents via ToString() but never cleared them. In contrast, GetAndClearDiagnosticMessages() and GetResultFiles() already followed a get-then-clear pattern.

Fix

Replace GetOut(), GetErr(), GetTrace() with GetAndClearOutput(), GetAndClearError(), GetAndClearTrace() that clear the StringBuilder after extracting the value — matching the existing GetAndClearDiagnosticMessages() pattern.

Changes

  • TestContextImplementation.cs — Replace non-clearing getters with GetAndClearOutput/Error/Trace methods
  • TestMethodInfo.cs — Update call site in InvokeAsync finally block
  • UnitTestRunner.cs — Update call sites in assembly init/cleanup
  • TestClassInfo.cs — Update call sites in class init/cleanup
  • TestMethodInfoTests.cs — Add 3 regression tests verifying buffers don't accumulate across invocations
  • TestContextImplementationTests.cs — Update existing thread-safety test to use renamed methods

GetOut(), GetErr(), and GetTrace() on TestContextImplementation returned
accumulated buffer contents but never cleared them. When DynamicData tests
reuse the same TestContext across multiple invocations, each subsequent
result contained all logs from all previous invocations, causing exponential
memory growth (>20 GB for simple projects).
Replace these with GetAndClearOutput(), GetAndClearError(), and
GetAndClearTrace() that clear the StringBuilder after extracting the value,
matching the existing GetAndClearDiagnosticMessages() pattern.
Fixes#7846
CopilotAI review requested due to automatic review settings April 29, 2026 05:27

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Fixes MSTest adapter log buffer accumulation across multiple DynamicData invocations by switching stdout/stderr/trace retrieval to a get-then-clear pattern, preventing exponential growth in captured output and memory usage.

Changes:

  • Replace GetOut/GetErr/GetTrace with GetAndClearOutput/GetAndClearError/GetAndClearTrace in TestContextImplementation.
  • Update execution pipeline call sites (test method invoke; assembly/class init & cleanup) to use the new draining APIs.
  • Add regression unit tests to ensure output/error/trace don’t accumulate across multiple invocations.
Show a summary per file
FileDescription
test/UnitTests/MSTestAdapter.PlatformServices.UnitTests/Services/TestContextImplementationTests.csUpdates background-thread write test to use new get-and-clear APIs.
test/UnitTests/MSTestAdapter.PlatformServices.UnitTests/Execution/TestMethodInfoTests.csAdds regression tests validating buffers don’t accumulate across repeated InvokeAsync calls.
src/Adapter/MSTestAdapter.PlatformServices/Services/TestContextImplementation.csIntroduces get-and-clear methods for stdout/stderr/trace buffers.
src/Adapter/MSTestAdapter.PlatformServices/Execution/UnitTestRunner.csUpdates assembly init/cleanup log capture to drain buffers.
src/Adapter/MSTestAdapter.PlatformServices/Execution/TestMethodInfo.csUpdates per-test invocation log capture to drain buffers.
src/Adapter/MSTestAdapter.PlatformServices/Execution/TestClassInfo.csUpdates class init/cleanup log capture to drain buffers.

Copilot's findings

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

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Summary

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

Key Findings

  1. [Coverage — Minor]WritesFromBackgroundThreadShouldNotThrow in TestContextImplementationTests.cs was updated for Output and Error but not for Trace. GetAndClearTrace() also calls Clear() under concurrency and should be included in the thread-safety test.

  2. [Coverage — Important] No test in TestContextImplementationTests.cs directly verifies the clearing contract of the new GetAndClear* methods. The only test for these methods is the thread-safety check (which only asserts no exception). A dedicated read-then-clear test would document and protect the core invariant introduced by this fix.

  3. [Coverage — Minor] Regression tests for log accumulation are scoped to TestMethodInfo.InvokeAsync. The TestClassInfo (class init/cleanup) and UnitTestRunner (assembly init/cleanup) call sites that were also changed have no equivalent accumulation tests.

Recommendations

  • Add a GetAndClearOutput_ShouldReturnContentThenClearBuffer-style test (and equivalents for Error/Trace) in TestContextImplementationTests.cs to directly validate the clearing behavior.
  • Extend WritesFromBackgroundThreadShouldNotThrow to also call GetAndClearTrace() for completeness.
  • Consider adding accumulation regression tests for the class/assembly init-cleanup paths, or add a comment noting they're exercised by acceptance tests.

The existing accumulation tests in TestMethodInfoTests.cs are well-structured and correctly detect the regression: result2.LogOutput.Should().Be("invocation_output") would fail with the old non-clearing code (it would produce "invocation_outputinvocation_output"), so the core fix is properly verified.


Generated by Test Expert Reviewer 🧪

🧪 Test quality reviewed by Test Expert Reviewer 🧪

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Summary

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

Key Findings

[Threading — Minor/Accepted]GetAndClearOutput/Error/Trace each perform two individually-synchronized calls (ToString() + Clear()). Content written by a background thread in the TOCTOU window is silently discarded. This matches the pre-existing GetAndClearDiagnosticMessages() pattern and is an accepted design trade-off in this codebase. See inline comment.

Positive Observations

  • Root cause addressed correctly: The TestContextImplementation output buffers now clear after each read, consistent with the established GetAndClearDiagnosticMessages pattern. The exponential memory growth scenario is fully closed.
  • Bonus fix in multi-assembly cleanup: In UnitTestRunner.RunAssemblyCleanupAsync, the foreach over multiple TestAssemblyInfo entries previously accumulated logs across iterations (each GetOut() returned all prior logs); the new GetAndClearOutput() correctly scopes each iteration to only its own cleanup logs.
  • Regression tests are valid: The three new public async Task tests in TestMethodInfoTests.cs are properly discoverable by TestFramework.ForTestingMSTest (which discovers all public parameterless methods returning void or Task in TestContainer subclasses — no [TestMethod] attribute required).
  • Exclusive control flow: In both TestClassInfo.DoRunAsync and RunAssemblyCleanupAsync, the failure path (creates a new TestResult and returns) and the success path (lastResult +=) are mutually exclusive, so there is no double-clear or double-append risk.
  • No public API surface changed: All three renamed methods remain internal.

Recommendations

  • Consider adding a brief comment to the three new methods noting the intentional TOCTOU (consistent with GetAndClearDiagnosticMessages), to signal future maintainers that this is deliberate rather than an oversight.

Generated by Expert Code Reviewer

🧠 Reviewed by Expert Code Reviewer 🧠

Address review feedback: ToString() + Clear() as two separate synchronized
calls could lose appended text if another thread writes between them. Add
a single synchronized GetAndClear() method to SynchronizedStringBuilder
that performs both operations under one lock.
- Add focused GetAndClear{Output,Error,Trace} unit tests in
TestContextImplementationTests that verify the buffer is cleared after
the first call (returns empty on second call).
- Add GetAndClearTrace() call to WritesFromBackgroundThreadShouldNotThrow
to complete thread-safety coverage for all three buffer types.
- Consolidate regression test comments: single block comment noting that
TestClassInfo and UnitTestRunner call sites use the same GetAndClear*
methods tested in isolation.
CopilotAI review requested due to automatic review settings April 29, 2026 05:39

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot's findings

  • Files reviewed: 6/6 changed files
  • Comments generated: 0 new

@Evangelink
Amaury Levé (Evangelink) merged commit 37cd497 into mainApr 29, 2026
28 checks passed
@Evangelink
Amaury Levé (Evangelink) deleted the dev/amauryleve/fix-dynamic-data-log-accumulation branch April 29, 2026 09:06
@Evangelink

Copy link
Copy Markdown
MemberAuthor

/backport to rel/4.2

@github-actions

Copy link
Copy Markdown
Contributor

Started backporting to rel/4.2: https://github.com/microsoft/testfx/actions/runs/25104582574

Sign up for freeto 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.

Log collection is broken, >20 GB RAM usage for simple project

2 participants

@Evangelink
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Strip utm_, fbclid, gclid, etc. from all links on page\n(function() {\n var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',\n 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',\n 'ref', 'ref_src', 'source', 'medium', 'campaign'];\n \n function cleanUrl(url) {\n try {\n var u = new URL(url, window.location.origin);\n var changed = false;\n trackingParams.forEach(function(p) {\n if (u.searchParams.has(p)) {\n u.searchParams.delete(p);\n changed = true;\n }\n });\n return changed ? u.toString() : url;\n } catch (e) {\n return url;\n }\n }\n \n function cleanLinks() {\n document.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n \n cleanLinks();\n \n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1) {\n if (node.tagName === 'A') cleanLinks();\n node.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Remove Tracking Parameters from Links"); } } catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); } })(); (function(){ try { var __m = "youtube.com"; var __re = new RegExp('^' + "youtube\\.com" + '
Skip to content

Fix log accumulation across DynamicData test invocations - #7925

Merged
Amaury Levé (Evangelink) merged 3 commits into
mainfrom
dev/amauryleve/fix-dynamic-data-log-accumulation
Apr 29, 2026
Merged

Fix log accumulation across DynamicData test invocations#7925
Amaury Levé (Evangelink) merged 3 commits into
mainfrom
dev/amauryleve/fix-dynamic-data-log-accumulation

Conversation

@Evangelink

Copy link
Copy Markdown
Member

Summary

Fixes#7846

When running DynamicData tests with multiple invocations, the TestContextImplementation output buffers (_stdOutStringBuilder, _stdErrStringBuilder, _traceStringBuilder) were never cleared between invocations. Since DynamicData tests reuse the same TestContext instance across all invocations, logs accumulated exponentially:

  • Test 1 result: logs from test 1
  • Test 2 result: logs from test 1 + test 2
  • Test 3 result: logs from test 1 + test 2 + test 3
  • ...

This caused >20 GB RAM usage for simple projects and truncated/broken log output in test summaries.

Root Cause

GetOut(), GetErr(), and GetTrace() on TestContextImplementation returned the StringBuilder contents via ToString() but never cleared them. In contrast, GetAndClearDiagnosticMessages() and GetResultFiles() already followed a get-then-clear pattern.

Fix

Replace GetOut(), GetErr(), GetTrace() with GetAndClearOutput(), GetAndClearError(), GetAndClearTrace() that clear the StringBuilder after extracting the value — matching the existing GetAndClearDiagnosticMessages() pattern.

Changes

  • TestContextImplementation.cs — Replace non-clearing getters with GetAndClearOutput/Error/Trace methods
  • TestMethodInfo.cs — Update call site in InvokeAsync finally block
  • UnitTestRunner.cs — Update call sites in assembly init/cleanup
  • TestClassInfo.cs — Update call sites in class init/cleanup
  • TestMethodInfoTests.cs — Add 3 regression tests verifying buffers don't accumulate across invocations
  • TestContextImplementationTests.cs — Update existing thread-safety test to use renamed methods

GetOut(), GetErr(), and GetTrace() on TestContextImplementation returned
accumulated buffer contents but never cleared them. When DynamicData tests
reuse the same TestContext across multiple invocations, each subsequent
result contained all logs from all previous invocations, causing exponential
memory growth (>20 GB for simple projects).
Replace these with GetAndClearOutput(), GetAndClearError(), and
GetAndClearTrace() that clear the StringBuilder after extracting the value,
matching the existing GetAndClearDiagnosticMessages() pattern.
Fixes#7846
CopilotAI review requested due to automatic review settings April 29, 2026 05:27

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Fixes MSTest adapter log buffer accumulation across multiple DynamicData invocations by switching stdout/stderr/trace retrieval to a get-then-clear pattern, preventing exponential growth in captured output and memory usage.

Changes:

  • Replace GetOut/GetErr/GetTrace with GetAndClearOutput/GetAndClearError/GetAndClearTrace in TestContextImplementation.
  • Update execution pipeline call sites (test method invoke; assembly/class init & cleanup) to use the new draining APIs.
  • Add regression unit tests to ensure output/error/trace don’t accumulate across multiple invocations.
Show a summary per file
FileDescription
test/UnitTests/MSTestAdapter.PlatformServices.UnitTests/Services/TestContextImplementationTests.csUpdates background-thread write test to use new get-and-clear APIs.
test/UnitTests/MSTestAdapter.PlatformServices.UnitTests/Execution/TestMethodInfoTests.csAdds regression tests validating buffers don’t accumulate across repeated InvokeAsync calls.
src/Adapter/MSTestAdapter.PlatformServices/Services/TestContextImplementation.csIntroduces get-and-clear methods for stdout/stderr/trace buffers.
src/Adapter/MSTestAdapter.PlatformServices/Execution/UnitTestRunner.csUpdates assembly init/cleanup log capture to drain buffers.
src/Adapter/MSTestAdapter.PlatformServices/Execution/TestMethodInfo.csUpdates per-test invocation log capture to drain buffers.
src/Adapter/MSTestAdapter.PlatformServices/Execution/TestClassInfo.csUpdates class init/cleanup log capture to drain buffers.

Copilot's findings

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

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Summary

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

Key Findings

  1. [Coverage — Minor]WritesFromBackgroundThreadShouldNotThrow in TestContextImplementationTests.cs was updated for Output and Error but not for Trace. GetAndClearTrace() also calls Clear() under concurrency and should be included in the thread-safety test.

  2. [Coverage — Important] No test in TestContextImplementationTests.cs directly verifies the clearing contract of the new GetAndClear* methods. The only test for these methods is the thread-safety check (which only asserts no exception). A dedicated read-then-clear test would document and protect the core invariant introduced by this fix.

  3. [Coverage — Minor] Regression tests for log accumulation are scoped to TestMethodInfo.InvokeAsync. The TestClassInfo (class init/cleanup) and UnitTestRunner (assembly init/cleanup) call sites that were also changed have no equivalent accumulation tests.

Recommendations

  • Add a GetAndClearOutput_ShouldReturnContentThenClearBuffer-style test (and equivalents for Error/Trace) in TestContextImplementationTests.cs to directly validate the clearing behavior.
  • Extend WritesFromBackgroundThreadShouldNotThrow to also call GetAndClearTrace() for completeness.
  • Consider adding accumulation regression tests for the class/assembly init-cleanup paths, or add a comment noting they're exercised by acceptance tests.

The existing accumulation tests in TestMethodInfoTests.cs are well-structured and correctly detect the regression: result2.LogOutput.Should().Be("invocation_output") would fail with the old non-clearing code (it would produce "invocation_outputinvocation_output"), so the core fix is properly verified.


Generated by Test Expert Reviewer 🧪

🧪 Test quality reviewed by Test Expert Reviewer 🧪

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Summary

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

Key Findings

[Threading — Minor/Accepted]GetAndClearOutput/Error/Trace each perform two individually-synchronized calls (ToString() + Clear()). Content written by a background thread in the TOCTOU window is silently discarded. This matches the pre-existing GetAndClearDiagnosticMessages() pattern and is an accepted design trade-off in this codebase. See inline comment.

Positive Observations

  • Root cause addressed correctly: The TestContextImplementation output buffers now clear after each read, consistent with the established GetAndClearDiagnosticMessages pattern. The exponential memory growth scenario is fully closed.
  • Bonus fix in multi-assembly cleanup: In UnitTestRunner.RunAssemblyCleanupAsync, the foreach over multiple TestAssemblyInfo entries previously accumulated logs across iterations (each GetOut() returned all prior logs); the new GetAndClearOutput() correctly scopes each iteration to only its own cleanup logs.
  • Regression tests are valid: The three new public async Task tests in TestMethodInfoTests.cs are properly discoverable by TestFramework.ForTestingMSTest (which discovers all public parameterless methods returning void or Task in TestContainer subclasses — no [TestMethod] attribute required).
  • Exclusive control flow: In both TestClassInfo.DoRunAsync and RunAssemblyCleanupAsync, the failure path (creates a new TestResult and returns) and the success path (lastResult +=) are mutually exclusive, so there is no double-clear or double-append risk.
  • No public API surface changed: All three renamed methods remain internal.

Recommendations

  • Consider adding a brief comment to the three new methods noting the intentional TOCTOU (consistent with GetAndClearDiagnosticMessages), to signal future maintainers that this is deliberate rather than an oversight.

Generated by Expert Code Reviewer

🧠 Reviewed by Expert Code Reviewer 🧠

Address review feedback: ToString() + Clear() as two separate synchronized
calls could lose appended text if another thread writes between them. Add
a single synchronized GetAndClear() method to SynchronizedStringBuilder
that performs both operations under one lock.
- Add focused GetAndClear{Output,Error,Trace} unit tests in
TestContextImplementationTests that verify the buffer is cleared after
the first call (returns empty on second call).
- Add GetAndClearTrace() call to WritesFromBackgroundThreadShouldNotThrow
to complete thread-safety coverage for all three buffer types.
- Consolidate regression test comments: single block comment noting that
TestClassInfo and UnitTestRunner call sites use the same GetAndClear*
methods tested in isolation.
CopilotAI review requested due to automatic review settings April 29, 2026 05:39

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot's findings

  • Files reviewed: 6/6 changed files
  • Comments generated: 0 new

@Evangelink
Amaury Levé (Evangelink) merged commit 37cd497 into mainApr 29, 2026
28 checks passed
@Evangelink
Amaury Levé (Evangelink) deleted the dev/amauryleve/fix-dynamic-data-log-accumulation branch April 29, 2026 09:06
@Evangelink

Copy link
Copy Markdown
MemberAuthor

/backport to rel/4.2

@github-actions

Copy link
Copy Markdown
Contributor

Started backporting to rel/4.2: https://github.com/microsoft/testfx/actions/runs/25104582574

Sign up for freeto 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.

Log collection is broken, >20 GB RAM usage for simple project

2 participants

@Evangelink
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Auto-enable theater mode on YouTube\n(function() {\n function tryTheater() {\n var btn = document.querySelector('button[aria-label=\"Theater mode\"], ytd-player #player button[title=\"Theater mode\"]');\n if (btn && !btn.classList.contains('activated')) {\n btn.click();\n }\n }\n \n // Try immediately\n tryTheater();\n \n // Try after navigation (SPA)\n var lastUrl = location.href;\n setInterval(function() {\n if (location.href !== lastUrl) {\n lastUrl = location.href;\n setTimeout(tryTheater, 500);\n }\n }, 1000);\n \n // Also try on player load\n var observer = new MutationObserver(tryTheater);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "YouTube Theater Mode Default"); } } catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Fix log accumulation across DynamicData test invocations - #7925

Merged
Amaury Levé (Evangelink) merged 3 commits into
mainfrom
dev/amauryleve/fix-dynamic-data-log-accumulation
Apr 29, 2026
Merged

Fix log accumulation across DynamicData test invocations#7925
Amaury Levé (Evangelink) merged 3 commits into
mainfrom
dev/amauryleve/fix-dynamic-data-log-accumulation

Conversation

@Evangelink

Copy link
Copy Markdown
Member

Summary

Fixes#7846

When running DynamicData tests with multiple invocations, the TestContextImplementation output buffers (_stdOutStringBuilder, _stdErrStringBuilder, _traceStringBuilder) were never cleared between invocations. Since DynamicData tests reuse the same TestContext instance across all invocations, logs accumulated exponentially:

  • Test 1 result: logs from test 1
  • Test 2 result: logs from test 1 + test 2
  • Test 3 result: logs from test 1 + test 2 + test 3
  • ...

This caused >20 GB RAM usage for simple projects and truncated/broken log output in test summaries.

Root Cause

GetOut(), GetErr(), and GetTrace() on TestContextImplementation returned the StringBuilder contents via ToString() but never cleared them. In contrast, GetAndClearDiagnosticMessages() and GetResultFiles() already followed a get-then-clear pattern.

Fix

Replace GetOut(), GetErr(), GetTrace() with GetAndClearOutput(), GetAndClearError(), GetAndClearTrace() that clear the StringBuilder after extracting the value — matching the existing GetAndClearDiagnosticMessages() pattern.

Changes

  • TestContextImplementation.cs — Replace non-clearing getters with GetAndClearOutput/Error/Trace methods
  • TestMethodInfo.cs — Update call site in InvokeAsync finally block
  • UnitTestRunner.cs — Update call sites in assembly init/cleanup
  • TestClassInfo.cs — Update call sites in class init/cleanup
  • TestMethodInfoTests.cs — Add 3 regression tests verifying buffers don't accumulate across invocations
  • TestContextImplementationTests.cs — Update existing thread-safety test to use renamed methods

GetOut(), GetErr(), and GetTrace() on TestContextImplementation returned
accumulated buffer contents but never cleared them. When DynamicData tests
reuse the same TestContext across multiple invocations, each subsequent
result contained all logs from all previous invocations, causing exponential
memory growth (>20 GB for simple projects).
Replace these with GetAndClearOutput(), GetAndClearError(), and
GetAndClearTrace() that clear the StringBuilder after extracting the value,
matching the existing GetAndClearDiagnosticMessages() pattern.
Fixes#7846
CopilotAI review requested due to automatic review settings April 29, 2026 05:27

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Fixes MSTest adapter log buffer accumulation across multiple DynamicData invocations by switching stdout/stderr/trace retrieval to a get-then-clear pattern, preventing exponential growth in captured output and memory usage.

Changes:

  • Replace GetOut/GetErr/GetTrace with GetAndClearOutput/GetAndClearError/GetAndClearTrace in TestContextImplementation.
  • Update execution pipeline call sites (test method invoke; assembly/class init & cleanup) to use the new draining APIs.
  • Add regression unit tests to ensure output/error/trace don’t accumulate across multiple invocations.
Show a summary per file
FileDescription
test/UnitTests/MSTestAdapter.PlatformServices.UnitTests/Services/TestContextImplementationTests.csUpdates background-thread write test to use new get-and-clear APIs.
test/UnitTests/MSTestAdapter.PlatformServices.UnitTests/Execution/TestMethodInfoTests.csAdds regression tests validating buffers don’t accumulate across repeated InvokeAsync calls.
src/Adapter/MSTestAdapter.PlatformServices/Services/TestContextImplementation.csIntroduces get-and-clear methods for stdout/stderr/trace buffers.
src/Adapter/MSTestAdapter.PlatformServices/Execution/UnitTestRunner.csUpdates assembly init/cleanup log capture to drain buffers.
src/Adapter/MSTestAdapter.PlatformServices/Execution/TestMethodInfo.csUpdates per-test invocation log capture to drain buffers.
src/Adapter/MSTestAdapter.PlatformServices/Execution/TestClassInfo.csUpdates class init/cleanup log capture to drain buffers.

Copilot's findings

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

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Summary

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

Key Findings

  1. [Coverage — Minor]WritesFromBackgroundThreadShouldNotThrow in TestContextImplementationTests.cs was updated for Output and Error but not for Trace. GetAndClearTrace() also calls Clear() under concurrency and should be included in the thread-safety test.

  2. [Coverage — Important] No test in TestContextImplementationTests.cs directly verifies the clearing contract of the new GetAndClear* methods. The only test for these methods is the thread-safety check (which only asserts no exception). A dedicated read-then-clear test would document and protect the core invariant introduced by this fix.

  3. [Coverage — Minor] Regression tests for log accumulation are scoped to TestMethodInfo.InvokeAsync. The TestClassInfo (class init/cleanup) and UnitTestRunner (assembly init/cleanup) call sites that were also changed have no equivalent accumulation tests.

Recommendations

  • Add a GetAndClearOutput_ShouldReturnContentThenClearBuffer-style test (and equivalents for Error/Trace) in TestContextImplementationTests.cs to directly validate the clearing behavior.
  • Extend WritesFromBackgroundThreadShouldNotThrow to also call GetAndClearTrace() for completeness.
  • Consider adding accumulation regression tests for the class/assembly init-cleanup paths, or add a comment noting they're exercised by acceptance tests.

The existing accumulation tests in TestMethodInfoTests.cs are well-structured and correctly detect the regression: result2.LogOutput.Should().Be("invocation_output") would fail with the old non-clearing code (it would produce "invocation_outputinvocation_output"), so the core fix is properly verified.


Generated by Test Expert Reviewer 🧪

🧪 Test quality reviewed by Test Expert Reviewer 🧪

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Summary

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

Key Findings

[Threading — Minor/Accepted]GetAndClearOutput/Error/Trace each perform two individually-synchronized calls (ToString() + Clear()). Content written by a background thread in the TOCTOU window is silently discarded. This matches the pre-existing GetAndClearDiagnosticMessages() pattern and is an accepted design trade-off in this codebase. See inline comment.

Positive Observations

  • Root cause addressed correctly: The TestContextImplementation output buffers now clear after each read, consistent with the established GetAndClearDiagnosticMessages pattern. The exponential memory growth scenario is fully closed.
  • Bonus fix in multi-assembly cleanup: In UnitTestRunner.RunAssemblyCleanupAsync, the foreach over multiple TestAssemblyInfo entries previously accumulated logs across iterations (each GetOut() returned all prior logs); the new GetAndClearOutput() correctly scopes each iteration to only its own cleanup logs.
  • Regression tests are valid: The three new public async Task tests in TestMethodInfoTests.cs are properly discoverable by TestFramework.ForTestingMSTest (which discovers all public parameterless methods returning void or Task in TestContainer subclasses — no [TestMethod] attribute required).
  • Exclusive control flow: In both TestClassInfo.DoRunAsync and RunAssemblyCleanupAsync, the failure path (creates a new TestResult and returns) and the success path (lastResult +=) are mutually exclusive, so there is no double-clear or double-append risk.
  • No public API surface changed: All three renamed methods remain internal.

Recommendations

  • Consider adding a brief comment to the three new methods noting the intentional TOCTOU (consistent with GetAndClearDiagnosticMessages), to signal future maintainers that this is deliberate rather than an oversight.

Generated by Expert Code Reviewer

🧠 Reviewed by Expert Code Reviewer 🧠

Address review feedback: ToString() + Clear() as two separate synchronized
calls could lose appended text if another thread writes between them. Add
a single synchronized GetAndClear() method to SynchronizedStringBuilder
that performs both operations under one lock.
- Add focused GetAndClear{Output,Error,Trace} unit tests in
TestContextImplementationTests that verify the buffer is cleared after
the first call (returns empty on second call).
- Add GetAndClearTrace() call to WritesFromBackgroundThreadShouldNotThrow
to complete thread-safety coverage for all three buffer types.
- Consolidate regression test comments: single block comment noting that
TestClassInfo and UnitTestRunner call sites use the same GetAndClear*
methods tested in isolation.
CopilotAI review requested due to automatic review settings April 29, 2026 05:39

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot's findings

  • Files reviewed: 6/6 changed files
  • Comments generated: 0 new

@Evangelink
Amaury Levé (Evangelink) merged commit 37cd497 into mainApr 29, 2026
28 checks passed
@Evangelink
Amaury Levé (Evangelink) deleted the dev/amauryleve/fix-dynamic-data-log-accumulation branch April 29, 2026 09:06
@Evangelink

Copy link
Copy Markdown
MemberAuthor

/backport to rel/4.2

@github-actions

Copy link
Copy Markdown
Contributor

Started backporting to rel/4.2: https://github.com/microsoft/testfx/actions/runs/25104582574

Sign up for freeto 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.

Log collection is broken, >20 GB RAM usage for simple project

2 participants

@Evangelink
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Remove or un-stick sticky/fixed headers that block content\n(function() {\n function unstick() {\n document.querySelectorAll('header, nav, [role=\"banner\"], .header, .navbar, .sticky, .fixed-top, [style*=\"position: fixed\"], [style*=\"position:sticky\"]').forEach(function(el) {\n if (el.style.position === 'fixed' || el.style.position === 'sticky' || \n getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {\n el.style.position = 'static';\n el.style.top = 'auto';\n el.style.zIndex = 'auto';\n }\n });\n }\n \n unstick();\n \n var observer = new MutationObserver(unstick);\n observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });\n})();", "Kill Sticky Headers"); } } catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Fix log accumulation across DynamicData test invocations - #7925

Merged
Amaury Levé (Evangelink) merged 3 commits into
mainfrom
dev/amauryleve/fix-dynamic-data-log-accumulation
Apr 29, 2026
Merged

Fix log accumulation across DynamicData test invocations#7925
Amaury Levé (Evangelink) merged 3 commits into
mainfrom
dev/amauryleve/fix-dynamic-data-log-accumulation

Conversation

@Evangelink

Copy link
Copy Markdown
Member

Summary

Fixes#7846

When running DynamicData tests with multiple invocations, the TestContextImplementation output buffers (_stdOutStringBuilder, _stdErrStringBuilder, _traceStringBuilder) were never cleared between invocations. Since DynamicData tests reuse the same TestContext instance across all invocations, logs accumulated exponentially:

  • Test 1 result: logs from test 1
  • Test 2 result: logs from test 1 + test 2
  • Test 3 result: logs from test 1 + test 2 + test 3
  • ...

This caused >20 GB RAM usage for simple projects and truncated/broken log output in test summaries.

Root Cause

GetOut(), GetErr(), and GetTrace() on TestContextImplementation returned the StringBuilder contents via ToString() but never cleared them. In contrast, GetAndClearDiagnosticMessages() and GetResultFiles() already followed a get-then-clear pattern.

Fix

Replace GetOut(), GetErr(), GetTrace() with GetAndClearOutput(), GetAndClearError(), GetAndClearTrace() that clear the StringBuilder after extracting the value — matching the existing GetAndClearDiagnosticMessages() pattern.

Changes

  • TestContextImplementation.cs — Replace non-clearing getters with GetAndClearOutput/Error/Trace methods
  • TestMethodInfo.cs — Update call site in InvokeAsync finally block
  • UnitTestRunner.cs — Update call sites in assembly init/cleanup
  • TestClassInfo.cs — Update call sites in class init/cleanup
  • TestMethodInfoTests.cs — Add 3 regression tests verifying buffers don't accumulate across invocations
  • TestContextImplementationTests.cs — Update existing thread-safety test to use renamed methods

GetOut(), GetErr(), and GetTrace() on TestContextImplementation returned
accumulated buffer contents but never cleared them. When DynamicData tests
reuse the same TestContext across multiple invocations, each subsequent
result contained all logs from all previous invocations, causing exponential
memory growth (>20 GB for simple projects).
Replace these with GetAndClearOutput(), GetAndClearError(), and
GetAndClearTrace() that clear the StringBuilder after extracting the value,
matching the existing GetAndClearDiagnosticMessages() pattern.
Fixes#7846
CopilotAI review requested due to automatic review settings April 29, 2026 05:27

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Fixes MSTest adapter log buffer accumulation across multiple DynamicData invocations by switching stdout/stderr/trace retrieval to a get-then-clear pattern, preventing exponential growth in captured output and memory usage.

Changes:

  • Replace GetOut/GetErr/GetTrace with GetAndClearOutput/GetAndClearError/GetAndClearTrace in TestContextImplementation.
  • Update execution pipeline call sites (test method invoke; assembly/class init & cleanup) to use the new draining APIs.
  • Add regression unit tests to ensure output/error/trace don’t accumulate across multiple invocations.
Show a summary per file
FileDescription
test/UnitTests/MSTestAdapter.PlatformServices.UnitTests/Services/TestContextImplementationTests.csUpdates background-thread write test to use new get-and-clear APIs.
test/UnitTests/MSTestAdapter.PlatformServices.UnitTests/Execution/TestMethodInfoTests.csAdds regression tests validating buffers don’t accumulate across repeated InvokeAsync calls.
src/Adapter/MSTestAdapter.PlatformServices/Services/TestContextImplementation.csIntroduces get-and-clear methods for stdout/stderr/trace buffers.
src/Adapter/MSTestAdapter.PlatformServices/Execution/UnitTestRunner.csUpdates assembly init/cleanup log capture to drain buffers.
src/Adapter/MSTestAdapter.PlatformServices/Execution/TestMethodInfo.csUpdates per-test invocation log capture to drain buffers.
src/Adapter/MSTestAdapter.PlatformServices/Execution/TestClassInfo.csUpdates class init/cleanup log capture to drain buffers.

Copilot's findings

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

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Summary

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

Key Findings

  1. [Coverage — Minor]WritesFromBackgroundThreadShouldNotThrow in TestContextImplementationTests.cs was updated for Output and Error but not for Trace. GetAndClearTrace() also calls Clear() under concurrency and should be included in the thread-safety test.

  2. [Coverage — Important] No test in TestContextImplementationTests.cs directly verifies the clearing contract of the new GetAndClear* methods. The only test for these methods is the thread-safety check (which only asserts no exception). A dedicated read-then-clear test would document and protect the core invariant introduced by this fix.

  3. [Coverage — Minor] Regression tests for log accumulation are scoped to TestMethodInfo.InvokeAsync. The TestClassInfo (class init/cleanup) and UnitTestRunner (assembly init/cleanup) call sites that were also changed have no equivalent accumulation tests.

Recommendations

  • Add a GetAndClearOutput_ShouldReturnContentThenClearBuffer-style test (and equivalents for Error/Trace) in TestContextImplementationTests.cs to directly validate the clearing behavior.
  • Extend WritesFromBackgroundThreadShouldNotThrow to also call GetAndClearTrace() for completeness.
  • Consider adding accumulation regression tests for the class/assembly init-cleanup paths, or add a comment noting they're exercised by acceptance tests.

The existing accumulation tests in TestMethodInfoTests.cs are well-structured and correctly detect the regression: result2.LogOutput.Should().Be("invocation_output") would fail with the old non-clearing code (it would produce "invocation_outputinvocation_output"), so the core fix is properly verified.


Generated by Test Expert Reviewer 🧪

🧪 Test quality reviewed by Test Expert Reviewer 🧪

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Summary

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

Key Findings

[Threading — Minor/Accepted]GetAndClearOutput/Error/Trace each perform two individually-synchronized calls (ToString() + Clear()). Content written by a background thread in the TOCTOU window is silently discarded. This matches the pre-existing GetAndClearDiagnosticMessages() pattern and is an accepted design trade-off in this codebase. See inline comment.

Positive Observations

  • Root cause addressed correctly: The TestContextImplementation output buffers now clear after each read, consistent with the established GetAndClearDiagnosticMessages pattern. The exponential memory growth scenario is fully closed.
  • Bonus fix in multi-assembly cleanup: In UnitTestRunner.RunAssemblyCleanupAsync, the foreach over multiple TestAssemblyInfo entries previously accumulated logs across iterations (each GetOut() returned all prior logs); the new GetAndClearOutput() correctly scopes each iteration to only its own cleanup logs.
  • Regression tests are valid: The three new public async Task tests in TestMethodInfoTests.cs are properly discoverable by TestFramework.ForTestingMSTest (which discovers all public parameterless methods returning void or Task in TestContainer subclasses — no [TestMethod] attribute required).
  • Exclusive control flow: In both TestClassInfo.DoRunAsync and RunAssemblyCleanupAsync, the failure path (creates a new TestResult and returns) and the success path (lastResult +=) are mutually exclusive, so there is no double-clear or double-append risk.
  • No public API surface changed: All three renamed methods remain internal.

Recommendations

  • Consider adding a brief comment to the three new methods noting the intentional TOCTOU (consistent with GetAndClearDiagnosticMessages), to signal future maintainers that this is deliberate rather than an oversight.

Generated by Expert Code Reviewer

🧠 Reviewed by Expert Code Reviewer 🧠

Address review feedback: ToString() + Clear() as two separate synchronized
calls could lose appended text if another thread writes between them. Add
a single synchronized GetAndClear() method to SynchronizedStringBuilder
that performs both operations under one lock.
- Add focused GetAndClear{Output,Error,Trace} unit tests in
TestContextImplementationTests that verify the buffer is cleared after
the first call (returns empty on second call).
- Add GetAndClearTrace() call to WritesFromBackgroundThreadShouldNotThrow
to complete thread-safety coverage for all three buffer types.
- Consolidate regression test comments: single block comment noting that
TestClassInfo and UnitTestRunner call sites use the same GetAndClear*
methods tested in isolation.
CopilotAI review requested due to automatic review settings April 29, 2026 05:39

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot's findings

  • Files reviewed: 6/6 changed files
  • Comments generated: 0 new

@Evangelink
Amaury Levé (Evangelink) merged commit 37cd497 into mainApr 29, 2026
28 checks passed
@Evangelink
Amaury Levé (Evangelink) deleted the dev/amauryleve/fix-dynamic-data-log-accumulation branch April 29, 2026 09:06
@Evangelink

Copy link
Copy Markdown
MemberAuthor

/backport to rel/4.2

@github-actions

Copy link
Copy Markdown
Contributor

Started backporting to rel/4.2: https://github.com/microsoft/testfx/actions/runs/25104582574

Sign up for freeto 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.

Log collection is broken, >20 GB RAM usage for simple project

2 participants

@Evangelink
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Universal Dark Mode - works on any site\n(function() {\n var enabled = true;\n \n function applyDarkMode() {\n if (!enabled) return;\n \n // Create style element if it doesn't exist\n var style = document.getElementById('universal-dark-mode-style');\n if (!style) {\n style = document.createElement('style');\n style.id = 'universal-dark-mode-style';\n document.head.appendChild(style);\n }\n \n // Dark mode CSS - inverts colors but preserves images/video\n style.textContent = '\n /* Invert everything except media */\n html {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #1a1a2e !important;\n }\n \n /* Restore images, videos, iframes, canvas */\n img, video, iframe, canvas, svg, picture, [style*=\"background-image\"] {\n filter: invert(1) hue-rotate(180deg) !important;\n }\n \n /* Preserve specific elements that should not be inverted */\n .no-dark-mode, .no-dark-mode *,\n [data-theme=\"light\"], [data-theme=\"light\"],\n .ace_editor, .ace_editor *,\n .CodeMirror, .CodeMirror *,\n .monaco-editor, .monaco-editor *,\n .markdown-body pre, .markdown-body pre *,\n .highlight, .highlight *,\n pre code, pre code * {\n filter: none !important;\n }\n \n /* Fix common UI elements */\n .modal, .popup, .dropdown-menu, .tooltip, .popover {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #2d2d44 !important;\n border-color: #444 !important;\n }\n \n /* Scrollbars */\n ::-webkit-scrollbar { background: #1a1a2e !important; }\n ::-webkit-scrollbar-thumb { background: #444 !important; }\n ::-webkit-scrollbar-thumb:hover { background: #555 !important; }\n \n /* Selection */\n ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ';\n }\n \n function removeDarkMode() {\n var style = document.getElementById('universal-dark-mode-style');\n if (style) style.remove();\n }\n \n // Toggle with Alt+Shift+D\n document.addEventListener('keydown', function(e) {\n if (e.altKey && e.shiftKey && e.key === 'D') {\n e.preventDefault();\n enabled = !enabled;\n if (enabled) {\n applyDarkMode();\n console.log('[Universal Dark Mode] Enabled');\n } else {\n removeDarkMode();\n console.log('[Universal Dark Mode] Disabled');\n }\n }\n });\n \n // Apply on load\n applyDarkMode();\n \n // Re-apply on dynamic content\n var observer = new MutationObserver(function(mutations) {\n if (enabled && !document.getElementById('universal-dark-mode-style')) {\n applyDarkMode();\n }\n });\n observer.observe(document.head, { childList: true });\n \n console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');\n})();", "Universal Dark Mode"); } } catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); } })(); })();
Skip to content

Fix log accumulation across DynamicData test invocations - #7925

Merged
Amaury Levé (Evangelink) merged 3 commits into
mainfrom
dev/amauryleve/fix-dynamic-data-log-accumulation
Apr 29, 2026
Merged

Fix log accumulation across DynamicData test invocations#7925
Amaury Levé (Evangelink) merged 3 commits into
mainfrom
dev/amauryleve/fix-dynamic-data-log-accumulation

Conversation

@Evangelink

Copy link
Copy Markdown
Member

Summary

Fixes#7846

When running DynamicData tests with multiple invocations, the TestContextImplementation output buffers (_stdOutStringBuilder, _stdErrStringBuilder, _traceStringBuilder) were never cleared between invocations. Since DynamicData tests reuse the same TestContext instance across all invocations, logs accumulated exponentially:

  • Test 1 result: logs from test 1
  • Test 2 result: logs from test 1 + test 2
  • Test 3 result: logs from test 1 + test 2 + test 3
  • ...

This caused >20 GB RAM usage for simple projects and truncated/broken log output in test summaries.

Root Cause

GetOut(), GetErr(), and GetTrace() on TestContextImplementation returned the StringBuilder contents via ToString() but never cleared them. In contrast, GetAndClearDiagnosticMessages() and GetResultFiles() already followed a get-then-clear pattern.

Fix

Replace GetOut(), GetErr(), GetTrace() with GetAndClearOutput(), GetAndClearError(), GetAndClearTrace() that clear the StringBuilder after extracting the value — matching the existing GetAndClearDiagnosticMessages() pattern.

Changes

  • TestContextImplementation.cs — Replace non-clearing getters with GetAndClearOutput/Error/Trace methods
  • TestMethodInfo.cs — Update call site in InvokeAsync finally block
  • UnitTestRunner.cs — Update call sites in assembly init/cleanup
  • TestClassInfo.cs — Update call sites in class init/cleanup
  • TestMethodInfoTests.cs — Add 3 regression tests verifying buffers don't accumulate across invocations
  • TestContextImplementationTests.cs — Update existing thread-safety test to use renamed methods

GetOut(), GetErr(), and GetTrace() on TestContextImplementation returned
accumulated buffer contents but never cleared them. When DynamicData tests
reuse the same TestContext across multiple invocations, each subsequent
result contained all logs from all previous invocations, causing exponential
memory growth (>20 GB for simple projects).
Replace these with GetAndClearOutput(), GetAndClearError(), and
GetAndClearTrace() that clear the StringBuilder after extracting the value,
matching the existing GetAndClearDiagnosticMessages() pattern.
Fixes#7846
CopilotAI review requested due to automatic review settings April 29, 2026 05:27

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Fixes MSTest adapter log buffer accumulation across multiple DynamicData invocations by switching stdout/stderr/trace retrieval to a get-then-clear pattern, preventing exponential growth in captured output and memory usage.

Changes:

  • Replace GetOut/GetErr/GetTrace with GetAndClearOutput/GetAndClearError/GetAndClearTrace in TestContextImplementation.
  • Update execution pipeline call sites (test method invoke; assembly/class init & cleanup) to use the new draining APIs.
  • Add regression unit tests to ensure output/error/trace don’t accumulate across multiple invocations.
Show a summary per file
FileDescription
test/UnitTests/MSTestAdapter.PlatformServices.UnitTests/Services/TestContextImplementationTests.csUpdates background-thread write test to use new get-and-clear APIs.
test/UnitTests/MSTestAdapter.PlatformServices.UnitTests/Execution/TestMethodInfoTests.csAdds regression tests validating buffers don’t accumulate across repeated InvokeAsync calls.
src/Adapter/MSTestAdapter.PlatformServices/Services/TestContextImplementation.csIntroduces get-and-clear methods for stdout/stderr/trace buffers.
src/Adapter/MSTestAdapter.PlatformServices/Execution/UnitTestRunner.csUpdates assembly init/cleanup log capture to drain buffers.
src/Adapter/MSTestAdapter.PlatformServices/Execution/TestMethodInfo.csUpdates per-test invocation log capture to drain buffers.
src/Adapter/MSTestAdapter.PlatformServices/Execution/TestClassInfo.csUpdates class init/cleanup log capture to drain buffers.

Copilot's findings

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

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Summary

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

Key Findings

  1. [Coverage — Minor]WritesFromBackgroundThreadShouldNotThrow in TestContextImplementationTests.cs was updated for Output and Error but not for Trace. GetAndClearTrace() also calls Clear() under concurrency and should be included in the thread-safety test.

  2. [Coverage — Important] No test in TestContextImplementationTests.cs directly verifies the clearing contract of the new GetAndClear* methods. The only test for these methods is the thread-safety check (which only asserts no exception). A dedicated read-then-clear test would document and protect the core invariant introduced by this fix.

  3. [Coverage — Minor] Regression tests for log accumulation are scoped to TestMethodInfo.InvokeAsync. The TestClassInfo (class init/cleanup) and UnitTestRunner (assembly init/cleanup) call sites that were also changed have no equivalent accumulation tests.

Recommendations

  • Add a GetAndClearOutput_ShouldReturnContentThenClearBuffer-style test (and equivalents for Error/Trace) in TestContextImplementationTests.cs to directly validate the clearing behavior.
  • Extend WritesFromBackgroundThreadShouldNotThrow to also call GetAndClearTrace() for completeness.
  • Consider adding accumulation regression tests for the class/assembly init-cleanup paths, or add a comment noting they're exercised by acceptance tests.

The existing accumulation tests in TestMethodInfoTests.cs are well-structured and correctly detect the regression: result2.LogOutput.Should().Be("invocation_output") would fail with the old non-clearing code (it would produce "invocation_outputinvocation_output"), so the core fix is properly verified.


Generated by Test Expert Reviewer 🧪

🧪 Test quality reviewed by Test Expert Reviewer 🧪

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Summary

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

Key Findings

[Threading — Minor/Accepted]GetAndClearOutput/Error/Trace each perform two individually-synchronized calls (ToString() + Clear()). Content written by a background thread in the TOCTOU window is silently discarded. This matches the pre-existing GetAndClearDiagnosticMessages() pattern and is an accepted design trade-off in this codebase. See inline comment.

Positive Observations

  • Root cause addressed correctly: The TestContextImplementation output buffers now clear after each read, consistent with the established GetAndClearDiagnosticMessages pattern. The exponential memory growth scenario is fully closed.
  • Bonus fix in multi-assembly cleanup: In UnitTestRunner.RunAssemblyCleanupAsync, the foreach over multiple TestAssemblyInfo entries previously accumulated logs across iterations (each GetOut() returned all prior logs); the new GetAndClearOutput() correctly scopes each iteration to only its own cleanup logs.
  • Regression tests are valid: The three new public async Task tests in TestMethodInfoTests.cs are properly discoverable by TestFramework.ForTestingMSTest (which discovers all public parameterless methods returning void or Task in TestContainer subclasses — no [TestMethod] attribute required).
  • Exclusive control flow: In both TestClassInfo.DoRunAsync and RunAssemblyCleanupAsync, the failure path (creates a new TestResult and returns) and the success path (lastResult +=) are mutually exclusive, so there is no double-clear or double-append risk.
  • No public API surface changed: All three renamed methods remain internal.

Recommendations

  • Consider adding a brief comment to the three new methods noting the intentional TOCTOU (consistent with GetAndClearDiagnosticMessages), to signal future maintainers that this is deliberate rather than an oversight.

Generated by Expert Code Reviewer

🧠 Reviewed by Expert Code Reviewer 🧠

Address review feedback: ToString() + Clear() as two separate synchronized
calls could lose appended text if another thread writes between them. Add
a single synchronized GetAndClear() method to SynchronizedStringBuilder
that performs both operations under one lock.
- Add focused GetAndClear{Output,Error,Trace} unit tests in
TestContextImplementationTests that verify the buffer is cleared after
the first call (returns empty on second call).
- Add GetAndClearTrace() call to WritesFromBackgroundThreadShouldNotThrow
to complete thread-safety coverage for all three buffer types.
- Consolidate regression test comments: single block comment noting that
TestClassInfo and UnitTestRunner call sites use the same GetAndClear*
methods tested in isolation.
CopilotAI review requested due to automatic review settings April 29, 2026 05:39

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot's findings

  • Files reviewed: 6/6 changed files
  • Comments generated: 0 new

@Evangelink
Amaury Levé (Evangelink) merged commit 37cd497 into mainApr 29, 2026
28 checks passed
@Evangelink
Amaury Levé (Evangelink) deleted the dev/amauryleve/fix-dynamic-data-log-accumulation branch April 29, 2026 09:06
@Evangelink

Copy link
Copy Markdown
MemberAuthor

/backport to rel/4.2

@github-actions

Copy link
Copy Markdown
Contributor

Started backporting to rel/4.2: https://github.com/microsoft/testfx/actions/runs/25104582574

Sign up for freeto 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.

Log collection is broken, >20 GB RAM usage for simple project

2 participants

@Evangelink