Uh oh!
There was an error while loading. Please reload this page.
Align CTRF report writer overwrite behavior with TRX/HTML/JUnit - #9782
Conversation
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR aligns the CTRF report writer's file-overwrite behavior with the TRX, HTML, and JUnit report extensions. Previously CtrfReportEngine was the only report extension that, for default-generated file names, used FileMode.CreateNew plus a _1/_2 disambiguating-suffix retry loop rather than overwriting. The change makes CTRF follow the shared, product-wide rule: always overwrite (FileMode.Create) and emit a warning when the file already existed, giving users a single predictable behavior across all report formats.
Changes:
- Replaced
WriteWithRetryAsyncand theSplitCtrfExtensionhelper with a smallWriteAsyncthat mirrorsHtmlReportEngine.WriteAsync(overwrite + warn). - Updated
GenerateReportCoreAsyncto callWriteAsyncand stop threading thefileNameExplicitlyProvidedflag. - Updated unit tests to expect
FileMode.Create, replacing the disambiguation test with an overwrite-and-warn test and renaming the IOException test to reflect write-failure propagation.
Show a summary per file
| File | Description |
|---|---|
src/Platform/Microsoft.Testing.Extensions.CtrfReport/CtrfReportEngine.FileWriter.cs | Removed the retry/disambiguation logic and SplitCtrfExtension; added WriteAsync mirroring the shared overwrite-and-warn rule. |
src/Platform/Microsoft.Testing.Extensions.CtrfReport/CtrfReportEngine.cs | GenerateReportCoreAsync now calls WriteAsync and discards the explicit-name flag. |
test/UnitTests/Microsoft.Testing.Extensions.UnitTests/CtrfReportEngineTests.cs | Updated tests to expect FileMode.Create; replaced the suffix test with an overwrite-and-warn test and renamed the IOException test. |
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 0
- Review effort level: Medium
🧪 Test quality grade — PR #9782
This advisory comment was generated automatically. Grades are heuristic
|
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.
Review Summary
Clean PR — solid simplification that aligns CTRF report behavior with the TRX, HTML, and JUnit report extensions. The new WriteAsync implementation is a near-identical structural match to HtmlReportEngine.WriteAsync (verified), which is the stated goal.
Verdict Table
| # | Dimension | Severity | Verdict | Notes |
|---|---|---|---|---|
| 1 | Algorithmic Correctness | MAJOR | ✅ Clean | Overwrite-and-warn logic is straightforward and correct |
| 2 | Threading & Concurrency | BLOCKING | ✅ Clean | ConfigureAwait(false) used correctly; no async anti-patterns |
| 3 | Security & IPC Contract Safety | BLOCKING | ✅ Clean | No new security surface |
| 4 | Public API & Binary Compatibility | BLOCKING | ✅ Clean | internal sealed class — no public API changes |
| 5 | Performance & Allocations | MAJOR | ✅ Clean | Simpler code path; retry loop and string allocations removed |
| 6 | Cross-TFM Compatibility | MAJOR | N/A | No TFM-specific APIs |
| 7 | Test Quality | MAJOR | ✅ Clean | MSTest assertions used correctly; parameter order verified (Assert.Contains(substring, value)) |
| 8 | Error Handling | MAJOR | ✅ Clean | IOException propagation tested; ExistFile mock properly set up |
| 9 | Localization | MAJOR | ✅ Clean | Reuses existing CtrfReportFileExistsAndWillBeOverwritten resource |
| 10 | Naming & Style | MINOR | ✅ Clean | Follows .editorconfig conventions |
| 11 | Documentation | MINOR | ✅ Clean | Comments explain why (alignment rationale) not what |
| 12 | Logging | MINOR | N/A | Warning surfaced via return tuple — no direct logging changes |
| 13 | Null Safety | MAJOR | ✅ Clean | warning! used only after Assert.IsNotNull guard |
| 14 | Resource Management | MAJOR | ✅ Clean | No resource leaks introduced |
| 15 | Scope & PR Hygiene | MINOR | ✅ Clean | Single concern: behavior alignment |
| 16 | Test Coverage | MAJOR | ✅ Clean | Old behavior tests replaced with new behavior equivalents |
| 17 | IPC Contract Stability | BLOCKING | N/A | No wire format changes |
| 18 | Backward Compatibility | BLOCKING | Behavioral change for default file names (overwrite vs. suffix). Intentional normalization — internal class, aligns with all other report extensions. | |
| 19 | Simplicity & Readability | MINOR | ✅ Clean | −66 / +13 net — major simplification |
| 20 | Dependency Management | MINOR | ✅ Clean | No dependency changes |
| 21 | Configuration & Defaults | MINOR | ✅ Clean | Default behavior now consistent across all report formats |
| 22 | PowerShell / Script Quality | MINOR | N/A | No scripts |
Notes
- The benign TOCTOU race between
ExistFileandWriteBytesAsync(only affects warning accuracy, not write correctness) mirrors the identical pattern inHtmlReportEngine,JUnitReportEngine, and existing explicit-path handling — acceptable for a best-effort informational warning. - The discard
(string finalPath, _) = ResolveOutputPath(...)is clean — thefileNameExplicitlyProvidedflag is genuinely unused now.
No blocking or major issues found. LGTM.
🔍 Build Failure AnalysisSummary — The build fails because two new Root cause: Missing InternalAPI declarations for |
| Metric | Value |
|---|---|
| Status | ❌ FAILED |
| Duration | 160.2 s |
| Projects | 49 |
| Errors | 7 (6 RS0051 + 1 "Build failed") |
| Warnings | 0 |
Failed projects (cascade):
Microsoft.Testing.Platform.csproj(root cause — net8.0, net9.0, netstandard2.0)Microsoft.Testing.Extensions.CrashDump.csproj(depends on Platform)Microsoft.Testing.Extensions.AzureFoundry.csproj(depends on Platform)Microsoft.Testing.Extensions.AzureDevOpsReport.csproj(depends on Platform)
All MSBuild errors (7)
| # | Code | File | Line | Message |
|---|---|---|---|---|
| 1 | RS0051 | BaseSerializer.cs | 359 | ReadFields(...) not part of declared API |
| 2 | RS0051 | BaseSerializer.cs | 380 | WriteListPayload<T>(...) not part of declared API |
| 3 | RS0051 | BaseSerializer.cs | 359 | (duplicate — net9.0 TFM) |
| 4 | RS0051 | BaseSerializer.cs | 380 | (duplicate — net9.0 TFM) |
| 5 | RS0051 | BaseSerializer.cs | 359 | (duplicate — netstandard2.0 TFM) |
| 6 | RS0051 | BaseSerializer.cs | 380 | (duplicate — netstandard2.0 TFM) |
| 7 | — | — | — | Build failed. (summary) |
🤖 Generated by the Build Failure Analysis workflow using binlog-mcp · commit 297ab1d
🤖 Automated content by GitHub Copilot. Generated by the Build Failure Analysis workflow. · 120.6 AIC · ⌖ 6.99 AIC · ⊞ 7.3K · [◷]( · ◷)
Uh oh!
There was an error while loading. Please reload this page.
Summary
Aligns the CTRF report writer with the shared overwrite rule already used by the TRX, HTML and JUnit report extensions.
Previously
CtrfReportEnginewas the only report extension that, for a default-generated file name, usedFileMode.CreateNewplus a_1/_2disambiguating-suffix retry loop instead of overwriting. This change makes it always overwrite (FileMode.Create) and emit a warning when the file already existed, giving users a single, predictable rule across all report formats.Changes
CtrfReportEngine.FileWriter.cs: replacedWriteWithRetryAsync(and theSplitCtrfExtensionhelper) with a smallWriteAsyncthat mirrorsHtmlReportEngine.CtrfReportEngine.cs:GenerateReportCoreAsyncnow callsWriteAsyncand no longer threads thefileNameExplicitlyProvidedflag.CtrfReportEngineTests.cs: default-name test now expectsFileMode.Create;GenerateReportAsync_AppendsDisambiguatingSuffix_When_DefaultFileExistsreplaced byGenerateReportAsync_OverwritesAndWarns_When_DefaultFileExists, and the IOException test renamed to..._When_WriteFails— both mirroring theHtmlReportEngineTestsequivalents.The
CtrfReportFileExistsAndWillBeOverwrittenresource already existed, so no resx/xlf changes were needed.Verification
Built
Microsoft.Testing.Extensions.UnitTestsand ran--filter "FullyQualifiedName~CtrfReportEngineTests"— 27/27 passing.Note
This is one of two PRs split out from #9780 (the other adds a public
IArtifactNamingService). Heads-up: there is a pre-existing, unrelated RS0051 InternalAPI baseline break from #9774 that can block a fullbuild.cmd; it is tracked/explained in the companion PR that touchesMicrosoft.Testing.Platform.