Uh oh!
There was an error while loading. Please reload this page.
Add comprehensive console tests for the orchestrator reporter surface (5/n) - #9268
Conversation
First slice of unifying the shared MTP terminal reporter with the dotnet/sdk orchestrator fork, validated by porting the SDK's own reporter test as the expected-UI oracle. Adds the orchestrator handshake surface to the shared reporter (additive; the in-process host never reports handshake failures, so output stays byte-identical): - HandshakeFailure(assemblyPath, tfm, exitCode, output, error, reportEvenWhenHelp) + HasHandshakeFailure + end-of-run AppendHandshakeFailureRecap. - Rich AssemblyRunCompleted(executionId, exitCode, outputData, errorData) overload that surfaces an unknown execution id as a handshake failure (no throw) and prints the executable summary (exit code + stdout/stderr) on non-zero exit. - TerminalTestReporter(IConsole, options) ctor overload for orchestrator callers that drive cancellation out-of-band via StartCancelling. - TestProgressState.Success; string-based AppendAssemblyLinkTargetFrameworkAndArchitecture overload; AppendExecutableSummary; new TerminalResources HandshakeFailuresHeader/ExitCode (resx + xlf regenerated via UpdateXlf, hand-maintained !IS_CORE_MTP accessors updated). Ports the SDK's AssemblyRunCompleted_WhenExecutionIdUnknown test (dotnet/sdk#51608) to MSTest, asserting both HasHandshakeFailure and the rendered failure context. Verified: platform clean net8.0/net9.0/netstandard2.0 (0 warnings incl. IDE/SA); full Microsoft.Testing.Platform.UnitTests suite green (1199, 0 failed); standalone TerminalReporterContract consumer still compiles. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Second slice of unifying the shared MTP terminal reporter with the dotnet/sdk orchestrator fork, validated by porting the SDK's per-assembly-counts test. Adds the orchestrator per-assembly summary rendering (additive; gated on the new ShowAssembly options which default off, so the in-process host is unaffected and its output stays byte-identical): - ShowAssembly + ShowAssemblyStartAndComplete options. - Rich AssemblyRunCompleted now prints the per-assembly summary line (link + result + compact counts + duration) when ShowAssembly && ShowAssemblyStartAndComplete. - AppendAssemblySummary/AppendAssemblyResult/AppendAssemblyTestCounts/AppendGlyphCount porting the compact '[+P/xF/?S]' (ASCII) / '[checkP/xF/downS]' (ANSI) counts block, with the '/rN' retry segment wired to TestProgressState.RetriedFailedTests. - Orchestrator TestCompleted overload (assembly, tfm, arch, executionId, instanceId, ...) that updates per-assembly counts (delegates to the shared per-execution-id path; instanceId-based retry attribution arrives in the retry slice). - New TerminalResources.FailedWithErrors (resx + xlf + !IS_CORE_MTP accessor). Ports the SDK's AssemblyRunCompleted_WithShowAssemblyStartAndComplete_PrintsPerAssemblyCounts test (dotnet/sdk#52128) to MSTest, asserting the rendered '[+3/x0/?1]' counts block. Stacked on #9263. Verified: platform clean net8.0/net9.0/netstandard2.0 (0 warnings); full unit suite green (1200, 0 failed); standalone contract consumer compiles. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…3/n) Third slice of unifying the shared MTP terminal reporter with the dotnet/sdk orchestrator fork. Adds the per-assembly listing to the FINAL run summary: when ShowAssembly is set and more than one assembly ran, each assembly is listed under the run-level verdict with its own result + compact counts. Additive — the in-process host leaves ShowAssembly off, so its summary stays byte-identical. Ports the SDK's TestExecutionCompleted_WithMultipleAssemblies_PrintsPerAssemblyCountsInSummary test (dotnet/sdk#52128) to MSTest, asserting the rendered '[+2/x1/?0]' and '[+5/x0/?2]' per-assembly count blocks. Stacked on #9265. Verified: platform clean net8.0/net9.0/netstandard2.0 (0 warnings); full unit suite green (1201, 0 failed). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Fourth slice of unifying the shared MTP terminal reporter with the dotnet/sdk
orchestrator fork: adopt the instanceId-based retry counting model so retried
assemblies render the '/r{N}' segment and report the latest attempt's results.
- TestProgressState now derives PassedTests/FailedTests/SkippedTests from per-test
reported results keyed by (testNodeUid, attempt). TotalTests is computed. A new
instance id (registered via NotifyHandshake on AssemblyRunStarted) is a retry
attempt that supersedes the previous attempt's tally and bumps RetriedFailedTests.
Adds ReportPassingTest/ReportSkippedTest/ReportFailedTest, NotifyHandshake, TryCount.
- TestCompleted now threads the instance id (the in-process host uses its single
fixed execution id as the instance id; the orchestrator overload passes the real
one) and routes through the Report* methods.
- AssemblyRunStarted registers the attempt via NotifyHandshake.
- Updated the SilenceDrivenHeartbeatRenderer test helper to seed counts via Report*.
Ports the SDK's AssemblyRunCompleted_WhenTestsWereRetried_ShowsRetriedCount test,
asserting the rendered '[+1/x0/?0/r1]' block.
Stacked on #9266. Verified: platform clean net8.0/net9.0/netstandard2.0 (0 warnings);
full unit suite green (1202, 0 failed) — the ~89 byte-exact in-process tests confirm
the single-attempt counts are unchanged; contract consumer compiles.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>… (5/n) Pure test additions (no production change) covering edge cases of the shared terminal reporter's orchestrator surface introduced in slices 1-4: - AssemblyRunCompleted prints/omits the executable summary on non-zero/zero exit. - Per-assembly result text: 'failed with N error(s)', 'Zero tests ran', and the '[+P/xF/?S]' counts for failed and empty assemblies. - Handshake failures: HasHandshakeFailure flips, the end-of-run recap header and captured stdout/stderr are surfaced. - NotifyHandshake retry attempts: re-registering the same instance id does not count as a new attempt; the latest attempt's counts win. Stacked on #9267. Verified: full unit suite green (1208, 0 failed). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Amaury Levé (Evangelink)
left a comment
There was a problem hiding this comment.
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.
Expert Review — PR #9268 "Add comprehensive console tests for the orchestrator reporter surface (5/n)"
Pure test additions verified against the production code in TerminalTestReporter.Lifecycle.cs, .Handshake.cs, .Formatting.cs, and TestProgressState.cs.
22-Dimension Verdict Table
| # | Dimension | Severity | Status | Notes |
|---|---|---|---|---|
| 1 | Algorithmic Correctness | MAJOR | ✅ PASS | All six new tests trace correctly through the production code paths they invoke. |
| 2 | Threading & Concurrency | BLOCKING | ✅ N/A | Test-only additions; no new shared state introduced. |
| 3 | Security & IPC Contract Safety | BLOCKING | ✅ N/A | No production changes. |
| 4 | Public API & Binary Compatibility | BLOCKING | ✅ N/A | No new public surface; CreateOrchestratorReporter is private static. |
| 5 | Performance & Allocations | MAJOR | ✅ N/A | Test code only. |
| 6 | Cross-TFM Compatibility | MAJOR | ✅ PASS | Consistently uses RuntimeInformation.IsOSPlatform for platform-specific paths, matching the pattern in every other test in this file. |
| 7 | Resource & IDisposable Management | MAJOR | ✅ N/A | All objects are in-memory; nothing disposable is created. |
| 8 | Defensive Coding at Boundaries | MAJOR | ✅ N/A | Test code. |
| 9 | Localization & Resources | MAJOR | ✅ PASS | All resource strings accessed via TerminalResources.* constants — no hardcoded user-facing strings. |
| 10 | Test Isolation | MAJOR | ✅ PASS | Each test constructs its own TerminalTestReporter and StringBuilderConsole; no static mutable state shared across tests. |
| 11 | Assertion Quality | MAJOR | ❌ FLAG | Two defects (see inline comments): (a) assemblyA/assemblyB are declared but never used in TestExecutionCompleted_WhenHandshakeFailures_... — dead code indicating the test may be incomplete; (b) showAssemblyStartAndComplete: false in WhenKnownAssemblySucceeds_DoesNotPrintExecutableSummary silences all output, making the DoesNotContain assertion vacuously true. |
| 12 | Flakiness Patterns | BLOCKING | ✅ PASS | No Thread.Sleep, no hard-coded ports, no duration-string regex. |
| 13 | Test Completeness & Coverage | MAJOR | Related to (a) above: the handshake-failure test does not verify which assembly the failure is attributed to (assembly path in the recap). This is a coverage gap if that identity is observable; if it isn't (the production code uses string.Empty for the assembly path on unknown IDs), the unused variables confirm the gap rather than fill it. | |
| 14 | Data-Driven Test Coverage | MODERATE | ✅ N/A | No data rows in the new tests. |
| 15 | Code Structure & Simplification | MODERATE | ✅ PASS | Follows existing conventions; CreateOrchestratorReporter helper nicely DRYs up reporter construction. |
| 16 | Naming & Conventions | NIT | AssemblyRunStarted_WhenRetried_IncrementsTryCountPerInstanceId — the name implies TryCount is being asserted, but the assertion checks test-result counts only. See inline comment. | |
| 17 | Documentation Accuracy | NIT | The comment "retried once (2 attempts)" in the same test will mislead readers into expecting /r1 in the counts block; the absence of /r is correct but unexplained. | |
| 18 | Analyzer & Code Fix Quality | MAJOR | ✅ N/A | Not in src/Analyzers/. |
| 19 | IPC Wire Compatibility | MAJOR | ✅ N/A | No serialization/protocol changes. |
| 20 | Build Infrastructure & Dependencies | MODERATE | ✅ N/A | No build changes. |
| 21 | Scope & PR Discipline | MODERATE | ✅ PASS | Pure test additions, appropriately scoped for a stacked PR. CultureInfo.InvariantCulture used correctly with string.Format in the FailedWithErrors assertion. |
| 22 | PowerShell Scripting Hygiene | MAJOR | ✅ N/A | No PowerShell changes. |
Summary
Three inline comments filed:
- [Lines 1424–1425]
assemblyA/assemblyB— dead variables in the handshake-failure test; remove or wire up to an assertion. - [Line 1367]
showAssemblyStartAndComplete: falsemakes theDoesNotContainassertion vacuous; change to the default (true) to verify the invariant against real output. - [Lines 1455–1461] Test name and inline comment don't match the assertion; the name implies
TryCountverification but only test-result counts are checked.
Items 1 and 2 are the grounds for requesting changes — they represent either dead code (issue 1) or a trivially-passing assertion that does not exercise the intended invariant (issue 2). Item 3 is a NIT but worth fixing before merge to avoid future reader confusion.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
- Remove the unused assemblyA/assemblyB locals in the handshake recap test (the unregistered-completion path stores assemblyPath: string.Empty, so they are unobservable). - Make DoesNotPrintExecutableSummary non-vacuous: keep ShowAssemblyStartAndComplete on so the per-assembly summary IS printed, then assert it is present while the executable-summary block is suppressed on success. - Rename AssemblyRunStarted_WhenRetried_IncrementsTryCountPerInstanceId to AssemblyRunStarted_AfterRetry_RendersLatestAttemptCounts and correct the comment, since the test asserts the latest-attempt counts block, not TryCount or a /r segment. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…l-more-console-tests # Conflicts: # src/Platform/Microsoft.Testing.Platform/OutputDevice/Terminal/TerminalTestReporter.Handshake.cs # src/Platform/Microsoft.Testing.Platform/OutputDevice/Terminal/TerminalTestReporter.Lifecycle.cs # src/Platform/Microsoft.Testing.Platform/OutputDevice/Terminal/TerminalTestReporter.TestCompletion.cs # src/Platform/Microsoft.Testing.Platform/OutputDevice/Terminal/TerminalTestReporterOptions.cs # src/Platform/Microsoft.Testing.Platform/OutputDevice/Terminal/TestProgressState.cs # test/UnitTests/Microsoft.Testing.Platform.UnitTests/OutputDevice/Terminal/TerminalTestReporterTests.cs
There was a problem hiding this comment.
Pull request overview
This PR (slice 5/n) adds unit tests to harden the Microsoft.Testing.Platform shared terminal reporter’s orchestrator surface (no production changes), continuing the stacked terminal-reporter parity work with dotnet/sdk.
Changes:
- Added tests validating when
AssemblyRunCompletedprints (non-zero exit) vs omits (zero exit) the executable summary (exit code + captured stdout/stderr). - Added tests covering handshake-failure recap rendering at end-of-run and ensuring
HasHandshakeFailurebehavior is exercised. - Added a test ensuring retry handshakes and subsequent completion render the latest attempt’s per-assembly counts.
Show a summary per file
| File | Description |
|---|---|
| test/UnitTests/Microsoft.Testing.Platform.UnitTests/OutputDevice/Terminal/TerminalTestReporterTests.cs | Adds orchestrator-focused tests for executable summary suppression/printing, handshake failure recap output, and retry/latest-attempt count rendering. |
Copilot's findings
- Files reviewed: 1/1 changed files
- Comments generated: 1
| // The end-of-run recap header is printed and the captured failure output is surfaced. | ||
| Assert.Contains(TerminalResources.HandshakeFailuresHeader, output); | ||
| Assert.Contains("A failed", output); | ||
| Assert.Contains("B failed", output); | ||
| } |
Amaury Levé (Evangelink)
commented
Jun 20, 2026
🧪 Test quality grade — PR #9268
This advisory comment was generated automatically. Grades are heuristic
|
Uh oh!
There was an error while loading. Please reload this page.
Amaury Levé (Evangelink)
commented
Jun 20, 2026
🔴 Build Failure AnalysisRoot cause: The build failed with 9 errors (4 unique × 2 TFMs) all in one file:
Why only these lines? The Fix: Replace
|
Amaury Levé (Evangelink)
left a comment
There was a problem hiding this comment.
🤖 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. · 197.5 AIC · ⌖ 12.8 AIC · ⊞ 46.9K · ◷
| public void AssemblyRunCompleted_WhenKnownAssemblyFails_PrintsExecutableSummary() | ||
| { | ||
| var stringBuilderConsole = new StringBuilderConsole(); | ||
| var terminalReporter = CreateOrchestratorReporter(stringBuilderConsole); |
There was a problem hiding this comment.
IDE0008 build error — the repo enforces explicit types when the type is not apparent from the initializer. CreateOrchestratorReporter is a method call, so var is not permitted here.
| varterminalReporter=CreateOrchestratorReporter(stringBuilderConsole); | |
| TerminalTestReporterterminalReporter=CreateOrchestratorReporter(stringBuilderConsole); |
| // Keep the default ShowAssemblyStartAndComplete: true so the reporter DOES print the per-assembly summary | ||
| // line. Otherwise a zero-exit run writes nothing at all and the DoesNotContain assertion below passes | ||
| // vacuously instead of verifying that the executable-summary block is specifically suppressed on success. | ||
| var terminalReporter = CreateOrchestratorReporter(stringBuilderConsole); |
There was a problem hiding this comment.
IDE0008 build error — var not permitted when the type is not evident from the right-hand side.
| varterminalReporter=CreateOrchestratorReporter(stringBuilderConsole); | |
| TerminalTestReporterterminalReporter=CreateOrchestratorReporter(stringBuilderConsole); |
| public void AssemblyRunStarted_AfterRetry_RendersLatestAttemptCounts() | ||
| { | ||
| var stringBuilderConsole = new StringBuilderConsole(); | ||
| var terminalReporter = CreateOrchestratorReporter(stringBuilderConsole); |
There was a problem hiding this comment.
IDE0008 build error — var not permitted when the type is not evident from the right-hand side.
| varterminalReporter=CreateOrchestratorReporter(stringBuilderConsole); | |
| TerminalTestReporterterminalReporter=CreateOrchestratorReporter(stringBuilderConsole); |
| public void TestExecutionCompleted_WhenHandshakeFailures_PrintsRecapAndFailsRun() | ||
| { | ||
| var stringBuilderConsole = new StringBuilderConsole(); | ||
| var terminalReporter = CreateOrchestratorReporter(stringBuilderConsole, showAssemblyStartAndComplete: false); |
There was a problem hiding this comment.
IDE0008 build error — var not permitted when the type is not evident from the right-hand side.
| varterminalReporter=CreateOrchestratorReporter(stringBuilderConsole,showAssemblyStartAndComplete:false); | |
| TerminalTestReporterterminalReporter=CreateOrchestratorReporter(stringBuilderConsole,showAssemblyStartAndComplete:false); |
What
Slice 5/n: pure test additions (no production change) hardening the shared terminal reporter's orchestrator surface from slices 1-4. Stacked on #9267.
Covers:
AssemblyRunCompletedprints / omits the executable summary on non-zero / zero exit.failed with N error(s),Zero tests ran, and the[+P/xF/?S]counts for failed and empty assemblies.HasHandshakeFailureflips, the end-of-run recap header + captured stdout/stderr are surfaced.NotifyHandshakeretry attempts: re-registering the same instance id is not a new attempt; latest attempt's counts win.Verification
Full
Microsoft.Testing.Platform.UnitTestssuite green: 1208, 0 failed.