Uh oh!
There was an error while loading. Please reload this page.
Align CTRF report writer overwrite behavior and add public IArtifactNamingService - #9780
Closed
Amaury Levé (Evangelink) wants to merge 1 commit into
Closed
Align CTRF report writer overwrite behavior and add public IArtifactNamingService#9780Amaury Levé (Evangelink) wants to merge 1 commit into
Amaury Levé (Evangelink) wants to merge 1 commit into
Conversation
…amingService Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
This was referenced Jul 9, 2026
Amaury Levé (Evangelink)
commented
Jul 9, 2026
MemberAuthor
Contributor
There was a problem hiding this comment.
Pull request overview
This PR makes two related changes to Microsoft.Testing.Platform (MTP) artifact/report file naming:
- Aligns the CTRF report writer to the shared "overwrite + warn" rule.
CtrfReportEnginewas the only report extension that, for default-generated file names, usedFileMode.CreateNewplus a_1/_2disambiguating-suffix retry loop. The change replaces that with a smallWriteAsyncthat always overwrites (FileMode.Create) and warns when the file pre-existed — mirroring theHtmlReportEngine/TrxReportEngine/JUnitReportEnginebehavior (verified all three already share this exact rule viaReportEngineBase). - Adds a new public, injectable
IArtifactNamingServicein MTP core. It exposes file-name templating (placeholder expansion + leaf sanitization) as a common service, backed by an internalArtifactNamingService(wrapping the existing[Embedded]ArtifactNamingHelper) and a core-internalArtifactFileNameSanitizer(an intentional, documented copy of the extension-sideReportFileNameSanitizer).
Changes:
- Replace CTRF's
WriteWithRetryAsync/SplitCtrfExtensionwith an overwrite-and-warnWriteAsync, and drop the now-unusedfileNameExplicitlyProvidedthreading. - Introduce
IArtifactNamingService(public) +ArtifactNamingService/ArtifactFileNameSanitizer(internal), register it as a common service, and add aServiceProviderExtensions.GetArtifactNamingServiceaccessor. - Update Public/Internal API tracking files and add unit tests for both changes.
Show a summary per file
| File | Description |
|---|---|
src/Platform/Microsoft.Testing.Extensions.CtrfReport/CtrfReportEngine.FileWriter.cs | Replaces retry/suffix loop with overwrite-and-warn WriteAsync, matching sibling report engines. |
src/Platform/Microsoft.Testing.Extensions.CtrfReport/CtrfReportEngine.cs | GenerateReportCoreAsync calls WriteAsync and discards the WasExplicit flag. |
src/Platform/Microsoft.Testing.Platform/Services/IArtifactNamingService.cs | New public interface with documented ResolveFileName(template) and placeholder list. |
src/Platform/Microsoft.Testing.Platform/Services/ArtifactNamingService.cs | Internal implementation wrapping ArtifactNamingHelper + leaf sanitization/directory preservation. |
src/Platform/Microsoft.Testing.Platform/Services/ArtifactFileNameSanitizer.cs | Core-internal copy of ReportFileNameSanitizer, documented as pending consolidation. |
src/Platform/Microsoft.Testing.Platform/Services/ServiceProviderExtensions.cs | Adds public GetArtifactNamingService accessor. |
src/Platform/Microsoft.Testing.Platform/Hosts/TestHostBuilder.CommonServices.cs | Registers ArtifactNamingService as a common service. |
src/Platform/Microsoft.Testing.Platform/PublicAPI/PublicAPI.Unshipped.txt | Declares the new public interface + extension accessor. |
src/Platform/Microsoft.Testing.Platform/InternalAPI/InternalAPI.Unshipped.txt | Declares the new internal types/members. |
test/UnitTests/Microsoft.Testing.Platform.UnitTests/Services/ArtifactNamingServiceTests.cs | New tests for placeholder expansion, sanitization, directory preservation, unknown placeholders. |
test/UnitTests/Microsoft.Testing.Extensions.UnitTests/CtrfReportEngineTests.cs | Updates default-name tests to expect FileMode.Create overwrite-and-warn semantics. |
Notes from verification (no comments stored):
- The service resolves correctly through the public accessor:
ServiceProvider.GetServicesInternalusesIsInstanceOfType, so registering the concreteArtifactNamingServicesatisfiesGetRequiredService<IArtifactNamingService>(), and the type is not inInternalOnlyExtensions. - The new public interface uses a plain method (no
initaccessors) and is declared inPublicAPI.Unshipped.txt; InternalAPI entries for the new internal types/members are complete. ArtifactNamingService.ResolveFileNamematches the establishedReportFileNameHelper.ResolveAndSanitizebehavior (sanitize leaf, preserve directory), andArtifactFileNameSanitizeris a byte-for-byte copy ofReportFileNameSanitizeraside from namespace — duplication is explicitly documented as intentional pending consolidation.- The CTRF change leaves no dangling references to the removed helpers, and the base fields it stopped using directly remain in use elsewhere.
Review details
- Files reviewed: 11/11 changed files
- Comments generated: 0
- Review effort level: Medium
Contributor
🔍 Build Failure AnalysisSummary — Two new Root cause: Missing internal API declarations for |
| Metric | Value |
|---|---|
| Status | ❌ FAILED |
| Duration | 164.1 s |
| Projects | 49 |
| Errors | 7 (6 unique RS0051 + 1 "Build failed") |
| Warnings | 0 |
Failed projects:
| Project | Duration | Reason |
|---|---|---|
Microsoft.Testing.Platform.csproj | 46.4 s | RS0051 errors (root cause) |
Microsoft.Testing.Extensions.CrashDump.csproj | 79.3 s | Cascading (depends on MTP) |
Microsoft.Testing.Extensions.CtrfReport.csproj | 78.2 s | Cascading (depends on MTP) |
All MSBuild errors (7)
| Code | Project | File:Line | Message |
|---|---|---|---|
| RS0051 | Microsoft.Testing.Platform | BaseSerializer.cs:359 | ReadFields(...) not part of declared API |
| RS0051 | Microsoft.Testing.Platform | BaseSerializer.cs:380 | WriteListPayload<T>(...) not part of declared API |
| RS0051 | Microsoft.Testing.Platform | BaseSerializer.cs:359 | (duplicate — net9.0 TFM) |
| RS0051 | Microsoft.Testing.Platform | BaseSerializer.cs:380 | (duplicate — net9.0 TFM) |
| RS0051 | Microsoft.Testing.Platform | BaseSerializer.cs:359 | (duplicate — netstandard2.0 TFM) |
| RS0051 | Microsoft.Testing.Platform | BaseSerializer.cs:380 | (duplicate — netstandard2.0 TFM) |
| — | Build.proj | — | Build failed. |
🤖 Generated by the Build Failure Analysis workflow · commit 7591119
🤖 Automated content by GitHub Copilot. Generated by the Build Failure Analysis workflow. · 168 AIC · ⌖ 6.91 AIC · ⊞ 7.3K · [◷]( · ◷)
Contributor
There was a problem hiding this comment.
🤖 Automated content by GitHub Copilot. Generated by the Build Failure Analysis workflow. · 168 AIC · ⌖ 6.91 AIC · ⊞ 7.3K · ◷
| Microsoft.Testing.Platform.Services.ArtifactNamingService | ||
| Microsoft.Testing.Platform.Services.ArtifactNamingService.ArtifactNamingService(Microsoft.Testing.Platform.Services.ITestApplicationModuleInfo! testApplicationModuleInfo, Microsoft.Testing.Platform.Helpers.IEnvironment! environment, Microsoft.Testing.Platform.Helpers.IClock! clock) -> void | ||
| Microsoft.Testing.Platform.Services.ArtifactNamingService.ResolveFileName(string! template) -> string! | ||
| static Microsoft.Testing.Platform.Services.ArtifactFileNameSanitizer.ReplaceInvalidFileNameChars(string! fileName) -> string! |
Contributor
There was a problem hiding this comment.
The base branch (a946ef7) introduced BaseSerializer.ReadFields and BaseSerializer.WriteListPayload<T> as protected static methods but they were never declared here, causing RS0051 errors across all 3 TFMs.
Suggested change
| static Microsoft.Testing.Platform.Services.ArtifactFileNameSanitizer.ReplaceInvalidFileNameChars(string! fileName) -> string! | |
| static Microsoft.Testing.Platform.Services.ArtifactFileNameSanitizer.ReplaceInvalidFileNameChars(string! fileName) -> string! | |
| static Microsoft.Testing.Platform.IPC.Serializers.BaseSerializer.ReadFields(System.IO.Stream! stream, System.Func<ushort, int, bool>! tryReadField) -> void | |
| static Microsoft.Testing.Platform.IPC.Serializers.BaseSerializer.WriteListPayload<T>(System.IO.Stream! stream, ushort fieldId, T[]? list, System.Action<System.IO.Stream!, T>! writeItem) -> void |
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.
Summary
Two related changes to MTP report file naming.
1. Align CTRF report writer to overwrite + warn (matches TRX/HTML/JUnit)
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 aligns it with the shared rule already used by the TRX, HTML and JUnit report extensions: always overwrite (FileMode.Create) and emit a warning when the file already existed, giving users a single, predictable rule.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_DefaultFileExistswas replaced byGenerateReportAsync_OverwritesAndWarns_When_DefaultFileExists, and the IOException test was renamed to..._When_WriteFails— both mirroring theHtmlReportEngineTestsequivalents. (27/27 passing.)The
CtrfReportFileExistsAndWillBeOverwrittenresource already existed, so no resx/xlf changes were needed.2. New public, injectable
IArtifactNamingServicein MTP coreExposes file-name templating as a public, injectable core service so it can be consumed outside the existing linked-source (
[Embedded]helper) pattern.IArtifactNamingService(public) — single memberResolveFileName(string template); expands the standard placeholders ({pname},{pid},{asm},{tfm},{arch},{time}) and sanitizes the leaf file name while preserving any directory portion.ArtifactNamingService(internal) — wraps the existingArtifactNamingHelpertemplating with process/name/time resolution fromITestApplicationModuleInfo/IEnvironment/IClock.ArtifactFileNameSanitizer(internal) — a core-internal copy of the invalid-char/reserved-name sanitization from the extension-sideReportFileNameSanitizer(which has shipped InternalAPI entries and is intentionally left in place; the two are marked for future consolidation).TestHostBuilder.CommonServices.csand resolvable via a new publicServiceProviderExtensions.GetArtifactNamingService(...)accessor.ArtifactNamingServiceTests(4/4 passing) cover placeholder expansion, sanitization, directory preservation, and unknown-placeholder passthrough.Verification
Built
Microsoft.Testing.Platform.UnitTestsandMicrosoft.Testing.Extensions.UnitTestsand ran the targeted filters:ArtifactNamingService— 4/4 passingCtrfReportEngineTests— 27/27 passingThere is a pre-existing repo-wide build break introduced by #9774 that is not addressed here and needs a separate fix:
IPC/Serializers/BaseSerializer.csgained twoprotected staticmethods (ReadFields,WriteListPayload) but no matchingInternalAPI.Unshipped.txtentries were added, so analyzer RS0051 fails as an error in every project that links that source (Microsoft.Testing.Platform and the HangDump / MSBuild / Retry / TrxReport extensions). A transient local workaround was applied only to verify this PR and then reverted, so it is intentionally not part of this change.