Uh oh!
There was an error while loading. Please reload this page.
Split TerminalTestReporter.Summary.cs into focused partial files - #10387
Conversation
There was a problem hiding this comment.
Pull request overview
Splits the oversized terminal reporter summary into focused partial-class files without changing behavior.
Changes:
- Separates coverage and discovery reporting.
- Extracts flaky/retry and slowest-test rendering.
- Keeps all files below 350 lines.
Show a summary per file
| File | Description |
|---|---|
TerminalTestReporter.Summary.cs | Retains run-summary orchestration. |
TerminalTestReporter.Coverage.cs | Contains coverage summary rendering. |
TerminalTestReporter.Discovery.cs | Contains discovery tracking and summaries. |
TerminalTestReporter.FlakyTests.cs | Contains retry and flaky-test output. |
TerminalTestReporter.SlowestTests.cs | Contains slowest-test output. |
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 0
- Review effort level: Balanced
There was a problem hiding this comment.
Note
🤖 Automated review by GitHub Copilot. Generated by the Expert Code Review workflow. To request a follow-up action, reply by tagging @copilot directly.
Expert Review Summary
Verdict: No issues found ✅
This is a pure code-move refactoring — methods were relocated from TerminalTestReporter.Summary.cs into four new partial class files (Coverage, Discovery, FlakyTests, SlowestTests) with no behavioral changes.
| Dimension | Result |
|---|---|
| Threading & Concurrency | LGTM — no new shared state or synchronization changes |
| Public API & Binary Compatibility | LGTM — all moved members are internal or private; no public API surface change |
| Performance & Allocations | LGTM — identical code paths |
| Cross-TFM Compatibility | LGTM — no new conditional compilation or TFM-specific code |
| Build Infrastructure | LGTM — *.cs glob picks up new files automatically |
| Scope & PR Discipline | LGTM — clean separation along logical boundaries, verified line-identical |
| Localization & Resources | LGTM — using sets correctly narrowed per file |
| Code Structure | LGTM — follows established TerminalTestReporter.<x>.cs convention |
All new files correctly declare [UnsupportedOSPlatform("browser")] internal sealed partial class TerminalTestReporter and include only the using directives they actually need. No issues identified across any applicable review dimension.
e06458e to
c450377CompareThere was a problem hiding this comment.
Review details
Suppressed comments (1)
src/Platform/Microsoft.Testing.Platform/OutputDevice/Terminal/TerminalTestReporter.cs:18
- The file inventory no longer matches the PR description: the implementation uses
TerminalTestReporter.TestDiscovery.cs(notDiscovery.cs), andAppendRetrySummaryLinesremains inSummary.cs, making the actual Summary/FlakyTests sizes 248/83 rather than the documented 222/109. Since the downstream vendoring note depends on exact paths, please update the PR table and split rationale to reflect the committed layout, or move/rename the files to match it.
/// <c>TestDiscovery</c> (per-test execution and discovery output), <c>Summary</c> (the post-run verdict and counts),
/// <c>FlakyTests</c> and <c>SlowestTests</c> (the optional summary sections), <c>Coverage</c> (code-coverage and
- Files reviewed: 6/6 changed files
- Comments generated: 0 new
- Review effort level: Balanced
TerminalTestReporter.Summary.cs had grown to 715 lines and mixed several unrelated concerns. Move each into its own partial file, following the existing TerminalTestReporter.<Concern>.cs convention: - TerminalTestReporter.Coverage.cs: AppendCoverageSummary (public and private), FormatThresholdPercentages and the coverage label helpers. - TerminalTestReporter.TestDiscovery.cs: both TestDiscovered overloads and AppendTestDiscoverySummary. Named to pair with the existing TerminalTestReporter.TestCompletion.cs, its mirror concern. - TerminalTestReporter.FlakyTests.cs: AppendFlakyTests and AppendFlakyTestLine, which render the optional "Flaky tests" section. - TerminalTestReporter.SlowestTests.cs: AppendSlowestTests and AppendSlowestTestLine. TerminalTestReporter.Summary.cs keeps AppendTestRunSummary and AppendRetrySummaryLines; the latter emits the "flaky:" / "retried:" counter lines inline between the skipped count and the duration, so it belongs with the rest of the counter block rather than with the separate flaky-test listing. Every file is now well under 350 lines. No member body was changed: the 606 non-empty class-body lines of the original file and of the five files that replace it are identical. Two private helpers did change position relative to the whole file, each moving next to its sole caller: AppendSlowestTestLine (previously after AppendFlakyTestLine) and AppendRetrySummaryLines. The other non-move change is narrowing each file's using set to what it actually needs. Also add a <remarks> map of the 13 partials to the type doc on TerminalTestReporter.cs, following the precedent in TrxResultStreamingStore.cs. Fixes#10367 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: d3b865a0-de90-4543-a493-9538aa8f1e2a
c450377 to
58192e3CompareUh oh!
There was an error while loading. Please reload this page.
…s manifest testfx split TerminalTestReporter.Summary.cs into focused partials (microsoft/testfx#10387), adding four upstream paths that the dotnet-test-terminal-reporter entry does not list. Until they are tracked, edits to the coverage, discovery, flaky-test and slowest-test rendering are invisible to the drift detector. The new sources are appended rather than sorted into the list: the drift bot keys issue identity on the positional source index, so inserting would orphan the open issues for .Summary.cs (dotnet#7) and .TestCompletion.cs (dotnet#8) and file duplicates at the shifted positions. Document that sources is append-only. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: b8858a93-4a8d-4028-b0db-c221ef158206
microsoft/testfx#10387 split TerminalTestReporter.Summary.cs into focused partials, adding Coverage, FlakyTests, SlowestTests and TestDiscovery. Upstream includes the folder with a glob, so nothing flagged the addition here and edits to those four files were invisible to the drift detector (microsoft/testfx#10390). Append the four paths to the dotnet-test-terminal-reporter entry, baselined at the split commit acb5bafaa2. They are appended rather than sorted in because the tracking-issue marker keys on the source index, and #7 (Summary) and #8 (TestCompletion) have open issues (dotnet#55472, dotnet#55473) that inserting would orphan. Those two keep their old baselines: their drift is real, unported feature work (retry/flaky accounting, in-process retry attribution), not just the move. Also bump source #0 (TerminalTestReporter.cs): the only upstream change since its baseline is a <remarks> comment describing upstream's partial layout, which needs no port into the SDK's single-file fork. Document both gotchas in eng/vendored-files.md. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 3328d629-8443-45a7-8ee2-97d3ce23dee5
Fixes#10367
TerminalTestReporter.Summary.cshad grown to 715 lines and mixed several unrelated concerns. This splits it along the existing logical boundaries, following the establishedTerminalTestReporter.<Concern>.csconvention.TerminalTestReporter.Summary.cs(trimmed)AppendTestRunSummary,AppendRetrySummaryLinesTerminalTestReporter.Coverage.cs(new)AppendCoverageSummary(internal + private),FormatThresholdPercentages,GetCoverage*LabelhelpersTerminalTestReporter.TestDiscovery.cs(new)TestDiscovered(both overloads),AppendTestDiscoverySummaryTerminalTestReporter.FlakyTests.cs(new)AppendFlakyTests,AppendFlakyTestLineTerminalTestReporter.SlowestTests.cs(new)AppendSlowestTests,AppendSlowestTestLineTwo deviations from the issue text, both deliberate:
Summary.csat ~394 lines. Breaking out the flaky and slowest sections is what actually satisfies the stated criterion, andSlowestTests.csat 79 lines is in line with the existingErroredAssemblies.csat 75.TestDiscovery.cs, notDiscovery.cs, so it pairs with the existingTerminalTestReporter.TestCompletion.cs— the mirror concern.AppendRetrySummaryLinesstays inSummary.cs: it emits theflaky:/retried:counter lines inline between the skipped count and the duration, so it belongs with the rest of the counter block rather than with the separate "Flaky tests" listing.Also adds a
<remarks>map of the partials to the type doc onTerminalTestReporter.cs, following the precedent inTrxResultStreamingStore.cs. At 13 partials the type had no signpost for where anything lives.Not a behavior change
No member body was modified. Verified mechanically rather than by eye: the 606 non-empty class-body lines of the original file and of the five files replacing it are an identical multiset with zero diffs, and a per-member brace-depth chunker matches all 17 members byte-for-byte. Two private helpers changed position relative to the whole file, each moving adjacent to its sole caller —
AppendSlowestTestLine(previously sitting afterAppendFlakyTestLine) andAppendRetrySummaryLines. The only other non-move change is narrowing each file'susingset; note that the issue's guideline #5 ("no new using directives needed, all files share the same using set as today") is wrong — copying all three usings everywhere would trip IDE0005, which iswarningin.editorconfigwithEnforceCodeStyleInBuild=true.Note for downstream:
dotnet/sdkThis reporter source is hard-forked into
dotnet/sdk, whoseeng/vendored-files.jsontracks each partial individually (I checked — it listsTerminalTestReporter.cs,.ErroredAssemblies.cs,.Formatting.cs,.Handshake.cs,.Lifecycle.cs,.Messaging.cs,.Summary.cs,.TestCompletion.cs… each with"scope": "reporter partial").TerminalReporterContract.propsglobs*.cs, so nothing needed to change on our side. But the four new paths are absent from the SDK manifest, so until it is updated, edits to the coverage / discovery / flaky / slowest code would be invisible to the SDK's drift workflow. The shrinking ofSummary.cswill fire that workflow, so it should get noticed — tracked in #10390 regardless.Validation
Microsoft.Testing.Platformbuilds for net8.0 / net9.0 / netstandard2.0 — 0 warnings, which is what proves theusingsets are exactly minimal.Microsoft.Testing.Platform.UnitTests— passing on net8.0, net9.0, net462.Microsoft.Testing.Platform.TerminalReporterContract.UnitTests— passing on net8.0 and net9.0, confirming the files still compile as a standalone shared-source consumer with no platform reference.