Uh oh!
There was an error while loading. Please reload this page.
Unify template-resolve-and-sanitize pipeline in ArtifactNamingHelper - #9855
Conversation
Extract the shared resolve-template/split-path/sanitize-leaf/recombine pipeline into a single static ArtifactNamingHelper.ResolveAndSanitize overload that accepts a leaf sanitizer delegate. ArtifactNamingService and ReportFileNameHelper now both delegate to it, removing the near-identical duplicated logic described in #9841. The non-empty leaf guard is now applied on both paths. Fixes#9841 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 8c5b1087-7e14-45e5-a444-daf19d40d832
There was a problem hiding this comment.
Pull request overview
Unifies artifact template resolution and leaf-name sanitization in ArtifactNamingHelper, resolving #9841.
Changes:
- Extracts the shared resolve-and-sanitize pipeline.
- Delegates platform and report naming paths to the shared helper.
- Tracks the new internal API across all linked-source assemblies.
Show a summary per file
| File | Description |
|---|---|
SharedExtensionHelpers/ReportFileNameHelper.cs | Delegates report naming to the shared pipeline. |
Microsoft.Testing.Platform/Services/ArtifactNamingService.cs | Delegates artifact naming to the shared pipeline. |
Microsoft.Testing.Platform/Services/ArtifactNamingHelper.cs | Adds the shared resolve-and-sanitize implementation. |
Microsoft.Testing.Platform/InternalAPI/InternalAPI.Unshipped.txt | Tracks the new internal method. |
Microsoft.Testing.Extensions.VideoRecorder/InternalAPI/InternalAPI.Unshipped.txt | Tracks the linked method. |
Microsoft.Testing.Extensions.TrxReport/InternalAPI/InternalAPI.Unshipped.txt | Tracks the linked method. |
Microsoft.Testing.Extensions.JUnitReport/InternalAPI/InternalAPI.Unshipped.txt | Tracks the linked method. |
Microsoft.Testing.Extensions.HtmlReport/InternalAPI/InternalAPI.Unshipped.txt | Tracks the linked method. |
Microsoft.Testing.Extensions.HangDump/InternalAPI/InternalAPI.Unshipped.txt | Tracks the linked method. |
Microsoft.Testing.Extensions.CtrfReport/InternalAPI/InternalAPI.Unshipped.txt | Tracks the linked method. |
Microsoft.Testing.Extensions.CrashDump/InternalAPI/InternalAPI.Unshipped.txt | Tracks the linked method. |
Review details
- Files reviewed: 11/11 changed files
- Comments generated: 0
- Review effort level: Medium
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, well-scoped refactoring that unifies two near-identical template-resolve-and-sanitize pipelines into a single ArtifactNamingHelper.ResolveAndSanitize method. Both callers (ArtifactNamingService.ResolveFileName and ReportFileNameHelper.ResolveAndSanitize) now delegate to it, passing their respective sanitizer functions.
| # | Dimension | Verdict |
|---|---|---|
| 1 | Algorithmic Correctness | ✅ Logic is identical to the extracted code. One minor behavioral tightening noted inline (empty leaf name now throws for ReportFileNameHelper path too). |
| 2 | Edge Cases & Error Handling | ✅ |
| 3 | Concurrency & Thread Safety | N/A |
| 4 | Performance & Allocations | ✅ No change in allocation profile |
| 5 | API Design & Public Surface | ✅ Internal API, properly declared in InternalAPI.Unshipped.txt for all referencing projects |
| 6 | Backward Compatibility | ✅ Internal-only change |
| 7 | Cross-TFM Correctness | ✅ No TFM-specific code |
| 8 | Security | ✅ |
| 9 | Naming & Conventions | ✅ |
| 10 | Code Style & Formatting | ✅ |
| 11 | Documentation | ✅ Thorough XML docs |
| 12 | Localization | N/A |
| 13 | Testing | ✅ Existing tests cover both callers |
| 14–22 | Remaining dimensions | N/A or ✅ |
Overall: This is a textbook DRY refactoring. No blocking issues found.
Uh oh!
There was an error while loading. Please reload this page.
🔴 Build Failure AnalysisBuild: FAILED | Duration: 255s | Errors: 5 (4 unique) | Warnings: 1 Root CauseAll 4 compilation errors are CA1416 (platform compatibility) in The Relationship to This PR
Suggested FixAdd #pragma warning disable CA1416// Platform compatibility — tests don't target browserpublicTaskRunLongRunning(Func<Task>action,stringname,CancellationTokencancellationToken)=>_inner.RunLongRunning(action,name,cancellationToken);
#pragma warning restore CA1416This fix would need to be applied to both
|
Amaury Levé (Evangelink)
commented
Jul 11, 2026
Thanks for the analysis. Confirmed these CA1416 errors are pre-existing on main and not caused by this PR — the failing file (\ est/UnitTests/Microsoft.Testing.Platform.UnitTests/Logging/FileLoggerTests.cs) is not modified here, and the \ITask.RunLongRunning\ delegations already exist on \main. They're being addressed separately, so this PR (a scoped dedup refactor) intentionally leaves them alone; CI should go green once main is fixed and this branch is rebased. |
Uh oh!
There was an error while loading. Please reload this page.
Fixes#9841.
Problem
ArtifactNamingService.ResolveFileName(inMicrosoft.Testing.Platform) andReportFileNameHelper.ResolveAndSanitize(inSharedExtensionHelpers) implemented the same algorithm: resolve template placeholders viaArtifactNamingHelper, split the result into directory + leaf, sanitize the leaf, and reassemble the path. The only real differences were where the process name/id came from and which sanitizer was used.Change
Applied recommendation #2 from the issue (smaller, less invasive): extracted the shared pipeline into a single static overload
ArtifactNamingHelper.ResolveAndSanitize(template, processName, processId, timestamp, sanitizeLeafFileName)that takes the leaf sanitizer as aFunc<string, string>delegate.ArtifactNamingService.ResolveFileNamenow delegates, passingArtifactFileNameSanitizer.ReplaceInvalidFileNameChars.ReportFileNameHelper.ResolveAndSanitizenow delegates, passingReportFileNameSanitizer.ReplaceInvalidFileNameChars. Its public signature is unchanged (matches the shipped InternalAPI), so no shipped-API churn.ArtifactNamingServicepreviously had is now applied on both paths.The two sanitizers (
ArtifactFileNameSanitizer/ReportFileNameSanitizer) are intentionally left as-is and passed in as delegates — consolidating them is tracked separately (the related issue referenced in #9841).Notes
ArtifactNamingHelper.ResolveAndSanitizemember is added to theInternalAPI.Unshipped.txtof every project that links theArtifactNamingHelpersource (RS0051).Testing
Microsoft.Testing.Platform.UnitTests— ArtifactNaming* tests: 24/24 passed.Microsoft.Testing.Extensions.UnitTests— ReportFileName* tests: 12/12 passed.