Add public injectable IArtifactNamingService to Microsoft.Testing.Platform#9783
Conversation
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Pull request overview
This PR exposes Microsoft.Testing.Platform's artifact file-name templating as a new public, injectable core service, so that consumers can resolve placeholder templates ({pname}, {pid}, {asm}, {tfm}, {arch}, {time}) and get a sanitized leaf file name without relying on the existing linked-source ([Embedded] helper) pattern. It is one of two PRs split out from #9780 (the companion #9782 handles CTRF overwrite behavior).
The implementation wraps the existing ArtifactNamingHelper templating and mirrors the extension-side sanitization logic. I verified the service is registered as a common service and resolves correctly by interface (the ServiceProvider matches instances via IsInstanceOfType), the constructor dependencies (systemEnvironment, systemClock, _testApplicationModuleInfo) are all in scope at the registration site, IEnvironment.ProcessId/IClock.UtcNow/ArgumentGuard.Ensure all exist as used, and the new internal/public API tracking entries are correctly formatted and placed (InternalAPI tracking is in use in this project — a prior memory to the contrary was outdated and has been down-voted).
Changes:
- Adds public
IArtifactNamingService+ServiceProviderExtensions.GetArtifactNamingServiceaccessor, backed by internalArtifactNamingService. - Adds a core-internal
ArtifactFileNameSanitizercopy of the extension-sideReportFileNameSanitizer(intentional duplication, documented for future consolidation). - Registers the service in
TestHostBuilder.CommonServices.cs, updates Public/Internal API baselines, and addsArtifactNamingServiceTests.
Show a summary per file
| File | Description |
|---|---|
Services/IArtifactNamingService.cs |
New public interface with a single ResolveFileName(string) member and placeholder docs. |
Services/ArtifactNamingService.cs |
Internal implementation resolving process name/id/time and sanitizing the leaf name. |
Services/ArtifactFileNameSanitizer.cs |
Core-internal copy of the extension invalid-char/reserved-name sanitizer. |
Services/ServiceProviderExtensions.cs |
New public GetArtifactNamingService accessor. |
Hosts/TestHostBuilder.CommonServices.cs |
Registers ArtifactNamingService as a common service. |
PublicAPI/PublicAPI.Unshipped.txt |
Declares the new public interface + accessor. |
InternalAPI/InternalAPI.Unshipped.txt |
Declares the new internal types/members. |
Services/ArtifactNamingServiceTests.cs |
Tests placeholder expansion, invalid-char sanitization, directory preservation, unknown-placeholder passthrough. |
Review details
- Files reviewed: 8/8 changed files
- Comments generated: 1
- 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 design — exposing the file-name templating as a public injectable service is the right move. The interface is minimal (single member), the implementation correctly delegates to the existing ArtifactNamingHelper, and the sanitizer mirrors the shipped extension-side copy byte-for-byte. Service registration and resolution via IsInstanceOfType will work correctly.
Dimension Verdicts
| # | Dimension | Verdict |
|---|---|---|
| 1 | Algorithmic Correctness | Path.GetFileName → empty on trailing separator) |
| 2 | Public API Design | ✅ Minimal surface, no init, correctly in PublicAPI.Unshipped.txt |
| 3 | Backward Compatibility | ✅ Additive only — new interface, new extension method |
| 4 | Performance | ✅ No hot-path concerns; per-call GetStandardReplacements is acceptable for a naming utility |
| 5 | Cross-TFM Correctness | ✅ GeneratedRegex conditional on NET7_0_OR_GREATER with fallback |
| 6 | Thread Safety | ✅ No mutable shared state; each call is self-contained |
| 7 | Error Handling | |
| 8 | Localization | N/A — no user-facing strings added |
| 9 | IPC Contract | N/A |
| 10 | Security | ✅ Path sanitization prevents invalid characters; directory portion preserved as-is (callers control the base directory) |
| 11 | Testability | ✅ Constructor-injected dependencies, Moq-based tests |
| 12 | Test Quality | ✅ Good coverage of core scenarios; minor gap on reserved-name and {asm}/{tfm}/{arch} paths (hard to mock static reflection) |
| 13 | Code Style | CommonServices.cs (see inline) |
| 14 | Documentation | {asm}/{tfm} should note "unknown" fallback (see inline) |
| 15 | InternalAPI Tracking | ✅ Entries match the actual internal surface |
| 16 | Scope Discipline | ✅ Single concern |
| 17 | Simplicity | ✅ Clean delegation pattern |
| 18 | Build Infrastructure | N/A |
| 19 | Analyzer/CodeFix | N/A |
| 20 | Extension Points | ✅ Registered as common service; extensible via DI replacement |
| 21 | CLI Options | N/A |
| 22 | Agentic Workflows | N/A |
CI Status
Linux builds (Debug & Release) are failing. Please verify the failures are unrelated to this change before merging.
Summary
Three inline comments posted — one style nit, one robustness improvement for an edge case, and one documentation accuracy suggestion. None are blocking. Overall this is a well-structured addition to the platform.
🔴 Build Failure AnalysisStatus: 7 errors, all RS0051 (undeclared API) Errors (deduplicated — repeated per TFM: net8.0, net9.0, netstandard2.0)
Why this failsThe project tracks internal API surface via Cascading failures
FixAdd the following two lines to These should be inserted in alphabetical order among the existing entries.
|
There was a problem hiding this comment.
🤖 Automated content by GitHub Copilot. Generated by the Build Failure Analysis workflow. · 89.8 AIC · ⌖ 6.41 AIC · ⊞ 7.3K · ◷
17095c1 to
425820e
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
d58afa3 to
fe9bf60
Compare
…tform Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
fe9bf60 to
3640da3
Compare
🧪 Test quality grade — PR #9783
This advisory comment was generated automatically. Grades are heuristic
|
Summary
Exposes file-name templating as a public, injectable core service in Microsoft.Testing.Platform, so it can be consumed outside the existing linked-source (
[Embedded]helper) pattern.Changes
IArtifactNamingService(public) — single memberstring ResolveFileName(string template). Expands the standard placeholders ({pname},{pid},{asm},{tfm},{arch},{time}) and sanitizes the leaf file name while preserving any directory portion of the template.ArtifactNamingService(internal) — implementation that wraps the existingArtifactNamingHelpertemplating with process name/id/timestamp resolved fromITestApplicationModuleInfo,IEnvironmentandIClock.ArtifactFileNameSanitizer(internal) — a core-internal copy of the invalid-char / reserved-name (CON/PRN/AUX/NUL/COM1-9/LPT1-9/CLOCK$) sanitization from the extension-sideReportFileNameSanitizer. The extension copy has shipped InternalAPI entries and is intentionally left in place; a comment marks the two for future consolidation.TestHostBuilder.CommonServices.csand resolvable via a new publicServiceProviderExtensions.GetArtifactNamingService(this IServiceProvider)accessor.ArtifactNamingServiceTestscover placeholder expansion, invalid-char sanitization, directory-portion preservation, and unknown-placeholder passthrough.Verification
Built
Microsoft.Testing.Platform.UnitTestsand ran--filter "FullyQualifiedName~ArtifactNamingService"— 4/4 passing.Note
This is one of two PRs split out from #9780 (the other, #9782, aligns the CTRF report writer overwrite behavior).
The pre-existing RS0051 InternalAPI baseline break from #9774 (
BaseSerializer.ReadFields/WriteListPayloadmissing InternalAPI entries) that previously blocked the build has since been fixed onmain; this branch was rebased onto that fix, so the two BaseSerializer entries now appear alongside the new service entries inInternalAPI.Unshipped.txt.