Unify MTP terminal reporter to handle 1..N assemblies - #9256

Merged
Amaury Levé (Evangelink) merged 4 commits into
mainfrom
copilot/terminal-reporter-1-to-n
Jun 19, 2026
Merged

Unify MTP terminal reporter to handle 1..N assemblies#9256
Amaury Levé (Evangelink) merged 4 commits into
mainfrom
copilot/terminal-reporter-1-to-n

Conversation

@Evangelink

Copy link
Copy Markdown
Member

What

Converts the Microsoft.Testing.Platform terminal test reporter from a single-assembly model to a multi-assembly (1..N) model, so a single shared source can render both the in-process MTP host (one assembly) and the dotnet test orchestrator (N child assemblies).

This is the prerequisite called out in the terminal-sharing effort (follow-up to the protocol-sharing PRs #9218 / #9231 and the package PRs #9249 / #9250 / #9253): the SDK currently hard-forks the reporter because the shared MTP copy was single-assembly. With this change the reporter core exposes the multi-assembly API the SDK needs.

How

  • Replace the single TestProgressState + _assembly/_targetFramework/_architecture fields with a ConcurrentDictionary<string, TestProgressState> _assemblies keyed by a caller-provided execution id. The in-process host passes one fixed id; the orchestrator passes one per child assembly.
  • Route TestCompleted / TestInProgress / TestDiscovered / AssemblyRunStarted / AssemblyRunCompleted / ArtifactAdded per execution id.
  • Aggregate the run and discovery summaries across all assemblies. N=1 (in-process) output stays byte-identical via an assemblies.Count == 1 branch that still appends the per-assembly link to the verdict line; for N>1 the verdict line is link-free (per-assembly identity lives in the progress area).
  • Expand TestRunArtifact and ArtifactAdded with assembly / targetFramework / architecture / executionId.
  • Add a TotalTests aggregate and surface isHelp / isRetry on TestExecutionStarted.
  • Adapt the in-process caller (TerminalOutputDevice) to the new API using a single fixed InProcessExecutionId.

Scope / deferred

The SDK-orchestrator-only surface — handshake-failure recap, instanceId-based retry counting, build-error tracking, and exit-code-in-summary — is intentionally not ported here. That logic depends on the SDK's richer TestProgressState retry model and is only exercised by real multi-process runs, so it cannot be verified from the in-process unit tests. It will land with the SDK plug-in PR where it can be driven and tested end-to-end against actual N-assembly runs.

Verification

  • Platform builds clean on net8.0 / net9.0 / netstandard2.0 (0 warnings).
  • Full Microsoft.Testing.Platform.UnitTests suite green: 1197 total, 0 failed — the 94 existing byte-exact terminal tests confirm the in-process UI is unchanged.
  • New TerminalTestReporter_WhenMultipleAssemblies_AggregatesCountsAndOmitsAssemblyLinkOnVerdict test covers the N>1 path (aggregated counts + link-free verdict).
  • The standalone TerminalReporterContract consumer still compiles in isolation (the Share dotnet test wire contract (ObjectFieldIds + Constants) as source #9218-equivalent gate).

The terminal test reporter was single-assembly (in-process MTP host). The
dotnet test orchestrator runs N child assemblies, so the SDK had to hard-fork
the reporter. This converts the reporter core to a multi-assembly model so a
single shared source can render both cases:
- Replace the single TestProgressState + _assembly/_targetFramework/_architecture
fields with a ConcurrentDictionary<string, TestProgressState> keyed by a
caller-provided execution id (in-process passes one fixed id).
- Route TestCompleted/TestInProgress/TestDiscovered/AssemblyRunStarted/
AssemblyRunCompleted/ArtifactAdded per execution id.
- Aggregate run/discovery summaries across all assemblies; keep the N=1
(in-process) output byte-identical via an assemblies.Count == 1 branch that
still appends the per-assembly link to the verdict line.
- Expand TestRunArtifact and ArtifactAdded with assembly/tfm/arch/executionId.
- Add TotalTests aggregate and surface isHelp/isRetry on TestExecutionStarted.
- Adapt the in-process caller (TerminalOutputDevice) to the new API with a
single fixed InProcessExecutionId.
The SDK-orchestrator-only surface (handshake-failure recap, instanceId-based
retry counting, build errors, exit-code-in-summary) is intentionally deferred
to the SDK plug-in PR, where it can be driven and verified against real
multi-assembly runs.
Verified: platform builds clean on net8.0/net9.0/netstandard2.0;
full Microsoft.Testing.Platform.UnitTests suite green (1197, 0 failed),
including a new N>1 test; the standalone TerminalReporterContract consumer
still compiles in isolation.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
CopilotAI review requested due to automatic review settings June 19, 2026 14:54

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

This PR updates the Microsoft.Testing.Platform terminal reporter to support a multi-assembly (1..N) execution model (keyed by execution id), while preserving byte-identical output for the existing single-assembly in-process host.

Changes:

  • Replace single-assembly reporter state with a per-execution-id map and aggregate run/discovery summaries across assemblies.
  • Extend reporter events/artifact tracking to carry assembly identity (assembly/TFM/architecture/executionId) and route events by execution id.
  • Update the in-process TerminalOutputDevice and unit tests to use the new multi-assembly reporter API; add a dedicated multi-assembly aggregation test.
Show a summary per file
FileDescription
test/UnitTests/Microsoft.Testing.Platform.UnitTests/OutputDevice/Terminal/TerminalTestReporterTests.csUpdates tests for new reporter APIs and adds a multi-assembly aggregation test.
src/Platform/Microsoft.Testing.Platform/OutputDevice/TerminalOutputDevice.SessionLifecycle.csAdapts in-process session lifecycle to call multi-assembly reporter methods using a fixed execution id.
src/Platform/Microsoft.Testing.Platform/OutputDevice/TerminalOutputDevice.Initialization.csIntroduces an in-process execution id constant and updates reporter construction for the new API.
src/Platform/Microsoft.Testing.Platform/OutputDevice/TerminalOutputDevice.DataConsumption.csRoutes test state/artifacts into reporter with the in-process execution id and richer artifact metadata.
src/Platform/Microsoft.Testing.Platform/OutputDevice/Terminal/TestRunArtifact.csExpands artifact record to include assembly identity fields.
src/Platform/Microsoft.Testing.Platform/OutputDevice/Terminal/TestProgressState.csStores the full assembly path/display name for later linking and summary formatting.
src/Platform/Microsoft.Testing.Platform/OutputDevice/Terminal/TerminalTestReporter.TestCompletion.csKeys TestCompleted handling by execution id and looks up per-assembly state.
src/Platform/Microsoft.Testing.Platform/OutputDevice/Terminal/TerminalTestReporter.Summary.csAggregates counts across assemblies and removes verdict-line assembly link when N>1.
src/Platform/Microsoft.Testing.Platform/OutputDevice/Terminal/TerminalTestReporter.Messaging.csKeys TestInProgress by execution id for multi-assembly progress rendering.
src/Platform/Microsoft.Testing.Platform/OutputDevice/Terminal/TerminalTestReporter.Lifecycle.csAdds multi-assembly lifecycle APIs and clears per-assembly state on completion.
src/Platform/Microsoft.Testing.Platform/OutputDevice/Terminal/TerminalTestReporter.Formatting.csUpdates assembly link formatting to operate on an assembly-run state instance.
src/Platform/Microsoft.Testing.Platform/OutputDevice/Terminal/TerminalTestReporter.csRemoves single-assembly constructor inputs, adds per-assembly dictionary state and richer ArtifactAdded.

Copilot's findings

Comments suppressed due to low confidence (2)

src/Platform/Microsoft.Testing.Platform/OutputDevice/TerminalOutputDevice.DataConsumption.cs:205

  • The TestCompleted argument indentation in the TimeoutTestNodeStateProperty case has an extra leading space compared to the surrounding blocks, which can trigger IDE0055 (formatting) during builds with code-style enforcement.
 case TimeoutTestNodeStateProperty timeoutState:
_terminalTestReporter.TestCompleted(
InProcessExecutionId,
testNodeStateChanged.TestNode.Uid.Value,
testNodeStateChanged.TestNode.DisplayName,
TestOutcome.Timeout,
duration,

src/Platform/Microsoft.Testing.Platform/OutputDevice/TerminalOutputDevice.DataConsumption.cs:223

  • The TestCompleted argument indentation in the CancelledTestNodeStateProperty case is inconsistent (extra leading space), which may trigger IDE0055 formatting diagnostics when code style is enforced in build.
#pragma warning disable CS0618, MTP0001 // Type or member is obsolete
case CancelledTestNodeStateProperty cancelledState:
#pragma warning restore CS0618, MTP0001 // Type or member is obsolete
_terminalTestReporter.TestCompleted(
InProcessExecutionId,
testNodeStateChanged.TestNode.Uid.Value,
testNodeStateChanged.TestNode.DisplayName,
TestOutcome.Canceled,
duration,
  • Files reviewed: 12/12 changed files
  • Comments generated: 5

@Evangelink

This comment has been minimized.

@Evangelink

Copy link
Copy Markdown
MemberAuthor

🔍 Build Failure Analysis

Summary — The build of Microsoft.Testing.Platform.csproj fails due to three distinct analyzer violations introduced in the OutputDevice/Terminal/ area: two SA1623 violations (property XML doc summaries not starting with "Gets") and one IDE0052 violation (a private field that is assigned but never read).


Root cause 1 — SA1623: Property doc summaries missing "Gets" prefix

SA1623 requires that the <summary> of any property with a getter must begin with the word "Gets" (e.g. /// <summary>Gets the ...</summary>). Two new properties were added with prose-style summaries that omit this prefix — both are lifted to errors by TreatWarningsAsErrors in this project.

Affected locations

Fix — prepend Gets to each <summary>:

// TestProgressState.cs:23
- /// <summary>The assembly path or display name as provided by the caller (used for the summary link).</summary>+ /// <summary>Gets the assembly path or display name as provided by the caller (used for the summary link).</summary>
// TerminalTestReporter.cs:67
- /// <summary>Total number of tests across all registered assemblies.</summary>+ /// <summary>Gets the total number of tests across all registered assemblies.</summary>

Root cause 2 — IDE0052: _isRetry field is write-only

[IDE0052]((learn.microsoft.com/redacted) fires when a private member is assigned but never read. _isRetry was introduced in TerminalTestReporter.Lifecycle.cs, assigned in TestExecutionStarted, but consumed nowhere — unlike its sibling _isHelp, which is already read in TestExecutionCompleted to suppress the summary when --help is active.

Affected location

Fix — remove the field and its assignment. If retry-specific behaviour is planned, add the corresponding read-site (e.g. in TestExecutionCompleted) and keep the field; otherwise also remove the bool isRetry parameter from TestExecutionStarted and update callers.

 private bool _isHelp;
- private bool _isRetry;
public void TestExecutionStarted(DateTimeOffset testStartTime, int workerCount, bool isDiscovery, bool isHelp, bool isRetry)
{
_isDiscovery = isDiscovery;
_isHelp = isHelp;
- _isRetry = isRetry;
_testExecutionStartTime = testStartTime;

Build overview
MSBuild18.7.0-preview
Duration154 s
Projects45
Errors10 (3 unique issues, each repeated across multiple TFMs)
Warnings0

All errors originate in Microsoft.Testing.Platform.csproj. The failures of Microsoft.Testing.Extensions.AzureDevOpsReport, Microsoft.Testing.Extensions.CrashDump, and MSTest.TestAdapter are cascades from that single project.

All MSBuild errors (3 unique, 10 total across TFMs)
CodeFileLineMessage
SA1623TestProgressState.cs24The property's documentation summary text should begin with: 'Gets'
SA1623TerminalTestReporter.cs68The property's documentation summary text should begin with: 'Gets'
IDE0052TerminalTestReporter.Lifecycle.cs12Private member _isRetry can be removed as the value assigned to it is never read

🤖 Generated by the Build Failure Analysis workflow · commit 476750cda2a7f588e1e5476448945f10789afff4

🤖 Automated content by GitHub Copilot. Posted via a maintainer's GitHub token, so it appears under their account — the account owner did not write or approve this content personally. Generated by the Build Failure Analysis workflow. · 246.3 AIC · ⌖ 12.8 AIC · ⊞ 46.9K · [◷]( · )

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.

🤖 Automated content by GitHub Copilot. Posted via a maintainer's GitHub token, so it appears under their account — the account owner did not write or approve this content personally. Generated by the Build Failure Analysis workflow. · 246.3 AIC · ⌖ 12.8 AIC · ⊞ 46.9K ·

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.

Note

🤖 Automated review by GitHub Copilot. Posted via a maintainer's GitHub token, so it appears under their account — the account owner did not write or approve this content personally. Generated by the Expert Code Review workflow. To request a follow-up action, reply by tagging @copilot directly.

Review Summary

The multi-assembly generalisation is well-designed: the ConcurrentDictionary approach, the single InProcessExecutionId constant for the in-process host, the assemblies.Count == 1 branch that keeps byte-identical single-assembly output, and the new aggregating test — all read cleanly. The 94 byte-exact existing tests continuing to pass confirms the in-process path is unchanged.

One MAJOR correctness regression was found in the factory pattern, plus two MODERATE dead-API issues and two NITs.


#DimensionVerdictFileLine
2Threading & Concurrency🔴 1 MAJORTerminalTestReporter.Lifecycle.cs26–37
15Code Structure🟡 2 MODERATETerminalTestReporter.Lifecycle.cs12, 23
5Performance & Allocations🔵 1 NITTerminalTestReporter.cs68
8Defensive Coding🔵 1 NITTerminalTestReporter.Lifecycle.cs50

✅ 18/22 dimensions clean.


Findings

  • [MAJOR – Threading]GetOrAdd(key, valueFactory) can invoke the factory more than once for the same key if two threads race (the inner lock serialises the invocations sequentially but does not prevent both from running). Both invocations call AddWorker, occupying two slots; only one TestProgressState is kept in the dictionary, leaking the other's progress slot permanently. Current callers guarantee unique IDs so the race is not reachable today, but the pattern is a correctness regression from the old double-checked locking. Replace with TryGetValue / lock / TryGetValue / _assemblies[key] = ... DCL. See inline comment.

  • [MODERATE – Dead API]instanceId is accepted by AssemblyRunStarted but never forwarded or stored. If keeping it as scaffolding, add a // Reserved for SDK retry logic comment; otherwise remove and re-introduce with its consumer.

  • [MODERATE – Dead Code]_isRetry is assigned in TestExecutionStarted but never read anywhere in the partial class. _isHelp has a consumer; _isRetry does not. Remove or add an explanatory comment.

  • [NIT – Performance]TotalTests property lambda should use static for consistency with all other lambdas in this file: Sum(static a => a.TotalTests).

  • [NIT – Dead Param]exitCode in TestExecutionCompleted is never stored. A short inline comment noting this is deferred to the SDK plug-in PR would be enough.

- GetOrAddAssemblyRun: replace ConcurrentDictionary.GetOrAdd (whose value
factory could run multiple times under contention and orphan worker slots /
bump _counter) with an explicit lock + TryGetValue + add, guaranteeing exactly
one worker per executionId.
- Remove the unused _isRetry field/assignment (IDE0052); keep the isRetry
parameter for SDK API parity.
- Prefix the Assembly and TotalTests doc summaries with 'Gets' (SA1623).
- Normalize the TestCompleted argument indentation in the Failed/Timeout/
Cancelled cases (IDE0055).
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- GetOrAddAssemblyRun: add a lock-free TryGetValue fast path (double-checked
locking) so repeat AssemblyRunStarted calls don't take the lock, matching the
reviewer's suggested pattern.
- TotalTests: make the Sum lambda static to avoid a per-call delegate allocation
on the SDK orchestrator's progress-tick path.
- Document instanceId on AssemblyRunStarted as reserved for the SDK orchestrator
retry-counting follow-up (unused by the in-process host path).
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
CopilotAI review requested due to automatic review settings June 19, 2026 15:24

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: 12/12 changed files
  • Comments generated: 1

TestExecutionCompleted cleared the per-assembly runs but left _artifacts and
WasCancelled intact (pre-existing, but contradicts the documented HotReload
'start fresh' intent on a method this PR reworks): a later session would
re-print the previous session's artifacts or stay stuck in the aborted state.
Now also clear _artifacts and reset WasCancelled after the summary has consumed
them. Adds a regression test exercising two sessions on the same reporter.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@Evangelink

Copy link
Copy Markdown
MemberAuthor

🧪 Test quality grade — PR #9256

ΔTestGradeBandNotes
newTerminalTestReporterTests.
TerminalTestReporter_
WhenReusedAcrossSessions_
DoesNotLeakArtifactsOrCancelledState
B80–893 meaningful assertions cover both leak vectors; body is ~42 lines — consider a session-run helper to trim setup verbosity.

This advisory comment was generated automatically. Grades are heuristic
and informational — they do not block merging. Re-run with
/grade-tests.

🤖 Automated content by GitHub Copilot. Posted via a maintainer's GitHub token, so it appears under their account — the account owner did not write or approve this content personally. Generated by the Grade Tests on PR (on open / sync) workflow. · 464 AIC · ⌖ 13.2 AIC · ⊞ 45.6K · [◷]( · )

@Evangelink
Amaury Levé (Evangelink) merged commit 4545e5d into mainJun 19, 2026
34 of 39 checks passed
@Evangelink
Amaury Levé (Evangelink) deleted the copilot/terminal-reporter-1-to-n branch June 19, 2026 16:06
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.

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

Unify MTP terminal reporter to handle 1..N assemblies - #9256

Merged
Amaury Levé (Evangelink) merged 4 commits into
mainfrom
copilot/terminal-reporter-1-to-n
Jun 19, 2026
Merged

Unify MTP terminal reporter to handle 1..N assemblies#9256
Amaury Levé (Evangelink) merged 4 commits into
mainfrom
copilot/terminal-reporter-1-to-n

Conversation

@Evangelink

Copy link
Copy Markdown
Member

What

Converts the Microsoft.Testing.Platform terminal test reporter from a single-assembly model to a multi-assembly (1..N) model, so a single shared source can render both the in-process MTP host (one assembly) and the dotnet test orchestrator (N child assemblies).

This is the prerequisite called out in the terminal-sharing effort (follow-up to the protocol-sharing PRs #9218 / #9231 and the package PRs #9249 / #9250 / #9253): the SDK currently hard-forks the reporter because the shared MTP copy was single-assembly. With this change the reporter core exposes the multi-assembly API the SDK needs.

How

  • Replace the single TestProgressState + _assembly/_targetFramework/_architecture fields with a ConcurrentDictionary<string, TestProgressState> _assemblies keyed by a caller-provided execution id. The in-process host passes one fixed id; the orchestrator passes one per child assembly.
  • Route TestCompleted / TestInProgress / TestDiscovered / AssemblyRunStarted / AssemblyRunCompleted / ArtifactAdded per execution id.
  • Aggregate the run and discovery summaries across all assemblies. N=1 (in-process) output stays byte-identical via an assemblies.Count == 1 branch that still appends the per-assembly link to the verdict line; for N>1 the verdict line is link-free (per-assembly identity lives in the progress area).
  • Expand TestRunArtifact and ArtifactAdded with assembly / targetFramework / architecture / executionId.
  • Add a TotalTests aggregate and surface isHelp / isRetry on TestExecutionStarted.
  • Adapt the in-process caller (TerminalOutputDevice) to the new API using a single fixed InProcessExecutionId.

Scope / deferred

The SDK-orchestrator-only surface — handshake-failure recap, instanceId-based retry counting, build-error tracking, and exit-code-in-summary — is intentionally not ported here. That logic depends on the SDK's richer TestProgressState retry model and is only exercised by real multi-process runs, so it cannot be verified from the in-process unit tests. It will land with the SDK plug-in PR where it can be driven and tested end-to-end against actual N-assembly runs.

Verification

  • Platform builds clean on net8.0 / net9.0 / netstandard2.0 (0 warnings).
  • Full Microsoft.Testing.Platform.UnitTests suite green: 1197 total, 0 failed — the 94 existing byte-exact terminal tests confirm the in-process UI is unchanged.
  • New TerminalTestReporter_WhenMultipleAssemblies_AggregatesCountsAndOmitsAssemblyLinkOnVerdict test covers the N>1 path (aggregated counts + link-free verdict).
  • The standalone TerminalReporterContract consumer still compiles in isolation (the Share dotnet test wire contract (ObjectFieldIds + Constants) as source #9218-equivalent gate).

The terminal test reporter was single-assembly (in-process MTP host). The
dotnet test orchestrator runs N child assemblies, so the SDK had to hard-fork
the reporter. This converts the reporter core to a multi-assembly model so a
single shared source can render both cases:
- Replace the single TestProgressState + _assembly/_targetFramework/_architecture
fields with a ConcurrentDictionary<string, TestProgressState> keyed by a
caller-provided execution id (in-process passes one fixed id).
- Route TestCompleted/TestInProgress/TestDiscovered/AssemblyRunStarted/
AssemblyRunCompleted/ArtifactAdded per execution id.
- Aggregate run/discovery summaries across all assemblies; keep the N=1
(in-process) output byte-identical via an assemblies.Count == 1 branch that
still appends the per-assembly link to the verdict line.
- Expand TestRunArtifact and ArtifactAdded with assembly/tfm/arch/executionId.
- Add TotalTests aggregate and surface isHelp/isRetry on TestExecutionStarted.
- Adapt the in-process caller (TerminalOutputDevice) to the new API with a
single fixed InProcessExecutionId.
The SDK-orchestrator-only surface (handshake-failure recap, instanceId-based
retry counting, build errors, exit-code-in-summary) is intentionally deferred
to the SDK plug-in PR, where it can be driven and verified against real
multi-assembly runs.
Verified: platform builds clean on net8.0/net9.0/netstandard2.0;
full Microsoft.Testing.Platform.UnitTests suite green (1197, 0 failed),
including a new N>1 test; the standalone TerminalReporterContract consumer
still compiles in isolation.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
CopilotAI review requested due to automatic review settings June 19, 2026 14:54

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

This PR updates the Microsoft.Testing.Platform terminal reporter to support a multi-assembly (1..N) execution model (keyed by execution id), while preserving byte-identical output for the existing single-assembly in-process host.

Changes:

  • Replace single-assembly reporter state with a per-execution-id map and aggregate run/discovery summaries across assemblies.
  • Extend reporter events/artifact tracking to carry assembly identity (assembly/TFM/architecture/executionId) and route events by execution id.
  • Update the in-process TerminalOutputDevice and unit tests to use the new multi-assembly reporter API; add a dedicated multi-assembly aggregation test.
Show a summary per file
FileDescription
test/UnitTests/Microsoft.Testing.Platform.UnitTests/OutputDevice/Terminal/TerminalTestReporterTests.csUpdates tests for new reporter APIs and adds a multi-assembly aggregation test.
src/Platform/Microsoft.Testing.Platform/OutputDevice/TerminalOutputDevice.SessionLifecycle.csAdapts in-process session lifecycle to call multi-assembly reporter methods using a fixed execution id.
src/Platform/Microsoft.Testing.Platform/OutputDevice/TerminalOutputDevice.Initialization.csIntroduces an in-process execution id constant and updates reporter construction for the new API.
src/Platform/Microsoft.Testing.Platform/OutputDevice/TerminalOutputDevice.DataConsumption.csRoutes test state/artifacts into reporter with the in-process execution id and richer artifact metadata.
src/Platform/Microsoft.Testing.Platform/OutputDevice/Terminal/TestRunArtifact.csExpands artifact record to include assembly identity fields.
src/Platform/Microsoft.Testing.Platform/OutputDevice/Terminal/TestProgressState.csStores the full assembly path/display name for later linking and summary formatting.
src/Platform/Microsoft.Testing.Platform/OutputDevice/Terminal/TerminalTestReporter.TestCompletion.csKeys TestCompleted handling by execution id and looks up per-assembly state.
src/Platform/Microsoft.Testing.Platform/OutputDevice/Terminal/TerminalTestReporter.Summary.csAggregates counts across assemblies and removes verdict-line assembly link when N>1.
src/Platform/Microsoft.Testing.Platform/OutputDevice/Terminal/TerminalTestReporter.Messaging.csKeys TestInProgress by execution id for multi-assembly progress rendering.
src/Platform/Microsoft.Testing.Platform/OutputDevice/Terminal/TerminalTestReporter.Lifecycle.csAdds multi-assembly lifecycle APIs and clears per-assembly state on completion.
src/Platform/Microsoft.Testing.Platform/OutputDevice/Terminal/TerminalTestReporter.Formatting.csUpdates assembly link formatting to operate on an assembly-run state instance.
src/Platform/Microsoft.Testing.Platform/OutputDevice/Terminal/TerminalTestReporter.csRemoves single-assembly constructor inputs, adds per-assembly dictionary state and richer ArtifactAdded.

Copilot's findings

Comments suppressed due to low confidence (2)

src/Platform/Microsoft.Testing.Platform/OutputDevice/TerminalOutputDevice.DataConsumption.cs:205

  • The TestCompleted argument indentation in the TimeoutTestNodeStateProperty case has an extra leading space compared to the surrounding blocks, which can trigger IDE0055 (formatting) during builds with code-style enforcement.
 case TimeoutTestNodeStateProperty timeoutState:
_terminalTestReporter.TestCompleted(
InProcessExecutionId,
testNodeStateChanged.TestNode.Uid.Value,
testNodeStateChanged.TestNode.DisplayName,
TestOutcome.Timeout,
duration,

src/Platform/Microsoft.Testing.Platform/OutputDevice/TerminalOutputDevice.DataConsumption.cs:223

  • The TestCompleted argument indentation in the CancelledTestNodeStateProperty case is inconsistent (extra leading space), which may trigger IDE0055 formatting diagnostics when code style is enforced in build.
#pragma warning disable CS0618, MTP0001 // Type or member is obsolete
case CancelledTestNodeStateProperty cancelledState:
#pragma warning restore CS0618, MTP0001 // Type or member is obsolete
_terminalTestReporter.TestCompleted(
InProcessExecutionId,
testNodeStateChanged.TestNode.Uid.Value,
testNodeStateChanged.TestNode.DisplayName,
TestOutcome.Canceled,
duration,
  • Files reviewed: 12/12 changed files
  • Comments generated: 5

@Evangelink

This comment has been minimized.

@Evangelink

Copy link
Copy Markdown
MemberAuthor

🔍 Build Failure Analysis

Summary — The build of Microsoft.Testing.Platform.csproj fails due to three distinct analyzer violations introduced in the OutputDevice/Terminal/ area: two SA1623 violations (property XML doc summaries not starting with "Gets") and one IDE0052 violation (a private field that is assigned but never read).


Root cause 1 — SA1623: Property doc summaries missing "Gets" prefix

SA1623 requires that the <summary> of any property with a getter must begin with the word "Gets" (e.g. /// <summary>Gets the ...</summary>). Two new properties were added with prose-style summaries that omit this prefix — both are lifted to errors by TreatWarningsAsErrors in this project.

Affected locations

Fix — prepend Gets to each <summary>:

// TestProgressState.cs:23
- /// <summary>The assembly path or display name as provided by the caller (used for the summary link).</summary>+ /// <summary>Gets the assembly path or display name as provided by the caller (used for the summary link).</summary>
// TerminalTestReporter.cs:67
- /// <summary>Total number of tests across all registered assemblies.</summary>+ /// <summary>Gets the total number of tests across all registered assemblies.</summary>

Root cause 2 — IDE0052: _isRetry field is write-only

[IDE0052]((learn.microsoft.com/redacted) fires when a private member is assigned but never read. _isRetry was introduced in TerminalTestReporter.Lifecycle.cs, assigned in TestExecutionStarted, but consumed nowhere — unlike its sibling _isHelp, which is already read in TestExecutionCompleted to suppress the summary when --help is active.

Affected location

Fix — remove the field and its assignment. If retry-specific behaviour is planned, add the corresponding read-site (e.g. in TestExecutionCompleted) and keep the field; otherwise also remove the bool isRetry parameter from TestExecutionStarted and update callers.

 private bool _isHelp;
- private bool _isRetry;
public void TestExecutionStarted(DateTimeOffset testStartTime, int workerCount, bool isDiscovery, bool isHelp, bool isRetry)
{
_isDiscovery = isDiscovery;
_isHelp = isHelp;
- _isRetry = isRetry;
_testExecutionStartTime = testStartTime;

Build overview
MSBuild18.7.0-preview
Duration154 s
Projects45
Errors10 (3 unique issues, each repeated across multiple TFMs)
Warnings0

All errors originate in Microsoft.Testing.Platform.csproj. The failures of Microsoft.Testing.Extensions.AzureDevOpsReport, Microsoft.Testing.Extensions.CrashDump, and MSTest.TestAdapter are cascades from that single project.

All MSBuild errors (3 unique, 10 total across TFMs)
CodeFileLineMessage
SA1623TestProgressState.cs24The property's documentation summary text should begin with: 'Gets'
SA1623TerminalTestReporter.cs68The property's documentation summary text should begin with: 'Gets'
IDE0052TerminalTestReporter.Lifecycle.cs12Private member _isRetry can be removed as the value assigned to it is never read

🤖 Generated by the Build Failure Analysis workflow · commit 476750cda2a7f588e1e5476448945f10789afff4

🤖 Automated content by GitHub Copilot. Posted via a maintainer's GitHub token, so it appears under their account — the account owner did not write or approve this content personally. Generated by the Build Failure Analysis workflow. · 246.3 AIC · ⌖ 12.8 AIC · ⊞ 46.9K · [◷]( · )

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.

🤖 Automated content by GitHub Copilot. Posted via a maintainer's GitHub token, so it appears under their account — the account owner did not write or approve this content personally. Generated by the Build Failure Analysis workflow. · 246.3 AIC · ⌖ 12.8 AIC · ⊞ 46.9K ·

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.

Note

🤖 Automated review by GitHub Copilot. Posted via a maintainer's GitHub token, so it appears under their account — the account owner did not write or approve this content personally. Generated by the Expert Code Review workflow. To request a follow-up action, reply by tagging @copilot directly.

Review Summary

The multi-assembly generalisation is well-designed: the ConcurrentDictionary approach, the single InProcessExecutionId constant for the in-process host, the assemblies.Count == 1 branch that keeps byte-identical single-assembly output, and the new aggregating test — all read cleanly. The 94 byte-exact existing tests continuing to pass confirms the in-process path is unchanged.

One MAJOR correctness regression was found in the factory pattern, plus two MODERATE dead-API issues and two NITs.


#DimensionVerdictFileLine
2Threading & Concurrency🔴 1 MAJORTerminalTestReporter.Lifecycle.cs26–37
15Code Structure🟡 2 MODERATETerminalTestReporter.Lifecycle.cs12, 23
5Performance & Allocations🔵 1 NITTerminalTestReporter.cs68
8Defensive Coding🔵 1 NITTerminalTestReporter.Lifecycle.cs50

✅ 18/22 dimensions clean.


Findings

  • [MAJOR – Threading]GetOrAdd(key, valueFactory) can invoke the factory more than once for the same key if two threads race (the inner lock serialises the invocations sequentially but does not prevent both from running). Both invocations call AddWorker, occupying two slots; only one TestProgressState is kept in the dictionary, leaking the other's progress slot permanently. Current callers guarantee unique IDs so the race is not reachable today, but the pattern is a correctness regression from the old double-checked locking. Replace with TryGetValue / lock / TryGetValue / _assemblies[key] = ... DCL. See inline comment.

  • [MODERATE – Dead API]instanceId is accepted by AssemblyRunStarted but never forwarded or stored. If keeping it as scaffolding, add a // Reserved for SDK retry logic comment; otherwise remove and re-introduce with its consumer.

  • [MODERATE – Dead Code]_isRetry is assigned in TestExecutionStarted but never read anywhere in the partial class. _isHelp has a consumer; _isRetry does not. Remove or add an explanatory comment.

  • [NIT – Performance]TotalTests property lambda should use static for consistency with all other lambdas in this file: Sum(static a => a.TotalTests).

  • [NIT – Dead Param]exitCode in TestExecutionCompleted is never stored. A short inline comment noting this is deferred to the SDK plug-in PR would be enough.

- GetOrAddAssemblyRun: replace ConcurrentDictionary.GetOrAdd (whose value
factory could run multiple times under contention and orphan worker slots /
bump _counter) with an explicit lock + TryGetValue + add, guaranteeing exactly
one worker per executionId.
- Remove the unused _isRetry field/assignment (IDE0052); keep the isRetry
parameter for SDK API parity.
- Prefix the Assembly and TotalTests doc summaries with 'Gets' (SA1623).
- Normalize the TestCompleted argument indentation in the Failed/Timeout/
Cancelled cases (IDE0055).
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- GetOrAddAssemblyRun: add a lock-free TryGetValue fast path (double-checked
locking) so repeat AssemblyRunStarted calls don't take the lock, matching the
reviewer's suggested pattern.
- TotalTests: make the Sum lambda static to avoid a per-call delegate allocation
on the SDK orchestrator's progress-tick path.
- Document instanceId on AssemblyRunStarted as reserved for the SDK orchestrator
retry-counting follow-up (unused by the in-process host path).
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
CopilotAI review requested due to automatic review settings June 19, 2026 15:24

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: 12/12 changed files
  • Comments generated: 1

TestExecutionCompleted cleared the per-assembly runs but left _artifacts and
WasCancelled intact (pre-existing, but contradicts the documented HotReload
'start fresh' intent on a method this PR reworks): a later session would
re-print the previous session's artifacts or stay stuck in the aborted state.
Now also clear _artifacts and reset WasCancelled after the summary has consumed
them. Adds a regression test exercising two sessions on the same reporter.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@Evangelink

Copy link
Copy Markdown
MemberAuthor

🧪 Test quality grade — PR #9256

ΔTestGradeBandNotes
newTerminalTestReporterTests.
TerminalTestReporter_
WhenReusedAcrossSessions_
DoesNotLeakArtifactsOrCancelledState
B80–893 meaningful assertions cover both leak vectors; body is ~42 lines — consider a session-run helper to trim setup verbosity.

This advisory comment was generated automatically. Grades are heuristic
and informational — they do not block merging. Re-run with
/grade-tests.

🤖 Automated content by GitHub Copilot. Posted via a maintainer's GitHub token, so it appears under their account — the account owner did not write or approve this content personally. Generated by the Grade Tests on PR (on open / sync) workflow. · 464 AIC · ⌖ 13.2 AIC · ⊞ 45.6K · [◷]( · )

@Evangelink
Amaury Levé (Evangelink) merged commit 4545e5d into mainJun 19, 2026
34 of 39 checks passed
@Evangelink
Amaury Levé (Evangelink) deleted the copilot/terminal-reporter-1-to-n branch June 19, 2026 16:06
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.

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

Unify MTP terminal reporter to handle 1..N assemblies - #9256

Merged
Amaury Levé (Evangelink) merged 4 commits into
mainfrom
copilot/terminal-reporter-1-to-n
Jun 19, 2026
Merged

Unify MTP terminal reporter to handle 1..N assemblies#9256
Amaury Levé (Evangelink) merged 4 commits into
mainfrom
copilot/terminal-reporter-1-to-n

Conversation

@Evangelink

Copy link
Copy Markdown
Member

What

Converts the Microsoft.Testing.Platform terminal test reporter from a single-assembly model to a multi-assembly (1..N) model, so a single shared source can render both the in-process MTP host (one assembly) and the dotnet test orchestrator (N child assemblies).

This is the prerequisite called out in the terminal-sharing effort (follow-up to the protocol-sharing PRs #9218 / #9231 and the package PRs #9249 / #9250 / #9253): the SDK currently hard-forks the reporter because the shared MTP copy was single-assembly. With this change the reporter core exposes the multi-assembly API the SDK needs.

How

  • Replace the single TestProgressState + _assembly/_targetFramework/_architecture fields with a ConcurrentDictionary<string, TestProgressState> _assemblies keyed by a caller-provided execution id. The in-process host passes one fixed id; the orchestrator passes one per child assembly.
  • Route TestCompleted / TestInProgress / TestDiscovered / AssemblyRunStarted / AssemblyRunCompleted / ArtifactAdded per execution id.
  • Aggregate the run and discovery summaries across all assemblies. N=1 (in-process) output stays byte-identical via an assemblies.Count == 1 branch that still appends the per-assembly link to the verdict line; for N>1 the verdict line is link-free (per-assembly identity lives in the progress area).
  • Expand TestRunArtifact and ArtifactAdded with assembly / targetFramework / architecture / executionId.
  • Add a TotalTests aggregate and surface isHelp / isRetry on TestExecutionStarted.
  • Adapt the in-process caller (TerminalOutputDevice) to the new API using a single fixed InProcessExecutionId.

Scope / deferred

The SDK-orchestrator-only surface — handshake-failure recap, instanceId-based retry counting, build-error tracking, and exit-code-in-summary — is intentionally not ported here. That logic depends on the SDK's richer TestProgressState retry model and is only exercised by real multi-process runs, so it cannot be verified from the in-process unit tests. It will land with the SDK plug-in PR where it can be driven and tested end-to-end against actual N-assembly runs.

Verification

  • Platform builds clean on net8.0 / net9.0 / netstandard2.0 (0 warnings).
  • Full Microsoft.Testing.Platform.UnitTests suite green: 1197 total, 0 failed — the 94 existing byte-exact terminal tests confirm the in-process UI is unchanged.
  • New TerminalTestReporter_WhenMultipleAssemblies_AggregatesCountsAndOmitsAssemblyLinkOnVerdict test covers the N>1 path (aggregated counts + link-free verdict).
  • The standalone TerminalReporterContract consumer still compiles in isolation (the Share dotnet test wire contract (ObjectFieldIds + Constants) as source #9218-equivalent gate).

The terminal test reporter was single-assembly (in-process MTP host). The
dotnet test orchestrator runs N child assemblies, so the SDK had to hard-fork
the reporter. This converts the reporter core to a multi-assembly model so a
single shared source can render both cases:
- Replace the single TestProgressState + _assembly/_targetFramework/_architecture
fields with a ConcurrentDictionary<string, TestProgressState> keyed by a
caller-provided execution id (in-process passes one fixed id).
- Route TestCompleted/TestInProgress/TestDiscovered/AssemblyRunStarted/
AssemblyRunCompleted/ArtifactAdded per execution id.
- Aggregate run/discovery summaries across all assemblies; keep the N=1
(in-process) output byte-identical via an assemblies.Count == 1 branch that
still appends the per-assembly link to the verdict line.
- Expand TestRunArtifact and ArtifactAdded with assembly/tfm/arch/executionId.
- Add TotalTests aggregate and surface isHelp/isRetry on TestExecutionStarted.
- Adapt the in-process caller (TerminalOutputDevice) to the new API with a
single fixed InProcessExecutionId.
The SDK-orchestrator-only surface (handshake-failure recap, instanceId-based
retry counting, build errors, exit-code-in-summary) is intentionally deferred
to the SDK plug-in PR, where it can be driven and verified against real
multi-assembly runs.
Verified: platform builds clean on net8.0/net9.0/netstandard2.0;
full Microsoft.Testing.Platform.UnitTests suite green (1197, 0 failed),
including a new N>1 test; the standalone TerminalReporterContract consumer
still compiles in isolation.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
CopilotAI review requested due to automatic review settings June 19, 2026 14:54

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

This PR updates the Microsoft.Testing.Platform terminal reporter to support a multi-assembly (1..N) execution model (keyed by execution id), while preserving byte-identical output for the existing single-assembly in-process host.

Changes:

  • Replace single-assembly reporter state with a per-execution-id map and aggregate run/discovery summaries across assemblies.
  • Extend reporter events/artifact tracking to carry assembly identity (assembly/TFM/architecture/executionId) and route events by execution id.
  • Update the in-process TerminalOutputDevice and unit tests to use the new multi-assembly reporter API; add a dedicated multi-assembly aggregation test.
Show a summary per file
FileDescription
test/UnitTests/Microsoft.Testing.Platform.UnitTests/OutputDevice/Terminal/TerminalTestReporterTests.csUpdates tests for new reporter APIs and adds a multi-assembly aggregation test.
src/Platform/Microsoft.Testing.Platform/OutputDevice/TerminalOutputDevice.SessionLifecycle.csAdapts in-process session lifecycle to call multi-assembly reporter methods using a fixed execution id.
src/Platform/Microsoft.Testing.Platform/OutputDevice/TerminalOutputDevice.Initialization.csIntroduces an in-process execution id constant and updates reporter construction for the new API.
src/Platform/Microsoft.Testing.Platform/OutputDevice/TerminalOutputDevice.DataConsumption.csRoutes test state/artifacts into reporter with the in-process execution id and richer artifact metadata.
src/Platform/Microsoft.Testing.Platform/OutputDevice/Terminal/TestRunArtifact.csExpands artifact record to include assembly identity fields.
src/Platform/Microsoft.Testing.Platform/OutputDevice/Terminal/TestProgressState.csStores the full assembly path/display name for later linking and summary formatting.
src/Platform/Microsoft.Testing.Platform/OutputDevice/Terminal/TerminalTestReporter.TestCompletion.csKeys TestCompleted handling by execution id and looks up per-assembly state.
src/Platform/Microsoft.Testing.Platform/OutputDevice/Terminal/TerminalTestReporter.Summary.csAggregates counts across assemblies and removes verdict-line assembly link when N>1.
src/Platform/Microsoft.Testing.Platform/OutputDevice/Terminal/TerminalTestReporter.Messaging.csKeys TestInProgress by execution id for multi-assembly progress rendering.
src/Platform/Microsoft.Testing.Platform/OutputDevice/Terminal/TerminalTestReporter.Lifecycle.csAdds multi-assembly lifecycle APIs and clears per-assembly state on completion.
src/Platform/Microsoft.Testing.Platform/OutputDevice/Terminal/TerminalTestReporter.Formatting.csUpdates assembly link formatting to operate on an assembly-run state instance.
src/Platform/Microsoft.Testing.Platform/OutputDevice/Terminal/TerminalTestReporter.csRemoves single-assembly constructor inputs, adds per-assembly dictionary state and richer ArtifactAdded.

Copilot's findings

Comments suppressed due to low confidence (2)

src/Platform/Microsoft.Testing.Platform/OutputDevice/TerminalOutputDevice.DataConsumption.cs:205

  • The TestCompleted argument indentation in the TimeoutTestNodeStateProperty case has an extra leading space compared to the surrounding blocks, which can trigger IDE0055 (formatting) during builds with code-style enforcement.
 case TimeoutTestNodeStateProperty timeoutState:
_terminalTestReporter.TestCompleted(
InProcessExecutionId,
testNodeStateChanged.TestNode.Uid.Value,
testNodeStateChanged.TestNode.DisplayName,
TestOutcome.Timeout,
duration,

src/Platform/Microsoft.Testing.Platform/OutputDevice/TerminalOutputDevice.DataConsumption.cs:223

  • The TestCompleted argument indentation in the CancelledTestNodeStateProperty case is inconsistent (extra leading space), which may trigger IDE0055 formatting diagnostics when code style is enforced in build.
#pragma warning disable CS0618, MTP0001 // Type or member is obsolete
case CancelledTestNodeStateProperty cancelledState:
#pragma warning restore CS0618, MTP0001 // Type or member is obsolete
_terminalTestReporter.TestCompleted(
InProcessExecutionId,
testNodeStateChanged.TestNode.Uid.Value,
testNodeStateChanged.TestNode.DisplayName,
TestOutcome.Canceled,
duration,
  • Files reviewed: 12/12 changed files
  • Comments generated: 5

@Evangelink

This comment has been minimized.

@Evangelink

Copy link
Copy Markdown
MemberAuthor

🔍 Build Failure Analysis

Summary — The build of Microsoft.Testing.Platform.csproj fails due to three distinct analyzer violations introduced in the OutputDevice/Terminal/ area: two SA1623 violations (property XML doc summaries not starting with "Gets") and one IDE0052 violation (a private field that is assigned but never read).


Root cause 1 — SA1623: Property doc summaries missing "Gets" prefix

SA1623 requires that the <summary> of any property with a getter must begin with the word "Gets" (e.g. /// <summary>Gets the ...</summary>). Two new properties were added with prose-style summaries that omit this prefix — both are lifted to errors by TreatWarningsAsErrors in this project.

Affected locations

Fix — prepend Gets to each <summary>:

// TestProgressState.cs:23
- /// <summary>The assembly path or display name as provided by the caller (used for the summary link).</summary>+ /// <summary>Gets the assembly path or display name as provided by the caller (used for the summary link).</summary>
// TerminalTestReporter.cs:67
- /// <summary>Total number of tests across all registered assemblies.</summary>+ /// <summary>Gets the total number of tests across all registered assemblies.</summary>

Root cause 2 — IDE0052: _isRetry field is write-only

[IDE0052]((learn.microsoft.com/redacted) fires when a private member is assigned but never read. _isRetry was introduced in TerminalTestReporter.Lifecycle.cs, assigned in TestExecutionStarted, but consumed nowhere — unlike its sibling _isHelp, which is already read in TestExecutionCompleted to suppress the summary when --help is active.

Affected location

Fix — remove the field and its assignment. If retry-specific behaviour is planned, add the corresponding read-site (e.g. in TestExecutionCompleted) and keep the field; otherwise also remove the bool isRetry parameter from TestExecutionStarted and update callers.

 private bool _isHelp;
- private bool _isRetry;
public void TestExecutionStarted(DateTimeOffset testStartTime, int workerCount, bool isDiscovery, bool isHelp, bool isRetry)
{
_isDiscovery = isDiscovery;
_isHelp = isHelp;
- _isRetry = isRetry;
_testExecutionStartTime = testStartTime;

Build overview
MSBuild18.7.0-preview
Duration154 s
Projects45
Errors10 (3 unique issues, each repeated across multiple TFMs)
Warnings0

All errors originate in Microsoft.Testing.Platform.csproj. The failures of Microsoft.Testing.Extensions.AzureDevOpsReport, Microsoft.Testing.Extensions.CrashDump, and MSTest.TestAdapter are cascades from that single project.

All MSBuild errors (3 unique, 10 total across TFMs)
CodeFileLineMessage
SA1623TestProgressState.cs24The property's documentation summary text should begin with: 'Gets'
SA1623TerminalTestReporter.cs68The property's documentation summary text should begin with: 'Gets'
IDE0052TerminalTestReporter.Lifecycle.cs12Private member _isRetry can be removed as the value assigned to it is never read

🤖 Generated by the Build Failure Analysis workflow · commit 476750cda2a7f588e1e5476448945f10789afff4

🤖 Automated content by GitHub Copilot. Posted via a maintainer's GitHub token, so it appears under their account — the account owner did not write or approve this content personally. Generated by the Build Failure Analysis workflow. · 246.3 AIC · ⌖ 12.8 AIC · ⊞ 46.9K · [◷]( · )

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.

🤖 Automated content by GitHub Copilot. Posted via a maintainer's GitHub token, so it appears under their account — the account owner did not write or approve this content personally. Generated by the Build Failure Analysis workflow. · 246.3 AIC · ⌖ 12.8 AIC · ⊞ 46.9K ·

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.

Note

🤖 Automated review by GitHub Copilot. Posted via a maintainer's GitHub token, so it appears under their account — the account owner did not write or approve this content personally. Generated by the Expert Code Review workflow. To request a follow-up action, reply by tagging @copilot directly.

Review Summary

The multi-assembly generalisation is well-designed: the ConcurrentDictionary approach, the single InProcessExecutionId constant for the in-process host, the assemblies.Count == 1 branch that keeps byte-identical single-assembly output, and the new aggregating test — all read cleanly. The 94 byte-exact existing tests continuing to pass confirms the in-process path is unchanged.

One MAJOR correctness regression was found in the factory pattern, plus two MODERATE dead-API issues and two NITs.


#DimensionVerdictFileLine
2Threading & Concurrency🔴 1 MAJORTerminalTestReporter.Lifecycle.cs26–37
15Code Structure🟡 2 MODERATETerminalTestReporter.Lifecycle.cs12, 23
5Performance & Allocations🔵 1 NITTerminalTestReporter.cs68
8Defensive Coding🔵 1 NITTerminalTestReporter.Lifecycle.cs50

✅ 18/22 dimensions clean.


Findings

  • [MAJOR – Threading]GetOrAdd(key, valueFactory) can invoke the factory more than once for the same key if two threads race (the inner lock serialises the invocations sequentially but does not prevent both from running). Both invocations call AddWorker, occupying two slots; only one TestProgressState is kept in the dictionary, leaking the other's progress slot permanently. Current callers guarantee unique IDs so the race is not reachable today, but the pattern is a correctness regression from the old double-checked locking. Replace with TryGetValue / lock / TryGetValue / _assemblies[key] = ... DCL. See inline comment.

  • [MODERATE – Dead API]instanceId is accepted by AssemblyRunStarted but never forwarded or stored. If keeping it as scaffolding, add a // Reserved for SDK retry logic comment; otherwise remove and re-introduce with its consumer.

  • [MODERATE – Dead Code]_isRetry is assigned in TestExecutionStarted but never read anywhere in the partial class. _isHelp has a consumer; _isRetry does not. Remove or add an explanatory comment.

  • [NIT – Performance]TotalTests property lambda should use static for consistency with all other lambdas in this file: Sum(static a => a.TotalTests).

  • [NIT – Dead Param]exitCode in TestExecutionCompleted is never stored. A short inline comment noting this is deferred to the SDK plug-in PR would be enough.

- GetOrAddAssemblyRun: replace ConcurrentDictionary.GetOrAdd (whose value
factory could run multiple times under contention and orphan worker slots /
bump _counter) with an explicit lock + TryGetValue + add, guaranteeing exactly
one worker per executionId.
- Remove the unused _isRetry field/assignment (IDE0052); keep the isRetry
parameter for SDK API parity.
- Prefix the Assembly and TotalTests doc summaries with 'Gets' (SA1623).
- Normalize the TestCompleted argument indentation in the Failed/Timeout/
Cancelled cases (IDE0055).
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- GetOrAddAssemblyRun: add a lock-free TryGetValue fast path (double-checked
locking) so repeat AssemblyRunStarted calls don't take the lock, matching the
reviewer's suggested pattern.
- TotalTests: make the Sum lambda static to avoid a per-call delegate allocation
on the SDK orchestrator's progress-tick path.
- Document instanceId on AssemblyRunStarted as reserved for the SDK orchestrator
retry-counting follow-up (unused by the in-process host path).
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
CopilotAI review requested due to automatic review settings June 19, 2026 15:24

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: 12/12 changed files
  • Comments generated: 1

TestExecutionCompleted cleared the per-assembly runs but left _artifacts and
WasCancelled intact (pre-existing, but contradicts the documented HotReload
'start fresh' intent on a method this PR reworks): a later session would
re-print the previous session's artifacts or stay stuck in the aborted state.
Now also clear _artifacts and reset WasCancelled after the summary has consumed
them. Adds a regression test exercising two sessions on the same reporter.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@Evangelink

Copy link
Copy Markdown
MemberAuthor

🧪 Test quality grade — PR #9256

ΔTestGradeBandNotes
newTerminalTestReporterTests.
TerminalTestReporter_
WhenReusedAcrossSessions_
DoesNotLeakArtifactsOrCancelledState
B80–893 meaningful assertions cover both leak vectors; body is ~42 lines — consider a session-run helper to trim setup verbosity.

This advisory comment was generated automatically. Grades are heuristic
and informational — they do not block merging. Re-run with
/grade-tests.

🤖 Automated content by GitHub Copilot. Posted via a maintainer's GitHub token, so it appears under their account — the account owner did not write or approve this content personally. Generated by the Grade Tests on PR (on open / sync) workflow. · 464 AIC · ⌖ 13.2 AIC · ⊞ 45.6K · [◷]( · )

@Evangelink
Amaury Levé (Evangelink) merged commit 4545e5d into mainJun 19, 2026
34 of 39 checks passed
@Evangelink
Amaury Levé (Evangelink) deleted the copilot/terminal-reporter-1-to-n branch June 19, 2026 16:06
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.

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

Unify MTP terminal reporter to handle 1..N assemblies - #9256

Merged
Amaury Levé (Evangelink) merged 4 commits into
mainfrom
copilot/terminal-reporter-1-to-n
Jun 19, 2026
Merged

Unify MTP terminal reporter to handle 1..N assemblies#9256
Amaury Levé (Evangelink) merged 4 commits into
mainfrom
copilot/terminal-reporter-1-to-n

Conversation

@Evangelink

Copy link
Copy Markdown
Member

What

Converts the Microsoft.Testing.Platform terminal test reporter from a single-assembly model to a multi-assembly (1..N) model, so a single shared source can render both the in-process MTP host (one assembly) and the dotnet test orchestrator (N child assemblies).

This is the prerequisite called out in the terminal-sharing effort (follow-up to the protocol-sharing PRs #9218 / #9231 and the package PRs #9249 / #9250 / #9253): the SDK currently hard-forks the reporter because the shared MTP copy was single-assembly. With this change the reporter core exposes the multi-assembly API the SDK needs.

How

  • Replace the single TestProgressState + _assembly/_targetFramework/_architecture fields with a ConcurrentDictionary<string, TestProgressState> _assemblies keyed by a caller-provided execution id. The in-process host passes one fixed id; the orchestrator passes one per child assembly.
  • Route TestCompleted / TestInProgress / TestDiscovered / AssemblyRunStarted / AssemblyRunCompleted / ArtifactAdded per execution id.
  • Aggregate the run and discovery summaries across all assemblies. N=1 (in-process) output stays byte-identical via an assemblies.Count == 1 branch that still appends the per-assembly link to the verdict line; for N>1 the verdict line is link-free (per-assembly identity lives in the progress area).
  • Expand TestRunArtifact and ArtifactAdded with assembly / targetFramework / architecture / executionId.
  • Add a TotalTests aggregate and surface isHelp / isRetry on TestExecutionStarted.
  • Adapt the in-process caller (TerminalOutputDevice) to the new API using a single fixed InProcessExecutionId.

Scope / deferred

The SDK-orchestrator-only surface — handshake-failure recap, instanceId-based retry counting, build-error tracking, and exit-code-in-summary — is intentionally not ported here. That logic depends on the SDK's richer TestProgressState retry model and is only exercised by real multi-process runs, so it cannot be verified from the in-process unit tests. It will land with the SDK plug-in PR where it can be driven and tested end-to-end against actual N-assembly runs.

Verification

  • Platform builds clean on net8.0 / net9.0 / netstandard2.0 (0 warnings).
  • Full Microsoft.Testing.Platform.UnitTests suite green: 1197 total, 0 failed — the 94 existing byte-exact terminal tests confirm the in-process UI is unchanged.
  • New TerminalTestReporter_WhenMultipleAssemblies_AggregatesCountsAndOmitsAssemblyLinkOnVerdict test covers the N>1 path (aggregated counts + link-free verdict).
  • The standalone TerminalReporterContract consumer still compiles in isolation (the Share dotnet test wire contract (ObjectFieldIds + Constants) as source #9218-equivalent gate).

The terminal test reporter was single-assembly (in-process MTP host). The
dotnet test orchestrator runs N child assemblies, so the SDK had to hard-fork
the reporter. This converts the reporter core to a multi-assembly model so a
single shared source can render both cases:
- Replace the single TestProgressState + _assembly/_targetFramework/_architecture
fields with a ConcurrentDictionary<string, TestProgressState> keyed by a
caller-provided execution id (in-process passes one fixed id).
- Route TestCompleted/TestInProgress/TestDiscovered/AssemblyRunStarted/
AssemblyRunCompleted/ArtifactAdded per execution id.
- Aggregate run/discovery summaries across all assemblies; keep the N=1
(in-process) output byte-identical via an assemblies.Count == 1 branch that
still appends the per-assembly link to the verdict line.
- Expand TestRunArtifact and ArtifactAdded with assembly/tfm/arch/executionId.
- Add TotalTests aggregate and surface isHelp/isRetry on TestExecutionStarted.
- Adapt the in-process caller (TerminalOutputDevice) to the new API with a
single fixed InProcessExecutionId.
The SDK-orchestrator-only surface (handshake-failure recap, instanceId-based
retry counting, build errors, exit-code-in-summary) is intentionally deferred
to the SDK plug-in PR, where it can be driven and verified against real
multi-assembly runs.
Verified: platform builds clean on net8.0/net9.0/netstandard2.0;
full Microsoft.Testing.Platform.UnitTests suite green (1197, 0 failed),
including a new N>1 test; the standalone TerminalReporterContract consumer
still compiles in isolation.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
CopilotAI review requested due to automatic review settings June 19, 2026 14:54

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

This PR updates the Microsoft.Testing.Platform terminal reporter to support a multi-assembly (1..N) execution model (keyed by execution id), while preserving byte-identical output for the existing single-assembly in-process host.

Changes:

  • Replace single-assembly reporter state with a per-execution-id map and aggregate run/discovery summaries across assemblies.
  • Extend reporter events/artifact tracking to carry assembly identity (assembly/TFM/architecture/executionId) and route events by execution id.
  • Update the in-process TerminalOutputDevice and unit tests to use the new multi-assembly reporter API; add a dedicated multi-assembly aggregation test.
Show a summary per file
FileDescription
test/UnitTests/Microsoft.Testing.Platform.UnitTests/OutputDevice/Terminal/TerminalTestReporterTests.csUpdates tests for new reporter APIs and adds a multi-assembly aggregation test.
src/Platform/Microsoft.Testing.Platform/OutputDevice/TerminalOutputDevice.SessionLifecycle.csAdapts in-process session lifecycle to call multi-assembly reporter methods using a fixed execution id.
src/Platform/Microsoft.Testing.Platform/OutputDevice/TerminalOutputDevice.Initialization.csIntroduces an in-process execution id constant and updates reporter construction for the new API.
src/Platform/Microsoft.Testing.Platform/OutputDevice/TerminalOutputDevice.DataConsumption.csRoutes test state/artifacts into reporter with the in-process execution id and richer artifact metadata.
src/Platform/Microsoft.Testing.Platform/OutputDevice/Terminal/TestRunArtifact.csExpands artifact record to include assembly identity fields.
src/Platform/Microsoft.Testing.Platform/OutputDevice/Terminal/TestProgressState.csStores the full assembly path/display name for later linking and summary formatting.
src/Platform/Microsoft.Testing.Platform/OutputDevice/Terminal/TerminalTestReporter.TestCompletion.csKeys TestCompleted handling by execution id and looks up per-assembly state.
src/Platform/Microsoft.Testing.Platform/OutputDevice/Terminal/TerminalTestReporter.Summary.csAggregates counts across assemblies and removes verdict-line assembly link when N>1.
src/Platform/Microsoft.Testing.Platform/OutputDevice/Terminal/TerminalTestReporter.Messaging.csKeys TestInProgress by execution id for multi-assembly progress rendering.
src/Platform/Microsoft.Testing.Platform/OutputDevice/Terminal/TerminalTestReporter.Lifecycle.csAdds multi-assembly lifecycle APIs and clears per-assembly state on completion.
src/Platform/Microsoft.Testing.Platform/OutputDevice/Terminal/TerminalTestReporter.Formatting.csUpdates assembly link formatting to operate on an assembly-run state instance.
src/Platform/Microsoft.Testing.Platform/OutputDevice/Terminal/TerminalTestReporter.csRemoves single-assembly constructor inputs, adds per-assembly dictionary state and richer ArtifactAdded.

Copilot's findings

Comments suppressed due to low confidence (2)

src/Platform/Microsoft.Testing.Platform/OutputDevice/TerminalOutputDevice.DataConsumption.cs:205

  • The TestCompleted argument indentation in the TimeoutTestNodeStateProperty case has an extra leading space compared to the surrounding blocks, which can trigger IDE0055 (formatting) during builds with code-style enforcement.
 case TimeoutTestNodeStateProperty timeoutState:
_terminalTestReporter.TestCompleted(
InProcessExecutionId,
testNodeStateChanged.TestNode.Uid.Value,
testNodeStateChanged.TestNode.DisplayName,
TestOutcome.Timeout,
duration,

src/Platform/Microsoft.Testing.Platform/OutputDevice/TerminalOutputDevice.DataConsumption.cs:223

  • The TestCompleted argument indentation in the CancelledTestNodeStateProperty case is inconsistent (extra leading space), which may trigger IDE0055 formatting diagnostics when code style is enforced in build.
#pragma warning disable CS0618, MTP0001 // Type or member is obsolete
case CancelledTestNodeStateProperty cancelledState:
#pragma warning restore CS0618, MTP0001 // Type or member is obsolete
_terminalTestReporter.TestCompleted(
InProcessExecutionId,
testNodeStateChanged.TestNode.Uid.Value,
testNodeStateChanged.TestNode.DisplayName,
TestOutcome.Canceled,
duration,
  • Files reviewed: 12/12 changed files
  • Comments generated: 5

@Evangelink

This comment has been minimized.

@Evangelink

Copy link
Copy Markdown
MemberAuthor

🔍 Build Failure Analysis

Summary — The build of Microsoft.Testing.Platform.csproj fails due to three distinct analyzer violations introduced in the OutputDevice/Terminal/ area: two SA1623 violations (property XML doc summaries not starting with "Gets") and one IDE0052 violation (a private field that is assigned but never read).


Root cause 1 — SA1623: Property doc summaries missing "Gets" prefix

SA1623 requires that the <summary> of any property with a getter must begin with the word "Gets" (e.g. /// <summary>Gets the ...</summary>). Two new properties were added with prose-style summaries that omit this prefix — both are lifted to errors by TreatWarningsAsErrors in this project.

Affected locations

Fix — prepend Gets to each <summary>:

// TestProgressState.cs:23
- /// <summary>The assembly path or display name as provided by the caller (used for the summary link).</summary>+ /// <summary>Gets the assembly path or display name as provided by the caller (used for the summary link).</summary>
// TerminalTestReporter.cs:67
- /// <summary>Total number of tests across all registered assemblies.</summary>+ /// <summary>Gets the total number of tests across all registered assemblies.</summary>

Root cause 2 — IDE0052: _isRetry field is write-only

[IDE0052]((learn.microsoft.com/redacted) fires when a private member is assigned but never read. _isRetry was introduced in TerminalTestReporter.Lifecycle.cs, assigned in TestExecutionStarted, but consumed nowhere — unlike its sibling _isHelp, which is already read in TestExecutionCompleted to suppress the summary when --help is active.

Affected location

Fix — remove the field and its assignment. If retry-specific behaviour is planned, add the corresponding read-site (e.g. in TestExecutionCompleted) and keep the field; otherwise also remove the bool isRetry parameter from TestExecutionStarted and update callers.

 private bool _isHelp;
- private bool _isRetry;
public void TestExecutionStarted(DateTimeOffset testStartTime, int workerCount, bool isDiscovery, bool isHelp, bool isRetry)
{
_isDiscovery = isDiscovery;
_isHelp = isHelp;
- _isRetry = isRetry;
_testExecutionStartTime = testStartTime;

Build overview
MSBuild18.7.0-preview
Duration154 s
Projects45
Errors10 (3 unique issues, each repeated across multiple TFMs)
Warnings0

All errors originate in Microsoft.Testing.Platform.csproj. The failures of Microsoft.Testing.Extensions.AzureDevOpsReport, Microsoft.Testing.Extensions.CrashDump, and MSTest.TestAdapter are cascades from that single project.

All MSBuild errors (3 unique, 10 total across TFMs)
CodeFileLineMessage
SA1623TestProgressState.cs24The property's documentation summary text should begin with: 'Gets'
SA1623TerminalTestReporter.cs68The property's documentation summary text should begin with: 'Gets'
IDE0052TerminalTestReporter.Lifecycle.cs12Private member _isRetry can be removed as the value assigned to it is never read

🤖 Generated by the Build Failure Analysis workflow · commit 476750cda2a7f588e1e5476448945f10789afff4

🤖 Automated content by GitHub Copilot. Posted via a maintainer's GitHub token, so it appears under their account — the account owner did not write or approve this content personally. Generated by the Build Failure Analysis workflow. · 246.3 AIC · ⌖ 12.8 AIC · ⊞ 46.9K · [◷]( · )

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.

🤖 Automated content by GitHub Copilot. Posted via a maintainer's GitHub token, so it appears under their account — the account owner did not write or approve this content personally. Generated by the Build Failure Analysis workflow. · 246.3 AIC · ⌖ 12.8 AIC · ⊞ 46.9K ·

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.

Note

🤖 Automated review by GitHub Copilot. Posted via a maintainer's GitHub token, so it appears under their account — the account owner did not write or approve this content personally. Generated by the Expert Code Review workflow. To request a follow-up action, reply by tagging @copilot directly.

Review Summary

The multi-assembly generalisation is well-designed: the ConcurrentDictionary approach, the single InProcessExecutionId constant for the in-process host, the assemblies.Count == 1 branch that keeps byte-identical single-assembly output, and the new aggregating test — all read cleanly. The 94 byte-exact existing tests continuing to pass confirms the in-process path is unchanged.

One MAJOR correctness regression was found in the factory pattern, plus two MODERATE dead-API issues and two NITs.


#DimensionVerdictFileLine
2Threading & Concurrency🔴 1 MAJORTerminalTestReporter.Lifecycle.cs26–37
15Code Structure🟡 2 MODERATETerminalTestReporter.Lifecycle.cs12, 23
5Performance & Allocations🔵 1 NITTerminalTestReporter.cs68
8Defensive Coding🔵 1 NITTerminalTestReporter.Lifecycle.cs50

✅ 18/22 dimensions clean.


Findings

  • [MAJOR – Threading]GetOrAdd(key, valueFactory) can invoke the factory more than once for the same key if two threads race (the inner lock serialises the invocations sequentially but does not prevent both from running). Both invocations call AddWorker, occupying two slots; only one TestProgressState is kept in the dictionary, leaking the other's progress slot permanently. Current callers guarantee unique IDs so the race is not reachable today, but the pattern is a correctness regression from the old double-checked locking. Replace with TryGetValue / lock / TryGetValue / _assemblies[key] = ... DCL. See inline comment.

  • [MODERATE – Dead API]instanceId is accepted by AssemblyRunStarted but never forwarded or stored. If keeping it as scaffolding, add a // Reserved for SDK retry logic comment; otherwise remove and re-introduce with its consumer.

  • [MODERATE – Dead Code]_isRetry is assigned in TestExecutionStarted but never read anywhere in the partial class. _isHelp has a consumer; _isRetry does not. Remove or add an explanatory comment.

  • [NIT – Performance]TotalTests property lambda should use static for consistency with all other lambdas in this file: Sum(static a => a.TotalTests).

  • [NIT – Dead Param]exitCode in TestExecutionCompleted is never stored. A short inline comment noting this is deferred to the SDK plug-in PR would be enough.

- GetOrAddAssemblyRun: replace ConcurrentDictionary.GetOrAdd (whose value
factory could run multiple times under contention and orphan worker slots /
bump _counter) with an explicit lock + TryGetValue + add, guaranteeing exactly
one worker per executionId.
- Remove the unused _isRetry field/assignment (IDE0052); keep the isRetry
parameter for SDK API parity.
- Prefix the Assembly and TotalTests doc summaries with 'Gets' (SA1623).
- Normalize the TestCompleted argument indentation in the Failed/Timeout/
Cancelled cases (IDE0055).
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- GetOrAddAssemblyRun: add a lock-free TryGetValue fast path (double-checked
locking) so repeat AssemblyRunStarted calls don't take the lock, matching the
reviewer's suggested pattern.
- TotalTests: make the Sum lambda static to avoid a per-call delegate allocation
on the SDK orchestrator's progress-tick path.
- Document instanceId on AssemblyRunStarted as reserved for the SDK orchestrator
retry-counting follow-up (unused by the in-process host path).
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
CopilotAI review requested due to automatic review settings June 19, 2026 15:24

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: 12/12 changed files
  • Comments generated: 1

TestExecutionCompleted cleared the per-assembly runs but left _artifacts and
WasCancelled intact (pre-existing, but contradicts the documented HotReload
'start fresh' intent on a method this PR reworks): a later session would
re-print the previous session's artifacts or stay stuck in the aborted state.
Now also clear _artifacts and reset WasCancelled after the summary has consumed
them. Adds a regression test exercising two sessions on the same reporter.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@Evangelink

Copy link
Copy Markdown
MemberAuthor

🧪 Test quality grade — PR #9256

ΔTestGradeBandNotes
newTerminalTestReporterTests.
TerminalTestReporter_
WhenReusedAcrossSessions_
DoesNotLeakArtifactsOrCancelledState
B80–893 meaningful assertions cover both leak vectors; body is ~42 lines — consider a session-run helper to trim setup verbosity.

This advisory comment was generated automatically. Grades are heuristic
and informational — they do not block merging. Re-run with
/grade-tests.

🤖 Automated content by GitHub Copilot. Posted via a maintainer's GitHub token, so it appears under their account — the account owner did not write or approve this content personally. Generated by the Grade Tests on PR (on open / sync) workflow. · 464 AIC · ⌖ 13.2 AIC · ⊞ 45.6K · [◷]( · )

@Evangelink
Amaury Levé (Evangelink) merged commit 4545e5d into mainJun 19, 2026
34 of 39 checks passed
@Evangelink
Amaury Levé (Evangelink) deleted the copilot/terminal-reporter-1-to-n branch June 19, 2026 16:06
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.

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

Unify MTP terminal reporter to handle 1..N assemblies - #9256

Merged
Amaury Levé (Evangelink) merged 4 commits into
mainfrom
copilot/terminal-reporter-1-to-n
Jun 19, 2026
Merged

Unify MTP terminal reporter to handle 1..N assemblies#9256
Amaury Levé (Evangelink) merged 4 commits into
mainfrom
copilot/terminal-reporter-1-to-n

Conversation

@Evangelink

Copy link
Copy Markdown
Member

What

Converts the Microsoft.Testing.Platform terminal test reporter from a single-assembly model to a multi-assembly (1..N) model, so a single shared source can render both the in-process MTP host (one assembly) and the dotnet test orchestrator (N child assemblies).

This is the prerequisite called out in the terminal-sharing effort (follow-up to the protocol-sharing PRs #9218 / #9231 and the package PRs #9249 / #9250 / #9253): the SDK currently hard-forks the reporter because the shared MTP copy was single-assembly. With this change the reporter core exposes the multi-assembly API the SDK needs.

How

  • Replace the single TestProgressState + _assembly/_targetFramework/_architecture fields with a ConcurrentDictionary<string, TestProgressState> _assemblies keyed by a caller-provided execution id. The in-process host passes one fixed id; the orchestrator passes one per child assembly.
  • Route TestCompleted / TestInProgress / TestDiscovered / AssemblyRunStarted / AssemblyRunCompleted / ArtifactAdded per execution id.
  • Aggregate the run and discovery summaries across all assemblies. N=1 (in-process) output stays byte-identical via an assemblies.Count == 1 branch that still appends the per-assembly link to the verdict line; for N>1 the verdict line is link-free (per-assembly identity lives in the progress area).
  • Expand TestRunArtifact and ArtifactAdded with assembly / targetFramework / architecture / executionId.
  • Add a TotalTests aggregate and surface isHelp / isRetry on TestExecutionStarted.
  • Adapt the in-process caller (TerminalOutputDevice) to the new API using a single fixed InProcessExecutionId.

Scope / deferred

The SDK-orchestrator-only surface — handshake-failure recap, instanceId-based retry counting, build-error tracking, and exit-code-in-summary — is intentionally not ported here. That logic depends on the SDK's richer TestProgressState retry model and is only exercised by real multi-process runs, so it cannot be verified from the in-process unit tests. It will land with the SDK plug-in PR where it can be driven and tested end-to-end against actual N-assembly runs.

Verification

  • Platform builds clean on net8.0 / net9.0 / netstandard2.0 (0 warnings).
  • Full Microsoft.Testing.Platform.UnitTests suite green: 1197 total, 0 failed — the 94 existing byte-exact terminal tests confirm the in-process UI is unchanged.
  • New TerminalTestReporter_WhenMultipleAssemblies_AggregatesCountsAndOmitsAssemblyLinkOnVerdict test covers the N>1 path (aggregated counts + link-free verdict).
  • The standalone TerminalReporterContract consumer still compiles in isolation (the Share dotnet test wire contract (ObjectFieldIds + Constants) as source #9218-equivalent gate).

The terminal test reporter was single-assembly (in-process MTP host). The
dotnet test orchestrator runs N child assemblies, so the SDK had to hard-fork
the reporter. This converts the reporter core to a multi-assembly model so a
single shared source can render both cases:
- Replace the single TestProgressState + _assembly/_targetFramework/_architecture
fields with a ConcurrentDictionary<string, TestProgressState> keyed by a
caller-provided execution id (in-process passes one fixed id).
- Route TestCompleted/TestInProgress/TestDiscovered/AssemblyRunStarted/
AssemblyRunCompleted/ArtifactAdded per execution id.
- Aggregate run/discovery summaries across all assemblies; keep the N=1
(in-process) output byte-identical via an assemblies.Count == 1 branch that
still appends the per-assembly link to the verdict line.
- Expand TestRunArtifact and ArtifactAdded with assembly/tfm/arch/executionId.
- Add TotalTests aggregate and surface isHelp/isRetry on TestExecutionStarted.
- Adapt the in-process caller (TerminalOutputDevice) to the new API with a
single fixed InProcessExecutionId.
The SDK-orchestrator-only surface (handshake-failure recap, instanceId-based
retry counting, build errors, exit-code-in-summary) is intentionally deferred
to the SDK plug-in PR, where it can be driven and verified against real
multi-assembly runs.
Verified: platform builds clean on net8.0/net9.0/netstandard2.0;
full Microsoft.Testing.Platform.UnitTests suite green (1197, 0 failed),
including a new N>1 test; the standalone TerminalReporterContract consumer
still compiles in isolation.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
CopilotAI review requested due to automatic review settings June 19, 2026 14:54

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

This PR updates the Microsoft.Testing.Platform terminal reporter to support a multi-assembly (1..N) execution model (keyed by execution id), while preserving byte-identical output for the existing single-assembly in-process host.

Changes:

  • Replace single-assembly reporter state with a per-execution-id map and aggregate run/discovery summaries across assemblies.
  • Extend reporter events/artifact tracking to carry assembly identity (assembly/TFM/architecture/executionId) and route events by execution id.
  • Update the in-process TerminalOutputDevice and unit tests to use the new multi-assembly reporter API; add a dedicated multi-assembly aggregation test.
Show a summary per file
FileDescription
test/UnitTests/Microsoft.Testing.Platform.UnitTests/OutputDevice/Terminal/TerminalTestReporterTests.csUpdates tests for new reporter APIs and adds a multi-assembly aggregation test.
src/Platform/Microsoft.Testing.Platform/OutputDevice/TerminalOutputDevice.SessionLifecycle.csAdapts in-process session lifecycle to call multi-assembly reporter methods using a fixed execution id.
src/Platform/Microsoft.Testing.Platform/OutputDevice/TerminalOutputDevice.Initialization.csIntroduces an in-process execution id constant and updates reporter construction for the new API.
src/Platform/Microsoft.Testing.Platform/OutputDevice/TerminalOutputDevice.DataConsumption.csRoutes test state/artifacts into reporter with the in-process execution id and richer artifact metadata.
src/Platform/Microsoft.Testing.Platform/OutputDevice/Terminal/TestRunArtifact.csExpands artifact record to include assembly identity fields.
src/Platform/Microsoft.Testing.Platform/OutputDevice/Terminal/TestProgressState.csStores the full assembly path/display name for later linking and summary formatting.
src/Platform/Microsoft.Testing.Platform/OutputDevice/Terminal/TerminalTestReporter.TestCompletion.csKeys TestCompleted handling by execution id and looks up per-assembly state.
src/Platform/Microsoft.Testing.Platform/OutputDevice/Terminal/TerminalTestReporter.Summary.csAggregates counts across assemblies and removes verdict-line assembly link when N>1.
src/Platform/Microsoft.Testing.Platform/OutputDevice/Terminal/TerminalTestReporter.Messaging.csKeys TestInProgress by execution id for multi-assembly progress rendering.
src/Platform/Microsoft.Testing.Platform/OutputDevice/Terminal/TerminalTestReporter.Lifecycle.csAdds multi-assembly lifecycle APIs and clears per-assembly state on completion.
src/Platform/Microsoft.Testing.Platform/OutputDevice/Terminal/TerminalTestReporter.Formatting.csUpdates assembly link formatting to operate on an assembly-run state instance.
src/Platform/Microsoft.Testing.Platform/OutputDevice/Terminal/TerminalTestReporter.csRemoves single-assembly constructor inputs, adds per-assembly dictionary state and richer ArtifactAdded.

Copilot's findings

Comments suppressed due to low confidence (2)

src/Platform/Microsoft.Testing.Platform/OutputDevice/TerminalOutputDevice.DataConsumption.cs:205

  • The TestCompleted argument indentation in the TimeoutTestNodeStateProperty case has an extra leading space compared to the surrounding blocks, which can trigger IDE0055 (formatting) during builds with code-style enforcement.
 case TimeoutTestNodeStateProperty timeoutState:
_terminalTestReporter.TestCompleted(
InProcessExecutionId,
testNodeStateChanged.TestNode.Uid.Value,
testNodeStateChanged.TestNode.DisplayName,
TestOutcome.Timeout,
duration,

src/Platform/Microsoft.Testing.Platform/OutputDevice/TerminalOutputDevice.DataConsumption.cs:223

  • The TestCompleted argument indentation in the CancelledTestNodeStateProperty case is inconsistent (extra leading space), which may trigger IDE0055 formatting diagnostics when code style is enforced in build.
#pragma warning disable CS0618, MTP0001 // Type or member is obsolete
case CancelledTestNodeStateProperty cancelledState:
#pragma warning restore CS0618, MTP0001 // Type or member is obsolete
_terminalTestReporter.TestCompleted(
InProcessExecutionId,
testNodeStateChanged.TestNode.Uid.Value,
testNodeStateChanged.TestNode.DisplayName,
TestOutcome.Canceled,
duration,
  • Files reviewed: 12/12 changed files
  • Comments generated: 5

@Evangelink

This comment has been minimized.

@Evangelink

Copy link
Copy Markdown
MemberAuthor

🔍 Build Failure Analysis

Summary — The build of Microsoft.Testing.Platform.csproj fails due to three distinct analyzer violations introduced in the OutputDevice/Terminal/ area: two SA1623 violations (property XML doc summaries not starting with "Gets") and one IDE0052 violation (a private field that is assigned but never read).


Root cause 1 — SA1623: Property doc summaries missing "Gets" prefix

SA1623 requires that the <summary> of any property with a getter must begin with the word "Gets" (e.g. /// <summary>Gets the ...</summary>). Two new properties were added with prose-style summaries that omit this prefix — both are lifted to errors by TreatWarningsAsErrors in this project.

Affected locations

Fix — prepend Gets to each <summary>:

// TestProgressState.cs:23
- /// <summary>The assembly path or display name as provided by the caller (used for the summary link).</summary>+ /// <summary>Gets the assembly path or display name as provided by the caller (used for the summary link).</summary>
// TerminalTestReporter.cs:67
- /// <summary>Total number of tests across all registered assemblies.</summary>+ /// <summary>Gets the total number of tests across all registered assemblies.</summary>

Root cause 2 — IDE0052: _isRetry field is write-only

[IDE0052]((learn.microsoft.com/redacted) fires when a private member is assigned but never read. _isRetry was introduced in TerminalTestReporter.Lifecycle.cs, assigned in TestExecutionStarted, but consumed nowhere — unlike its sibling _isHelp, which is already read in TestExecutionCompleted to suppress the summary when --help is active.

Affected location

Fix — remove the field and its assignment. If retry-specific behaviour is planned, add the corresponding read-site (e.g. in TestExecutionCompleted) and keep the field; otherwise also remove the bool isRetry parameter from TestExecutionStarted and update callers.

 private bool _isHelp;
- private bool _isRetry;
public void TestExecutionStarted(DateTimeOffset testStartTime, int workerCount, bool isDiscovery, bool isHelp, bool isRetry)
{
_isDiscovery = isDiscovery;
_isHelp = isHelp;
- _isRetry = isRetry;
_testExecutionStartTime = testStartTime;

Build overview
MSBuild18.7.0-preview
Duration154 s
Projects45
Errors10 (3 unique issues, each repeated across multiple TFMs)
Warnings0

All errors originate in Microsoft.Testing.Platform.csproj. The failures of Microsoft.Testing.Extensions.AzureDevOpsReport, Microsoft.Testing.Extensions.CrashDump, and MSTest.TestAdapter are cascades from that single project.

All MSBuild errors (3 unique, 10 total across TFMs)
CodeFileLineMessage
SA1623TestProgressState.cs24The property's documentation summary text should begin with: 'Gets'
SA1623TerminalTestReporter.cs68The property's documentation summary text should begin with: 'Gets'
IDE0052TerminalTestReporter.Lifecycle.cs12Private member _isRetry can be removed as the value assigned to it is never read

🤖 Generated by the Build Failure Analysis workflow · commit 476750cda2a7f588e1e5476448945f10789afff4

🤖 Automated content by GitHub Copilot. Posted via a maintainer's GitHub token, so it appears under their account — the account owner did not write or approve this content personally. Generated by the Build Failure Analysis workflow. · 246.3 AIC · ⌖ 12.8 AIC · ⊞ 46.9K · [◷]( · )

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.

🤖 Automated content by GitHub Copilot. Posted via a maintainer's GitHub token, so it appears under their account — the account owner did not write or approve this content personally. Generated by the Build Failure Analysis workflow. · 246.3 AIC · ⌖ 12.8 AIC · ⊞ 46.9K ·

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.

Note

🤖 Automated review by GitHub Copilot. Posted via a maintainer's GitHub token, so it appears under their account — the account owner did not write or approve this content personally. Generated by the Expert Code Review workflow. To request a follow-up action, reply by tagging @copilot directly.

Review Summary

The multi-assembly generalisation is well-designed: the ConcurrentDictionary approach, the single InProcessExecutionId constant for the in-process host, the assemblies.Count == 1 branch that keeps byte-identical single-assembly output, and the new aggregating test — all read cleanly. The 94 byte-exact existing tests continuing to pass confirms the in-process path is unchanged.

One MAJOR correctness regression was found in the factory pattern, plus two MODERATE dead-API issues and two NITs.


#DimensionVerdictFileLine
2Threading & Concurrency🔴 1 MAJORTerminalTestReporter.Lifecycle.cs26–37
15Code Structure🟡 2 MODERATETerminalTestReporter.Lifecycle.cs12, 23
5Performance & Allocations🔵 1 NITTerminalTestReporter.cs68
8Defensive Coding🔵 1 NITTerminalTestReporter.Lifecycle.cs50

✅ 18/22 dimensions clean.


Findings

  • [MAJOR – Threading]GetOrAdd(key, valueFactory) can invoke the factory more than once for the same key if two threads race (the inner lock serialises the invocations sequentially but does not prevent both from running). Both invocations call AddWorker, occupying two slots; only one TestProgressState is kept in the dictionary, leaking the other's progress slot permanently. Current callers guarantee unique IDs so the race is not reachable today, but the pattern is a correctness regression from the old double-checked locking. Replace with TryGetValue / lock / TryGetValue / _assemblies[key] = ... DCL. See inline comment.

  • [MODERATE – Dead API]instanceId is accepted by AssemblyRunStarted but never forwarded or stored. If keeping it as scaffolding, add a // Reserved for SDK retry logic comment; otherwise remove and re-introduce with its consumer.

  • [MODERATE – Dead Code]_isRetry is assigned in TestExecutionStarted but never read anywhere in the partial class. _isHelp has a consumer; _isRetry does not. Remove or add an explanatory comment.

  • [NIT – Performance]TotalTests property lambda should use static for consistency with all other lambdas in this file: Sum(static a => a.TotalTests).

  • [NIT – Dead Param]exitCode in TestExecutionCompleted is never stored. A short inline comment noting this is deferred to the SDK plug-in PR would be enough.

- GetOrAddAssemblyRun: replace ConcurrentDictionary.GetOrAdd (whose value
factory could run multiple times under contention and orphan worker slots /
bump _counter) with an explicit lock + TryGetValue + add, guaranteeing exactly
one worker per executionId.
- Remove the unused _isRetry field/assignment (IDE0052); keep the isRetry
parameter for SDK API parity.
- Prefix the Assembly and TotalTests doc summaries with 'Gets' (SA1623).
- Normalize the TestCompleted argument indentation in the Failed/Timeout/
Cancelled cases (IDE0055).
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- GetOrAddAssemblyRun: add a lock-free TryGetValue fast path (double-checked
locking) so repeat AssemblyRunStarted calls don't take the lock, matching the
reviewer's suggested pattern.
- TotalTests: make the Sum lambda static to avoid a per-call delegate allocation
on the SDK orchestrator's progress-tick path.
- Document instanceId on AssemblyRunStarted as reserved for the SDK orchestrator
retry-counting follow-up (unused by the in-process host path).
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
CopilotAI review requested due to automatic review settings June 19, 2026 15:24

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: 12/12 changed files
  • Comments generated: 1

TestExecutionCompleted cleared the per-assembly runs but left _artifacts and
WasCancelled intact (pre-existing, but contradicts the documented HotReload
'start fresh' intent on a method this PR reworks): a later session would
re-print the previous session's artifacts or stay stuck in the aborted state.
Now also clear _artifacts and reset WasCancelled after the summary has consumed
them. Adds a regression test exercising two sessions on the same reporter.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@Evangelink

Copy link
Copy Markdown
MemberAuthor

🧪 Test quality grade — PR #9256

ΔTestGradeBandNotes
newTerminalTestReporterTests.
TerminalTestReporter_
WhenReusedAcrossSessions_
DoesNotLeakArtifactsOrCancelledState
B80–893 meaningful assertions cover both leak vectors; body is ~42 lines — consider a session-run helper to trim setup verbosity.

This advisory comment was generated automatically. Grades are heuristic
and informational — they do not block merging. Re-run with
/grade-tests.

🤖 Automated content by GitHub Copilot. Posted via a maintainer's GitHub token, so it appears under their account — the account owner did not write or approve this content personally. Generated by the Grade Tests on PR (on open / sync) workflow. · 464 AIC · ⌖ 13.2 AIC · ⊞ 45.6K · [◷]( · )

@Evangelink
Amaury Levé (Evangelink) merged commit 4545e5d into mainJun 19, 2026
34 of 39 checks passed
@Evangelink
Amaury Levé (Evangelink) deleted the copilot/terminal-reporter-1-to-n branch June 19, 2026 16:06
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.

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

Unify MTP terminal reporter to handle 1..N assemblies - #9256

Merged
Amaury Levé (Evangelink) merged 4 commits into
mainfrom
copilot/terminal-reporter-1-to-n
Jun 19, 2026
Merged

Unify MTP terminal reporter to handle 1..N assemblies#9256
Amaury Levé (Evangelink) merged 4 commits into
mainfrom
copilot/terminal-reporter-1-to-n

Conversation

@Evangelink

Copy link
Copy Markdown
Member

What

Converts the Microsoft.Testing.Platform terminal test reporter from a single-assembly model to a multi-assembly (1..N) model, so a single shared source can render both the in-process MTP host (one assembly) and the dotnet test orchestrator (N child assemblies).

This is the prerequisite called out in the terminal-sharing effort (follow-up to the protocol-sharing PRs #9218 / #9231 and the package PRs #9249 / #9250 / #9253): the SDK currently hard-forks the reporter because the shared MTP copy was single-assembly. With this change the reporter core exposes the multi-assembly API the SDK needs.

How

  • Replace the single TestProgressState + _assembly/_targetFramework/_architecture fields with a ConcurrentDictionary<string, TestProgressState> _assemblies keyed by a caller-provided execution id. The in-process host passes one fixed id; the orchestrator passes one per child assembly.
  • Route TestCompleted / TestInProgress / TestDiscovered / AssemblyRunStarted / AssemblyRunCompleted / ArtifactAdded per execution id.
  • Aggregate the run and discovery summaries across all assemblies. N=1 (in-process) output stays byte-identical via an assemblies.Count == 1 branch that still appends the per-assembly link to the verdict line; for N>1 the verdict line is link-free (per-assembly identity lives in the progress area).
  • Expand TestRunArtifact and ArtifactAdded with assembly / targetFramework / architecture / executionId.
  • Add a TotalTests aggregate and surface isHelp / isRetry on TestExecutionStarted.
  • Adapt the in-process caller (TerminalOutputDevice) to the new API using a single fixed InProcessExecutionId.

Scope / deferred

The SDK-orchestrator-only surface — handshake-failure recap, instanceId-based retry counting, build-error tracking, and exit-code-in-summary — is intentionally not ported here. That logic depends on the SDK's richer TestProgressState retry model and is only exercised by real multi-process runs, so it cannot be verified from the in-process unit tests. It will land with the SDK plug-in PR where it can be driven and tested end-to-end against actual N-assembly runs.

Verification

  • Platform builds clean on net8.0 / net9.0 / netstandard2.0 (0 warnings).
  • Full Microsoft.Testing.Platform.UnitTests suite green: 1197 total, 0 failed — the 94 existing byte-exact terminal tests confirm the in-process UI is unchanged.
  • New TerminalTestReporter_WhenMultipleAssemblies_AggregatesCountsAndOmitsAssemblyLinkOnVerdict test covers the N>1 path (aggregated counts + link-free verdict).
  • The standalone TerminalReporterContract consumer still compiles in isolation (the Share dotnet test wire contract (ObjectFieldIds + Constants) as source #9218-equivalent gate).

The terminal test reporter was single-assembly (in-process MTP host). The
dotnet test orchestrator runs N child assemblies, so the SDK had to hard-fork
the reporter. This converts the reporter core to a multi-assembly model so a
single shared source can render both cases:
- Replace the single TestProgressState + _assembly/_targetFramework/_architecture
fields with a ConcurrentDictionary<string, TestProgressState> keyed by a
caller-provided execution id (in-process passes one fixed id).
- Route TestCompleted/TestInProgress/TestDiscovered/AssemblyRunStarted/
AssemblyRunCompleted/ArtifactAdded per execution id.
- Aggregate run/discovery summaries across all assemblies; keep the N=1
(in-process) output byte-identical via an assemblies.Count == 1 branch that
still appends the per-assembly link to the verdict line.
- Expand TestRunArtifact and ArtifactAdded with assembly/tfm/arch/executionId.
- Add TotalTests aggregate and surface isHelp/isRetry on TestExecutionStarted.
- Adapt the in-process caller (TerminalOutputDevice) to the new API with a
single fixed InProcessExecutionId.
The SDK-orchestrator-only surface (handshake-failure recap, instanceId-based
retry counting, build errors, exit-code-in-summary) is intentionally deferred
to the SDK plug-in PR, where it can be driven and verified against real
multi-assembly runs.
Verified: platform builds clean on net8.0/net9.0/netstandard2.0;
full Microsoft.Testing.Platform.UnitTests suite green (1197, 0 failed),
including a new N>1 test; the standalone TerminalReporterContract consumer
still compiles in isolation.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
CopilotAI review requested due to automatic review settings June 19, 2026 14:54

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

This PR updates the Microsoft.Testing.Platform terminal reporter to support a multi-assembly (1..N) execution model (keyed by execution id), while preserving byte-identical output for the existing single-assembly in-process host.

Changes:

  • Replace single-assembly reporter state with a per-execution-id map and aggregate run/discovery summaries across assemblies.
  • Extend reporter events/artifact tracking to carry assembly identity (assembly/TFM/architecture/executionId) and route events by execution id.
  • Update the in-process TerminalOutputDevice and unit tests to use the new multi-assembly reporter API; add a dedicated multi-assembly aggregation test.
Show a summary per file
FileDescription
test/UnitTests/Microsoft.Testing.Platform.UnitTests/OutputDevice/Terminal/TerminalTestReporterTests.csUpdates tests for new reporter APIs and adds a multi-assembly aggregation test.
src/Platform/Microsoft.Testing.Platform/OutputDevice/TerminalOutputDevice.SessionLifecycle.csAdapts in-process session lifecycle to call multi-assembly reporter methods using a fixed execution id.
src/Platform/Microsoft.Testing.Platform/OutputDevice/TerminalOutputDevice.Initialization.csIntroduces an in-process execution id constant and updates reporter construction for the new API.
src/Platform/Microsoft.Testing.Platform/OutputDevice/TerminalOutputDevice.DataConsumption.csRoutes test state/artifacts into reporter with the in-process execution id and richer artifact metadata.
src/Platform/Microsoft.Testing.Platform/OutputDevice/Terminal/TestRunArtifact.csExpands artifact record to include assembly identity fields.
src/Platform/Microsoft.Testing.Platform/OutputDevice/Terminal/TestProgressState.csStores the full assembly path/display name for later linking and summary formatting.
src/Platform/Microsoft.Testing.Platform/OutputDevice/Terminal/TerminalTestReporter.TestCompletion.csKeys TestCompleted handling by execution id and looks up per-assembly state.
src/Platform/Microsoft.Testing.Platform/OutputDevice/Terminal/TerminalTestReporter.Summary.csAggregates counts across assemblies and removes verdict-line assembly link when N>1.
src/Platform/Microsoft.Testing.Platform/OutputDevice/Terminal/TerminalTestReporter.Messaging.csKeys TestInProgress by execution id for multi-assembly progress rendering.
src/Platform/Microsoft.Testing.Platform/OutputDevice/Terminal/TerminalTestReporter.Lifecycle.csAdds multi-assembly lifecycle APIs and clears per-assembly state on completion.
src/Platform/Microsoft.Testing.Platform/OutputDevice/Terminal/TerminalTestReporter.Formatting.csUpdates assembly link formatting to operate on an assembly-run state instance.
src/Platform/Microsoft.Testing.Platform/OutputDevice/Terminal/TerminalTestReporter.csRemoves single-assembly constructor inputs, adds per-assembly dictionary state and richer ArtifactAdded.

Copilot's findings

Comments suppressed due to low confidence (2)

src/Platform/Microsoft.Testing.Platform/OutputDevice/TerminalOutputDevice.DataConsumption.cs:205

  • The TestCompleted argument indentation in the TimeoutTestNodeStateProperty case has an extra leading space compared to the surrounding blocks, which can trigger IDE0055 (formatting) during builds with code-style enforcement.
 case TimeoutTestNodeStateProperty timeoutState:
_terminalTestReporter.TestCompleted(
InProcessExecutionId,
testNodeStateChanged.TestNode.Uid.Value,
testNodeStateChanged.TestNode.DisplayName,
TestOutcome.Timeout,
duration,

src/Platform/Microsoft.Testing.Platform/OutputDevice/TerminalOutputDevice.DataConsumption.cs:223

  • The TestCompleted argument indentation in the CancelledTestNodeStateProperty case is inconsistent (extra leading space), which may trigger IDE0055 formatting diagnostics when code style is enforced in build.
#pragma warning disable CS0618, MTP0001 // Type or member is obsolete
case CancelledTestNodeStateProperty cancelledState:
#pragma warning restore CS0618, MTP0001 // Type or member is obsolete
_terminalTestReporter.TestCompleted(
InProcessExecutionId,
testNodeStateChanged.TestNode.Uid.Value,
testNodeStateChanged.TestNode.DisplayName,
TestOutcome.Canceled,
duration,
  • Files reviewed: 12/12 changed files
  • Comments generated: 5

@Evangelink

This comment has been minimized.

@Evangelink

Copy link
Copy Markdown
MemberAuthor

🔍 Build Failure Analysis

Summary — The build of Microsoft.Testing.Platform.csproj fails due to three distinct analyzer violations introduced in the OutputDevice/Terminal/ area: two SA1623 violations (property XML doc summaries not starting with "Gets") and one IDE0052 violation (a private field that is assigned but never read).


Root cause 1 — SA1623: Property doc summaries missing "Gets" prefix

SA1623 requires that the <summary> of any property with a getter must begin with the word "Gets" (e.g. /// <summary>Gets the ...</summary>). Two new properties were added with prose-style summaries that omit this prefix — both are lifted to errors by TreatWarningsAsErrors in this project.

Affected locations

Fix — prepend Gets to each <summary>:

// TestProgressState.cs:23
- /// <summary>The assembly path or display name as provided by the caller (used for the summary link).</summary>+ /// <summary>Gets the assembly path or display name as provided by the caller (used for the summary link).</summary>
// TerminalTestReporter.cs:67
- /// <summary>Total number of tests across all registered assemblies.</summary>+ /// <summary>Gets the total number of tests across all registered assemblies.</summary>

Root cause 2 — IDE0052: _isRetry field is write-only

[IDE0052]((learn.microsoft.com/redacted) fires when a private member is assigned but never read. _isRetry was introduced in TerminalTestReporter.Lifecycle.cs, assigned in TestExecutionStarted, but consumed nowhere — unlike its sibling _isHelp, which is already read in TestExecutionCompleted to suppress the summary when --help is active.

Affected location

Fix — remove the field and its assignment. If retry-specific behaviour is planned, add the corresponding read-site (e.g. in TestExecutionCompleted) and keep the field; otherwise also remove the bool isRetry parameter from TestExecutionStarted and update callers.

 private bool _isHelp;
- private bool _isRetry;
public void TestExecutionStarted(DateTimeOffset testStartTime, int workerCount, bool isDiscovery, bool isHelp, bool isRetry)
{
_isDiscovery = isDiscovery;
_isHelp = isHelp;
- _isRetry = isRetry;
_testExecutionStartTime = testStartTime;

Build overview
MSBuild18.7.0-preview
Duration154 s
Projects45
Errors10 (3 unique issues, each repeated across multiple TFMs)
Warnings0

All errors originate in Microsoft.Testing.Platform.csproj. The failures of Microsoft.Testing.Extensions.AzureDevOpsReport, Microsoft.Testing.Extensions.CrashDump, and MSTest.TestAdapter are cascades from that single project.

All MSBuild errors (3 unique, 10 total across TFMs)
CodeFileLineMessage
SA1623TestProgressState.cs24The property's documentation summary text should begin with: 'Gets'
SA1623TerminalTestReporter.cs68The property's documentation summary text should begin with: 'Gets'
IDE0052TerminalTestReporter.Lifecycle.cs12Private member _isRetry can be removed as the value assigned to it is never read

🤖 Generated by the Build Failure Analysis workflow · commit 476750cda2a7f588e1e5476448945f10789afff4

🤖 Automated content by GitHub Copilot. Posted via a maintainer's GitHub token, so it appears under their account — the account owner did not write or approve this content personally. Generated by the Build Failure Analysis workflow. · 246.3 AIC · ⌖ 12.8 AIC · ⊞ 46.9K · [◷]( · )

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.

🤖 Automated content by GitHub Copilot. Posted via a maintainer's GitHub token, so it appears under their account — the account owner did not write or approve this content personally. Generated by the Build Failure Analysis workflow. · 246.3 AIC · ⌖ 12.8 AIC · ⊞ 46.9K ·

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.

Note

🤖 Automated review by GitHub Copilot. Posted via a maintainer's GitHub token, so it appears under their account — the account owner did not write or approve this content personally. Generated by the Expert Code Review workflow. To request a follow-up action, reply by tagging @copilot directly.

Review Summary

The multi-assembly generalisation is well-designed: the ConcurrentDictionary approach, the single InProcessExecutionId constant for the in-process host, the assemblies.Count == 1 branch that keeps byte-identical single-assembly output, and the new aggregating test — all read cleanly. The 94 byte-exact existing tests continuing to pass confirms the in-process path is unchanged.

One MAJOR correctness regression was found in the factory pattern, plus two MODERATE dead-API issues and two NITs.


#DimensionVerdictFileLine
2Threading & Concurrency🔴 1 MAJORTerminalTestReporter.Lifecycle.cs26–37
15Code Structure🟡 2 MODERATETerminalTestReporter.Lifecycle.cs12, 23
5Performance & Allocations🔵 1 NITTerminalTestReporter.cs68
8Defensive Coding🔵 1 NITTerminalTestReporter.Lifecycle.cs50

✅ 18/22 dimensions clean.


Findings

  • [MAJOR – Threading]GetOrAdd(key, valueFactory) can invoke the factory more than once for the same key if two threads race (the inner lock serialises the invocations sequentially but does not prevent both from running). Both invocations call AddWorker, occupying two slots; only one TestProgressState is kept in the dictionary, leaking the other's progress slot permanently. Current callers guarantee unique IDs so the race is not reachable today, but the pattern is a correctness regression from the old double-checked locking. Replace with TryGetValue / lock / TryGetValue / _assemblies[key] = ... DCL. See inline comment.

  • [MODERATE – Dead API]instanceId is accepted by AssemblyRunStarted but never forwarded or stored. If keeping it as scaffolding, add a // Reserved for SDK retry logic comment; otherwise remove and re-introduce with its consumer.

  • [MODERATE – Dead Code]_isRetry is assigned in TestExecutionStarted but never read anywhere in the partial class. _isHelp has a consumer; _isRetry does not. Remove or add an explanatory comment.

  • [NIT – Performance]TotalTests property lambda should use static for consistency with all other lambdas in this file: Sum(static a => a.TotalTests).

  • [NIT – Dead Param]exitCode in TestExecutionCompleted is never stored. A short inline comment noting this is deferred to the SDK plug-in PR would be enough.

- GetOrAddAssemblyRun: replace ConcurrentDictionary.GetOrAdd (whose value
factory could run multiple times under contention and orphan worker slots /
bump _counter) with an explicit lock + TryGetValue + add, guaranteeing exactly
one worker per executionId.
- Remove the unused _isRetry field/assignment (IDE0052); keep the isRetry
parameter for SDK API parity.
- Prefix the Assembly and TotalTests doc summaries with 'Gets' (SA1623).
- Normalize the TestCompleted argument indentation in the Failed/Timeout/
Cancelled cases (IDE0055).
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- GetOrAddAssemblyRun: add a lock-free TryGetValue fast path (double-checked
locking) so repeat AssemblyRunStarted calls don't take the lock, matching the
reviewer's suggested pattern.
- TotalTests: make the Sum lambda static to avoid a per-call delegate allocation
on the SDK orchestrator's progress-tick path.
- Document instanceId on AssemblyRunStarted as reserved for the SDK orchestrator
retry-counting follow-up (unused by the in-process host path).
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
CopilotAI review requested due to automatic review settings June 19, 2026 15:24

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: 12/12 changed files
  • Comments generated: 1

TestExecutionCompleted cleared the per-assembly runs but left _artifacts and
WasCancelled intact (pre-existing, but contradicts the documented HotReload
'start fresh' intent on a method this PR reworks): a later session would
re-print the previous session's artifacts or stay stuck in the aborted state.
Now also clear _artifacts and reset WasCancelled after the summary has consumed
them. Adds a regression test exercising two sessions on the same reporter.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@Evangelink

Copy link
Copy Markdown
MemberAuthor

🧪 Test quality grade — PR #9256

ΔTestGradeBandNotes
newTerminalTestReporterTests.
TerminalTestReporter_
WhenReusedAcrossSessions_
DoesNotLeakArtifactsOrCancelledState
B80–893 meaningful assertions cover both leak vectors; body is ~42 lines — consider a session-run helper to trim setup verbosity.

This advisory comment was generated automatically. Grades are heuristic
and informational — they do not block merging. Re-run with
/grade-tests.

🤖 Automated content by GitHub Copilot. Posted via a maintainer's GitHub token, so it appears under their account — the account owner did not write or approve this content personally. Generated by the Grade Tests on PR (on open / sync) workflow. · 464 AIC · ⌖ 13.2 AIC · ⊞ 45.6K · [◷]( · )

@Evangelink
Amaury Levé (Evangelink) merged commit 4545e5d into mainJun 19, 2026
34 of 39 checks passed
@Evangelink
Amaury Levé (Evangelink) deleted the copilot/terminal-reporter-1-to-n branch June 19, 2026 16:06
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.

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

Unify MTP terminal reporter to handle 1..N assemblies - #9256

Merged
Amaury Levé (Evangelink) merged 4 commits into
mainfrom
copilot/terminal-reporter-1-to-n
Jun 19, 2026
Merged

Unify MTP terminal reporter to handle 1..N assemblies#9256
Amaury Levé (Evangelink) merged 4 commits into
mainfrom
copilot/terminal-reporter-1-to-n

Conversation

@Evangelink

Copy link
Copy Markdown
Member

What

Converts the Microsoft.Testing.Platform terminal test reporter from a single-assembly model to a multi-assembly (1..N) model, so a single shared source can render both the in-process MTP host (one assembly) and the dotnet test orchestrator (N child assemblies).

This is the prerequisite called out in the terminal-sharing effort (follow-up to the protocol-sharing PRs #9218 / #9231 and the package PRs #9249 / #9250 / #9253): the SDK currently hard-forks the reporter because the shared MTP copy was single-assembly. With this change the reporter core exposes the multi-assembly API the SDK needs.

How

  • Replace the single TestProgressState + _assembly/_targetFramework/_architecture fields with a ConcurrentDictionary<string, TestProgressState> _assemblies keyed by a caller-provided execution id. The in-process host passes one fixed id; the orchestrator passes one per child assembly.
  • Route TestCompleted / TestInProgress / TestDiscovered / AssemblyRunStarted / AssemblyRunCompleted / ArtifactAdded per execution id.
  • Aggregate the run and discovery summaries across all assemblies. N=1 (in-process) output stays byte-identical via an assemblies.Count == 1 branch that still appends the per-assembly link to the verdict line; for N>1 the verdict line is link-free (per-assembly identity lives in the progress area).
  • Expand TestRunArtifact and ArtifactAdded with assembly / targetFramework / architecture / executionId.
  • Add a TotalTests aggregate and surface isHelp / isRetry on TestExecutionStarted.
  • Adapt the in-process caller (TerminalOutputDevice) to the new API using a single fixed InProcessExecutionId.

Scope / deferred

The SDK-orchestrator-only surface — handshake-failure recap, instanceId-based retry counting, build-error tracking, and exit-code-in-summary — is intentionally not ported here. That logic depends on the SDK's richer TestProgressState retry model and is only exercised by real multi-process runs, so it cannot be verified from the in-process unit tests. It will land with the SDK plug-in PR where it can be driven and tested end-to-end against actual N-assembly runs.

Verification

  • Platform builds clean on net8.0 / net9.0 / netstandard2.0 (0 warnings).
  • Full Microsoft.Testing.Platform.UnitTests suite green: 1197 total, 0 failed — the 94 existing byte-exact terminal tests confirm the in-process UI is unchanged.
  • New TerminalTestReporter_WhenMultipleAssemblies_AggregatesCountsAndOmitsAssemblyLinkOnVerdict test covers the N>1 path (aggregated counts + link-free verdict).
  • The standalone TerminalReporterContract consumer still compiles in isolation (the Share dotnet test wire contract (ObjectFieldIds + Constants) as source #9218-equivalent gate).

The terminal test reporter was single-assembly (in-process MTP host). The
dotnet test orchestrator runs N child assemblies, so the SDK had to hard-fork
the reporter. This converts the reporter core to a multi-assembly model so a
single shared source can render both cases:
- Replace the single TestProgressState + _assembly/_targetFramework/_architecture
fields with a ConcurrentDictionary<string, TestProgressState> keyed by a
caller-provided execution id (in-process passes one fixed id).
- Route TestCompleted/TestInProgress/TestDiscovered/AssemblyRunStarted/
AssemblyRunCompleted/ArtifactAdded per execution id.
- Aggregate run/discovery summaries across all assemblies; keep the N=1
(in-process) output byte-identical via an assemblies.Count == 1 branch that
still appends the per-assembly link to the verdict line.
- Expand TestRunArtifact and ArtifactAdded with assembly/tfm/arch/executionId.
- Add TotalTests aggregate and surface isHelp/isRetry on TestExecutionStarted.
- Adapt the in-process caller (TerminalOutputDevice) to the new API with a
single fixed InProcessExecutionId.
The SDK-orchestrator-only surface (handshake-failure recap, instanceId-based
retry counting, build errors, exit-code-in-summary) is intentionally deferred
to the SDK plug-in PR, where it can be driven and verified against real
multi-assembly runs.
Verified: platform builds clean on net8.0/net9.0/netstandard2.0;
full Microsoft.Testing.Platform.UnitTests suite green (1197, 0 failed),
including a new N>1 test; the standalone TerminalReporterContract consumer
still compiles in isolation.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
CopilotAI review requested due to automatic review settings June 19, 2026 14:54

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

This PR updates the Microsoft.Testing.Platform terminal reporter to support a multi-assembly (1..N) execution model (keyed by execution id), while preserving byte-identical output for the existing single-assembly in-process host.

Changes:

  • Replace single-assembly reporter state with a per-execution-id map and aggregate run/discovery summaries across assemblies.
  • Extend reporter events/artifact tracking to carry assembly identity (assembly/TFM/architecture/executionId) and route events by execution id.
  • Update the in-process TerminalOutputDevice and unit tests to use the new multi-assembly reporter API; add a dedicated multi-assembly aggregation test.
Show a summary per file
FileDescription
test/UnitTests/Microsoft.Testing.Platform.UnitTests/OutputDevice/Terminal/TerminalTestReporterTests.csUpdates tests for new reporter APIs and adds a multi-assembly aggregation test.
src/Platform/Microsoft.Testing.Platform/OutputDevice/TerminalOutputDevice.SessionLifecycle.csAdapts in-process session lifecycle to call multi-assembly reporter methods using a fixed execution id.
src/Platform/Microsoft.Testing.Platform/OutputDevice/TerminalOutputDevice.Initialization.csIntroduces an in-process execution id constant and updates reporter construction for the new API.
src/Platform/Microsoft.Testing.Platform/OutputDevice/TerminalOutputDevice.DataConsumption.csRoutes test state/artifacts into reporter with the in-process execution id and richer artifact metadata.
src/Platform/Microsoft.Testing.Platform/OutputDevice/Terminal/TestRunArtifact.csExpands artifact record to include assembly identity fields.
src/Platform/Microsoft.Testing.Platform/OutputDevice/Terminal/TestProgressState.csStores the full assembly path/display name for later linking and summary formatting.
src/Platform/Microsoft.Testing.Platform/OutputDevice/Terminal/TerminalTestReporter.TestCompletion.csKeys TestCompleted handling by execution id and looks up per-assembly state.
src/Platform/Microsoft.Testing.Platform/OutputDevice/Terminal/TerminalTestReporter.Summary.csAggregates counts across assemblies and removes verdict-line assembly link when N>1.
src/Platform/Microsoft.Testing.Platform/OutputDevice/Terminal/TerminalTestReporter.Messaging.csKeys TestInProgress by execution id for multi-assembly progress rendering.
src/Platform/Microsoft.Testing.Platform/OutputDevice/Terminal/TerminalTestReporter.Lifecycle.csAdds multi-assembly lifecycle APIs and clears per-assembly state on completion.
src/Platform/Microsoft.Testing.Platform/OutputDevice/Terminal/TerminalTestReporter.Formatting.csUpdates assembly link formatting to operate on an assembly-run state instance.
src/Platform/Microsoft.Testing.Platform/OutputDevice/Terminal/TerminalTestReporter.csRemoves single-assembly constructor inputs, adds per-assembly dictionary state and richer ArtifactAdded.

Copilot's findings

Comments suppressed due to low confidence (2)

src/Platform/Microsoft.Testing.Platform/OutputDevice/TerminalOutputDevice.DataConsumption.cs:205

  • The TestCompleted argument indentation in the TimeoutTestNodeStateProperty case has an extra leading space compared to the surrounding blocks, which can trigger IDE0055 (formatting) during builds with code-style enforcement.
 case TimeoutTestNodeStateProperty timeoutState:
_terminalTestReporter.TestCompleted(
InProcessExecutionId,
testNodeStateChanged.TestNode.Uid.Value,
testNodeStateChanged.TestNode.DisplayName,
TestOutcome.Timeout,
duration,

src/Platform/Microsoft.Testing.Platform/OutputDevice/TerminalOutputDevice.DataConsumption.cs:223

  • The TestCompleted argument indentation in the CancelledTestNodeStateProperty case is inconsistent (extra leading space), which may trigger IDE0055 formatting diagnostics when code style is enforced in build.
#pragma warning disable CS0618, MTP0001 // Type or member is obsolete
case CancelledTestNodeStateProperty cancelledState:
#pragma warning restore CS0618, MTP0001 // Type or member is obsolete
_terminalTestReporter.TestCompleted(
InProcessExecutionId,
testNodeStateChanged.TestNode.Uid.Value,
testNodeStateChanged.TestNode.DisplayName,
TestOutcome.Canceled,
duration,
  • Files reviewed: 12/12 changed files
  • Comments generated: 5

@Evangelink

This comment has been minimized.

@Evangelink

Copy link
Copy Markdown
MemberAuthor

🔍 Build Failure Analysis

Summary — The build of Microsoft.Testing.Platform.csproj fails due to three distinct analyzer violations introduced in the OutputDevice/Terminal/ area: two SA1623 violations (property XML doc summaries not starting with "Gets") and one IDE0052 violation (a private field that is assigned but never read).


Root cause 1 — SA1623: Property doc summaries missing "Gets" prefix

SA1623 requires that the <summary> of any property with a getter must begin with the word "Gets" (e.g. /// <summary>Gets the ...</summary>). Two new properties were added with prose-style summaries that omit this prefix — both are lifted to errors by TreatWarningsAsErrors in this project.

Affected locations

Fix — prepend Gets to each <summary>:

// TestProgressState.cs:23
- /// <summary>The assembly path or display name as provided by the caller (used for the summary link).</summary>+ /// <summary>Gets the assembly path or display name as provided by the caller (used for the summary link).</summary>
// TerminalTestReporter.cs:67
- /// <summary>Total number of tests across all registered assemblies.</summary>+ /// <summary>Gets the total number of tests across all registered assemblies.</summary>

Root cause 2 — IDE0052: _isRetry field is write-only

[IDE0052]((learn.microsoft.com/redacted) fires when a private member is assigned but never read. _isRetry was introduced in TerminalTestReporter.Lifecycle.cs, assigned in TestExecutionStarted, but consumed nowhere — unlike its sibling _isHelp, which is already read in TestExecutionCompleted to suppress the summary when --help is active.

Affected location

Fix — remove the field and its assignment. If retry-specific behaviour is planned, add the corresponding read-site (e.g. in TestExecutionCompleted) and keep the field; otherwise also remove the bool isRetry parameter from TestExecutionStarted and update callers.

 private bool _isHelp;
- private bool _isRetry;
public void TestExecutionStarted(DateTimeOffset testStartTime, int workerCount, bool isDiscovery, bool isHelp, bool isRetry)
{
_isDiscovery = isDiscovery;
_isHelp = isHelp;
- _isRetry = isRetry;
_testExecutionStartTime = testStartTime;

Build overview
MSBuild18.7.0-preview
Duration154 s
Projects45
Errors10 (3 unique issues, each repeated across multiple TFMs)
Warnings0

All errors originate in Microsoft.Testing.Platform.csproj. The failures of Microsoft.Testing.Extensions.AzureDevOpsReport, Microsoft.Testing.Extensions.CrashDump, and MSTest.TestAdapter are cascades from that single project.

All MSBuild errors (3 unique, 10 total across TFMs)
CodeFileLineMessage
SA1623TestProgressState.cs24The property's documentation summary text should begin with: 'Gets'
SA1623TerminalTestReporter.cs68The property's documentation summary text should begin with: 'Gets'
IDE0052TerminalTestReporter.Lifecycle.cs12Private member _isRetry can be removed as the value assigned to it is never read

🤖 Generated by the Build Failure Analysis workflow · commit 476750cda2a7f588e1e5476448945f10789afff4

🤖 Automated content by GitHub Copilot. Posted via a maintainer's GitHub token, so it appears under their account — the account owner did not write or approve this content personally. Generated by the Build Failure Analysis workflow. · 246.3 AIC · ⌖ 12.8 AIC · ⊞ 46.9K · [◷]( · )

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.

🤖 Automated content by GitHub Copilot. Posted via a maintainer's GitHub token, so it appears under their account — the account owner did not write or approve this content personally. Generated by the Build Failure Analysis workflow. · 246.3 AIC · ⌖ 12.8 AIC · ⊞ 46.9K ·

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.

Note

🤖 Automated review by GitHub Copilot. Posted via a maintainer's GitHub token, so it appears under their account — the account owner did not write or approve this content personally. Generated by the Expert Code Review workflow. To request a follow-up action, reply by tagging @copilot directly.

Review Summary

The multi-assembly generalisation is well-designed: the ConcurrentDictionary approach, the single InProcessExecutionId constant for the in-process host, the assemblies.Count == 1 branch that keeps byte-identical single-assembly output, and the new aggregating test — all read cleanly. The 94 byte-exact existing tests continuing to pass confirms the in-process path is unchanged.

One MAJOR correctness regression was found in the factory pattern, plus two MODERATE dead-API issues and two NITs.


#DimensionVerdictFileLine
2Threading & Concurrency🔴 1 MAJORTerminalTestReporter.Lifecycle.cs26–37
15Code Structure🟡 2 MODERATETerminalTestReporter.Lifecycle.cs12, 23
5Performance & Allocations🔵 1 NITTerminalTestReporter.cs68
8Defensive Coding🔵 1 NITTerminalTestReporter.Lifecycle.cs50

✅ 18/22 dimensions clean.


Findings

  • [MAJOR – Threading]GetOrAdd(key, valueFactory) can invoke the factory more than once for the same key if two threads race (the inner lock serialises the invocations sequentially but does not prevent both from running). Both invocations call AddWorker, occupying two slots; only one TestProgressState is kept in the dictionary, leaking the other's progress slot permanently. Current callers guarantee unique IDs so the race is not reachable today, but the pattern is a correctness regression from the old double-checked locking. Replace with TryGetValue / lock / TryGetValue / _assemblies[key] = ... DCL. See inline comment.

  • [MODERATE – Dead API]instanceId is accepted by AssemblyRunStarted but never forwarded or stored. If keeping it as scaffolding, add a // Reserved for SDK retry logic comment; otherwise remove and re-introduce with its consumer.

  • [MODERATE – Dead Code]_isRetry is assigned in TestExecutionStarted but never read anywhere in the partial class. _isHelp has a consumer; _isRetry does not. Remove or add an explanatory comment.

  • [NIT – Performance]TotalTests property lambda should use static for consistency with all other lambdas in this file: Sum(static a => a.TotalTests).

  • [NIT – Dead Param]exitCode in TestExecutionCompleted is never stored. A short inline comment noting this is deferred to the SDK plug-in PR would be enough.

- GetOrAddAssemblyRun: replace ConcurrentDictionary.GetOrAdd (whose value
factory could run multiple times under contention and orphan worker slots /
bump _counter) with an explicit lock + TryGetValue + add, guaranteeing exactly
one worker per executionId.
- Remove the unused _isRetry field/assignment (IDE0052); keep the isRetry
parameter for SDK API parity.
- Prefix the Assembly and TotalTests doc summaries with 'Gets' (SA1623).
- Normalize the TestCompleted argument indentation in the Failed/Timeout/
Cancelled cases (IDE0055).
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- GetOrAddAssemblyRun: add a lock-free TryGetValue fast path (double-checked
locking) so repeat AssemblyRunStarted calls don't take the lock, matching the
reviewer's suggested pattern.
- TotalTests: make the Sum lambda static to avoid a per-call delegate allocation
on the SDK orchestrator's progress-tick path.
- Document instanceId on AssemblyRunStarted as reserved for the SDK orchestrator
retry-counting follow-up (unused by the in-process host path).
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
CopilotAI review requested due to automatic review settings June 19, 2026 15:24

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: 12/12 changed files
  • Comments generated: 1

TestExecutionCompleted cleared the per-assembly runs but left _artifacts and
WasCancelled intact (pre-existing, but contradicts the documented HotReload
'start fresh' intent on a method this PR reworks): a later session would
re-print the previous session's artifacts or stay stuck in the aborted state.
Now also clear _artifacts and reset WasCancelled after the summary has consumed
them. Adds a regression test exercising two sessions on the same reporter.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@Evangelink

Copy link
Copy Markdown
MemberAuthor

🧪 Test quality grade — PR #9256

ΔTestGradeBandNotes
newTerminalTestReporterTests.
TerminalTestReporter_
WhenReusedAcrossSessions_
DoesNotLeakArtifactsOrCancelledState
B80–893 meaningful assertions cover both leak vectors; body is ~42 lines — consider a session-run helper to trim setup verbosity.

This advisory comment was generated automatically. Grades are heuristic
and informational — they do not block merging. Re-run with
/grade-tests.

🤖 Automated content by GitHub Copilot. Posted via a maintainer's GitHub token, so it appears under their account — the account owner did not write or approve this content personally. Generated by the Grade Tests on PR (on open / sync) workflow. · 464 AIC · ⌖ 13.2 AIC · ⊞ 45.6K · [◷]( · )

@Evangelink
Amaury Levé (Evangelink) merged commit 4545e5d into mainJun 19, 2026
34 of 39 checks passed
@Evangelink
Amaury Levé (Evangelink) deleted the copilot/terminal-reporter-1-to-n branch June 19, 2026 16:06
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.

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

Unify MTP terminal reporter to handle 1..N assemblies - #9256

Merged
Amaury Levé (Evangelink) merged 4 commits into
mainfrom
copilot/terminal-reporter-1-to-n
Jun 19, 2026
Merged

Unify MTP terminal reporter to handle 1..N assemblies#9256
Amaury Levé (Evangelink) merged 4 commits into
mainfrom
copilot/terminal-reporter-1-to-n

Conversation

@Evangelink

Copy link
Copy Markdown
Member

What

Converts the Microsoft.Testing.Platform terminal test reporter from a single-assembly model to a multi-assembly (1..N) model, so a single shared source can render both the in-process MTP host (one assembly) and the dotnet test orchestrator (N child assemblies).

This is the prerequisite called out in the terminal-sharing effort (follow-up to the protocol-sharing PRs #9218 / #9231 and the package PRs #9249 / #9250 / #9253): the SDK currently hard-forks the reporter because the shared MTP copy was single-assembly. With this change the reporter core exposes the multi-assembly API the SDK needs.

How

  • Replace the single TestProgressState + _assembly/_targetFramework/_architecture fields with a ConcurrentDictionary<string, TestProgressState> _assemblies keyed by a caller-provided execution id. The in-process host passes one fixed id; the orchestrator passes one per child assembly.
  • Route TestCompleted / TestInProgress / TestDiscovered / AssemblyRunStarted / AssemblyRunCompleted / ArtifactAdded per execution id.
  • Aggregate the run and discovery summaries across all assemblies. N=1 (in-process) output stays byte-identical via an assemblies.Count == 1 branch that still appends the per-assembly link to the verdict line; for N>1 the verdict line is link-free (per-assembly identity lives in the progress area).
  • Expand TestRunArtifact and ArtifactAdded with assembly / targetFramework / architecture / executionId.
  • Add a TotalTests aggregate and surface isHelp / isRetry on TestExecutionStarted.
  • Adapt the in-process caller (TerminalOutputDevice) to the new API using a single fixed InProcessExecutionId.

Scope / deferred

The SDK-orchestrator-only surface — handshake-failure recap, instanceId-based retry counting, build-error tracking, and exit-code-in-summary — is intentionally not ported here. That logic depends on the SDK's richer TestProgressState retry model and is only exercised by real multi-process runs, so it cannot be verified from the in-process unit tests. It will land with the SDK plug-in PR where it can be driven and tested end-to-end against actual N-assembly runs.

Verification

  • Platform builds clean on net8.0 / net9.0 / netstandard2.0 (0 warnings).
  • Full Microsoft.Testing.Platform.UnitTests suite green: 1197 total, 0 failed — the 94 existing byte-exact terminal tests confirm the in-process UI is unchanged.
  • New TerminalTestReporter_WhenMultipleAssemblies_AggregatesCountsAndOmitsAssemblyLinkOnVerdict test covers the N>1 path (aggregated counts + link-free verdict).
  • The standalone TerminalReporterContract consumer still compiles in isolation (the Share dotnet test wire contract (ObjectFieldIds + Constants) as source #9218-equivalent gate).

The terminal test reporter was single-assembly (in-process MTP host). The
dotnet test orchestrator runs N child assemblies, so the SDK had to hard-fork
the reporter. This converts the reporter core to a multi-assembly model so a
single shared source can render both cases:
- Replace the single TestProgressState + _assembly/_targetFramework/_architecture
fields with a ConcurrentDictionary<string, TestProgressState> keyed by a
caller-provided execution id (in-process passes one fixed id).
- Route TestCompleted/TestInProgress/TestDiscovered/AssemblyRunStarted/
AssemblyRunCompleted/ArtifactAdded per execution id.
- Aggregate run/discovery summaries across all assemblies; keep the N=1
(in-process) output byte-identical via an assemblies.Count == 1 branch that
still appends the per-assembly link to the verdict line.
- Expand TestRunArtifact and ArtifactAdded with assembly/tfm/arch/executionId.
- Add TotalTests aggregate and surface isHelp/isRetry on TestExecutionStarted.
- Adapt the in-process caller (TerminalOutputDevice) to the new API with a
single fixed InProcessExecutionId.
The SDK-orchestrator-only surface (handshake-failure recap, instanceId-based
retry counting, build errors, exit-code-in-summary) is intentionally deferred
to the SDK plug-in PR, where it can be driven and verified against real
multi-assembly runs.
Verified: platform builds clean on net8.0/net9.0/netstandard2.0;
full Microsoft.Testing.Platform.UnitTests suite green (1197, 0 failed),
including a new N>1 test; the standalone TerminalReporterContract consumer
still compiles in isolation.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
CopilotAI review requested due to automatic review settings June 19, 2026 14:54

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

This PR updates the Microsoft.Testing.Platform terminal reporter to support a multi-assembly (1..N) execution model (keyed by execution id), while preserving byte-identical output for the existing single-assembly in-process host.

Changes:

  • Replace single-assembly reporter state with a per-execution-id map and aggregate run/discovery summaries across assemblies.
  • Extend reporter events/artifact tracking to carry assembly identity (assembly/TFM/architecture/executionId) and route events by execution id.
  • Update the in-process TerminalOutputDevice and unit tests to use the new multi-assembly reporter API; add a dedicated multi-assembly aggregation test.
Show a summary per file
FileDescription
test/UnitTests/Microsoft.Testing.Platform.UnitTests/OutputDevice/Terminal/TerminalTestReporterTests.csUpdates tests for new reporter APIs and adds a multi-assembly aggregation test.
src/Platform/Microsoft.Testing.Platform/OutputDevice/TerminalOutputDevice.SessionLifecycle.csAdapts in-process session lifecycle to call multi-assembly reporter methods using a fixed execution id.
src/Platform/Microsoft.Testing.Platform/OutputDevice/TerminalOutputDevice.Initialization.csIntroduces an in-process execution id constant and updates reporter construction for the new API.
src/Platform/Microsoft.Testing.Platform/OutputDevice/TerminalOutputDevice.DataConsumption.csRoutes test state/artifacts into reporter with the in-process execution id and richer artifact metadata.
src/Platform/Microsoft.Testing.Platform/OutputDevice/Terminal/TestRunArtifact.csExpands artifact record to include assembly identity fields.
src/Platform/Microsoft.Testing.Platform/OutputDevice/Terminal/TestProgressState.csStores the full assembly path/display name for later linking and summary formatting.
src/Platform/Microsoft.Testing.Platform/OutputDevice/Terminal/TerminalTestReporter.TestCompletion.csKeys TestCompleted handling by execution id and looks up per-assembly state.
src/Platform/Microsoft.Testing.Platform/OutputDevice/Terminal/TerminalTestReporter.Summary.csAggregates counts across assemblies and removes verdict-line assembly link when N>1.
src/Platform/Microsoft.Testing.Platform/OutputDevice/Terminal/TerminalTestReporter.Messaging.csKeys TestInProgress by execution id for multi-assembly progress rendering.
src/Platform/Microsoft.Testing.Platform/OutputDevice/Terminal/TerminalTestReporter.Lifecycle.csAdds multi-assembly lifecycle APIs and clears per-assembly state on completion.
src/Platform/Microsoft.Testing.Platform/OutputDevice/Terminal/TerminalTestReporter.Formatting.csUpdates assembly link formatting to operate on an assembly-run state instance.
src/Platform/Microsoft.Testing.Platform/OutputDevice/Terminal/TerminalTestReporter.csRemoves single-assembly constructor inputs, adds per-assembly dictionary state and richer ArtifactAdded.

Copilot's findings

Comments suppressed due to low confidence (2)

src/Platform/Microsoft.Testing.Platform/OutputDevice/TerminalOutputDevice.DataConsumption.cs:205

  • The TestCompleted argument indentation in the TimeoutTestNodeStateProperty case has an extra leading space compared to the surrounding blocks, which can trigger IDE0055 (formatting) during builds with code-style enforcement.
 case TimeoutTestNodeStateProperty timeoutState:
_terminalTestReporter.TestCompleted(
InProcessExecutionId,
testNodeStateChanged.TestNode.Uid.Value,
testNodeStateChanged.TestNode.DisplayName,
TestOutcome.Timeout,
duration,

src/Platform/Microsoft.Testing.Platform/OutputDevice/TerminalOutputDevice.DataConsumption.cs:223

  • The TestCompleted argument indentation in the CancelledTestNodeStateProperty case is inconsistent (extra leading space), which may trigger IDE0055 formatting diagnostics when code style is enforced in build.
#pragma warning disable CS0618, MTP0001 // Type or member is obsolete
case CancelledTestNodeStateProperty cancelledState:
#pragma warning restore CS0618, MTP0001 // Type or member is obsolete
_terminalTestReporter.TestCompleted(
InProcessExecutionId,
testNodeStateChanged.TestNode.Uid.Value,
testNodeStateChanged.TestNode.DisplayName,
TestOutcome.Canceled,
duration,
  • Files reviewed: 12/12 changed files
  • Comments generated: 5

@Evangelink

This comment has been minimized.

@Evangelink

Copy link
Copy Markdown
MemberAuthor

🔍 Build Failure Analysis

Summary — The build of Microsoft.Testing.Platform.csproj fails due to three distinct analyzer violations introduced in the OutputDevice/Terminal/ area: two SA1623 violations (property XML doc summaries not starting with "Gets") and one IDE0052 violation (a private field that is assigned but never read).


Root cause 1 — SA1623: Property doc summaries missing "Gets" prefix

SA1623 requires that the <summary> of any property with a getter must begin with the word "Gets" (e.g. /// <summary>Gets the ...</summary>). Two new properties were added with prose-style summaries that omit this prefix — both are lifted to errors by TreatWarningsAsErrors in this project.

Affected locations

Fix — prepend Gets to each <summary>:

// TestProgressState.cs:23
- /// <summary>The assembly path or display name as provided by the caller (used for the summary link).</summary>+ /// <summary>Gets the assembly path or display name as provided by the caller (used for the summary link).</summary>
// TerminalTestReporter.cs:67
- /// <summary>Total number of tests across all registered assemblies.</summary>+ /// <summary>Gets the total number of tests across all registered assemblies.</summary>

Root cause 2 — IDE0052: _isRetry field is write-only

[IDE0052]((learn.microsoft.com/redacted) fires when a private member is assigned but never read. _isRetry was introduced in TerminalTestReporter.Lifecycle.cs, assigned in TestExecutionStarted, but consumed nowhere — unlike its sibling _isHelp, which is already read in TestExecutionCompleted to suppress the summary when --help is active.

Affected location

Fix — remove the field and its assignment. If retry-specific behaviour is planned, add the corresponding read-site (e.g. in TestExecutionCompleted) and keep the field; otherwise also remove the bool isRetry parameter from TestExecutionStarted and update callers.

 private bool _isHelp;
- private bool _isRetry;
public void TestExecutionStarted(DateTimeOffset testStartTime, int workerCount, bool isDiscovery, bool isHelp, bool isRetry)
{
_isDiscovery = isDiscovery;
_isHelp = isHelp;
- _isRetry = isRetry;
_testExecutionStartTime = testStartTime;

Build overview
MSBuild18.7.0-preview
Duration154 s
Projects45
Errors10 (3 unique issues, each repeated across multiple TFMs)
Warnings0

All errors originate in Microsoft.Testing.Platform.csproj. The failures of Microsoft.Testing.Extensions.AzureDevOpsReport, Microsoft.Testing.Extensions.CrashDump, and MSTest.TestAdapter are cascades from that single project.

All MSBuild errors (3 unique, 10 total across TFMs)
CodeFileLineMessage
SA1623TestProgressState.cs24The property's documentation summary text should begin with: 'Gets'
SA1623TerminalTestReporter.cs68The property's documentation summary text should begin with: 'Gets'
IDE0052TerminalTestReporter.Lifecycle.cs12Private member _isRetry can be removed as the value assigned to it is never read

🤖 Generated by the Build Failure Analysis workflow · commit 476750cda2a7f588e1e5476448945f10789afff4

🤖 Automated content by GitHub Copilot. Posted via a maintainer's GitHub token, so it appears under their account — the account owner did not write or approve this content personally. Generated by the Build Failure Analysis workflow. · 246.3 AIC · ⌖ 12.8 AIC · ⊞ 46.9K · [◷]( · )

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.

🤖 Automated content by GitHub Copilot. Posted via a maintainer's GitHub token, so it appears under their account — the account owner did not write or approve this content personally. Generated by the Build Failure Analysis workflow. · 246.3 AIC · ⌖ 12.8 AIC · ⊞ 46.9K ·

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.

Note

🤖 Automated review by GitHub Copilot. Posted via a maintainer's GitHub token, so it appears under their account — the account owner did not write or approve this content personally. Generated by the Expert Code Review workflow. To request a follow-up action, reply by tagging @copilot directly.

Review Summary

The multi-assembly generalisation is well-designed: the ConcurrentDictionary approach, the single InProcessExecutionId constant for the in-process host, the assemblies.Count == 1 branch that keeps byte-identical single-assembly output, and the new aggregating test — all read cleanly. The 94 byte-exact existing tests continuing to pass confirms the in-process path is unchanged.

One MAJOR correctness regression was found in the factory pattern, plus two MODERATE dead-API issues and two NITs.


#DimensionVerdictFileLine
2Threading & Concurrency🔴 1 MAJORTerminalTestReporter.Lifecycle.cs26–37
15Code Structure🟡 2 MODERATETerminalTestReporter.Lifecycle.cs12, 23
5Performance & Allocations🔵 1 NITTerminalTestReporter.cs68
8Defensive Coding🔵 1 NITTerminalTestReporter.Lifecycle.cs50

✅ 18/22 dimensions clean.


Findings

  • [MAJOR – Threading]GetOrAdd(key, valueFactory) can invoke the factory more than once for the same key if two threads race (the inner lock serialises the invocations sequentially but does not prevent both from running). Both invocations call AddWorker, occupying two slots; only one TestProgressState is kept in the dictionary, leaking the other's progress slot permanently. Current callers guarantee unique IDs so the race is not reachable today, but the pattern is a correctness regression from the old double-checked locking. Replace with TryGetValue / lock / TryGetValue / _assemblies[key] = ... DCL. See inline comment.

  • [MODERATE – Dead API]instanceId is accepted by AssemblyRunStarted but never forwarded or stored. If keeping it as scaffolding, add a // Reserved for SDK retry logic comment; otherwise remove and re-introduce with its consumer.

  • [MODERATE – Dead Code]_isRetry is assigned in TestExecutionStarted but never read anywhere in the partial class. _isHelp has a consumer; _isRetry does not. Remove or add an explanatory comment.

  • [NIT – Performance]TotalTests property lambda should use static for consistency with all other lambdas in this file: Sum(static a => a.TotalTests).

  • [NIT – Dead Param]exitCode in TestExecutionCompleted is never stored. A short inline comment noting this is deferred to the SDK plug-in PR would be enough.

- GetOrAddAssemblyRun: replace ConcurrentDictionary.GetOrAdd (whose value
factory could run multiple times under contention and orphan worker slots /
bump _counter) with an explicit lock + TryGetValue + add, guaranteeing exactly
one worker per executionId.
- Remove the unused _isRetry field/assignment (IDE0052); keep the isRetry
parameter for SDK API parity.
- Prefix the Assembly and TotalTests doc summaries with 'Gets' (SA1623).
- Normalize the TestCompleted argument indentation in the Failed/Timeout/
Cancelled cases (IDE0055).
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- GetOrAddAssemblyRun: add a lock-free TryGetValue fast path (double-checked
locking) so repeat AssemblyRunStarted calls don't take the lock, matching the
reviewer's suggested pattern.
- TotalTests: make the Sum lambda static to avoid a per-call delegate allocation
on the SDK orchestrator's progress-tick path.
- Document instanceId on AssemblyRunStarted as reserved for the SDK orchestrator
retry-counting follow-up (unused by the in-process host path).
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
CopilotAI review requested due to automatic review settings June 19, 2026 15:24

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: 12/12 changed files
  • Comments generated: 1

TestExecutionCompleted cleared the per-assembly runs but left _artifacts and
WasCancelled intact (pre-existing, but contradicts the documented HotReload
'start fresh' intent on a method this PR reworks): a later session would
re-print the previous session's artifacts or stay stuck in the aborted state.
Now also clear _artifacts and reset WasCancelled after the summary has consumed
them. Adds a regression test exercising two sessions on the same reporter.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@Evangelink

Copy link
Copy Markdown
MemberAuthor

🧪 Test quality grade — PR #9256

ΔTestGradeBandNotes
newTerminalTestReporterTests.
TerminalTestReporter_
WhenReusedAcrossSessions_
DoesNotLeakArtifactsOrCancelledState
B80–893 meaningful assertions cover both leak vectors; body is ~42 lines — consider a session-run helper to trim setup verbosity.

This advisory comment was generated automatically. Grades are heuristic
and informational — they do not block merging. Re-run with
/grade-tests.

🤖 Automated content by GitHub Copilot. Posted via a maintainer's GitHub token, so it appears under their account — the account owner did not write or approve this content personally. Generated by the Grade Tests on PR (on open / sync) workflow. · 464 AIC · ⌖ 13.2 AIC · ⊞ 45.6K · [◷]( · )

@Evangelink
Amaury Levé (Evangelink) merged commit 4545e5d into mainJun 19, 2026
34 of 39 checks passed
@Evangelink
Amaury Levé (Evangelink) deleted the copilot/terminal-reporter-1-to-n branch June 19, 2026 16:06
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.

2 participants

@Evangelink