Uh oh!
There was an error while loading. Please reload this page.
Share terminal per-assembly counts + port SDK oracle test (2/n) - #9265
Merged
Amaury Levé (Evangelink) merged 7 commits intoJun 20, 2026
Merged
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>
Amaury Levé (Evangelink)
commented
Jun 19, 2026
MemberAuthor
🧪 Test quality grade — PR #9265
This advisory comment was generated automatically. Grades are heuristic
|
Amaury Levé (Evangelink)
left a comment
MemberAuthor
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.
Review Summary
Clean slice-by-slice implementation. The additive gating (ShowAssembly defaults off) correctly keeps the in-process host byte-identical. No blocking issues found. Two minor items and one informational note are left as inline comments.
Key correctness observations
TestProgressStateisinternal sealed—RetriedFailedTestswithpublic int/internal setdoes not widen the public API surface. NoPublicAPI.Unshipped.txtentry needed. ✅AssemblyRunCompletedordering —assemblyRun.Successis assigned beforeAppendAssemblySummaryis called, soAppendAssemblyResultalways sees the final success flag. ✅- Impossible switch arm —
(FailedTests > 0, TotalTests == 0) = (true, true)can never occur because everyFailedTests++is coupled withTotalTests++in theTestCompletedswitch. The(true, _)arm safely absorbs it. ✅ exceptions ?? []— C# 12 collection-expression syntax; compiles correctly against all TFMs (compiler generatesArray.Empty<T>()for older targets). ✅- XLF hygiene — PR description confirms files were regenerated via
UpdateXlf, not hand-edited. Consistent with the repo's localization guidelines. ✅ - Failure + ShowAssemblyStartAndComplete — On a non-zero exit code, both
AppendAssemblySummary(counts line) andAppendExecutableSummary(full stdout/stderr) are printed. This is intentional and complementary (compact summary + verbose diagnostics). ✅
Verdict Table
| # | Dimension | Status |
|---|---|---|
| 1 | Algorithmic Correctness | ✅ Pass |
| 2 | Threading & Concurrency | ✅ Pass |
| 3 | Security & IPC Contract Safety | ✅ Pass |
| 4 | Public API & Binary Compatibility | ✅ Pass — all changed types are internal sealed; no PublicAPI.Unshipped.txt change needed |
| 5 | Performance & Allocations | ✅ Pass |
| 6 | Cross-TFM Compatibility | ✅ Pass |
| 7 | Resource & IDisposable Management | ✅ Pass |
| 8 | Defensive Coding at Boundaries | ✅ Pass |
| 9 | Localization & Resources | ✅ Pass — FailedWithErrors added to .resx; XLF regenerated via UpdateXlf; hand-maintained TerminalResources.cs accessor present |
| 10 | Test Isolation | ✅ Pass |
| 11 | Assertion Quality | ✅ Pass — Assert.Contains is the correct MSTest assertion here |
| 12 | Naming & Style | ✅ Pass |
| 13 | XML Documentation | RetriedFailedTests summary says "Gets or sets" but the setter is internal (see inline) |
| 14 | Error Handling | ✅ Pass |
| 15 | Architecture & Design | i️ Informational — four parameters in the orchestrator TestCompleted overload are intentionally dropped; documented but worth a call-site comment (see inline) |
| 16 | MSBuild & Project File Quality | N/A |
| 17 | Logging Quality | N/A |
| 18 | Nullability Annotations | ✅ Pass |
| 19 | Test Coverage | AppendAssemblyResult's three failure branches and the /rN retry-count path have no test coverage (see inline) |
| 20 | Dependency Management | ✅ Pass |
| 21 | Telemetry | N/A |
| 22 | PowerShell & Script Quality | N/A |
Overall: COMMENT — no merge-blocking issues. The test-coverage gap (Dimension 19) is the most substantive finding; addressing it in the next slice or as a follow-up is recommended.
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.
- Add tests for the three red AppendAssemblyResult branches (failed-with-errors,
zero-tests-ran, and process-failed-but-tests-passed) of the per-assembly summary line.
(The /r retried-count segment is exercised in the retry-counting slice, where
RetriedFailedTests is actually populated.)
- Fix the RetriedFailedTests XML doc ('Gets' - the setter is internal).
- Add a code comment at the orchestrator TestCompleted delegation noting the four
parameters reserved for the retry-attribution follow-up are intentionally not forwarded.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>…l-per-assembly-counts # Conflicts: # src/Platform/Microsoft.Testing.Platform/OutputDevice/Terminal/TerminalTestReporter.Handshake.cs # src/Platform/Microsoft.Testing.Platform/OutputDevice/Terminal/TerminalTestReporter.Lifecycle.cs # test/UnitTests/Microsoft.Testing.Platform.UnitTests/OutputDevice/Terminal/TerminalTestReporterTests.cs
Remove the XML doc summary on TestProgressState.RetriedFailedTests: its 'internal set' makes it read-only to any out-of-assembly consumer, and the sibling internal-set counters (SkippedTests/TotalTests) carry no doc. (Keeping a 'Gets the number...' summary would also violate SA1623, which requires 'Gets or sets' for a property with a setter.) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR is slice 2/n of aligning testfx’s shared Microsoft.Testing.Platform terminal reporter with the dotnet/sdk orchestrator fork by (1) porting the SDK’s per-assembly counts oracle test and (2) implementing per-assembly completion summary rendering gated behind new ShowAssembly* options (default off).
Changes:
- Added per-assembly completion summary line rendering (link + result + compact
[+P/xF/?S]counts + duration) behindShowAssembly && ShowAssemblyStartAndComplete. - Added an orchestrator-specific
TestCompleted(...)overload plus state required for future retry attribution (RetriedFailedTests). - Added
TerminalResources.FailedWithErrorsacross.resx+ regenerated.xlffiles and added new unit tests validating the new output.
Show a summary per file
| File | Description |
|---|---|
| test/UnitTests/Microsoft.Testing.Platform.UnitTests/OutputDevice/Terminal/TerminalTestReporterTests.cs | Ports the SDK oracle test for per-assembly counts and adds additional branch-coverage tests for per-assembly result strings. |
| src/Platform/Microsoft.Testing.Platform/OutputDevice/Terminal/TestProgressState.cs | Adds RetriedFailedTests state (used for future /r{N} rendering). |
| src/Platform/Microsoft.Testing.Platform/OutputDevice/Terminal/TerminalTestReporterOptions.cs | Introduces ShowAssembly and ShowAssemblyStartAndComplete flags to gate orchestrator-only behavior. |
| src/Platform/Microsoft.Testing.Platform/OutputDevice/Terminal/TerminalTestReporter.TestCompletion.cs | Adds orchestrator TestCompleted(...) overload (currently delegates to execution-id path). |
| src/Platform/Microsoft.Testing.Platform/OutputDevice/Terminal/TerminalTestReporter.Lifecycle.cs | Emits per-assembly completion summary line when the new options are enabled. |
| src/Platform/Microsoft.Testing.Platform/OutputDevice/Terminal/TerminalTestReporter.Formatting.cs | Implements per-assembly summary line formatting, including compact counts block and result text selection. |
| src/Platform/Microsoft.Testing.Platform/OutputDevice/Terminal/TerminalResources.resx | Adds FailedWithErrors resource string. |
| src/Platform/Microsoft.Testing.Platform/OutputDevice/Terminal/TerminalResources.cs | Adds accessor for FailedWithErrors. |
| src/Platform/Microsoft.Testing.Platform/OutputDevice/Terminal/xlf/TerminalResources.cs.xlf | Regenerated localization entry for FailedWithErrors. |
| src/Platform/Microsoft.Testing.Platform/OutputDevice/Terminal/xlf/TerminalResources.de.xlf | Regenerated localization entry for FailedWithErrors. |
| src/Platform/Microsoft.Testing.Platform/OutputDevice/Terminal/xlf/TerminalResources.es.xlf | Regenerated localization entry for FailedWithErrors. |
| src/Platform/Microsoft.Testing.Platform/OutputDevice/Terminal/xlf/TerminalResources.fr.xlf | Regenerated localization entry for FailedWithErrors. |
| src/Platform/Microsoft.Testing.Platform/OutputDevice/Terminal/xlf/TerminalResources.it.xlf | Regenerated localization entry for FailedWithErrors. |
| src/Platform/Microsoft.Testing.Platform/OutputDevice/Terminal/xlf/TerminalResources.ja.xlf | Regenerated localization entry for FailedWithErrors. |
| src/Platform/Microsoft.Testing.Platform/OutputDevice/Terminal/xlf/TerminalResources.ko.xlf | Regenerated localization entry for FailedWithErrors. |
| src/Platform/Microsoft.Testing.Platform/OutputDevice/Terminal/xlf/TerminalResources.pl.xlf | Regenerated localization entry for FailedWithErrors. |
| src/Platform/Microsoft.Testing.Platform/OutputDevice/Terminal/xlf/TerminalResources.pt-BR.xlf | Regenerated localization entry for FailedWithErrors. |
| src/Platform/Microsoft.Testing.Platform/OutputDevice/Terminal/xlf/TerminalResources.ru.xlf | Regenerated localization entry for FailedWithErrors. |
| src/Platform/Microsoft.Testing.Platform/OutputDevice/Terminal/xlf/TerminalResources.tr.xlf | Regenerated localization entry for FailedWithErrors. |
| src/Platform/Microsoft.Testing.Platform/OutputDevice/Terminal/xlf/TerminalResources.zh-Hans.xlf | Regenerated localization entry for FailedWithErrors. |
| src/Platform/Microsoft.Testing.Platform/OutputDevice/Terminal/xlf/TerminalResources.zh-Hant.xlf | Regenerated localization entry for FailedWithErrors. |
Copilot's findings
- Files reviewed: 21/21 changed files
- Comments generated: 5
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.
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.
- Per-assembly count assertions now build the expected '[+P/xF/?S]' bracket via an ExpectedCounts helper that formats with CultureInfo.CurrentCulture (matching the reporter), so they stay correct under cultures with non-Latin digit shapes. - Correct the ShowAssembly / ShowAssemblyStartAndComplete XML docs to describe only the behavior implemented here (the mid-stream per-assembly summary line); drop the references to the not-yet-implemented 'Running tests from' banner and the final-summary per-assembly entries (which land in a later slice). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Uh oh!
There was an error while loading. Please reload this page.
…le expression Replace the manual foreach/return/throw in the GetAssemblySummaryLine test helper with Array.Find(...) ?? throw, expressing the lookup as one expression. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Uh oh!
There was an error while loading. Please reload this page.
Amaury Levé (Evangelink)
deleted the
copilot/share-terminal-per-assembly-counts
branch
June 20, 2026 08:45
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Slice 2/n of unifying the shared Microsoft.Testing.Platform terminal reporter with the
dotnet/sdkorchestrator fork, continuing the test-first approach: port the SDK's reporter test as the expected-UI oracle, then implement the feature to make it pass.Stacked on #9263 (slice 1, handshake surface), which this extends.
Added (additive — gated on the new
ShowAssemblyoptions that default off, so the in-process host is unaffected and its output stays byte-identical)ShowAssembly+ShowAssemblyStartAndCompleteoptions.AssemblyRunCompleted(from Share terminal handshake-failure surface + port SDK oracle test (1/n) #9263) now prints the per-assembly summary line (link + result + compact counts + duration) whenShowAssembly && ShowAssemblyStartAndComplete.AppendAssemblySummary/AppendAssemblyResult/AppendAssemblyTestCounts/AppendGlyphCount— the compact[+P/xF/?S](ASCII) /[✓P/xF/↓S](full-ANSI) counts block, with the/r{N}retry segment wired toTestProgressState.RetriedFailedTests.TestCompleted(assembly, tfm, arch, executionId, instanceId, …)overload that updates per-assembly counts (delegates to the shared per-execution-id path;instanceId-based retry attribution lands in the retry slice).TerminalResources.FailedWithErrors(resx + 13 xlf viaUpdateXlf+ hand-maintained!IS_CORE_MTPaccessor).Oracle test ported
AssemblyRunCompleted_WithShowAssemblyStartAndComplete_PrintsPerAssemblyCounts— ported from the SDK'sTerminalTestReporterTests(regression for dotnet/sdk#52128) to MSTest + the testfxStringBuilderConsoleharness, asserting the rendered[+3/x0/?1]counts block. (NoAnsiis used so the assertion is plain text; it exercises the same ASCII glyph set the SDK asserts via itsSimpleTerminalpath — bothSimpleAnsiTerminalandNonAnsiTerminalderive fromSimpleTerminal, notAnsiTerminal.)Remaining slices
3/n = per-assembly counts in the final summary for N>1 (
TestExecutionCompleted_WithMultipleAssemblies_…); 4/n (high-risk) = the instanceId retry model/r{N}+ unifiedTestCompleted/TestInProgress/TestDiscoveredsignatures (…_WhenTestsWereRetried_…). After those, the SDK consumes the package and deletes its fork.Verification
Microsoft.Testing.Platform.UnitTestssuite green: 1200 total, 0 failed (byte-exact in-process tests unchanged).TerminalReporterContractconsumer compiles in isolation.